PodTolerationRestriction: ignore duplicates when changing pod toleration

[pacoxu]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Reproduce steps

1. make sure podtolerationrestriction is not enable at first.
2. create a daemonset A with toleration {"operator":"Exists","effect":"NoSchedule"},, and pod A-1 was created
3. enable podtolerationrestriction in all apiserver
4. try to update the pod annotation

"reason": "FieldvalueForbidden", "message" :"Forbidden: existing toleration can not be modified except tolerationSeconds",

Workarounds

1. just remove the pod to trigger a recreation

Which issue(s) this PR fixes:

Fixes #111745

Special notes for your reviewer:

See more in the issue reproduce steps and logics.

Does this PR introduce a user-facing change?

podtolerationrestriction: don't merge pod tolerations when existed tolerations already dups

[pacoxu]

		mergedOldTolerations := tolerations.MergeTolerations(pod.Spec.Tolerations, nil)
		// if mergedOldTolerations len is different with pod.Spec.Tolerations, it means there are already some dups.
		if len(mergedOldTolerations) != len(pod.Spec.Tolerations) {
			mergedNewTolerations := tolerations.MergeTolerations(mergedOldTolerations, extraTolerations)
			// skip if no change
			if len(mergedNewTolerations) > len(mergedOldTolerations) {
				pod.Spec.Tolerations = append(pod.Spec.Tolerations, extraTolerations...)
			}
		} else {
			// if there is no dup, we can keep the old logic
			mergedNewTolerations := tolerations.MergeTolerations(pod.Spec.Tolerations, extraTolerations)
			pod.Spec.Tolerations = mergedNewTolerations
		}

[pacoxu]

/cc @KevinWang15 @tallclair
/assign @liggitt
/sig api-machinery

[k8s-ci-robot]

@pacoxu: GitHub didn't allow me to request PR reviews from the following users: KevinWang15.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @KevinWang15 @tallclair
/assign @liggitt
/sig api-machinery

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pacoxu
Once this PR has been reviewed and has the lgtm label, please ask for approval from liggitt by writing /assign @liggitt in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• plugin/pkg/admission/OWNERS

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

[KevinWang15]

Given that my fix in #97850 will need to move MergeTolerations to another pkg, and that it might be considered as an api-change, I think it's okay to fix things this way, and close my PR. As long as the bug is fixed and the code owners agree on the method of the fix.

[pacoxu]

Given that my fix in #97850 will need to move MergeTolerations to another pkg and that it might be considered as an api-change, I think it's okay to fix things this way and close my PR as long as the bug is fixed and the code owners agree on the method of the fix.

We tried in different ways. You fix it on the validation side, and I wanted to fix it in the admission plugin.

[fedebongio]

/triage accepted

[pacoxu]

/priority important-longterm
This is not expected that users have to remove old pods after the apiserver configuration is changed.

[pacoxu]

Some users met this in their product cluster after enabling PodTolerationRestriction.
/cc @deads2k @derekwaynecarr
Would you take a look?

[pacoxu]

I am not sure who is the feature owner of PodTolerationRestriction.
Can someone confirm if this works as designed or a bug?
@liggitt (sorry for disturb)

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale
ACK

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale
/retest

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale
/check-cla

[pacoxu]

/easycla

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale

[googs1025]

I haven't studied the logic carefully, please correct me if I'm wrong. Intuitively, this call looks a bit strange. Can we modify the logic instead of calling the same method repeatedly with different parameters? This may make it difficult to understand the code.