feat: AWS EC2 route table initial support#1530
Conversation
There was a problem hiding this comment.
mrge found 9 issues across 8 files. View them in mrge.io
There was a problem hiding this comment.
mrge found 7 issues across 8 files. View them in mrge.io
### Summary > Describe your changes. Adds integ tests for AWS VPC and IGW to unblock #1530. Paying back some debt. ### Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: - [x] Update/add unit or integration tests. --------- Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
Signed-off-by: Alex Chantavy <alex@subimage.io>
jychp
left a comment
There was a problem hiding this comment.
General feedback:
Overall, the code looks correct and functional.
Design question:
Do we really want to have a RouteTableAssociationSchema node? It seems to be a 1-to-1 relation between the parent and child nodes. Is this node truly necessary, or could it be replaced by a direct relationship between RouteTable and EC2Subnet (or other relevant nodes)?
| route table for the VPC | ||
| """ | ||
| transformed = [] | ||
| is_main = False |
There was a problem hiding this comment.
Opinionated and non-blocking comment:
Having is_main default to false and targetdefault to"main"can be a bit confusing. I would have leftis_maindefaulting to false and settarget = "main"` inside the else block (since there's an else block, target will always be defined, so it's not an issue).
| parts.append(route[key]) | ||
| target = route[key] | ||
| break | ||
|
|
There was a problem hiding this comment.
May be we can add a debug log here if the route target key is not found, that would help in the future if new target are added.
| target = route[key] | ||
| break | ||
|
|
||
| return '|'.join(parts), target |
There was a problem hiding this comment.
Not sure if this is an actual issue, but since the function's goal is to generate a unique ID, could there be a scenario where two routes within the same route_table_id both point to a KEY that's not defined in the array? In that case, the function would return something like route_table_id,None for both, which wouldn't be unique.
There was a problem hiding this comment.
after further reading it seems there is no impact
There was a problem hiding this comment.
Right, this collision would happen if a new route target key is introduced by AWS that we don't support. I'll update so that we log a warning.
Otherwise though, the route data structure is like a 'union' data structure where it is designed so that only one of those target keys will be present.
Good catch. Yeah, I struggled here a bit. I'm pretty new to AWS networking so I started out just by doing a naive object by object model. I think you're right: association to subnet / gateway is 1-1, and I think the design would be better to remove the intermediary |
|
Oh, there is a reason why I did it:
These cases seem complex and not straightforward so I figured the simplest way to avoid future misunderstanding was to just model it API object by API object. I'm open to thoughts here though on a smarter way. |
|
I think both approaches are valid, but there should be clear guidance at the project level. After a bit of thought, here’s my take:
This way, we preserve the maximum amount of information, keep the tool easy to adopt for anyone (e.g., if I come from AWS, I don’t need to learn a new data model—it’s the same as AWS), and still allow for cross-provider intelligence through higher-level abstractions, if needed. |
Agree |
|
Will take an action to add that to intel module documentation |
Signed-off-by: Alex Chantavy <alex@subimage.io>
|
@krisek - we finally got around to it; thought you'd find this interesting :) should be in 0.102.0rc2 |
Summary
Adds initial support for AWS EC2 route tables, routes, route associations, and subnet / gateway targets.
Schema diagram of what this PR accomplishes:
These can be resolved in a fast follow. For now, I think this is a decent chunk ready for review.
Related issues or links
N/A
Checklist
Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:
I have run this locally e2e and it works.
If you are changing a node or relationship:
If you are implementing a new intel module: