Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore update pod without new image in alwayspullimages admission controller #96668

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Nov 18, 2020

What type of PR is this?
/kind bug

What this PR does / why we need it:
When AlwaysPullImages is enabled, the annotations/labels/finalizers of existing(old) pod(ImagePullPolicy=IfNotPresent) can not be updated.

The reason is that ImagePullPolicy is immutable container attribute. When we try to update the image of the pod, the ImagePullPolicy will be changed as AlwaysPullImages is enabled. The image change update will failed.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#container-v1-core

Fixed the `AlwaysPullImages` admission plugin to tolerate updates of pods with an imagePullPolicy other than `Always` already set

Which issue(s) this PR fixes:

Fixes #96624

Special notes for your reviewer:
As @bjrara and liggit commented in 96624, This pr tries to ignore update pod when alwayspullimages is enabled.

Another workaround would be that pod need to be recreated after enabling AlwaysPullImages.

The pod will be still ifNotPresent after updating, even if the AlwaysPullImages is enabled.

Does this PR introduce a user-facing change?:

ignore update pod with no new images in alwaysPullImages admission controller

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 18, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Nov 18, 2020

/sig node auth
/triage accepted

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 18, 2020
@liggitt
Copy link
Member

liggitt commented Nov 18, 2020

rather than allowing the imagePullPolicy to be updated, I'd expect the AlwaysPullImages admission plugin to be fixed to ignore/tolerate updates to pods which do not change the container images, even if the pull policy is not Always.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 18, 2020
@pacoxu pacoxu changed the title add imagePullPolicy update support for AlwaysPullImages admission controller ignore update pod in alwayspullimages admission controller Nov 18, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Nov 18, 2020

rather than allowing the imagePullPolicy to be updated, I'd expect the AlwaysPullImages admission plugin to be fixed to ignore/tolerate updates to pods which do not change the container images, even if the pull policy is not Always.

changed to the other solution, ignore update pod in alwayspullimages.

@pacoxu

This comment has been minimized.

@liggitt
Copy link
Member

liggitt commented Nov 18, 2020

we can ignore update requests if no images change like this:

if attributes.GetOperation() == admission.Update {
  oldImages := sets.NewString()
  pods.VisitContainersWithPath(&oldPod.Spec, field.NewPath("spec"), func(c *api.Container, _ *field.Path) bool {
    oldImages.Insert(c.Image)
    return true
  })
  newImages := sets.NewString()
  pods.VisitContainersWithPath(&newPod.Spec, field.NewPath("spec"), func(c *api.Container, _ *field.Path) bool {
    newImages.Insert(c.Image)
    return true
  })
  if oldImages.HasAll(newImages) {
    // ignore updates that don't change the images referenced by the pod spec
    return nil
  }
}

@zhouhaibing089
Copy link
Contributor

we can ignore update requests if no images change like this:

Could this strategy be applied to other admissions as well? For example:

  1. LimitRanger which might defaults resources.limits on updates.
  2. PodSecurityPolicy which might defaults securityContexts on updates.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 19, 2020
@pacoxu pacoxu force-pushed the fix/96624 branch 2 times, most recently from 1746dc6 to 3c7b4c4 Compare November 19, 2020 08:47
@pacoxu pacoxu force-pushed the fix/96624 branch 4 times, most recently from 0ba45e7 to d7cfbb6 Compare November 24, 2020 09:09
@pacoxu pacoxu requested a review from liggitt November 24, 2020 09:14
@pacoxu pacoxu changed the title ignore update pod in alwayspullimages admission controller ignore update pod without new image in alwayspullimages admission controller Nov 25, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Nov 26, 2020

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 26, 2020
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, looks good, just a couple stylistic comments

go ahead and squash to a single commit as well

plugin/pkg/admission/alwayspullimages/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/alwayspullimages/admission.go Outdated Show resolved Hide resolved
…ced by the pod spec

Signed-off-by: pacoxu <paco.xu@daocloud.io>
@pacoxu
Copy link
Member Author

pacoxu commented Nov 30, 2020

/retest

@pacoxu
Copy link
Member Author

pacoxu commented Dec 1, 2020

squashed and fixed

@liggitt
Copy link
Member

liggitt commented Dec 1, 2020

/priority backlog
/lgtm
/approve
/milestone v1.21

@k8s-ci-robot k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Dec 1, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Dec 1, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 1, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pacoxu

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020
@@ -101,12 +103,49 @@ func (*AlwaysPullImages) Validate(ctx context.Context, attributes admission.Attr
return nil
}

// check if it's update and it doesn't change the images referenced by the pod spec
func isUpdateWithNoNewImages(attributes admission.Attributes) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this could be extracted as a utility function and then can be shared in other admissions. :)

@k8s-ci-robot k8s-ci-robot merged commit af21206 into kubernetes:master Dec 9, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Dec 9, 2020

Could this strategy be applied to other admissions as well? For example:

LimitRanger which might defaults resources.limits on updates.
PodSecurityPolicy which might defaults securityContexts on updates.

@zhouhaibing089
I will extract isUpdateWithNoNewImages as util and check whether it can add to plugin PodSecurityPolicy and LimitRanger recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling AlwaysPullImages admission causes existing Pods cannot be updated
5 participants