Fix Egress Only Internet Gateway lookup when there are multiple in the account#18104
Fix Egress Only Internet Gateway lookup when there are multiple in the account#18104rlees85 wants to merge 1 commit intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @rlees85. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
other Egress Only Internet Gateways exist in the AWS account Signed-off-by: Rich <git0@bitservices.io>
|
|
||
| func findEgressOnlyInternetGateway(ctx context.Context, cloud awsup.AWSCloud, request *ec2.DescribeEgressOnlyInternetGatewaysInput) (*ec2types.EgressOnlyInternetGateway, error) { | ||
| func findEgressOnlyInternetGateway(ctx context.Context, cloud awsup.AWSCloud, request *ec2.DescribeEgressOnlyInternetGatewaysInput, vpcId string) (*ec2types.EgressOnlyInternetGateway, error) { | ||
| response, err := cloud.EC2().DescribeEgressOnlyInternetGateways(ctx, request) |
There was a problem hiding this comment.
The gateway should be tagged with "kubernetes.io/cluster/<cluster-name>" = "shared", I believe that is the expectation for any kOps resource. Filters should be able to work with it with tag-key:
tag-key - The key of a tag assigned to the resource. Use this filter to find all resources assigned a tag with a specific key, regardless of the tag value.
There was a problem hiding this comment.
I will experiment with that way of doing it. I know you can get away without tagging a lot of resources (internet gateway, possibly others) but if the intention is that all resources (not just VPC, subnet) are tagged I will see if I can make it work by looking up with the tag name
There was a problem hiding this comment.
The intention was to have everything tagged, but AWS took a while to add tags and filters to everything...
There was a problem hiding this comment.
Ah fair enough. Looking through the docs and code it looks like KOPS will add the tags for you "kubernetes.io/cluster/<cluster-name>" = "shared" if they do not already exist. It just so happens I use Terraform to pre-tag to stop endless diffs.
So having to "pre-tag" the egress only Internet gateway would be a significant change in behaviour to how VPC, subnet, everything else is currently handled.
If we don't "pre-tag" I am still not sure how Kops is going to find the correct egress only Internet gateway without either a manual VPC filter (as per this PR) or having to specify the egress only Internet gateway ID in the KOPS manifest (and command line parameter for those that do not use KOPS with a manifest).
3 participants
This is a possible fix for: #18088
With this approach, we still do not need to specify the Egress Only Internet Gateway ID in existing, shared VPCs which I think is probably more desirable than having to declare it.
make testseems happy with it. I've removed theattachment.vpc-idfrom the mocks because the AWS API does not support it so the mocks should also not support it.I'm not a developer and my domain knowledge of KOPS is rusty to say the least so any optimisations to what has been submitted is welcome.