Generated defaulters for Revision #1236

grantr · 2018-06-15T20:32:14Z

/area api
/kind cleanup

Proposed Changes

Replace the webhook defaulter that sets ServingState to Active with a scheme-level defaulter. The scheme defaulter can be used anywhere the generated client is available, allowing defaults to be set without depending on the webhook. This is currently a WIP because the defaulters aren't actually used yet.

Background on generated defaulters

Defaulters are a feature of runtime.Scheme that can set default values
for any runtime.Object. We can use kubernetes/code-generator's
defaulter-gen to generate defaulters for use with CRDs like Revision.

The // +k8s:defaulter-gen=TypeMeta comment tells defaulter-gen to generate defaulters for every struct with a TypeMeta field. In this case, we want to set the ServingState of a RevisionSpec to Active by default, so we need a defaulter for Revision since RevisionSpec doesn't embed TypeMeta.

The actual defaulting function is SetDefault_Revision. The generated code makes it easy to hook this function into the Scheme.

To set defaults on an object, import the clientset's generated scheme package and call scheme.Scheme.Default(object). This is demonstrated in the example in defaults.go. The test is unable to use the generated scheme because of an import cycle, so it creates a new scheme.

Issues

For some reason, code-generator's generate-groups.sh script doesn't generate defaulters (even though the usage doc says it does), so I had to generate them in hack/update-codegen.sh manually. I asked about this in kubernetes/code-generator#46. Hopefully this is a bug and not an intentional omission; if intentional I hope to get some guidance on why defaulters are not useful for CRDs (called "external objects" in code-generator).

@smarterclayton are defaulters the right tool here, and are we using them correctly?
@mattmoor @dprotaso since we've discussed this before
@drewinglis since you wrote the webhook defaulting code

generate-groups.sh won't generate defaulters, contrary to what the usage docs say. Until it does, this command will generate defaulters for the only package that wants to use them, serving/v1alpha1.

Defaulters are a feature of runtime.Scheme that can set default values for any runtime.Object. We can use kubernetes/code-generator's defaulter-gen to generate defaulters for use with CRDs like Revision. The k8s:defaulter-gen=TypeMeta comment tells defaulter-gen to generate defaulters for every struct with a TypeMeta field. In this case, we want to set the ServingState of a RevisionSpec to Active by default, so we need a defaulter for Revision since RevisionSpec doesn't embed TypeMeta. The actual defaulting function is SetDefault_Revision. The generated code makes it easy to hook this function into the Scheme. To set defaults on an object, import the clientset's generated scheme package and call scheme.Scheme.Default(object). The test can't do this because of an import cycle, so it creates a new scheme.

google-prow-robot · 2018-06-15T20:32:17Z

@grantr: GitHub didn't allow me to request PR reviews from the following users: since, you, wrote, the, webhook, defaulting, code.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Proposed Changes

Replace the webhook defaulter that sets ServingState to Active with a scheme-level defaulter. The scheme defaulter can be used anywhere the generated client is available, allowing defaults to be set without depending on the webhook. This is currently a WIP because the defaulters aren't actually used yet.

Background on generated defaulters

Defaulters are a feature of runtime.Scheme that can set default values
for any runtime.Object. We can use kubernetes/code-generator's
defaulter-gen to generate defaulters for use with CRDs like Revision.

The // +k8s:defaulter-gen=TypeMeta comment tells defaulter-gen to generate defaulters for every struct with a TypeMeta field. In this case, we want to set the ServingState of a RevisionSpec to Active by default, so we need a defaulter for Revision since RevisionSpec doesn't embed TypeMeta.

The actual defaulting function is SetDefault_Revision. The generated code makes it easy to hook this function into the Scheme.

To set defaults on an object, import the clientset's generated scheme package and call scheme.Scheme.Default(object). This is demonstrated in the example in defaults.go. The test is unable to use the generated scheme because of an import cycle, so it creates a new scheme.

Issues

For some reason, code-generator's generate-groups.sh script doesn't generate defaulters (even though the usage doc says it does), so I had to generate them in hack/update-codegen.sh manually. I asked about this in kubernetes/code-generator#46. Hopefully this is a bug and not an intentional omission; if intentional I hope to get some guidance on why defaulters are not useful for CRDs (called "external objects" in code-generator).

@smarterclayton are defaulters the right tool here, and are we using them correctly?
/cc @mattmoor since we've discussed this before
/cc @drewinglis since you wrote the webhook defaulting code

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.

google-prow-robot · 2018-06-15T20:32:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: grantr
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vaikas-google

Assign the PR to them by writing /assign @vaikas-google in a comment when ready.

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:

OWNERS

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

mattmoor

Thanks for doing this!

mattmoor · 2018-06-15T20:35:47Z

hack/update-codegen.sh

+${GOPATH}/bin/defaulter-gen \
+  --input-dirs github.com/knative/serving/pkg/apis/serving/v1alpha1 \
+   -O zz_generated.defaulters \
+   --go-header-file hack/../hack/boilerplate/boilerplate.go.txt


this path is a bit strange :)

Oops, this was copied from the expanded version. Fixed.

mattmoor · 2018-06-15T20:53:15Z

pkg/apis/serving/v1alpha1/defaults.go

+	return RegisterDefaults(scheme)
+}
+
+func SetDefaults_Revision(rev *Revision) {


How is this function discovered?

Somehow defaulter-gen knows that this function exists and generates code that sets up the proper hooks for it. See RegisterDefaults in https://github.com/knative/serving/pull/1236/files#diff-dee345d24fd055f2eeae84b3a53d1ea5R31.

RegisterDefaults is in turn called by addDefaultingFuncs which is given to SchemeBuilder as a scheme building function. I'm not sure why RegisterDefaults can't be given to SchemeBuilder directly, but all the examples I found (The rbac types, among others) do the addDefaultingFuncs thing.

https://www.youtube.com/watch?v=b6zEaA-nAlU

These were copied from the defaulters in pkg/webhook. The webhook defaulters are still used; these new ones remain unused. Moved defaulter tests to a new file and made them table-driven.

grantr · 2018-06-15T22:43:49Z

To keep this PR small, I'll create a separate PR that actually uses these defaulters to replace the webhook ones. This is no longer WIP.

grantr · 2018-06-15T23:14:49Z

On second thought, I'm removing the Configuration and Service defaulters. In both cases, those defaulters were setting defaults on nested revision templates. I think it's better to let the defaults be bound when the templated object is created instead of when the template is created.

Both of these defaulters were setting defaults on nested revision templates. I think it's better to let the defaults be bound when the templated object is created instead of when the template is created.

mattmoor · 2018-06-16T00:37:24Z

pkg/apis/serving/v1alpha1/defaults.go

+	if rev.Spec.ConcurrencyModel == "" {
+		rev.Spec.ConcurrencyModel = RevisionRequestConcurrencyModelMulti
+	}
+}


Did you want to move the call to rev.Status.InitializeConditions to here?

I'm thinking about it. Maybe in a separate PR.

dprotaso · 2018-06-18T16:49:34Z

/lgtm

Curious there's an issue about introducing validation to CRDs using OpenAPIv3 (#1155). OpenAPIv3 spec supports providing defaults for object fields. Do you know if the API server would then set these defaults automatically without the defaulter-gen and the need to use it in the web hook?

grantr · 2018-06-18T17:37:38Z

Do you know if the API server would then set these defaults automatically without the defaulter-gen and the need to use it in the web hook?

I've tried CRD validation with kubebuilder, which generates OpenAPI schemas. The validation is done by the apiserver, no webhook necessary. A webhook might still be required to implement more complex validation that can't be expressed in OpenAPI.

It's unclear to me if Kubernetes will support OpenAPIv3 defaults, but based on the validation implementation, if supported it would probably work the way you propose.

dprotaso · 2018-06-18T18:23:45Z

It's unclear to me if Kubernetes will support OpenAPIv3 defaults

At this point (1.10.2) it does not

Error from server (Invalid): error when creating "crd.yml": 
CustomResourceDefinition.apiextensions.k8s.io "tests.dave.com" is invalid: 
spec.validation.openAPIV3Schema.properties[spec].properties[some-property].default: 
Forbidden: default is not supported

bsnchan · 2018-06-18T19:29:11Z

Looks like defaulting via OpenAPIv3 for CRDs is currently in progress:

kubernetes/kubernetes#63604

grantr · 2018-06-18T21:10:14Z

Great! I'll add a comment and link noting how that feature could replace the generated defaulters.

mattmoor · 2018-07-12T19:42:56Z

I suspect this is abandoned, if not feel free to reopen.

…ically (knative#1236) * [RELEASE-v1.5] Add manifest patch for internal-tls to `openshift/release/artifacts` (knative#1202) * Add secret to 1.5 CI yaml * auto generated * Support config to deploy internal certificates automatically (knative#13005) * Add certificate reconciler for internal certs * Fix cert path * Temporary use local networking repo * Support internal-encryption configuration * Use const for cert name * Fix lint * rm blank line * Drop unused variable * Use one line style * Use one line code * Update net-kourier nightly bumping knative.dev/net-kourier d758682...b9b1e8b: > b9b1e8b Use `internal-encryption` to deploy internal certificates automatically (# 855) > 427434c bump kind and k8s versions in kind-e2e tests (# 859) Signed-off-by: Knative Automation <automation@knative.team> * Verify SecretPKKey as well * Do not drop activator always in the path * Comment about ctrl-ca suffix Co-authored-by: Knative Automation <automation@knative.team> * Update deps * Enable internal-tls on ocp-tls (knative#1203) * Enable internal-tls on OCP 4.8 * Use tls to match JOB name * Add a target to enable internal-tls in Makefile (knative#1224) * Add a target to enable internal-tls in Makefile * Update CI template for internal-tls enabled * Tests for encryption with Kourier local gateway (knative#13263) * Generate Secrets * Commit generated cert-secret.yaml * httpproxy enables tls client * httpproxy uses https when CA_CERT specified * Pass CA_CERT and SERVER_NAME env variables properly to tests * Avoid using cluster-local certificates for external services * Enable tls tests for cluster-local Kourier gateway * Need to create test resources including the test namespace first before installing Knative so that applying test/config/tls/cert-secret.yaml succeeds * TMP: Enable tls in the standard e2e make target - test purposes * Use knative-serving-ingress ns for deploying server-certs * Deploy certificates at test phase * Separate test and install of installing certs * Wait for knative-serving-ingress to exist * Revert "TMP: Enable tls in the standard e2e make target - test purposes" This reverts commit 5bb3549. Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com> Co-authored-by: Knative Automation <automation@knative.team>

grantr added 2 commits June 15, 2018 12:02

Generate defaulters for serving/v1alpha1

94c5ddd

generate-groups.sh won't generate defaulters, contrary to what the usage docs say. Until it does, this command will generate defaulters for the only package that wants to use them, serving/v1alpha1.

google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2018

google-prow-robot requested a review from drewinglis June 15, 2018 20:32

google-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 15, 2018

mattmoor reviewed Jun 15, 2018

View reviewed changes

grantr added 2 commits June 15, 2018 14:00

Unexpand boilerplate path

cd58ee1

Add defaulters for Configuration and Service

03a555a

These were copied from the defaulters in pkg/webhook. The webhook defaulters are still used; these new ones remain unused. Moved defaulter tests to a new file and made them table-driven.

grantr changed the title ~~[WIP] Generated defaulter for Revision~~ [WIP] Generated defaulters for Revision, Configuration, and Service Jun 15, 2018

grantr changed the title ~~[WIP] Generated defaulters for Revision, Configuration, and Service~~ Generated defaulters for Revision, Configuration, and Service Jun 15, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2018

grantr changed the title ~~Generated defaulters for Revision, Configuration, and Service~~ Generated defaulters for Revision Jun 15, 2018

Remove Configuration and Service defaulters

8143387

Both of these defaulters were setting defaults on nested revision templates. I think it's better to let the defaults be bound when the templated object is created instead of when the template is created.

grantr requested a review from dprotaso June 16, 2018 00:06

mattmoor reviewed Jun 16, 2018

View reviewed changes

google-prow-robot assigned dprotaso Jun 18, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2018

dprotaso approved these changes Jun 18, 2018

View reviewed changes

dprotaso mentioned this pull request Jun 18, 2018

Incorporate OpenAPI schema into our CRD definitions. #1155

Closed

grantr mentioned this pull request Jun 18, 2018

[WIP] Use generated defaulters instead of webhook defaulters #1262

Closed

mattmoor closed this Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generated defaulters for Revision #1236

Generated defaulters for Revision #1236

grantr commented Jun 15, 2018 •

edited

Loading

google-prow-robot commented Jun 15, 2018

Proposed Changes

Background on generated defaulters

Issues

google-prow-robot commented Jun 15, 2018

mattmoor left a comment

mattmoor Jun 15, 2018

grantr Jun 15, 2018

mattmoor Jun 15, 2018

grantr Jun 15, 2018

mattmoor Jun 15, 2018

grantr commented Jun 15, 2018

grantr commented Jun 15, 2018

mattmoor Jun 16, 2018

grantr Jun 16, 2018

dprotaso commented Jun 18, 2018

grantr commented Jun 18, 2018

dprotaso commented Jun 18, 2018

bsnchan commented Jun 18, 2018

grantr commented Jun 18, 2018

mattmoor commented Jul 12, 2018

Generated defaulters for Revision #1236

Generated defaulters for Revision #1236

Conversation

grantr commented Jun 15, 2018 • edited Loading

Proposed Changes

Background on generated defaulters

Issues

google-prow-robot commented Jun 15, 2018

Proposed Changes

Background on generated defaulters

Issues

google-prow-robot commented Jun 15, 2018

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor Jun 15, 2018

Choose a reason for hiding this comment

grantr Jun 15, 2018

Choose a reason for hiding this comment

mattmoor Jun 15, 2018

Choose a reason for hiding this comment

grantr Jun 15, 2018

Choose a reason for hiding this comment

mattmoor Jun 15, 2018

Choose a reason for hiding this comment

grantr commented Jun 15, 2018

grantr commented Jun 15, 2018

mattmoor Jun 16, 2018

Choose a reason for hiding this comment

grantr Jun 16, 2018

Choose a reason for hiding this comment

dprotaso commented Jun 18, 2018

grantr commented Jun 18, 2018

dprotaso commented Jun 18, 2018

bsnchan commented Jun 18, 2018

grantr commented Jun 18, 2018

mattmoor commented Jul 12, 2018

grantr commented Jun 15, 2018 •

edited

Loading