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
Disassociate secondary CIDR after subnets are deleted #3347
Disassociate secondary CIDR after subnets are deleted #3347
Conversation
@Ankitasw: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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. |
/test pull-cluster-api-provider-aws-e2e-eks |
LGTM. Wondering if you confirmed without this fix, the new e2e changes fail? So that we will know that the test is actually catching the current bug. |
87aaa13
to
5db42cb
Compare
I will confirm it locally. |
5db42cb
to
37f0b68
Compare
/test pull-cluster-api-provider-aws-e2e-eks |
/hold |
@sedefsavas confirmed below error with existing code if secondary CIDR block is provided:
Also added necessary permissions to controllers policy. |
/unhold |
37f0b68
to
67b5738
Compare
This PR is ready for review. |
Since Secondary CIDR is only supported in EKS, we should not add the permissions for associate/disassociate VPC CIDRs when EKS is disabled in the clusterawsadm config file. |
@sedefsavas will make the desired change. |
67b5738
to
593eb63
Compare
/test pull-cluster-api-provider-aws-e2e-eks |
Made the changes, ready for review again. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sedefsavas 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 |
/retest |
593eb63
to
f6a4928
Compare
@sedefsavas the test was failing as new fixture with_s3_bucket.yaml was added in main branch and didn't have the vpc cidr permissions, added those permissions now. |
/lgtm |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes following:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #3277
Fixes #3273
Checklist:
Release note: