Allow ImageReview backend to add audit annotations. #64597

wteiken · 2018-06-01T05:26:13Z

What this PR does / why we need it:
This can be used to create annotations that will allow auditing of the created
pods.

The change also introduces "fail open" audit annotations in addition to the
previously existing pod annotation for fail open. The pod annotations for
fail open will be deprecated soon.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Allow ImageReview backend to return annotations to be added to the created pod.

k8s-ci-robot · 2018-06-01T05:26:16Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

wteiken · 2018-06-01T08:17:40Z

/assign @derekwaynecarr @liggitt

wteiken · 2018-06-01T17:30:23Z

Ideally this would to into the 1.11 release, given that this is a minor change.

k8s-ci-robot · 2018-06-01T17:30:30Z

@wteiken: You must be a member of the kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.11

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.

dims · 2018-06-13T10:54:48Z

/ok-to-test

k8s-ci-robot · 2018-08-13T05:26:02Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

wteiken · 2018-08-13T07:17:16Z

/assign @derekwaynecarr @liggitt

wteiken · 2018-08-13T07:34:23Z

/test pull-kubernetes-e2e-gce-100-performance

wteiken · 2018-08-23T16:56:26Z

/test pull-kubernetes-e2e-kops-aws

wteiken · 2018-08-23T17:50:12Z

@liggitt @tallclair Updated the PR to use the attributes annotations

liggitt · 2018-08-23T17:58:51Z

pkg/apis/imagepolicy/types.go

+	// prefix-less (i.e., the admission controller will add an appropriate
+	// prefix).  Annotations are only added to the attributes if the the
+	// creation is allowed.
+	AttributeAnnotations map[string]string


let's match the admission webhook response field name of auditAnnotations

k, will do.

liggitt · 2018-08-23T17:59:12Z

pkg/apis/imagepolicy/types.go

+	// Annotations that should be added to the annotations in the attributes
+	// object of the admission controller request. The keys should be
+	// prefix-less (i.e., the admission controller will add an appropriate
+	// prefix).  Annotations are only added to the attributes if the the


I'd expect to be able to add annotations to the audit event even for rejected requests

Up to now I assumed you'd just put the data in the error, but I can see that constraint is a bit artificial, will change.

liggitt · 2018-08-23T18:01:15Z

plugin/pkg/admission/imagepolicy/admission.go

+		return nil
+	}
+	for k, v := range review.Status.AttributeAnnotations {
+		attributes.AddAnnotation(AuditKeyPrefix+k, v)


check error and log a warning if if failed

liggitt · 2018-08-23T18:07:21Z

plugin/pkg/admission/imagepolicy/admission.go

+
+	// AuditKeyPrefix is used as the prefix for all audit keys handled by
+	// this pluggin. Some well known suffixes are listed below.
+	AuditKeyPrefix = PluginName + ".image-policy.k8s.io/"


are upper-case prefix names allowed in audit annotations? I didn't think they were

@tallclair @sttts seems like audit annotations should allow the qualified keys metadata annotations does

The spec says it should be the plugin name. Should we just change the plugin name to all lower case?

Yeah, lowercase is required. We should update the documentation to clarify.

BTW - here is the validation we use on the annotation key:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L37-L70

that was for webhook-returned audit annotations... webhook admission plugin names are lowercase... I would lowercase the name for purposes of this annotation

tallclair · 2018-08-23T19:40:43Z

plugin/pkg/admission/imagepolicy/admission.go

+
+	// AuditKeyPrefix is used as the prefix for all audit keys handled by
+	// this pluggin. Some well known suffixes are listed below.
+	AuditKeyPrefix = PluginName + ".image-policy.k8s.io/"


Yeah, lowercase is required. We should update the documentation to clarify.

BTW - here is the validation we use on the annotation key:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L37-L70

tallclair · 2018-08-23T19:46:13Z

plugin/pkg/admission/imagepolicy/admission.go

@@ -181,6 +199,12 @@ func (a *Plugin) admitPod(pod *api.Pod, attributes admission.Attributes, review
 		return errors.New("one or more images rejected by webhook backend")
 	}

+	if review.Status.AttributeAnnotations == nil {
+		return nil
+	}


nit: you don't need this - nil in go is equivalent to an empty slice, so the for loop will just skip.

Thanks! Done

tallclair · 2018-08-23T19:47:28Z

plugin/pkg/admission/imagepolicy/admission_test.go

+	}
+	for _, tt := range tests {
+		// Use a closure so defer statements trigger between loop iterations.
+		func() {


prefer:

t.Run(tt.test, func(t *testing.T) { ... })

liggitt · 2018-08-24T01:37:36Z

LGTM, please squash down (can leave the generated files in their own commit if you like), then will tag for merge

This can be used to create annotations that will allow auditing of the created pods. The change also introduces "fail open" audit annotations in addition to the previously existing pod annotation for fail open. The pod annotations for fail open will be deprecated soon.

wteiken · 2018-08-24T05:40:15Z

@liggitt @tallclair Thanks for the review!

Squashed the branch, all tests pass.

liggitt · 2018-08-24T05:50:13Z

/lgtm

wteiken · 2018-08-24T21:15:59Z

/assign @derekwaynecarr

wteiken · 2018-08-27T19:13:32Z

@liggitt @tallclair Will this be picked up at some point or do I need to ping @derekwaynecarr explicitly?

liggitt · 2018-08-27T20:59:28Z

ping is fine or add @deads2k for approval

derekwaynecarr · 2018-08-28T02:49:53Z

/approve

k8s-ci-robot · 2018-08-28T02:49:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, wteiken

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:

~~pkg/apis/OWNERS~~ [liggitt]
~~plugin/pkg/admission/OWNERS~~ [derekwaynecarr]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

k8s-github-robot · 2018-08-28T03:11:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-28T03:12:08Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-ci-robot · 2018-08-28T04:56:20Z

@wteiken: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`73c522f`	link	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-kubemark-e2e-gce-big	`73c522f`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`
pull-kubernetes-e2e-gce-100-performance	`73c522f`	link	`/test pull-kubernetes-e2e-gce-100-performance`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-github-robot · 2018-08-28T05:18:05Z

Automatic merge from submit-queue (batch tested with PRs 64597, 67854, 67734, 67917, 67688). If you want to cherry-pick this change to another branch, please follow the instructions here.

wteiken · 2018-08-28T05:40:33Z

/retest

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 1, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 1, 2018

k8s-ci-robot requested review from deads2k and derekwaynecarr June 1, 2018 05:26

wteiken changed the title ~~Add an option for the ImageReview backend to add annotations to the pods~~ Allow ImageReview backend to return annotations to be added to the pod Jun 1, 2018

k8s-ci-robot assigned derekwaynecarr and liggitt Jun 1, 2018

wteiken mentioned this pull request Jun 1, 2018

Add an option for the ImageReview backend to add annotations to the pods wteiken/kubernetes#1

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2018

wteiken force-pushed the add_review_annotations2 branch from 1652e85 to 719a31c Compare August 13, 2018 05:03

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 13, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 13, 2018

wteiken force-pushed the add_review_annotations2 branch from 7991bcc to af5b242 Compare August 13, 2018 05:29

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 13, 2018

wteiken force-pushed the add_review_annotations2 branch 3 times, most recently from 2be487d to a17e101 Compare August 23, 2018 06:29

liggitt reviewed Aug 23, 2018

View reviewed changes

tallclair reviewed Aug 23, 2018

View reviewed changes

wteiken changed the title ~~Allow ImageReview backend to return annotations to be added to the pod~~ Allow ImageReview backend to add audit annotations. Aug 24, 2018

wteiken force-pushed the add_review_annotations2 branch from db54455 to 73c522f Compare August 24, 2018 03:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2018

k8s-github-robot merged commit 583dd0f into kubernetes:master Aug 28, 2018

wteiken mentioned this pull request Aug 28, 2018

Automated cherry pick of #64597: Allow ImageReview backend to add audit annotations. #67926

Closed

Allow ImageReview backend to add audit annotations. #64597

Allow ImageReview backend to add audit annotations. #64597

Conversation

wteiken commented Jun 1, 2018 • edited Loading

k8s-ci-robot commented Jun 1, 2018

wteiken commented Jun 1, 2018

wteiken commented Jun 1, 2018

k8s-ci-robot commented Jun 1, 2018

dims commented Jun 13, 2018

k8s-ci-robot commented Aug 13, 2018

wteiken commented Aug 13, 2018

wteiken commented Aug 13, 2018

wteiken commented Aug 23, 2018

wteiken commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 24, 2018

wteiken commented Aug 24, 2018

liggitt commented Aug 24, 2018

wteiken commented Aug 24, 2018

wteiken commented Aug 27, 2018

liggitt commented Aug 27, 2018

derekwaynecarr commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018

k8s-github-robot commented Aug 28, 2018

k8s-github-robot commented Aug 28, 2018

k8s-ci-robot commented Aug 28, 2018

k8s-github-robot commented Aug 28, 2018

wteiken commented Aug 28, 2018

wteiken commented Jun 1, 2018 •

edited

Loading