🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values

[nueavv]

This commit addresses a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The issue was caused by a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.

Fixes #5021

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR resolves a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The error was due to a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.

Which issue(s) this PR fixes:
Fixes #5021

Special notes for your reviewer:

Checklist:

• squashed commits
• includes documentation
• includes emojis
• adds unit tests
• adds or updates e2e tests

Release note:

Fixed a panic error during taint deletion in AWSManagedMachinePool by handling nil values for taint fields.

[k8s-ci-robot]

Hi @nueavv. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

[richardcase]

/ok-to-test

[richardcase]

Thanks for the this @nueavv . When you have time could you respond to @fiunchinho suggestion? Then i think we can look to get this merged.

[nueavv]

Okay. @richardcase

[richardcase]

@nueavv - there's a couple of failures with the checks. If you run the following locally (which is what he checks are running):

• make lint
• make test

Ping me on slack if i can help with anything.

[nueavv]

@richardcase I have fixed the issue. Can you please check if the commit message is appropriate?

[richardcase]

/test ?

[k8s-ci-robot]

@richardcase: The following commands are available to trigger required jobs:

• /test pull-cluster-api-provider-aws-build
• /test pull-cluster-api-provider-aws-build-docker
• /test pull-cluster-api-provider-aws-test
• /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

• /test pull-cluster-api-provider-aws-apidiff-main
• /test pull-cluster-api-provider-aws-e2e
• /test pull-cluster-api-provider-aws-e2e-blocking
• /test pull-cluster-api-provider-aws-e2e-clusterclass
• /test pull-cluster-api-provider-aws-e2e-conformance
• /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
• /test pull-cluster-api-provider-aws-e2e-eks
• /test pull-cluster-api-provider-aws-e2e-eks-gc
• /test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

• pull-cluster-api-provider-aws-apidiff-main
• pull-cluster-api-provider-aws-build
• pull-cluster-api-provider-aws-build-docker
• pull-cluster-api-provider-aws-test
• pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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.

[richardcase]

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

[AndiDog]

Regarding the issue and according release note: I don't quite get whether the issue is about deletion (the taint shouldn't even exist in the slice?!) or an empty value string – let's please clarify. The GitHub issue says it's about an empty string, and it looks like this is not a deletion issue?!

[nueavv]

Hi @AndiDog,

I was trying to delete the taint, but it failed because of the empty string. What made you confused? The issue is about taint deletion.

[nueavv]

You can reproduce it by first creating a taint without a value and then trying to delete it.

[nueavv]

@dlipovetsky Hi! can you review this?

[AndiDog]

The change now looks straightforward. Can you please make this a single commit? I'll take a look very soon – it's on my list 😉.

[AndiDog]

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

[AndiDog]

/approve
/hold

@fiunchinho @dlipovetsky @richardcase does someone have time to review and LGTM? Please /unhold once all is okay.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndiDog

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 [AndiDog]

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

[nueavv]

Hi, @dlipovetsky Could you please review this PR when you have time?

[dlipovetsky]

Thanks for this. The change looks good.

I noticed this file has no unit tests. In my experience, unit tests are especially helpful for functions that transform data. Would you please consider writing a unit test; it should fail without your fix, and pass with it.

[dlipovetsky]

On second thought, this PR has been open for a while, and I don't want to add a new requirement this late. But a unit test would help future maintenance. If it can be done before the /hold is removed, great; otherwise, a follow-up PR would be fine.

/lgtm

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[nueavv]

/retest

[nueavv]

/retest-required

[nueavv]

@dlipovetsky I've added test codes. Can you review them again?

[nueavv]

@dlipovetsky Hi.. Can you take a look at it?