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

AWSmappingfix-ELBv1&v2<--ENI #1331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

udm17
Copy link
Contributor

@udm17 udm17 commented Jul 11, 2024

Some testing lead me to find that the mapping between Loadbalancer V1 and V2 was not happening correctly and in response to that, I delved into the code for the mapping.

  1. The regex used to distinguish between V1 and V2 had the variable assignment inverted ie v1's were getting assigned to elbv2_id attribute and vice versa

  2. Creating a dynamic dnsname and then matching for LoadbalancerV2 was problematic as well since the dynamically created names were always partial matches for the dnsname. The elb_match[2] attribute was never right to match to the correct node. (the regex returns the id of the loadbalancer while the dnsname always has a different value)

created using carto -> elb_match[1]-elb_match[2].elb.region.amazonaws.com
actual dnsname -> elb_match[1]-elb_match[2]-9574787.region.elb.amazonaws.com

Fix ->

  1. Swapping about the variables lead to successful mapping between LBv1 and the Network interfaces

  2. For LBV2, rather than create a dns name, I suggest using a combination of name and region for the loadbalancerv2 as the combination needs to be unique across the AWS account.

@chandanchowdhury
Copy link
Collaborator

Hey @udm17, thank you for the PR :)

Does #996 explain/fix the issue you found?

If not, do you mind opening an Issue and attach that to this PR (to follow the process being established by Ramon via #1157).

@chandanchowdhury chandanchowdhury added bug Something isn't working AWS Related to cartography's AWS module and removed bug Something isn't working labels Jul 16, 2024
@achantavy
Copy link
Contributor

@udm17, thanks for digging and for the PR. It's been a while since I've looked at the ELB code.

It'd be ideal to write a test that catches this problem, but I also realize this is an old-ish part of the code where it'd probably be faster for me to add those than someone else at this time.

So, in place of that, would you be able to please post screenshots of what the relevant nodes and rels looked like before your change and after your change? This way it's more obvious to see the bug.

@achantavy
Copy link
Contributor

created using carto -> elb_match[1]-elb_match[2].elb.region.amazonaws.com
actual dnsname -> elb_match[1]-elb_match[2]-9574787.region.elb.amazonaws.com

@udm17 - I'm having some trouble reproducing what you described . I looked in my environment and see a load balancer and the dns name does have the shape elb.region.amazonaws.com:

image

Cartography also creates a node with that same name:
image

The node label here is a LoadBalancerV2 and I verified in my env that it is an NLB which is as intended. I can't see any incorrectly connected V1s or V2s, but would love to know how you found those in your graph, and I definitely want to help fix this.

Please write back with the answers to my questions earlier and I can take a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AWS Related to cartography's AWS module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants