🐛 Don't overwrite subnet spec tags with AWS tags

[fiunchinho]

What type of PR is this?

/kind bug

What this PR does / why we need it:

When trying to update a subnet tag that is already present on AWS to a different value, CAPA keeps reconciling the AWCluster forever, flapping between the different values of the tag. When reconciling the subnet tags this is what happens

• AWSCluster has subnets, let's say subnet-1a2b3c has the tag customtag: 1
• CAPA saves the tags to a local variable
• CAPA overwrites all the subnet fields with the info from AWS, including tags. Let's say that the subnet in AWS contained customtag: 2 (different from the value in the AWSCluster CR)
• CAPA takes the tags that it saved earlier to a local variable and it applies them to the AWS subnet. Now on AWS the subnet has the tag customtag: 1
• At the end of the reconciliation, all the modifications to the AWSCluster CR are saved back to the k8s api. So the tag from AWS customtag: 2 is saved to the AWSCluster CR
• CAPA reconciles again. It applies customtag: 2 to the subnet in AWS (because that's the value in the AWSCluster CR for this reconciliation), and it applies customtag: 1 to the AWSCluster CR (because that's the value in AWS)
• Repeat ad infinitum

With the change on this PR, what we are doing is that the tags already present on AWS are not saved back to the AWSCluster.

Special notes for your reviewer:

Checklist:

• squashed commits
• includes documentation
• includes emoji in title
• adds unit tests
• adds or updates e2e tests

Release note:

Don't overwrite subnet spec tags with tags from the subnet on AWS

[fiunchinho]

Pinging @mtulio as you were interested on the subnet tags in the past 🙂

[richardcase]

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

[mtulio]

Pinging @mtulio as you were interested on the subnet tags in the past 🙂

Hey @fiunchinho , Thanks for pinging! Good catch! Overall looks good to me. Do you think this scenario would be possible to reproduce in the tests?

[AndiDog]

/lgtm

[fiunchinho]

Pinging @mtulio as you were interested on the subnet tags in the past 🙂

Hey @fiunchinho , Thanks for pinging! Good catch! Overall looks good to me. Do you think this scenario would be possible to reproduce in the tests?

I don't think there is any test currently that asserts behavior after running multiple reconciliations. This bug only surfaces when reconciling the same subnet several times. So I'm not really sure if we can easy test this, or if it's even worth it.

[mtulio]

Pinging @mtulio as you were interested on the subnet tags in the past 🙂

Hey @fiunchinho , Thanks for pinging! Good catch! Overall looks good to me. Do you think this scenario would be possible to reproduce in the tests?

I don't think there is any test currently that asserts behavior after running multiple reconciliations. This bug only surfaces when reconciling the same subnet several times. So I'm not really sure if we can easy test this, or if it's even worth it.

/lgtm

[alexander-demicev]

/lgtm

[damdo]

/assign @AndiDog @richardcase @nrb

for approval

[nrb]

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

[fiunchinho]

/retest

[nrb]

/test pull-cluster-api-provider-aws-e2e

[nrb]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nrb

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:

• OWNERS [nrb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment