Node property classes

[achantavy]

This is a follow-up thread for https://github.com/lyft/cartography/pull/1219/files/4accca82017477d772aa2a65cddb62869931eb6d#r1309246010.

We're refactoring the AWS NetworkInterfaces to use the cartography data model and are needing to handle an interesting case. A NetworkInterface node's properties can come from multiple sources:

• (1) describe-network-interfaces
• (2) describe-instances
• And more.

Each of these AWS API calls contains different properties that we would like to unify in a single NetworkInterface node. To represent this using the cartography data model, we create a subclass of CartographyNodeSchema for each potential data source. For example:

• EC2NetworkInterfaceSchema represents a network interface as known by the describe-network-interfaces API. This class API call includes fields not returned by any other API call such as public_ip and requester_id.
• EC2NetworkInterfaceInstanceSchema represents a network interface as known by the describe-instances API.
• And more.

The end result is that after the cartography network interface and EC2 instance sync jobs run, the resulting graph will have NetworkInterface nodes with properties that are merged from all above sources. This works as long as the id fields match up.

We've identified 2 ways to represent CartographyNodeProperties classes to handle this:

1. Create the following class structure where we have duplicate fields across a EC2NetworkInterfaceNodeProperties and EC2NetworkInterfaceInstanceNodeProperties class:

@dataclass(frozen=True)
class EC2NetworkInterfaceNodeProperties(CartographyNodeProperties):
    """
    Network interface properties
    """
    id: PropertyRef = PropertyRef('NetworkInterfaceId')
    lastupdated: PropertyRef = PropertyRef('lastupdated', set_in_kwargs=True)
    description: PropertyRef = PropertyRef('Description')
    mac_address: PropertyRef = PropertyRef('MacAddress')
    private_dns_name: PropertyRef = PropertyRef('PrivateDnsName')
    private_ip_address: PropertyRef = PropertyRef('PrivateIpAddress')
    region: PropertyRef = PropertyRef('Region', set_in_kwargs=True)
    status: PropertyRef = PropertyRef('Status')

    # Properties only returned by describe-network-interfaces
    interface_type: PropertyRef = PropertyRef('InterfaceType')
    public_ip: PropertyRef = PropertyRef('PublicIp')
    requester_id: PropertyRef = PropertyRef('RequesterId')
    requester_managed: PropertyRef = PropertyRef('RequesterManaged')
    source_dest_check: PropertyRef = PropertyRef('SourceDestCheck')
    subnetid: PropertyRef = PropertyRef('SubnetId')


@dataclass(frozen=True)
class EC2NetworkInterfaceInstanceNodeProperties(CartographyNodeProperties):
    """
    Selection of properties of a network interface as known by an EC2 instance
    """
    # arn: PropertyRef = PropertyRef('Arn', extra_index=True)
    id: PropertyRef = PropertyRef('NetworkInterfaceId')
    status: PropertyRef = PropertyRef('Status')
    mac_address: PropertyRef = PropertyRef('MacAddress', extra_index=True)
    description: PropertyRef = PropertyRef('Description')
    private_dns_name: PropertyRef = PropertyRef('PrivateDnsName', extra_index=True)
    private_ip_address: PropertyRef = PropertyRef('PrivateIpAddress', extra_index=True)
    region: PropertyRef = PropertyRef('Region', set_in_kwargs=True)
    lastupdated: PropertyRef = PropertyRef('lastupdated', set_in_kwargs=True)

2. Use a base class:

@dataclass(frozen=True)
class EC2NetworkInterfaceBaseClassNodeProperties(CartographyNodeProperties):
    id: PropertyRef = PropertyRef('NetworkInterfaceId')
    lastupdated: PropertyRef = PropertyRef('lastupdated', set_in_kwargs=True)
    description: PropertyRef = PropertyRef('Description')
    mac_address: PropertyRef = PropertyRef('MacAddress')
    private_dns_name: PropertyRef = PropertyRef('PrivateDnsName')
    private_ip_address: PropertyRef = PropertyRef('PrivateIpAddress')
    region: PropertyRef = PropertyRef('Region', set_in_kwargs=True)
    status: PropertyRef = PropertyRef('Status')

@dataclass(frozen=True)
class EC2NetworkInterfaceNodeProperties(EC2NetworkInterfaceBaseClassNodeProperties):
    # Properties only returned by describe-network-interfaces
    interface_type: PropertyRef = PropertyRef('InterfaceType')
    public_ip: PropertyRef = PropertyRef('PublicIp')
    requester_id: PropertyRef = PropertyRef('RequesterId')
    requester_managed: PropertyRef = PropertyRef('RequesterManaged')
    source_dest_check: PropertyRef = PropertyRef('SourceDestCheck')
    subnetid: PropertyRef = PropertyRef('SubnetId')

I prefer option 1 because it is more explicit even though there is duplication. I don't think we should create sub-sub-classes of items in https://github.com/lyft/cartography/tree/master/cartography/models/core, and I think we should prefer composition over inheritance.

It will get complicated to handle cases using a class hierarchy where a NetworkInterface's "secondary API" like describe-instances includes more fields than its "primary API" (describe-network-interfaces), and vice versa.

Here's how I think of it: a CartographyNodeSchema object is composed of several other objects including a NodeProperties object. I don't want to need to think about any hierarchy or nesting structure beyond this.

[ramonpetgrave64]

I still prefer Option 2 because it's easier for myself to understand the similarities and differences. But that's my opinion that I won't impose on folks.

[achantavy]

My main concern with option 2 is if we try to introduce a new subclass where the base class has more fields than the new subclass, then we will need to refactor that field out and that will potentially be very messy.

[ramonpetgrave64]

 And conversely also messy when when the AWS resource now has a new property that will be returned by the apis of all subclasses. But which is least troublesome in cases when we only want to add the new field to a single class? Probably option 1.

[achantavy]

Yeah, I think it will be easier to handle these considerations with option 1. Shall we go with this as the preferred way going forward?

[ramonpetgrave64]

Yes