Use only one of ID or Name while updating Nodegroup

[sudohikumar]

Issue #, if available:

• EKS Nodegroup Includes both LaunchTemplate.ID and LaunchTemplate.Name when updating LaunchTemplate.Version community#2645

Description of changes:
Use only one of ID or Name while updating Nodegroup. ID is the priority here.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Yes

[ack-prow]

Hi @sudohikumar. Thanks for your PR.

I'm waiting for a aws-controllers-k8s 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/test-infra repository.

[a-hilaly]

Thank you @sudohikumar !
/ok-to-test

[ack-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, sudohikumar

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 [a-hilaly]

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

[knottnt]

@sudohikumar Thank you for very much for another contribution!

[knottnt]

I think we'll also need to cover the case where a user has changed the desired state for LaunchTemplate.ID and/or LaunchTemplate.Name. While it will lead to an error from the AWS API I think it would be best to attempt the requested change and present API error to the user. As is, if LaunchTemplate.Name is changed in the Spec populate the current LaunchTemplate.ID and silently ignore the change to the desired state.

cc: @a-hilaly thoughts on this?

[knottnt]

/hold

Until this comment has been resolved.

[sudohikumar]

From one of the other testing, it seems while creation, when we set both id and name of the Launch Template, it errors out with same message InvalidParameterException: Either provide launch template ID or launch template name in the request..

[knottnt]

From one of the other testing, it seems while creation, when we set both id and name of the Launch Template, it errors out with same message InvalidParameterException: Either provide launch template ID or launch template name in the request..

IMO, I think it's fine to surface that error when the user creates a new Nodegroup with both initially set. I'd rather the user see an error than silently ignore their input and take an action they didn't intend. For example, if someone accidentally sets a LaunchTemplate.ID and LaunchTemplate.Name pair that are mismatched and point to different templates. I don't want that error to not get surfaced because we chose to always give one priority over the other when making API calls.

The reason we need to make a priority call when no deltas are detected with either LaunchTemplate.ID and/or LaunchTemplate.Name is that the controller will set a value based on the return of the CreateNodegroup API call so both will be present in the update path. It's safe to make the priority call if there aren't any deltas because we know that the user hasn't made any changes to the manifest that differ from the state in AWS.

[sudohikumar]

Hi @knottnt
Thank you for the inputs. It makes sense.
I have reverted that change now.

[ack-prow]

@sudohikumar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
eks-verify-attribution	026cb7f	link	false	/test eks-verify-attribution

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[knottnt]

Hi @knottnt Thank you for the inputs. It makes sense. I have reverted that change now.

@sudohikumar Thanks for reverting that. I think this comment still needs to be handled. The reasoning is similar to why the create change was reverted. If the user makes a change we don't want to silently ignore it.