Skip to content

Prevent incorrect change in routeTable CR spec after adding route addition having vpcEndpointID #170

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Prevent incorrect change in routeTable CR spec after adding route addition having vpcEndpointID #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ee5632
Route addition in routeTable changes CR spec
Dec 15, 2023
7eed50d
Merge branch 'main' into routeTableFix
a-hilaly Feb 29, 2024
7c29604
Merge branch 'main' into routeTableFix
nnbu Sep 16, 2024
ec0c1e1
Route addition in routeTable changes CR spec
Dec 15, 2023
7a9d556
Route addition in routeTable changes CR spec
Dec 15, 2023
50e122d
Route addition in routeTable changes CR spec
Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apis/v1alpha1/ack-generate-metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
ack_generate_info:
build_date: "2024-09-12T18:02:26Z"
build_date: "2024-09-17T17:53:28Z"
build_hash: f8f98563404066ac3340db0a049d2e530e5c51cc
go_version: go1.22.6
version: v0.38.1
Expand Down
14 changes: 14 additions & 0 deletions pkg/resource/route_table/sdk.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 14 additions & 1 deletion templates/hooks/route_table/sdk_read_many_post_set_output.go.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,17 @@
// if resource's initial tags and response tags are equal,
// then assign resource's tags to maintain tag order
ko.Spec.Tags = r.ko.Spec.Tags
}
}

// Even if route is created with arguments as VPCEndpointID,
// when aws api is called to describe the route (inside skdFind), it
// returns VPCEndpointID as GatewayID. Due to this bug, spec section for
// routes is populated incorrectly in above auto-gen code.
// To solve this, if 'GatewayID' has prefix 'vpce-', then the entry is
// moved from 'GatewayID' to 'VPCEndpointID'.
Comment on lines +12 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @LikithaVemulapalli do you know how is this happening from the ec2 side? i'd prefer having EC2 team fix this bug than ec2-controller implementing a workaround (I don't think we'll be notified if/when they change this behaviour, which can cause more confusion for the users).
/hold

Copy link
Contributor Author

@nnbu nnbu Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LikithaVemulapalli FYI
aws-controllers-k8s/community#1935 (comment) provides the details about this issue being present when route is created with vpc-endpoint-id
aws-controllers-k8s/community#1935 (comment) provides the details about this issue not being present when route is created with nat-gateway-id

@a-hilaly Currently, routeTable CR is unusable when it has VPCEndpointID. aws-controllers-k8s/community#1935 mentions the issue with update of the CR only. But I realized later that this issue exists even in creation of the CR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this @LikithaVemulapalli ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @LikithaVemulapalli do you know how is this happening from the ec2 side? i'd prefer having EC2 team fix this bug than ec2-controller implementing a workaround (I don't think we'll be notified if/when they change this behaviour, which can cause more confusion for the users). /hold

Hello @a-hilaly @nnbu apologies for late response I was on vacation and started working a week ago. I'm not sure how this is happening from the ec2 side and I haven't worked on the route table create/update code as well, I agree with Amine and would prefer EC2 team fix this! Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LikithaVemulapalli
Is there any timeline for getting this fixed by EC2 team?

@a-hilaly
If not by this PR, how can we address the issue until the fix comes from EC2 team? Currently, routeTable CR is unusable when it has VPCEndpointID.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright @nnbu - just ok-to-test this PR, i'll do one more round of reviews and merge ASAP. Thank you for all your contributions :)

for i, route := range ko.Spec.Routes {
if route.GatewayID != nil && strings.HasPrefix(*route.GatewayID, "vpce-") {
ko.Spec.Routes[i].VPCEndpointID = route.GatewayID
ko.Spec.Routes[i].GatewayID = nil
}
}