Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#996: AWS Fix KeyError exception when getting DNSName for load_balancer_id #1215

Closed
wants to merge 6 commits into from
Closed

Conversation

aliahmed58
Copy link

Fix: KeyError exception when getting DNSName for load_balancer_id


The type gateway Load Balancer V2 does not have the field DNSName which was being used as id for the LoadBalancerV2 node during sync which resulted in KeyError if the sync loaded a gateway type load balancer.

Solution

The solution is to use the arn as the id for LoadBalancerV2 nodes and set dnsname property for the node only if it exists. If it returns None, then it won't be set in Neo4j.

Changing the id from dnsname to arn also requires to change cartography/intel/aws/ec2/network_interfaces.py since it creates a relationship between (:LoadBalancerV2)-[:NETWORK_INTERFACE]->(:NetworkInterface) which uses id (arn instead of dnsname now) to query the LoadBalancerV2. The arn can be easily generated from the description string of network interface.
The arn is of format:

arn:${Partition}:elasticloadbalancing:${Region}:${Account}:loadbalancer/${Type}/${LoadBalancerName}/${LoadBalancerId}

where the part ${Type}/${LoadBalancerName}/${LoadBalancerId} can be found in the description of the network interface and can be retrieved using split on the description string if it initially matches the regex for load balancer v2.

Sample description strings:

ELB gwy/test-gateway-load-balancer/19219281someid
ELB net/test-load-balancer/2akdfsomeid

The regex matching was only looking for net and app type load balancers but adding gwy makes it also find network interfaces for gateway load balancers and forms their relationships.
This also fixes the currently unidentified problem that cartography did not make a relationship for gateway load balancers with their network interfaces since the regex never matched gwy.

@achantavy
Copy link
Contributor

@aliahmed-58 - thanks for the write-up and the PR. The tests are currently failing with

FAILED tests/integration/cartography/intel/aws/test_route53.py::test_load_dnspointsto_ec2_relationships - KeyError: 'LoadBalancerArn'
FAILED tests/integration/cartography/intel/aws/ec2/test_ec2_load_balancers.py::test_load_load_balancer_v2s - KeyError: 'LoadBalancerArn'

so you might need to update them.

It definitely would be helpful to also include gateway types: can you please include a gateway type in the test data and write a test that verifies that it is loaded to the graph correctly? Write back if you need help with this.

Please also sign the CLA.

As a side note, we're currently working on #1024 to standardize AWS node IDs to use ARNs, and this will involve using our new data model which will make things cleaner and less error prone. Happy to share more here if you're interested too.

@aliahmed58
Copy link
Author

aliahmed58 commented Jul 15, 2023

@achantavy I have updated the tests to include the LoadBalancerArn field in test data. Could you guide me on how to include another type of load balancer in the test data? Do I just create a dict items in the list of test data?

Also, the mypy test keeps failing, I would be grateful if you could help me in that?

Yes I would be interested to look into https://github.com/lyft/cartography/issues/1024. Is there any communication channel where I can ask around (slack / discord etc) ? Thanks

@achantavy
Copy link
Contributor

@aliahmed58

Do I just create a dict items in the list of test data?

Yup!

Also, the mypy test keeps failing, I would be grateful if you could help me in that?

On your local machine, make sure that you have done pip install test-requirements.txt (probably want to do this in your virtualenv) and then run make test_lint so that you don't have to push code up to know if the linter will complain.

About Slack and other places to hang out, see https://github.com/lyft/cartography#community. We also have a community meeting coming up on 7/27 - please come if you can, would like to learn how you're using the tool.

@aliahmed58
Copy link
Author

aliahmed58 commented Jul 16, 2023

@achantavy I have written the test for type gateway load balancer. It works even when there's no DNSName in the field.

I couldn't edit the original test method since it would take a lot of changes to test for another load balancer in test_load_load_balancers_v2 since it only works with a specific load_balancer_id. Please let me know if this is okay or should I use two assert statements in the same method.

PS. Thank you for the links

@achantavy
Copy link
Contributor

Hi @aliahmed58 - let's actually pause work on this until...

I've created #1222 to track the problem you mentioned though. I'll write back if I need help on the refactors but at this point I think it's faster if I just power through some of these and then share the examples afterward. If you have cycles at that point to help let me know :)

@achantavy
Copy link
Contributor

Closing this out for now since we're aiming to solve a lot of this in #1219, and I've created #1222 to track supporting LoadBalancers of type gateway

@achantavy achantavy closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants