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.

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/deploy/api/test/ok.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,18 @@ func OkPodTemplate() *kapi.PodTemplateSpec {
}
}

func OkPodTemplateMissingImage(missing ...string) *kapi.PodTemplateSpec {
set := sets.NewString(missing...)
template := OkPodTemplate()
for i, c := range template.Spec.Containers {
if set.Has(c.Name) {
// rememeber that slices use copies, so have to ref array entry explicitly
template.Spec.Containers[i].Image = ""
}
}
return template
}

func OkConfigChangeTrigger() deployapi.DeploymentTriggerPolicy {
return deployapi.DeploymentTriggerPolicy{
Type: deployapi.DeploymentTriggerOnConfigChange,
Expand Down
60 changes: 60 additions & 0 deletions pkg/deploy/api/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
if spec.Template == nil {
allErrs = append(allErrs, field.Required(specPath.Child("template"), ""))
} else {
originalContainerImageNames := getContainerImageNames(spec.Template)
defer setContainerImageNames(spec.Template, originalContainerImageNames)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should defer only if we actually mutated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I want it unconditional, as it is now, to guarantee we restore. It's not expensive enough to worry about making conditional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Safety is more important here

Copy link
Contributor

@0xmichalis 0xmichalis Jun 14, 2016

Choose a reason for hiding this comment

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

Is defer working different or will the deferred function move in the stack once defer in encountered in the code? If the latter, I don't see any safety problem here, we just save a defer call that is redundant.

originalContainerImageNames := getContainerImageNames(spec.Template)
handleEmptyImageReferences(spec.Template, spec.Triggers)
defer setContainerImageNames(spec.Template, originalContainerImageNames)

vs

originalContainerImageNames := getContainerImageNames(spec.Template)
if updated := handleEmptyImageReferences(spec.Template, spec.Triggers); updated {
   defer setContainerImageNames(spec.Template, originalContainerImageNames)
}

Copy link
Contributor

@liggitt liggitt Jun 14, 2016

Choose a reason for hiding this comment

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

no, the temporary image name change still has to be in effect when we call ValidatePodTemplateSpec

Copy link
Contributor

Choose a reason for hiding this comment

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

the defer is fine as-is, let's move on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

duh :-)

OK, if we are moving on, and my prior test run passed, and api approval has been marked, can the merge comment be posted, or are we waiting on something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

nothing else from me. @bparees has merge, not sure if the handleEmptyImageReferences impl changed since he lgtm'ed the first time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - I though you had it as well. OK, we'll wait for @bparees / @smarterclayton , though I just realized I forgot another comment from @Kargakis up above which I think is worth doing. Will address after scrum.

handleEmptyImageReferences(spec.Template, spec.Triggers)
Copy link
Contributor

@liggitt liggitt Jun 8, 2016

Choose a reason for hiding this comment

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

a pair of functions that extracts the original container image names and restores them would feel safer to me:

func getContainerImageNames(template *kapi.PodTemplateSpec) []string {
  names := make([]string, len(template.Containers))
  for i := range template.Containers {
    names[i] = template.Containers[i].Image
  }
  return names
}

func setContainerImageNames(template *kapi.PodTemplateSpec, names []string) {
  for i := range template.Containers {
    template.Containers[i].Image = names[i]
  }
}

then you could use it like:

originalNames := getContainerImageNames(spec.Template)
defer setContainerImageNames(spec.Template, originalNames)

and continue on, overriding image names however you like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know how to reconcile this with @smarterclayton comment regarding the overkill nature of the array return with the earlier incarnation of this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he said deep copy was overkill... I think what I'm proposing is similar to what he suggested in #9167 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

He said both deep copy and array return were overkill. I'll try to ref the
comment: #9167 (comment)

But just in case, an old fashioned copy / paste:

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

It came after the comment you referenced.

On Wed, Jun 8, 2016 at 4:33 PM, Jordan Liggitt [email protected]
wrote:

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

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

  •   // rather than handleEmptyImageReferences returning an array of changed containers, PR review deemed it more
    
  •   // efficient to build a list of the original container to image mappings, and then just restore all of them (and hence,
    
  •   // even the ones not modified)
    
  •   originalContainerImages := make(map[string]string)
    
  •   for _, container := range spec.Template.Spec.Containers {
    
  •       originalContainerImages[container.Name] = container.Image
    
  •   }
    
  •   handleEmptyImageReferences(spec.Template, spec.Triggers)
    

I think he said deep copy was overkill... I think what I'm proposing is
similar to what he suggested in #9167 (comment)
#9167 (comment)


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/abd1b6fed22e427f856c6ac25b536ec3ce45b70f#r66334076,
or mute the thread
https://github.com/notifications/unsubscribe/ADbadHNAdUUVb5A9HGiCeJj7NZnq_Q1Nks5qJycPgaJpZM4It4tM
.

Copy link
Contributor

@liggitt liggitt Jun 10, 2016

Choose a reason for hiding this comment

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

a map has the potential to restore something other than the original image names (remember that this is pre-validation, where we don't know that all the containers have unique names). I'd still prefer helper funcs to get the image names as an array, and to set the image names from an array, using them like

originalNames := getContainerImageNames(spec.Template)
defer setContainerImageNames(spec.Template, originalNames)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented @liggitt 's latest suggestion

allErrs = append(allErrs, validation.ValidatePodTemplateSpec(spec.Template, specPath.Child("template"))...)
}
if spec.Replicas < 0 {
Expand All @@ -50,6 +53,63 @@ func ValidateDeploymentConfigSpec(spec deployapi.DeploymentConfigSpec) field.Err
return allErrs
}

func getContainerImageNames(template *kapi.PodTemplateSpec) []string {
originalContainerImageNames := make([]string, len(template.Spec.Containers))
for i := range template.Spec.Containers {
originalContainerImageNames[i] = template.Spec.Containers[i].Image
}
return originalContainerImageNames
}

func setContainerImageNames(template *kapi.PodTemplateSpec, originalNames []string) {
for i := range template.Spec.Containers {
template.Spec.Containers[i].Image = originalNames[i]
}
}

func handleEmptyImageReferences(template *kapi.PodTemplateSpec, triggers []deployapi.DeploymentTriggerPolicy) {
// if we have both an ICT defined and an empty Template->PodSpec->Container->Image field, we are going
// to modify this method's local copy (a pointer was NOT used for the parameter) by setting the field to a non-empty value to
// work around the k8s validation as our ICT will supply the image field value
containerEmptyImageInICT := make(map[string]bool)
for _, container := range template.Spec.Containers {
if len(container.Image) == 0 {
containerEmptyImageInICT[container.Name] = false
}
}

if len(containerEmptyImageInICT) == 0 {
return
}

needToChangeImageField := false
for _, trigger := range triggers {
// note, the validateTrigger call above will add an error if ImageChangeParams is nil, but
// we can still fall down this path so account for it being nil
if trigger.Type != deployapi.DeploymentTriggerOnImageChange || trigger.ImageChangeParams == nil {
continue
}

for _, container := range trigger.ImageChangeParams.ContainerNames {
if _, ok := containerEmptyImageInICT[container]; ok {
needToChangeImageField = true
containerEmptyImageInICT[container] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this is going to result in containers references by the ICT, but which have non-empty image field values, having those image field values changed to "unset" for validation purposes?

@Kargakis @smarterclayton is there ever a case where we'd have an ICT but still expect the image field value to be valid if it's non-empty and therefore need to do proper validation on the user supplied value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct (non-empty changed to unset) ... in the end it doesn't matter but will address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed update for the last 2 comments here.

}
}
}

if needToChangeImageField {
for i, container := range template.Spec.Containers {
// only update containers listed in the ict
match, ok := containerEmptyImageInICT[container.Name]
if match && ok {
template.Spec.Containers[i].Image = "unset"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

}

func ValidateDeploymentConfigStatus(status deployapi.DeploymentConfigStatus) field.ErrorList {
allErrs := field.ErrorList{}
statusPath := field.NewPath("status")
Expand Down
38 changes: 38 additions & 0 deletions pkg/deploy/api/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,50 @@ func TestValidateDeploymentConfigOK(t *testing.T) {
}
}

func TestValidateDeploymentConfigICTMissingImage(t *testing.T) {
dc := &api.DeploymentConfig{
ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: api.DeploymentConfigSpec{
Replicas: 1,
Triggers: []api.DeploymentTriggerPolicy{test.OkImageChangeTrigger()},
Selector: test.OkSelector(),
Strategy: test.OkStrategy(),
Template: test.OkPodTemplateMissingImage("container1"),
},
}
errs := ValidateDeploymentConfig(dc)

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

for _, c := range dc.Spec.Template.Spec.Containers {
if c.Image == "unset" {
t.Errorf("%s image field still has validation fake out value of %s", c.Name, c.Image)
}
}
}
Copy link
Contributor

@bparees bparees Jun 5, 2016

Choose a reason for hiding this comment

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

can't this test be part of TestValidateDeploymentMissingFields? (and should check the actual field reported in the error, like those tests do)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of next push


func TestValidateDeploymentConfigMissingFields(t *testing.T) {
errorCases := map[string]struct {
DeploymentConfig api.DeploymentConfig
ErrorType field.ErrorType
Field string
}{
"empty container field": {
api.DeploymentConfig{
ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"},
Spec: api.DeploymentConfigSpec{
Replicas: 1,
Triggers: []api.DeploymentTriggerPolicy{test.OkConfigChangeTrigger()},
Selector: test.OkSelector(),
Strategy: test.OkStrategy(),
Template: test.OkPodTemplateMissingImage("container1"),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to be missing something. this DeploymentConfig looks identical to the one defined above for the passing test. Why is validate returning an error on this DC? Shouldn't the Triggers[] be empty on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is this one has a ConfigChangeTrigger, where as the passing one above has an ImageChangeTrigger.

Copy link
Contributor

Choose a reason for hiding this comment

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

sigh. thanks @gabemontero :)

field.ErrorTypeRequired,
"spec.template.spec.containers[0].image",
},
"missing name": {
api.DeploymentConfig{
ObjectMeta: kapi.ObjectMeta{Name: "", Namespace: "bar"},
Expand Down