bypass k8s container image validation when ict's defined (allow for e… #9167

gabemontero · 2016-06-03T20:36:06Z

…mpty string)

per a discussion between @smarterclayton , @bparees , and myself (and perhaps @Kargakis as well, though maybe he got pulled in later on), @smarterclayton asked that we introduce a change to bypass the k8s validation in the container image field when an openshift ICT is defined (which will populate said field).

While the OpenShift sample templates will still set this field to non-empty for backward compatibility, we can at least allow customers with new templates to not have to put a meaningless value for the container.image field.

PTAL gentlemen

bparees · 2016-06-05T02:43:34Z

pkg/deploy/api/validation/validation_test.go

+	})
+
+	if len(errs) > 0 {
+		t.Errorf("Unxpected non-empty error list: %+v", errs)


part of next push

0xmichalis · 2016-06-05T14:59:57Z

cc: @ironcladlou @mfojtik

gabemontero · 2016-06-05T21:11:14Z

updates based on comments pushed

bparees · 2016-06-05T22:31:30Z

pkg/deploy/api/validation/validation.go

+		if needToChangeImageField {
+			for i, container := range spec.Template.Spec.Containers {
+				// only update containers listed in the icp
+				match, ok := containerEmptyImageInICT[container.Name]


it seems like you can get rid of this whole structure. all you need to do in this loop is again check if the container.Image field is length 0, and if so, update it.

nevermind, didn't see that the trigger itself only applies to specific containers in the template, not all of them. my bad.

bparees · 2016-06-05T22:39:32Z

@openshift/api-review needs api review since we're changing what is "valid" for a DC's image field.

smarterclayton · 2016-06-06T00:02:13Z

pkg/deploy/api/validation/validation.go

@@ -39,6 +39,39 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
 	if spec.Template == nil {
 		allErrs = append(allErrs, field.Required(specPath.Child("template"), ""))
 	} else {
+		// if we have both an ICT defined and an empty Template->PodSpec->Container->Image field, we are going


It would be better to extract this to its own method, and only mutate the field for the duration of the validate call, not permanently. It would also be better to use a valid value for image, rather than relying on the empty whitespace being treated for the field.

I.e.:

func setEmptyImageChangeReferences(template, triggers) sets.String { // ... mutate each container with a trigger, return the names of those containers ... containers[i].Image = "unset" ... } func resetEmptyImageChangeReferences(template, empty) { for i := range template.Containers { if empty.Has(template.Containers[i].Name) { template.Containers[i].Image = "" } } } func validateDeploymentConfigPodSpecTemplate(...) { emptyImages := setEmptyImageChangeReferences(template, triggers) errs := validatePodSpecTemplate(template) resetEmptyImageChangeReferences(template, emptyImages) return errs }

We should not let our mutation escape the validation method.

Names are arbitrary, just chosen for example.

Yeah, the span of mutation had originally occurred to me as well, but since the parameter into ValidateDeploymentConfigSpec is NOT a pointer, my understanding was that golang would pass in a copy of the param to the method.

Looking again though, while the DeploymentConfigSpec passed in is not a pointer, the underlying Template field is a pointer. Perhaps then even with DeploymentConfigSpec parameter being a "copy", the Template pointer field will be the same address in both DeploymentConfigSpec copies ... as I type, I'm thinking yes, but confirmation / correction of my train of thought here would be great.

I'll start working on the changes in the interim.

All slices are passed by pointer, so you'll need to revert those as well.

On Mon, Jun 6, 2016 at 10:39 AM, Gabe Montero [email protected]
wrote:

In pkg/deploy/api/validation/validation.go
#9167 (comment):

@@ -39,6 +39,39 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
if spec.Template == nil {
allErrs = append(allErrs, field.Required(specPath.Child("template"), ""))
} else {

// if we have both an ICT defined and an empty Template->PodSpec->Container->Image field, we are going

Yeah, the span of mutation had originally occurred to me as well, but
since the parameter into ValidateDeploymentConfigSpec is NOT a pointer,
my understanding was that golang would pass in a copy of the param to the
method.

Looking again though, while the DeploymentConfigSpec passed in is not a
pointer, the underlying Template field is a pointer. Perhaps then even
with DeploymentConfigSpec parameter being a "copy", the Template pointer
field with be the same address in both DeploymentConfigSpec copies ... as
I type, I'm thinking yes, but confirmation / correction of my train of
thought here would be great.

I'll start working on the changes in the interim.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9167/files/7108af0c910f3080a151b4fbd222c31265396ccb#r65901447,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6agQgWm-H2gYfbesbJMRIW-2rxbks5qJDE1gaJpZM4It4tM
.

whatever we do, we should have a test that validates a DC/DCSpec with empty image names and ensures the validation doesn't mutate (original DC or DC spec passed in still has empty image names)

I like deep copy as well ... will go down that route unless a strong objection surfaces.

Let's not deep copy, it's very expensive on a pod template. The fuzz test is sufficient to prove non mutation.

Apologies, don't know what is meant by "fuzz test". Can you clarify @smarterclayton ?

Pending that, I'll move forward with removing the deep copy and incorporating a temp update / revert pattern.

Create a test that calls validation with an object and verifies it doesn't
change, but do so with a range of input. Fuzzing would allow you to test
many more outcomes, but I'd be ok with a test that from inspection is the
same. I acknowledge Jordan's concerns, but deep copy is a hammer and I'm
not concerned with mutation if we have a very focused test.

On Wed, Jun 8, 2016 at 11:07 AM, Gabe Montero [email protected]
wrote:

In pkg/deploy/api/validation/validation.go
#9167 (comment):

@@ -39,6 +39,39 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
if spec.Template == nil {
allErrs = append(allErrs, field.Required(specPath.Child("template"), ""))
} else {

// if we have both an ICT defined and an empty Template->PodSpec->Container->Image field, we are going

Apologies, don't know what is meant by "fuzz test". Can you clarify
@smarterclayton https://github.com/smarterclayton ?

Pending that, I'll move forward with removing the deep copy and
incorporating a temp update / revert pattern.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9167/files/7108af0c910f3080a151b4fbd222c31265396ccb#r66274074,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p4UxNFf_r1BDUEyYtrNMFzbI6pWyks5qJtq0gaJpZM4It4tM
.

OK, I've pushed the change that removes deepCopy and introduces a mutate/restore semantic. I updated the test case that confirms the container name change does not extend beyond the validate call slightly, but certainly did not get expansive, given @smarterclayton said he'd be ok with a test that was the same.

liggitt · 2016-06-06T13:53:18Z

how would a client display what image the dc would actually deploy if the image name is empty. the UI already has issues when the name is set to " " (c.f. openshift/origin-web-console#55)

bparees · 2016-06-06T14:22:02Z

pretty sure the UI is/was going to be updated to show the ICT imagestream reference instead of the image field, for cases where the image field is blank (or " ") and an ICT exists.

gabemontero · 2016-06-06T17:28:11Z

OK, have pushed update with:

used "unset" as the fake out value
set the fake out value on and validated with a deep copy
added test verifying the fake out value does not persist beyond the validate call (btw, verified it did persist beyond the call prior to doing 2)

ptal - thx

0xmichalis · 2016-06-07T08:07:49Z

pkg/deploy/api/validation/validation.go

+	needToChangeImageField := false
+	if len(containerEmptyImageInICT) > 0 {
+		for _, trigger := range triggers {
+			if trigger.Type == deployapi.DeploymentTriggerOnImageChange {


reduce the nesting by blacklisting

if trigger.Type != ... || trigger,params == nil { continue } // ...

yep, looks better ... update pushed

bparees · 2016-06-07T18:18:59Z

lgtm

smarterclayton · 2016-06-08T18:51:22Z

pkg/deploy/api/validation/validation.go

+				changedContainers = append(changedContainers, &(template.Spec.Containers[i]))
+				template.Spec.Containers[i].Image = "unset"
+			}
+		}


It would be better to have a map of container name to original image and just set them all. Returning array could be overkill.

OK, removed the array return, did not add a map return, and inlined the saving of the original names and restoring of them in the validate method itself.

bparees · 2016-06-08T20:02:40Z

pkg/deploy/api/validation/validation.go

+
+	if needToChangeImageField {
+		for i, container := range template.Spec.Containers {
+			// only update containers listed in the icp


yep, ict ... will fix

gabemontero · 2016-06-13T17:29:43Z

also @openshift/api-review - does the recent implementation discussion have bearing on the api-review, or does the api-review only pertain to the specifics of what is a valid container image field?

smarterclayton · 2016-06-13T17:44:45Z

This change is approved to allow deployment configs to have empty image
values.

On Mon, Jun 13, 2016 at 1:29 PM, Gabe Montero [email protected]
wrote:

also @openshift/api-review
https://github.com/orgs/openshift/teams/api-review - does the recent
implementation discussion have bearing on the api-review, or does the
api-review only pertain to the specifics of what is a valid container image
field?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9167 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3pZH9c9ij-rGirCYTVuCxD98Khvks5qLZOKgaJpZM4It4tM
.

liggitt · 2016-06-13T18:58:46Z

pkg/deploy/api/validation/validation.go

@@ -50,6 +53,60 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
 	return allErrs
 }

+func getContainerImageNames(template *kapi.PodTemplateSpec) []string {
+	originalContainerImageNames := make([]string, 0)


make([]string, len(template.Spec.Containers))

suggestion pushed

gabemontero · 2016-06-13T20:19:14Z

[test]

0xmichalis · 2016-06-14T09:15:13Z

pkg/deploy/api/validation/validation.go

+		}
+	}
+	needToChangeImageField := false
+	if len(containerEmptyImageInICT) > 0 {


It seems that you can return early here if len(containerEmptyImageInICT) == 0. It will also reduce indentation.

update pushed

bparees · 2016-06-14T14:33:00Z

handleEmpty impl still lgtm, pending @Kargakis' if-inversion/indentation nit.

…mpty string)

gabemontero · 2016-06-14T14:50:18Z

last @Kargakis comment induced change pushed ... redoing [test]

openshift-bot · 2016-06-14T14:52:03Z

Evaluated for origin test up to 6049db4

openshift-bot · 2016-06-14T16:02:00Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4824/)

bparees · 2016-06-14T16:51:47Z

lgtm [merge]

openshift-bot · 2016-06-14T16:56:03Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4824/) (Image: devenv-rhel7_4373)

openshift-bot · 2016-06-14T16:56:03Z

Evaluated for origin merge up to 6049db4

bparees reviewed Jun 5, 2016
View reviewed changes

gabemontero force-pushed the bypassImageCheckWithICTs branch from 7a877c7 to 7108af0 Compare June 5, 2016 21:10

bparees reviewed Jun 5, 2016
View reviewed changes

bparees added the needs-api-review label Jun 5, 2016

smarterclayton reviewed Jun 6, 2016
View reviewed changes

liggitt mentioned this pull request Jun 6, 2016

One of the images has no text in web console using pipelinetemplate.json openshift/origin-web-console#55

Closed

gabemontero force-pushed the bypassImageCheckWithICTs branch from 7108af0 to e55285d Compare June 6, 2016 17:25

0xmichalis reviewed Jun 7, 2016
View reviewed changes

gabemontero force-pushed the bypassImageCheckWithICTs branch from e55285d to 7f128e4 Compare June 7, 2016 17:58

gabemontero force-pushed the bypassImageCheckWithICTs branch 2 times, most recently from 1a33ff9 to 2faa1aa Compare June 8, 2016 18:45

smarterclayton reviewed Jun 8, 2016
View reviewed changes

gabemontero force-pushed the bypassImageCheckWithICTs branch from 2faa1aa to abd1b6f Compare June 8, 2016 19:28

bparees reviewed Jun 8, 2016
View reviewed changes

gabemontero force-pushed the bypassImageCheckWithICTs branch 2 times, most recently from d6caf70 to e05e23e Compare June 13, 2016 17:27

gabemontero force-pushed the bypassImageCheckWithICTs branch from e05e23e to 30abda3 Compare June 13, 2016 17:40

smarterclayton added api-approved and removed needs-api-review labels Jun 13, 2016

liggitt reviewed Jun 13, 2016
View reviewed changes

gabemontero force-pushed the bypassImageCheckWithICTs branch from 30abda3 to be845a3 Compare June 13, 2016 19:04

0xmichalis reviewed Jun 14, 2016
View reviewed changes

bypass k8s container image validation when ict's defined (allow for e…

6049db4

…mpty string)

gabemontero force-pushed the bypassImageCheckWithICTs branch from be845a3 to 6049db4 Compare June 14, 2016 14:49

openshift-bot merged commit dc377a8 into openshift:master Jun 14, 2016

gabemontero deleted the bypassImageCheckWithICTs branch June 14, 2016 19:49

tnozicka mentioned this pull request Aug 7, 2018

Jenkins example template has empty image field #20531

Closed

bypass k8s container image validation when ict's defined (allow for e… #9167

bypass k8s container image validation when ict's defined (allow for e… #9167

Conversation

gabemontero commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jun 5, 2016 • edited Loading

gabemontero commented Jun 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jun 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero Jun 6, 2016 • edited Loading

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

smarterclayton Jun 7, 2016 via email

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 Jun 6, 2016

bparees commented Jun 6, 2016

gabemontero commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jun 7, 2016

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

gabemontero commented Jun 13, 2016

smarterclayton commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jun 14, 2016

gabemontero commented Jun 14, 2016

openshift-bot commented Jun 14, 2016

openshift-bot commented Jun 14, 2016

bparees commented Jun 14, 2016

openshift-bot commented Jun 14, 2016 • edited Loading

openshift-bot commented Jun 14, 2016

0xmichalis commented Jun 5, 2016 •

edited

Loading

gabemontero Jun 6, 2016 •

edited

Loading

openshift-bot commented Jun 14, 2016 •

edited

Loading