EC2 network ACL: Use get method to access dict keys to prevent exception #1392

cmm-lyft · 2024-11-27T00:44:08Z

Summary

Correcting error:

File "/code/venvs/venv/lib/python3.10/site-packages/cartography/intel/aws/ec2/network_acls.py", line 72, in transform_network_acl_data
    'CidrBlock': rule['CidrBlock'],
KeyError: 'CidrBlock'

Checklist

Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:

Update/add unit or integration tests.
Include a screenshot showing what the graph looked like before and after your changes.
Include console log trace showing what happened before and after your changes.

If you are changing a node or relationship:

Update the schema and readme.

If you are implementing a new intel module:

Use the NodeSchema data model.

cmm-lyft · 2024-11-27T00:44:41Z

👀 @achantavy

cmm-lyft · 2024-11-27T00:45:27Z

👀 @heryxpc @serge-wq @khanhldt

kledo-lyft · 2024-11-27T22:25:07Z

cartography/intel/aws/ec2/network_acls.py

+                    'Id': f"{network_acl['NetworkAclId']}/{direction}/{rule.get('RuleNumber')}",
+                    'CidrBlock': rule.get('CidrBlock'),
+                    'Egress': rule.get('Egress'),
+                    'Protocol': rule.get('Protocol'),
+                    'RuleAction': rule.get('RuleAction'),
+                    'RuleNumber': rule.get('RuleNumber'),
                    # Add pointer back to the nacl to create an edge


switching to get comes with the question: what's the default value? And what's the implication of having such broken / default data in our DB?

I think the data model for this is defined here:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-networkaclentry.html

CIDR block is indicated as "Conditional", and it looks like either this field or Ipv6CidrBlock must be present. So we should update our code here to take one or another instead.

Probably worth reviewing the rest of the parameters too.

I wouldn't consider this data as "broken". We trust the incoming data from the AWS ACLs API, considering the optional fields. Any required transformations or enrichments can be performed ad hoc by the client consuming this data.

Having said that, not sure if a validator / enrichment function for to consider the edge cases (like CidrBlock not being present) will actually be valuable here. 🤔
WDYT?

I think the question here is whether cartography should provide an accurate representation of the AWS data and its constraints.

If so, then we should adhere to what @kledo-lyft and provide some kind of validation to spot the cases where we don't get any of the two values. If not, then it's fine to just store the data as is and let the cartography consumers think about this.

I think the first option is better. Even though it can mean more maintenance work, consumers can be sure the data is as accurate as possible to AWS and it will also make easier to spot any bug when the data we're storing doesn't follow these constraints.

@cmm-lyft Sorry for the crash and thanks for working on this. It's annoying that AWS APIs are inconsistent and sometimes leave out entire fields.

I think the data model for this is defined here:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-networkaclentry.html

CIDR block is indicated as "Conditional", and it looks like either this field or Ipv6CidrBlock must be present. So we should update our code here to take one or another instead.

@kledo-lyft - good catch. I was referring to https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_NetworkAclEntry.html when I wrote this code. It looks like the cloudformation docs are better.

This fix is simple:

do a .get('CidrBlock').

do a .get('Ipv6CidrBlock').

leave everything else the same (unless we know there are others where the API leaves it out).

update

cartography/cartography/models/aws/ec2/network_acl_rules.py

Line 23 in 774d67b

cidrblock: PropertyRef = PropertyRef('CidrBlock')

to have an ipv6cidrblock field

This works because the neo4j python driver treats Nones as if the field does not even exist, so assuming that the aws conditional is correct, our end result is that the node written to the graph will have either cidrblock or ipv6cidrblock but not both.

As seen here, cidrblock and ipv6cidrblock are both not id fields so it's ok for these to be None.

cartography/cartography/models/aws/ec2/network_acl_rules.py

Line 23 in 774d67b

cidrblock: PropertyRef = PropertyRef('CidrBlock')

achantavy · 2024-12-02T06:22:01Z

cartography/intel/aws/ec2/network_acls.py

+                    'Id': f"{network_acl['NetworkAclId']}/{direction}/{rule.get('RuleNumber')}",
+                    'CidrBlock': rule.get('CidrBlock'),
+                    'Egress': rule.get('Egress'),
+                    'Protocol': rule.get('Protocol'),
+                    'RuleAction': rule.get('RuleAction'),
+                    'RuleNumber': rule.get('RuleNumber'),
                    # Add pointer back to the nacl to create an edge


@cmm-lyft Sorry for the crash and thanks for working on this. It's annoying that AWS APIs are inconsistent and sometimes leave out entire fields.

I think the data model for this is defined here:
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-networkaclentry.html

CIDR block is indicated as "Conditional", and it looks like either this field or Ipv6CidrBlock must be present. So we should update our code here to take one or another instead.

@kledo-lyft - good catch. I was referring to https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_NetworkAclEntry.html when I wrote this code. It looks like the cloudformation docs are better.

This fix is simple:

do a .get('CidrBlock').

do a .get('Ipv6CidrBlock').

leave everything else the same (unless we know there are others where the API leaves it out).

update

cartography/cartography/models/aws/ec2/network_acl_rules.py

Line 23 in 774d67b

cidrblock: PropertyRef = PropertyRef('CidrBlock')

to have an ipv6cidrblock field

This works because the neo4j python driver treats Nones as if the field does not even exist, so assuming that the aws conditional is correct, our end result is that the node written to the graph will have either cidrblock or ipv6cidrblock but not both.

As seen here, cidrblock and ipv6cidrblock are both not id fields so it's ok for these to be None.

cartography/cartography/models/aws/ec2/network_acl_rules.py

Line 23 in 774d67b

cidrblock: PropertyRef = PropertyRef('CidrBlock')

cmm-lyft · 2024-12-02T13:21:18Z

@achantavy done!
Thanks Alex!
#1392 (comment)

kledo-lyft · 2024-12-02T17:23:03Z

cartography/models/aws/ec2/network_acl_rules.py

    cidrblock: PropertyRef = PropertyRef('CidrBlock')
+    Ipv6CidrBlock: PropertyRef = PropertyRef('Ipv6CidrBlock')
    egress: PropertyRef = PropertyRef('Egress')


@cmm-lyft a bit late to comment but probably more consistent to use lowercase ip. If we haven't cut a release, maybe can still do a quick fix?

oh yeah good catch. lol sorry i got too eager with the approval

…ion (cartography-cncf#1392) ### Summary Correcting error: ``` File "/code/venvs/venv/lib/python3.10/site-packages/cartography/intel/aws/ec2/network_acls.py", line 72, in transform_network_acl_data 'CidrBlock': rule['CidrBlock'], KeyError: 'CidrBlock' ``` ### Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: - [ ] Update/add unit or integration tests. - [ ] Include a screenshot showing what the graph looked like before and after your changes. - [ ] Include console log trace showing what happened before and after your changes. If you are changing a node or relationship: - [ ] Update the [schema](https://github.com/lyft/cartography/tree/master/docs/root/modules) and [readme](https://github.com/lyft/cartography/blob/master/docs/schema/README.md). If you are implementing a new intel module: - [ ] Use the NodeSchema [data model](https://cartography-cncf.github.io/cartography/dev/writing-intel-modules.html#defining-a-node). Signed-off-by: cmm-lyft <[email protected]>

### Summary Fixing typo #1392 (comment) ### Related issues or links > Include links to relevant issues or other pages. - https://github.com/lyft/cartography/issues/... ### Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: - [ ] Update/add unit or integration tests. - [ ] Include a screenshot showing what the graph looked like before and after your changes. - [ ] Include console log trace showing what happened before and after your changes. If you are changing a node or relationship: - [ ] Update the [schema](https://github.com/lyft/cartography/tree/master/docs/root/modules) and [readme](https://github.com/lyft/cartography/blob/master/docs/schema/README.md). If you are implementing a new intel module: - [ ] Use the NodeSchema [data model](https://cartography-cncf.github.io/cartography/dev/writing-intel-modules.html#defining-a-node). ---------

kledo-lyft reviewed Nov 27, 2024

View reviewed changes

cmm-lyft requested a review from kledo-lyft November 28, 2024 14:14

achantavy requested changes Dec 2, 2024

View reviewed changes

Applying feedback

6ede90a

cmm-lyft force-pushed the fix_key_error branch from b5ba758 to 6ede90a Compare December 2, 2024 13:20

cmm-lyft requested review from serge-wq and achantavy December 2, 2024 13:20

achantavy approved these changes Dec 2, 2024

View reviewed changes

achantavy changed the title ~~Using get method to access dict keys to prevent exception~~ EC2 network ACL: Use get method to access dict keys to prevent exception Dec 2, 2024

achantavy merged commit 4480b90 into cartography-cncf:master Dec 2, 2024
7 checks passed

kledo-lyft reviewed Dec 2, 2024

View reviewed changes

cmm-lyft mentioned this pull request Dec 4, 2024

Fixing ipv6CidrBlock caps typo #1396

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EC2 network ACL: Use get method to access dict keys to prevent exception #1392

EC2 network ACL: Use get method to access dict keys to prevent exception #1392

cmm-lyft commented Nov 27, 2024

cmm-lyft commented Nov 27, 2024

cmm-lyft commented Nov 27, 2024

kledo-lyft Nov 27, 2024

kledo-lyft Nov 27, 2024

cmm-lyft Nov 27, 2024

cmm-lyft Nov 27, 2024

serge-wq Dec 2, 2024

achantavy Dec 2, 2024 •

edited

Loading

achantavy Dec 2, 2024 •

edited

Loading

cmm-lyft commented Dec 2, 2024

kledo-lyft Dec 2, 2024

achantavy Dec 2, 2024

EC2 network ACL: Use get method to access dict keys to prevent exception #1392

EC2 network ACL: Use get method to access dict keys to prevent exception #1392

Conversation

cmm-lyft commented Nov 27, 2024

Summary

Checklist

cmm-lyft commented Nov 27, 2024

cmm-lyft commented Nov 27, 2024

kledo-lyft Nov 27, 2024

Choose a reason for hiding this comment

kledo-lyft Nov 27, 2024

Choose a reason for hiding this comment

cmm-lyft Nov 27, 2024

Choose a reason for hiding this comment

cmm-lyft Nov 27, 2024

Choose a reason for hiding this comment

serge-wq Dec 2, 2024

Choose a reason for hiding this comment

achantavy Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

achantavy Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

cmm-lyft commented Dec 2, 2024

kledo-lyft Dec 2, 2024

Choose a reason for hiding this comment

achantavy Dec 2, 2024

Choose a reason for hiding this comment

achantavy Dec 2, 2024 •

edited

Loading

achantavy Dec 2, 2024 •

edited

Loading