Prevent incorrect change in routeTable CR spec after adding route addition having vpcEndpointID #170
Conversation
ack-prow
bot
added
the
needs-ok-to-test
|
Hi @nnbu. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing. Details explained in the issue description.
| // 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'. |
There was a problem hiding this comment.
/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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Any update on this @LikithaVemulapalli ?
There was a problem hiding this comment.
/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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
ack-prow
bot
added
the
do-not-merge/hold
|
/ok-to-test |
ack-prow
bot
added
ok-to-test
|
/test all |
|
@a-hilaly This was approved but |
This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing. Details explained in the issue description.
This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing. Details explained in the issue description.
This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing. Details explained in the issue description.
|
@nnbu: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
ack-prow
bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, nnbu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ition having vpcEndpointID (aws-controllers-k8s#170) Fixes [#1935](aws-controllers-k8s/community#1935) Description of changes: This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing. Details explained in the issue description. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Fixes #1935
Description of changes:
This fix accomodates an aws api bug so as to prevent routeTable CR spec from changing.
Details explained in the issue description.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.