Do not set default values when unsupported multiple containers are defined #4709

nak3 · 2019-07-11T06:22:45Z

Proposed Changes

If multiple containers are defined but one container is returned after
setting default values, inappropriate error happens.

To fix it, this patch stops setting default values when unsupported
multiple containers are defined.

/lint

Fixes #4706

After this PR, we will get the error as:

$ kubectl create -f test.yaml 
Error from server (InternalError): error when creating "test.yaml": Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: expected exactly one, got both: spec.template.spec.containers

Proposed Changes

Do not set default values when unsupported multiple containers are defined

Release Note

NONE

knative-prow-robot

@nak3: 0 warnings.

In response to this:

Proposed Changes

If multiple containers are defined but one container is returned after
setting default values, inappropriate error happens.

To fix it, this patch stops setting default values when unsupported
multiple containers are defined.

/lint

Fixes #4706

Proposed Changes

Do not set default values when unsupported multiple containers are defined

Release Note
NONE

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.

knative-prow-robot · 2019-07-11T06:23:00Z

Hi @nak3. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nak3 · 2019-07-11T06:40:06Z

cc @dgerd

markusthoemmes

/ok-to-test

Could you provide the error message that's returned now?

nak3 · 2019-07-11T06:51:05Z

Sure, here is the error message:

$ kubectl create -f test.yaml 
Error from server (InternalError): error when creating "test.yaml": Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: expected exactly one, got both: spec.template.spec.containers

(I have added it to my PR description as well.)

markusthoemmes · 2019-07-11T07:41:20Z

/assign @dgerd

markusthoemmes

I'd expect some test to change. Should we have a unit test that calls SetDefaults and then Validate to confirm this works as expected? Maybe our validation tests should call SetDefaults anyway because that's ultimately the flow it's used in real code.

nak3 · 2019-07-11T11:05:51Z

Thank you. Agreed. Current unit test already has the check, but we get wrong error actually.

serving/pkg/apis/serving/v1beta1/revision_validation_test.go

Line 219 in d5e39e6

Updated.

knative-metrics-robot · 2019-07-11T11:06:57Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1beta1/revision_defaults.go	87.5%	88.2%	0.7

markusthoemmes

Thanks @nak3!

/approve

Leaving for @dgerd to leave a LGTM.

dgerd · 2019-07-11T17:41:35Z

pkg/apis/serving/v1beta1/revision_defaults.go

@@ -36,6 +36,13 @@ func (rts *RevisionTemplateSpec) SetDefaults(ctx context.Context) {

 // SetDefaults implements apis.Defaultable
 func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
+	// More than one container is not allowed for now. Do not set default


While this works, it might be more appropriate to instead make it so that the code below sets defaults for all containers passed in through rs.PodSpec.Containers.

This change creates coupling between defaulting and validation. If we have the defaulter just apply defaults to the array of containers passed in we keep the separation of concerns between the defaulter and the validator.

WDYT?

👍 I was debating to post a similar thing.

nak3 · 2019-07-13T11:44:16Z

Thank you @dgerd and @markusthoemmes for the advice. I agreed and updated this PR.

nak3 · 2019-07-13T11:48:04Z

pkg/apis/serving/v1beta1/revision_defaults.go

-	if len(rs.PodSpec.Containers) == 1 {
-		container = rs.PodSpec.Containers[0]
+	if len(rs.PodSpec.Containers) == 0 {
+		rs.PodSpec.Containers = make([]corev1.Container, 1)


I am feeling that we do not need to set defaults when no container is specified, because it fails eventually due to missing image. But I added this as current code does so.

I think I agree with you here. Just because we used to do it doesn't mean we should keep doing it. This seems unnecessary and I would be okay removing it.

markusthoemmes

Seems like the change to the test was swallowed somewhere? Care to add that back in?

nak3 · 2019-07-15T15:30:29Z

Sure, thank you! I added defaulting multiple containers test into defaulter instead of SetDefaults to validator, as they should keep the separation. I hope I did not misunderstand it.

dgerd

Looking good. I think you can remove the 0 container special case and just rebase.

Can you also double check we have sufficient test coverage in the validator unit tests for empty container and multi-containers now that we are not adding the SetDefaults() before the test.

dgerd · 2019-07-16T21:44:52Z

pkg/apis/serving/v1beta1/revision_defaults.go

-	if len(rs.PodSpec.Containers) == 1 {
-		container = rs.PodSpec.Containers[0]
+	if len(rs.PodSpec.Containers) == 0 {
+		rs.PodSpec.Containers = make([]corev1.Container, 1)


I think I agree with you here. Just because we used to do it doesn't mean we should keep doing it. This seems unnecessary and I would be okay removing it.

…fined If multiple containers are defined but one container is returned after setting default values, inappropriate error happens. To fix it, this patch stops setting default values when unsupported multiple containers are defined.

nak3 · 2019-07-17T13:11:25Z

Thank you. Updated.

Can you also double check we have sufficient test coverage in the validator unit tests for empty container and multi-containers now that we are not adding the SetDefaults() before the test.

Yes, I checked we have the unit tests in both k8s validation and revision validation so it is sufficiently covered.

serving/pkg/apis/serving/k8s_validation_test.go

Line 153 in a9061a1

serving/pkg/apis/serving/k8s_validation_test.go

Line 204 in a9061a1

serving/pkg/apis/serving/v1beta1/revision_validation_test.go

Line 211 in a9061a1

serving/pkg/apis/serving/v1beta1/revision_validation_test.go

Line 219 in a9061a1

dgerd · 2019-07-17T20:55:55Z

/lgtm
/approve

knative-prow-robot · 2019-07-17T20:56:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd, markusthoemmes, nak3

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~~ [dgerd]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 11, 2019

knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/API API objects and controllers labels Jul 11, 2019

knative-prow-robot reviewed Jul 11, 2019

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2019

knative-prow-robot requested review from evankanderson and vaikas July 11, 2019 06:23

markusthoemmes reviewed Jul 11, 2019

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2019

knative-prow-robot assigned dgerd Jul 11, 2019

markusthoemmes reviewed Jul 11, 2019

View reviewed changes

nak3 force-pushed the multi-container branch from 6630998 to b119c64 Compare July 11, 2019 11:03

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 11, 2019

markusthoemmes approved these changes Jul 11, 2019

View reviewed changes

dgerd reviewed Jul 11, 2019

View reviewed changes

nak3 force-pushed the multi-container branch from b119c64 to debf139 Compare July 13, 2019 10:58

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2019

nak3 force-pushed the multi-container branch 2 times, most recently from 001ed32 to 874906f Compare July 13, 2019 11:11

nak3 commented Jul 13, 2019

View reviewed changes

markusthoemmes reviewed Jul 15, 2019

View reviewed changes

nak3 force-pushed the multi-container branch from 874906f to 8cadf36 Compare July 15, 2019 15:24

dgerd reviewed Jul 16, 2019

View reviewed changes

nak3 added 6 commits July 17, 2019 08:22

Add SetDefault to revision validation test

6801d17

Set defaults for all containers passed in thorugh revision

f0e4daf

Add defaulting multiple containers to revision default tests

d5cd54f

Add default probe to multi containers test

6cfd02f

Remove defaulting revision when container is not specified

d25d103

nak3 force-pushed the multi-container branch from 8cadf36 to d25d103 Compare July 17, 2019 00:31

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 17, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019

knative-prow-robot merged commit 6dbec1f into knative:master Jul 17, 2019

nak3 deleted the multi-container branch July 17, 2019 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not set default values when unsupported multiple containers are defined #4709

Do not set default values when unsupported multiple containers are defined #4709

nak3 commented Jul 11, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Jul 11, 2019

nak3 commented Jul 11, 2019

markusthoemmes left a comment

nak3 commented Jul 11, 2019

markusthoemmes commented Jul 11, 2019

markusthoemmes left a comment

nak3 commented Jul 11, 2019 •

edited

Loading

knative-metrics-robot commented Jul 11, 2019

markusthoemmes left a comment

dgerd Jul 11, 2019 •

edited

Loading

markusthoemmes Jul 11, 2019

nak3 commented Jul 13, 2019

nak3 Jul 13, 2019 •

edited

Loading

dgerd Jul 16, 2019

markusthoemmes left a comment

nak3 commented Jul 15, 2019

dgerd left a comment

dgerd Jul 16, 2019

nak3 commented Jul 17, 2019

dgerd commented Jul 17, 2019

knative-prow-robot commented Jul 17, 2019

Do not set default values when unsupported multiple containers are defined #4709

Do not set default values when unsupported multiple containers are defined #4709

Conversation

nak3 commented Jul 11, 2019 • edited Loading

Proposed Changes

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Proposed Changes

knative-prow-robot commented Jul 11, 2019

nak3 commented Jul 11, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

nak3 commented Jul 11, 2019

markusthoemmes commented Jul 11, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

nak3 commented Jul 11, 2019 • edited Loading

knative-metrics-robot commented Jul 11, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

dgerd Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

markusthoemmes Jul 11, 2019

Choose a reason for hiding this comment

nak3 commented Jul 13, 2019

nak3 Jul 13, 2019 • edited Loading

Choose a reason for hiding this comment

dgerd Jul 16, 2019

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

nak3 commented Jul 15, 2019

dgerd left a comment

Choose a reason for hiding this comment

dgerd Jul 16, 2019

Choose a reason for hiding this comment

nak3 commented Jul 17, 2019

dgerd commented Jul 17, 2019

knative-prow-robot commented Jul 17, 2019

nak3 commented Jul 11, 2019 •

edited

Loading

nak3 commented Jul 11, 2019 •

edited

Loading

dgerd Jul 11, 2019 •

edited

Loading

nak3 Jul 13, 2019 •

edited

Loading