Admission plugin: openshift.io/ImageQualify #18020

frobware · 2018-01-08T18:12:36Z

This plugin allows administrators to set a policy for bare image names. A "bare" image name is a docker image reference that contains no domain component (e.g., "repository.io", "docker.io", etc).

The preferred domain component to use, and hence pull from, for a bare image name is computed from a set of path-based pattern matching rules defined in the admission configuration.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1518378

smarterclayton · 2018-01-09T19:53:47Z

Why isn’t this part of image policy?

frobware · 2018-01-09T20:16:26Z

Why isn’t this part of image policy?

If you don't have that plugin enabled you would not be able to qualify images, hence a separate plugin.

/cc @liggitt

liggitt · 2018-01-09T21:25:54Z

admission-plugin.yaml

@@ -0,0 +1,7 @@
+admissionConfig:


remove this file, move to doc or PR description?

remove this file, move to doc or PR description?

Yep, to be removed. Was here for my dev convenience.

I have a few unit tests to complete then I'll split the PR up into more discrete commits which should make it easier to review.

liggitt · 2018-01-09T21:30:38Z

admission-plugin.yaml

+      configuration:
+        kind: ImagePolicyConfig
+        apiVersion: v1
+        qualifyRulesFilename: /home/centos/rules


since this admission plugin is distinct, we can just inline the rules in the plugin config type struct. if we want this externalized, we'd reference the whole plugin config externally:

master-config.yaml

... admissionConfig: pluginConfig: openshift.io/ImageQualification: location: /path/to/plugin.yaml ...

plugin.yaml:

kind: ImageQualificationConfig apiVersion: v1 rules: - ... - ... - ...

smarterclayton · 2018-01-10T13:58:52Z

We always run with image policy enabled.

Also, qualification has to happen after image reference policy, but before admission policy runs. So I don't think this can be a separate plugin.

Having them separate would be broken in this case:

User provides mysql to a pod
Image policy checks for local name references (finds none)
Image policy applies naming policy (block execution of images that aren't from docker.io), assumes mysql means docker.io/library/mysql
Image qualifier runs and turns mysql into foo.bar/mysql.

You can't run before, because you would break local name references.

liggitt · 2018-01-10T14:12:26Z

Also, qualification has to happen after image reference policy, but before admission policy runs.

Shouldn't have to... that's what the mutation/validation split was for... both should get their shot at mutating, then both should get their shot at validating.

frobware · 2018-01-10T15:06:15Z

then both should get their shot at validating.

Should qualification also happen in Validate()?

ATM, it does, but it was an open question (in my mind).

frobware · 2018-01-10T15:08:28Z

We always run with image policy enabled.

Will that always be true? If that does change at some point in the future you may then be surprised that qualification no longer happens. Should they be coupled like this?

smarterclayton · 2018-01-10T18:10:23Z

Shouldn't have to... that's what the mutation/validation split was for... both should get their shot at mutating, then both should get their shot at validating.

image policy was not split into mutation / validation that I'm aware of. You can't mutate before, you have to qualify within the image policy flow, not after.

smarterclayton · 2018-01-10T18:56:04Z

Note, I'm talking about OpenShift image policy, not upstream image policy. We don't use upstream image policy and have no plan to.

frobware · 2018-01-11T17:52:43Z

/test verify

miminar · 2018-01-12T15:33:14Z

pkg/image/admission/imagequalify/admission.go

+		return "", err
+	}
+	if domain != "" {
+		// image is already qualfied


typo

Thanks. I'm about to collapse all the commits into something sensible again. I ended up collapsing two packages and it should be more self contained now.

miminar · 2018-01-12T16:11:42Z

pkg/image/qualify/image.go

+//   "docker.io/busybox"            -> domain is "docker.io"
+//   "docker.io/library/busybox"    -> domain is "docker.io"
+//   "library/busybox:v1"           -> domain is ""
+func SplitImageName(image string) (string, string, error) {


We already have such parsers. There's also docker's reference package which as de facto referential implementation.

miminar · 2018-01-12T16:12:53Z

pkg/image/qualify/image_test.go

+		input:     "parser.test.io/foo/busybox:v1.2.3",
+		domain:    "parser.test.io",
+		remainder: "foo/busybox:v1.2.3",
+	}} {


Should test multiple slashes as well.

miminar · 2018-01-12T16:19:09Z

pkg/image/qualify/qualify.go

+// Qualify qualifies image with a domain based on the set of patterns
+// in rules. If there is a match then return true to indicate a match
+// was found in rules together with the discrete domain name and a
+// fully qualified image reference, otherwise returns false.


false maybe as a string

bparees · 2018-01-16T17:48:37Z

Ben, does this look ok to you? Is there anywhere in the build component where fully qualified image names are not used?

only in places where that image name is fed directly to the docker daemon (thus qualification would continue to be handled by the docker daemon, which I assume is what we want)

liggitt · 2018-01-16T19:46:53Z

pkg/image/admission/imagequalify/qualify.go

+// rules. Returns the Rule that matched, or nil if no match was found.
+func matchImage(image string, rules []api.ImageQualifyRule) *api.ImageQualifyRule {
+	for i := range rules {
+		if ok, _ := path.Match(rules[i].Pattern, image); ok {


not sure we want to support all the possible patterns path.Match allows... can we limit the charset of the pattern to characters that are allowed in image specs (something like a-zA-Z0-9_-/:@, I think), and *?

Do we want to support wildcards in the digest part?

I've looked at this in a number of ways:

take your suggested character set, and add "*", anything outside of that is bogus, though therre is the perennial risk that image specifications chnage underneath our feet

look at explicitly referencing these regexps, but a) they're internal and b) was curious as to why it's a copy

If we don't want to support wildcards in digests then (I think) an alternative pattern validation scheme may be:

tmp := strings.Replace(rule.Pattern, "*", "a", -1)) if _, _, _, err := parsers.ParseImageName(tmp); err != nil { // pattern is invalid. }

I had chosen to be liberal with patterns as we always validate the image reference when it gets qualified. It would, I agree, be nice to be aware up-front that the pattern is invalid. Either way, you would have to correct and restart whether we do validation up front or (possibly) much later when a match occurs.

Having attempted some validation I'm wondering if it really makes sense to have something like:

a/b@sha256:abcd*ff*11*

Part of me says I would prefer to fix my patterns vis-a-vis baking policy in and then having to wait for a binary release should things go south.

therre is the perennial risk that image specifications chnage underneath our feet

pretty sure we'd have other updates to make in that case as well. I'd rather start as limited as possible to address known use cases and expand further if needed.

liggitt · 2018-01-16T19:47:24Z

pkg/image/admission/imagequalify/api/validation/validation.go

+		return allErrs
+	}
+	for i, rule := range config.Rules {
+		if rule.Pattern == "" {


limit the charset in pattern to just characters valid in an image spec and *

If we do checking in validation then by the time this is exercised only have valid patterns. Basically, you either have a correct config, or you don't. This is an internal call so validation should prevent you getting this far with a bad pattern.

this is where you would do that validation

Yep, agreed. I had your two comments the wrong way round in my head.

limit the charset in pattern to just characters valid in an image spec and *

Done.

liggitt · 2018-01-18T16:27:36Z

pkg/image/admission/imagequalify/api/v1/register.go

+)
+
+// SchemeGroupVersion is group version used to register these objects
+var SchemeGroupVersion = schema.GroupVersion{Group: "", Version: "v1"}


@deads2k are we putting new config types in groups now? apiserver.config.openshift.io? I couldn't find any other config types in origin yet

@deads2k are we putting new config types in groups now? apiserver.config.openshift.io? I couldn't find any other config types in origin yet

"webconsole.config.openshift.io" from here https://github.com/openshift/api/blob/master/webconsole/v1/register.go#L10

For admission plugins it's going to be a question of versioning cadence. Because our admission plugins will be used in the openshift apiserver, I'm inclined to go "admission.config.openshift.io" without further subdivision.

For admission plugins it's going to be a question of versioning cadence. Because our admission plugins will be used in the openshift apiserver, I'm inclined to go "admission.config.openshift.io" without further subdivision.

Is there any change I need to make here?

here:

var GroupName = "admission.config.openshift.io" var SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1"}

in the internal register.go:

var GroupName = "admission.config.openshift.io" var SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: runtime.APIVersionInternal}

wherever you have config data you load:

apiVersion: admission.config.openshift.io/v1

liggitt · 2018-01-18T21:17:18Z

I didn't review the bash script
The rest lgtm, can go ahead and squash

frobware · 2018-01-18T21:27:23Z

I didn't review the bash script

I know @sjenning has run it successfully on two online nodes, so it has been exercised in the wild.

This plugin allows administrators to set a policy for bare image names. A "bare" image name is a docker image reference that contains no domain component (e.g., "repository.io", "docker.io", etc). The preferred domain component to use, and hence pull from, for a bare image name is computed from a set of path-based pattern matching rules defined in the admission configuration. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1518378

liggitt · 2018-01-18T22:37:10Z

/retest

liggitt · 2018-01-18T23:00:42Z

/lgtm

openshift-ci-robot · 2018-01-18T23:00:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

sjenning · 2018-01-19T02:01:40Z

/retest

sjenning · 2018-01-19T03:36:24Z

flakes #14296 #18048
/retest

openshift-merge-robot · 2018-01-19T12:33:04Z

Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165).

@liggitt

Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326). UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names kubernetes/kubernetes#58955 removes the need for #18020 see history: kubernetes/kubernetes#51751 kubernetes/kubernetes#52110 kubernetes/kubernetes#53161 @liggitt @derekwaynecarr @frobware @mrunalp @runcom

@liggitt

Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326). UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names kubernetes#58955 removes the need for openshift/origin#18020 see history: kubernetes#51751 kubernetes#52110 kubernetes#53161 @liggitt @derekwaynecarr @frobware @mrunalp @runcom Origin-commit: d91de347457d7f6f3d1859c671c7355cc193d038

Automatic merge from submit-queue (batch tested with PRs 18624, 18605). Revert "Admission plugin: openshift.io/ImageQualify" This reverts commit 8bc3e07. #18020 Render unneeded by upstream kubernetes/kubernetes#58955 and Origin pick #18340 Will live on forever in git history if we find a use case for it later.

@liggitt

Automatic merge from submit-queue (batch tested with PRs 18225, 18351, 18331, 18340, 18326). UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names kubernetes#58955 removes the need for openshift/origin#18020 see history: kubernetes#51751 kubernetes#52110 kubernetes#53161 @liggitt @derekwaynecarr @frobware @mrunalp @runcom Origin-commit: d91de347457d7f6f3d1859c671c7355cc193d038

openshift-merge-robot added the needs-api-review label Jan 8, 2018

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 8, 2018

openshift-ci-robot requested review from deads2k, enj, miminar and smarterclayton January 8, 2018 18:12

frobware force-pushed the qualify-images branch from ef6f01f to 20d914f Compare January 9, 2018 11:24

openshift-ci-robot requested a review from liggitt January 9, 2018 20:16

liggitt reviewed Jan 9, 2018

View reviewed changes

frobware force-pushed the qualify-images branch from ac79c45 to 486eda7 Compare January 10, 2018 18:43

frobware changed the title ~~[WIP] image qualify admission~~ Admission plugin: openshift.io/ImageQualify Jan 10, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2018

frobware force-pushed the qualify-images branch from 486eda7 to bd85dd5 Compare January 11, 2018 11:23

frobware force-pushed the qualify-images branch 2 times, most recently from d32d2af to dfa48de Compare January 12, 2018 14:01

miminar reviewed Jan 12, 2018

View reviewed changes

frobware changed the title ~~Admission plugin: openshift.io/ImageQualify~~ [WIP] Admission plugin: openshift.io/ImageQualify Jan 12, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2018

frobware force-pushed the qualify-images branch from 25a7a89 to 601ade5 Compare January 12, 2018 21:41

openshift-ci-robot requested a review from bparees January 16, 2018 17:15

miminar mentioned this pull request Jan 16, 2018

Additional registry whitelisting #17783

Merged

frobware force-pushed the qualify-images branch from e54c7c9 to a75eedb Compare January 16, 2018 18:22

liggitt reviewed Jan 16, 2018

View reviewed changes

liggitt reviewed Jan 18, 2018

View reviewed changes

liggitt added the api-approved label Jan 18, 2018

frobware force-pushed the qualify-images branch from 5f13704 to 8bc3e07 Compare January 18, 2018 21:51

openshift-ci-robot assigned liggitt Jan 18, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2018

sjenning removed the needs-api-review label Jan 19, 2018

openshift-merge-robot added the needs-api-review label Jan 19, 2018

openshift-merge-robot merged commit 0acd4e2 into openshift:master Jan 19, 2018

sjenning mentioned this pull request Jan 29, 2018

UPSTREAM: 58955: pkg: kubelet: do not assume anything about images names #18340

Merged

sjenning mentioned this pull request Feb 14, 2018

Revert "Admission plugin: openshift.io/ImageQualify" #18605

Merged

Admission plugin: openshift.io/ImageQualify #18020

Admission plugin: openshift.io/ImageQualify #18020

Conversation

frobware commented Jan 8, 2018 • edited Loading

smarterclayton commented Jan 9, 2018

frobware commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

smarterclayton commented Jan 10, 2018 • edited Loading

liggitt commented Jan 10, 2018

frobware commented Jan 10, 2018

frobware commented Jan 10, 2018

smarterclayton commented Jan 10, 2018

smarterclayton commented Jan 10, 2018

frobware commented Jan 11, 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

bparees commented Jan 16, 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

liggitt Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt commented Jan 18, 2018

frobware commented Jan 18, 2018

liggitt commented Jan 18, 2018

liggitt commented Jan 18, 2018

openshift-ci-robot commented Jan 18, 2018

sjenning commented Jan 19, 2018

sjenning commented Jan 19, 2018

openshift-merge-robot commented Jan 19, 2018

frobware commented Jan 8, 2018 •

edited

Loading

liggitt Jan 9, 2018 •

edited

Loading

smarterclayton commented Jan 10, 2018 •

edited

Loading

liggitt Jan 18, 2018 •

edited

Loading