🐛 fix: update network subnets prio tag failures #4917
Conversation
The subnet information from unmanaged subnets, like Availability Zone
name discovered when reconciling subnets, is not updated for additional
subnets when the are failures to tag subnets for the preceding subnets.
For example, given subnetId1 and subnetId2, if the subnet tag failed
when setting tags on subnetId1, the subnetId2 will not be discovered and
attributes not correctly set.
This change enforce subnet updates before trying to apply tags, and
not skip the lopp when checking existing subnets.
Additionally, a new option was added to the unit for
reconcileSubnets(), allowing to ensure expected SubnetSpec attributes
from Subnets{} post reconciliation.
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @mtulio. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Is it flake in /test pull-cluster-api-provider-aws-test |
k8s-ci-robot
added
release-note-none
|
/test pull-cluster-api-provider-aws-e2e |
Sounds like a flake in the cloudformation stack when creating IAM to bootstrap the node:
/test pull-cluster-api-provider-aws-e2e |
mtulio
changed the title
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
k8s-ci-robot
added
the
approved
| // Update subnet spec with the existing subnet details | ||
| existingSubnet.DeepCopyInto(sub) | ||
|
|
||
| // Make sure tags are up-to-date. | ||
| subnetTags := sub.Tags |
There was a problem hiding this comment.
I noticed that tags were not being applied when running CAPA v2.5.2, and I found this PR changing the subnet tagging behaviour.
Shouldn't subnetTags := sub.Tags run before existingSubnet.DeepCopyInto(sub)? Otherwise, we basically ignore the tags specified in the subnet spec, because they are overwritten by the tags found in the AWS Subnet. /cc @mtulio @vincepri @damdo
There was a problem hiding this comment.
Yeah looks that way. Although not looked too deeply into the code.
There was a problem hiding this comment.
Created this to fix it #5175
What type of PR is this?
/kind bug
What this PR does / why we need it:
The subnet information from unmanaged subnets, like the Availability Zone name discovered when reconciling subnets, is not updated for additional subnets when the are failures to tag subnets for the preceding subnets.
For example, given subnetId1 and subnetId2, if the subnet tag failed when setting tags on subnetId1, the subnetId2 will not be discovered and attributes not be correctly set.
This change enforces subnet updates before trying to apply tags and does not skip the loop when checking existing subnets.
Additionally, a new option was added to the unit for reconcileSubnets(), allowing to ensure expected SubnetSpec attributes from Subnets{} post reconciliation.
Which issue(s) this PR fixes
References:
Special notes for your reviewer:
Checklist:
Release note: