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.

Sign up for GitHub

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

Jump to bottom

Add ephemeralcontainer to imagepolicy securityaccount admission plugin #118356

Merged
merged 1 commit into from
Jun 3, 2023
Merged

Add ephemeralcontainer to imagepolicy securityaccount admission plugin #118356

merged 1 commit into from
Jun 3, 2023

Conversation

ritazh
Copy link
Member

@ritazh ritazh commented May 31, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes imagepolicy securityaccount admission plugins to handle ephemeral container.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

/sig auth

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 May 31, 2023
@ritazh
Copy link
Member Author

ritazh commented May 31, 2023

/assign @enj

@ritazh
Copy link
Member Author

ritazh commented May 31, 2023

/retest

aramase
aramase reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@aramase aramase left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added few comments!

plugin/pkg/admission/imagepolicy/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/imagepolicy/admission.go Outdated Show resolved Hide resolved
plugin/pkg/admission/imagepolicy/admission.go
Comment on lines +157 to +163
for _, c := range pod.Spec.EphemeralContainers {
imageReviewContainerSpecs = append(imageReviewContainerSpecs, v1alpha1.ImageReviewContainerSpec{
Image: c.Image,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

If the subresource is ephemeralcontainers, don't we still need the other containers and init containers in addition to the ephermeral containers? Or is it intended to only check the ephemeral containers?

Copy link
Member Author

Choose a reason for hiding this comment

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

a request to subresource=="" can only set initContainer and container images
a request to subresource=="ephemeralcontainers" can only set ephemeralContainer images

plugin/pkg/admission/imagepolicy/admission_test.go Outdated Show resolved Hide resolved
plugin/pkg/admission/imagepolicy/admission_test.go Outdated Show resolved Hide resolved
plugin/pkg/admission/serviceaccount/admission.go Outdated Show resolved Hide resolved
@ritazh
Add ephemeralcontainer to imagepolicy securityaccount admission plugin
d6168bb 
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
@enj
Copy link
Member

enj commented Jun 2, 2023

/lgtm
/approve
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 2, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a4c24036065e65a8d19ce5fda712a5f0c1e2ba49

@ritazh
Copy link
Member Author

ritazh commented Jun 2, 2023

/assign @deads2k

deads2k
deads2k reviewed Jun 3, 2023
View reviewed changes
plugin/pkg/admission/serviceaccount/admission.go
@@ -99,7 +99,7 @@ var _ = genericadmissioninitializer.WantsExternalKubeInformerFactory(&Plugin{})
// 5. If MountServiceAccountToken is true, it adds a VolumeMount with the pod's ServiceAccount's api token secret to containers
func NewServiceAccount() *Plugin {
return &Plugin{
Handler: admission.NewHandler(admission.Create),
Handler: admission.NewHandler(admission.Create, admission.Update),
// TODO: enable this once we've swept secret usage to account for adding secret references to service accounts
LimitSecretReferences: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

blast from the past

@deads2k
Copy link
Contributor

deads2k commented Jun 3, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj, ritazh

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 Jun 3, 2023
@k8s-ci-robot k8s-ci-robot merged commit bb87860 into kubernetes:master Jun 3, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jun 3, 2023
This was referenced Jun 5, 2023
Merged
Merged
@ritazh ritazh mentioned this pull request Jun 5, 2023
Merged
@humblec
Copy link
Contributor

humblec commented Jun 6, 2023

@ritazh don't we need to back port this to 1.24 release branch as well?

@ritazh
Copy link
Member Author

ritazh commented Jun 6, 2023

@ritazh don't we need to back port this to 1.24 release branch as well?

@humblec Not planning to since ephemeral container GA’ed after 1.24 and 1.24 has already entered maintenance mode, and its EOL is 2023-07-28.

@ritazh ritazh mentioned this pull request Jun 6, 2023
Merged
@ritazh
Copy link
Member Author

ritazh commented Jun 6, 2023

@humblec Not planning to since ephemeral container GA’ed after 1.24 and 1.24 has already entered maintenance mode, and its EOL is 2023-07-28.

Looks like we can still cherry pick to 1.24. PR ^

@ritazh ritazh deleted the ec-admission branch June 6, 2023 17:35
k8s-ci-robot added a commit that referenced this pull request Jun 6, 2023
@k8s-ci-robot
Merge pull request #118471 from ritazh/automated-cherry-pick-of-#1183…
3a77d5a 
…56-upstream-release-1.27

Automated cherry pick of #118356: Add ephemeralcontainer to imagepolicy securityaccount
k8s-ci-robot added a commit that referenced this pull request Jun 6, 2023
@k8s-ci-robot
Merge pull request #118473 from ritazh/automated-cherry-pick-of-#1183…
f58aab8 
…56-upstream-release-1.26

Automated cherry pick of #118356: Add ephemeralcontainer to imagepolicy securityaccount
k8s-ci-robot added a commit that referenced this pull request Jun 7, 2023
@k8s-ci-robot
Merge pull request #118474 from ritazh/automated-cherry-pick-of-#1183…
a813061 
…56-upstream-release-1.25

Automated cherry pick of #118356: Add ephemeralcontainer to imagepolicy securityaccount
@humblec
Copy link
Contributor

humblec commented Jun 7, 2023

@humblec Not planning to since ephemeral container GA’ed after 1.24 and 1.24 has already entered maintenance mode, and its EOL is 2023-07-28.

Looks like we can still cherry pick to 1.24. PR ^

Thanks.. Even Beta is a contract and 1.24 is still open . Considering that , I was asking, thanks for filing the PR @ritazh 👍

k8s-ci-robot added a commit that referenced this pull request Jun 7, 2023
@k8s-ci-robot
Merge pull request #118512 from ritazh/automated-cherry-pick-of-#1183…
77a97ce 
…56-upstream-release-1.24

Automated cherry pick of #118356: Add ephemeralcontainer to imagepolicy securityaccount
@ritazh ritazh mentioned this pull request Jun 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@aramase aramase aramase left review comments

@enj enj Awaiting requested review from enj

@krmayankk krmayankk Awaiting requested review from krmayankk

Assignees

@enj enj

@deads2k deads2k

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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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
SIG Auth
Archived in project
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.
6 participants
@ritazh @enj @k8s-ci-robot @deads2k @humblec @aramase