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

ImagePolicyWebhook Admission Controller #30631

Merged
merged 1 commit into from
Aug 20, 2016
Merged

ImagePolicyWebhook Admission Controller #30631

merged 1 commit into from
Aug 20, 2016

Conversation

ecordell
Copy link

@ecordell ecordell commented Aug 15, 2016 •
edited
Loading

What this PR does / why we need it: This is an implementation of the image provenance proposal. It also includes the API definitions by @Q-Lee from #30241

Special notes for your reviewer:
Please note that this is the first admission controller to make use of the admission controller config file (--admission-controller-config-file). I have defined a format for it but we may want to double check it's adequate for future use cases as well.

The format defined is:

{
  "imagePolicy": {
     "kubeConfigFile": "path/to/kubeconfig/for/backend",
     "allowTTL": 50,          # time in s to cache approval
     "denyTTL": 50,           # time in s to cache denial
     "retryBackoff": 500,      # time in ms to wait between retries
     "defaultAllow": true      # determines behavior if the webhook backend fails
  }
}

(or yaml)

Release note:

Adding ImagePolicyWebhook admission controller.

This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

5 similar comments
@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 15, 2016
@smarterclayton smarterclayton assigned deads2k and unassigned bgrant0607 Aug 15, 2016
@ericchiang
Copy link
Contributor

@k8s-bot ok to test

@deads2k deads2k mentioned this pull request Aug 15, 2016
Merged
@ericchiang
Copy link
Contributor

cc @kubernetes/sig-auth @erictune @smarterclayton @fabioy @philips

ericchiang
ericchiang reviewed Aug 15, 2016
View reviewed changes
plugin/pkg/admission/imagepolicy/admission.go
}
json.Unmarshal(raw, review)

// TODO: why doesn't this work?
Copy link
Contributor

Choose a reason for hiding this comment

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

@Q-Lee is this an issue with the PR for adding image types? KUBE_TEST_API_VERSIONS seems to be set properly but the actual type just errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a mistake in the API type that might have caused this. I'm regenerating things with a fix atm.

@ericchiang
Copy link
Contributor

@mikedanese actual PR for API group is #30241. This is a WIP that builds on top of that PR, so it pulls in the commits.

deads2k
deads2k reviewed Aug 15, 2016
View reviewed changes
plugin/pkg/admission/imagepolicy/admission.go
if err != nil {
glog.V(4).Infof("error contacting webhook backend: %s")
if a.defaultAllow {
glog.V(4).Infof("pod allowed in spite of webhook backend failure")
Copy link
Contributor

Choose a reason for hiding this comment

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

.V(2) I'd think

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we log some more pertinent information here (e.g., pod name or even the error)? If this is effecting more than 1 request, it will be hard to follow what's happening.

@mikedanese
Copy link
Member

Oops.

@ericchiang
Copy link
Contributor

Apparently I'm not actually an admin. Can someone else "ok to test" this?

@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2016

@Q-Lee @deads2k I think you both have to go through reviewable and approve every conversation, if I've understood correctly.

Yuck. It doesn't block merge for now (its come up on other pulls), so I'm just going to leave them in their current state.

@Q-Lee
Copy link
Contributor

Q-Lee commented Aug 18, 2016

@ecordell I tried doing that, but I think I can only resolve discussions that I started. However, it didn't block my last PR. The only thing I see here is that the branch has conflicts tht must be resolved. Try rebasing against master (the API changes were merged, so resolve those conflicts).

@k8s-github-robot
Copy link

@ecordell PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM. @Q-Lee @deads2k @ecordell

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2016
@ecordell
Copy link
Author

ecordell commented Aug 18, 2016 •
edited
Loading

hack/.linted_packages doesn't seem to ever rebase cleanly so I'm not sure how I'll ever get out of the submit queue -> rebase -> lgtm cycle.

@deads2k could you lgtm with a higher priority to hopefully get it through?

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@deads2k deads2k added this to the v1.4 milestone Aug 18, 2016
@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2016

@deads2k could you lgtm with a higher priority to hopefully get it through?

Sorry, I'm against queue jumping for net new features. I did add the milestone tag which helps a little bit.

@k8s-bot
Copy link

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 11033177529d282f675f7b81380a82d8d19feeeb.

@k8s-github-robot
Copy link

@ecordell PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2016
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM. @Q-Lee @deads2k @ecordell

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 19, 2016
@ecordell
Copy link
Author

@deads2k please LGTM

@k8s-bot
Copy link

k8s-bot commented Aug 19, 2016

GCE e2e build/test passed for commit 711e3cf.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2016
@Q-Lee
Copy link
Contributor

Q-Lee commented Aug 19, 2016

@ecordell I think .linted_packages has been dealt with. If we require 1 more rebase, then I'm going to up the priority.

This was referenced Aug 19, 2016
Merged
Closed
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 20, 2016

GCE e2e build/test passed for commit 711e3cf.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1de78d5 into kubernetes:master Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deads2k deads2k

@Q-Lee Q-Lee

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.