🐛: Tags defined in subnet spec should be applied #5175
Conversation
k8s-ci-robot
added
release-note
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
needs-priority
size/L
| if existing, err = s.describeVpcSubnets(); err != nil { | ||
| return err | ||
| } | ||
| // Describe subnets in the vpc. |
There was a problem hiding this comment.
We had two blocks to describe the subnets in the VPC. One was executed when the feature flag for tagging unmanaged network resources was enabled, and the other block was executed when the feature flag was disabled. So it was always executed anyway.
| // Make sure tags are up-to-date. | ||
| subnetTags := sub.Tags | ||
|
|
There was a problem hiding this comment.
This is the key change that fixes the issue. By storing the subnet tags from the spec to a local variable, we won't overwrite them when using DeepCopyInto.
There was a problem hiding this comment.
Hi @fiunchinho - is this behavior considering managed and unmanaged scenarios?
Checking the documentation about BYO/unmanaged and tags, it states:
Note: All the tagging of resources should be the responsibility of the users and are not managed by CAPA controllers.
My understanding is this block will be reached at once when unmanaged (existingSubnets!=nil), and later interactions in the reconcile loop when managed (with subnets created here , preserving subnets tags defined on spec), considering the documentation that unmanaged should not create additional tags, than required from k8s, the tags from spec should be skipped. Is it make sense? (+ @richardcase @nrb would love to hear your thoughts too)
There was a problem hiding this comment.
hey @mtulio thanks for the comment. If I understand it correctly, with this approach, when reconciling an unmanaged vpc, the tags in the subnet on AWS are applied to the AWSCluster spec, and new tags in the AWSCluster spec are ignored/never applied into AWS. When reconciling a managed VPC, the tags in the AWSCluster spec are applied to the subnet on AWS.
In the subnets.go file, line 626, we only take into consideration the tags defined in the AWSCluster spec (called manualTags in the function) if the vpc is managed.
So I think this approach is not going against current documentation of BYOI/unmanaged infra.
Also, I think finding the subnet in AWS (existingSubnets!=nil) doesn't mean it's unmanaged. When CAPA creates the subnets, on following reconciliations it will find the same subnets on AWS, and they are managed by CAPA. A managed VPC is defined as a VPC with an id set, and the sigs.k8s.io/cluster-api-provider-aws/cluster/$clusterName set.
There was a problem hiding this comment.
hey @mtulio thanks for the comment. If I understand it correctly, with this approach, when reconciling an unmanaged vpc, the tags in the subnet on AWS are applied to the AWSCluster spec, and new tags in the AWSCluster spec are ignored/never applied into AWS. [...]
Yep, that's the current behavior, isn't it aligned with the documentation for BYOI/unmanaged infra?
The change wasn't intentionally added, but revisiting the docs with your findings I am a little bit confused what would be the expected behavior for unmanaged on CAPA. @richardcase @nrb thoughts?
Just sharing some thoughts how we are using: we, on OpenShift, consider tags are managed by the user when they opt to provide their own network/subnets, and we don't touch on it when installing a cluster. So this change won't affect our automation as we are not setting the subnet tags for unmanaged VPC.
There was a problem hiding this comment.
For unmanaged, I'd say:
- Only apply required k8s tags to the subnets in AWS
- No other custom tags to be added to the subnets in AWS
As for syncing existing tags on the subnets back to AWSCluster, I'm not strongly opinionated on this.
Every time i look at the subnets code, it makes me 😢 IMHO, it's historically one of the most error-prone and horrible bits of code in CAPA.
There was a problem hiding this comment.
getSubnetTagParams still does the right thing for unmanaged VPCs (= don't use additional tags unless TagUnmanagedNetworkResources()), so I think the behavior is fine even with this PR. If tags are directly defined on AWSCluster.spec.network.subnets.tags, it's a conscious decision by the admin and those tags should be added even for unmanaged ones – why else have a field for tags?
The field description could be more precise, though (out of scope for the PR):
$ kubectl explain awscluster.spec.network.subnets.tags
GROUP: infrastructure.cluster.x-k8s.io
KIND: AWSCluster
VERSION: v1beta2
FIELD: tags <map[string]string>
DESCRIPTION:
Tags is a collection of tags describing the resource.
|
/test pull-cluster-api-provider-aws-e2e |
|
/cc @mtulio |
fiunchinho
force-pushed
the
fix-tag-subnets
branch
from
c8e12ac
to
c8eb479
Compare
|
/test pull-cluster-api-provider-aws-e2e |
|
@fiunchinho FYI E2E tests are broken until #5133 merges |
Ok, thanks for the heads up. BTW, since this is fixing a bug, shouldn't it be released as patch for v2.6 instead of v2.7? |
|
We should definitely release it in both. |
|
/cherrypick release-2.6 |
|
@damdo: once the present PR merges, I will cherry-pick it on top of release-2.6 in a new PR and assign it to you. In response to this:
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-sigs/prow repository. |
| @@ -2625,6 +2575,183 @@ func TestReconcileSubnets(t *testing.T) { | |||
| }, nil).AnyTimes() | |||
| }, | |||
| }, | |||
| { | |||
| name: "Managed VPC, existing public and private subnets, 2 subnets in spec, custom tags in spec should be created", | |||
There was a problem hiding this comment.
just making sure if I understood the scenario: is "Managed VPC" covers "existing subnets" on CAPA?
There was a problem hiding this comment.
In BYO infra / Configuring the AWSCluster Specification it states:
Specifying existing infrastructure for Cluster API to use takes place in the specification for the AWSCluster object. Specifically, you will need to add an entry with the VPC ID and the IDs of all applicable subnets into the network field.
So, should it be "unmanaged VPC" when you specify the VPC ID and subnet IDs?
There was a problem hiding this comment.
I tried explaining here. Let me know your thoughts, please!
|
/milestone v2.8.0 |
fiunchinho
force-pushed
the
fix-tag-subnets
branch
from
c8eb479
to
635eafb
Compare
|
/test pull-cluster-api-provider-aws-e2e |
| // Make sure tags are up-to-date. | ||
| subnetTags := sub.Tags | ||
|
|
There was a problem hiding this comment.
getSubnetTagParams still does the right thing for unmanaged VPCs (= don't use additional tags unless TagUnmanagedNetworkResources()), so I think the behavior is fine even with this PR. If tags are directly defined on AWSCluster.spec.network.subnets.tags, it's a conscious decision by the admin and those tags should be added even for unmanaged ones – why else have a field for tags?
The field description could be more precise, though (out of scope for the PR):
$ kubectl explain awscluster.spec.network.subnets.tags
GROUP: infrastructure.cluster.x-k8s.io
KIND: AWSCluster
VERSION: v1beta2
FIELD: tags <map[string]string>
DESCRIPTION:
Tags is a collection of tags describing the resource.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Since 2.5.0 release, we can't tag subnets anymore by adding tags to the subnet spec in the
AWSClusterCR. I believe the bug got introduced on this PR, by overwriting the subnet tags defined in the spec by the tags from the AWS Subnet. This means that the existing tags on the AWS Subnet will always be applied, while the tags defined in the subnet spec will be ignored.Checklist:
Release note: