update the Service schema with v1 schema #9436

joshuawilson · 2020-09-16T17:55:24Z

Releated to #912

Proposed Changes

This is the most basic level of the schema. The intention is create a foundation on which future updates to the schema will build.
This a bespoke schema. I used the docs and the API to make as complete a schema for only v1 as I could. Please note this was manually written, I did not generate it.
Currently the schema has nothing in it. Until the we can auto-generate it, this at least provides something.
Discussions on Incorporate schema into CRDs #912 suggest that we will soon drop support for alpha and beta so we would only need v1 anyway.

Release Note

Updated the Service schema to include a high level basic schema.

knative-prow-robot · 2020-09-16T17:55:37Z

Welcome @joshuawilson! It looks like this is your first PR to knative/serving 🎉

knative-prow-robot · 2020-09-16T17:55:37Z

Hi @joshuawilson. 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 · 2020-09-17T01:46:05Z

/cc @mattmoor @markusthoemmes @dprotaso

dprotaso · 2020-09-17T14:33:14Z

/ok-to-test

config/core/300-resources/service.yaml

dprotaso · 2020-09-17T14:56:20Z

config/core/300-resources/service.yaml

+        required:
+        - apiVersion
+        - kind
+        - metadata


I don't think definitions for these properties are necessary?

I know that they are expected but if we don't list them then any validation tool would not know they are required.

I removed these 3 objects completely.

dprotaso · 2020-09-17T17:06:34Z

config/core/300-resources/service.yaml

+                          deletionTimestamp is also set.
+                          \nMay only be shortened.
+                          \nRead-only."
+                      deletionTimestamp:


There's a bunch of properties like deletionTimestamp that we don't want to accept in the template

I think I found it here: https://github.com/knative/serving/blob/master/vendor/k8s.io/apimachinery/pkg/api/meta/meta.go#L131

If it's not to be used should it be removed from the API? Or Is this file not a part of the API and I should not be using this file?

I left this in waiting for further clarification.

Is there a way to push this into the tooling?

That would simplify things since across the Knative project there are probably close to ~50 CRDs

seems like k8s rejects this over-specification

see the e2e integration checks where installation fails

config/core/300-resources/service.yaml

dprotaso · 2020-09-17T17:22:49Z

config/core/300-resources/service.yaml

@@ -59,6 +59,1129 @@ spec:
  - << : *version
    name: v1
    storage: true
+    schema:
+      openAPIV3Schema:
+        x-kubernetes-preserve-unknown-fields: true


does this apply to nested properties now?

I think it applies to everything under it. I left it in place because I don't have a way to verify that I have a perfect match to the API.

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#controlling-pruning

Digging into the example you linked it seems like spec.something was pruned.

This means that the something field in the specified spec object is pruned, but everything outside is not.

It seems like x-kubernetes-preserve-unknown-fields doesn't apply to nested properties

I'll admit that I'm not entirely sure what it does. I thought it effected the whole tree based on
By default, all unspecified fields for a custom resource, across all versions, are pruned. It is possible though to opt-out of that for specifc sub-trees of fields by adding x-kubernetes-preserve-unknown-fields: true

for specifc sub-trees of fields

it seems like if you have a nested properties attribute pruning is enabled again

thus I wonder if we might need x-kubernetes-preserve-unknown-fields in a few places if we aren't specifying the full schema

I was wondering if we don't need it at all since I was trying to list out the full schema. Did I miss something?
I'm also fine with adding it multiple places.
WDTY?

yeah if the full schema is present we shouldn't need it

dprotaso · 2020-09-17T17:31:18Z

weird my comment went missing - I did a quick pass some initial feedback

codecov · 2020-09-21T19:04:20Z

Codecov Report

Merging #9436 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9436   +/-   ##
=======================================
  Coverage   87.81%   87.81%           
=======================================
  Files         183      183           
  Lines        8631     8631           
=======================================
  Hits         7579     7579           
  Misses        801      801           
  Partials      251      251

Impacted Files	Coverage Δ
pkg/reconciler/autoscaling/kpa/scaler.go	`88.57% <0.00%> (-1.43%)`	⬇️
pkg/reconciler/configuration/configuration.go	`88.28% <0.00%> (+1.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec7769b...b29aeb0. Read the comment docs.

knative-test-reporter-robot · 2020-09-22T22:06:37Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-autotls-tests	2020-09-22 21:38:54.96 +0000 UTC 2020-09-22 21:52:24.967 +0000 UTC	2/3
pull-knative-serving-integration-tests	2020-09-22 21:39:24.932 +0000 UTC 2020-09-22 21:52:54.775 +0000 UTC 2020-09-22 22:06:24.787 +0000 UTC	3/3
pull-knative-serving-upgrade-tests	2020-09-22 21:48:25.036 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

knative-test-reporter-robot · 2020-09-22T22:06:37Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-autotls-tests	2020-09-22 21:38:54.96 +0000 UTC 2020-09-22 21:52:24.967 +0000 UTC 2020-09-22 22:06:24.787 +0000 UTC	3/3
pull-knative-serving-integration-tests	2020-09-22 21:39:24.932 +0000 UTC 2020-09-22 21:52:54.775 +0000 UTC	2/3
pull-knative-serving-upgrade-tests	2020-09-22 21:48:25.036 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-autotls-tests

joshuawilson · 2020-09-23T13:54:09Z

/retest

markusthoemmes · 2020-09-23T14:55:31Z

config/core/300-resources/service.yaml

@@ -662,7 +662,7 @@ spec:
                                  description: |
                                    As specified in Kubernetes core/v1.Probe.
                                  default: 300
-                                  minimum: 0
+                                  minimum: 1


I think it's complaining about the default rather than the minimum. The default should be 0 for Knative too. We have some specialcased behavior for when periodSeconds is set to 0. The tests are failing in our webhook validation.

thanks, I'll update that

dprotaso · 2020-09-23T15:03:01Z

yeah we deviate from some of k8s defaults

you can see our validation code here:
https://github.com/knative/serving/blob/master/pkg/apis/serving/k8s_validation.go

markusthoemmes · 2020-10-07T07:11:19Z

I wonder if we should work on this a bit more incrementally: We could add the top-level Service fields first and let the PodSpec be an object that allows arbitrary keys. We then work our way down step by step to avoid having to debug an enormous schema.

WDYT @joshuawilson?

joshuawilson · 2020-10-07T12:54:33Z

I can try that. Thanks for the suggestion @markusthoemmes

rawc0der · 2020-10-20T19:05:38Z

Any progress on this? I am waiting for this PR for some time 😩.. ( I am extracting the schema from openAPIV3Schema field from chart crds to lint against templates)
We have template validation checks in CI Builds running and currently from a very large stack of projects, only Knative seems to have missed json schema spec for the CRDs.
The CloudNative community is using schema specs in their integrations already!😎✌️

thanks @joshuawilson and please pull harder to get this merged!

joshuawilson · 2020-10-23T23:00:36Z

/retest

joshuawilson · 2020-10-27T04:50:42Z

/retest

markusthoemmes · 2020-10-27T06:30:28Z

config/core/300-resources/service.yaml

+            x-kubernetes-preserve-unknown-fields: true
+            properties:
+              template:
+                x-kubernetes-preserve-unknown-fields: true


Does this (and traffic) lack type: object maybe?

According to the docs, you shouldn't need that if you have the flag I used. But I added it just to see if it would help.
For Traffic it is an array of objects, so set it that way in the latest commit.

dprotaso · 2020-10-27T14:28:48Z

/lgtm

dprotaso · 2020-10-27T14:29:00Z

/approve

knative-prow-robot · 2020-10-27T14:29:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, joshuawilson

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:

~~config/OWNERS~~ [dprotaso]

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

markusthoemmes · 2020-10-27T14:39:15Z

/test pull-knative-serving-unit-tests

flake.

knative-prow-robot · 2020-10-27T14:47:59Z

@joshuawilson: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-serving-integration-tests	`3ec7a64`	link	`/test pull-knative-serving-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

joshuawilson · 2020-10-27T14:51:10Z

/retest

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 16, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 16, 2020

knative-prow-robot requested review from grantr and nak3 September 16, 2020 17:55

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 Sep 16, 2020

nak3 added the area/API API objects and controllers label Sep 17, 2020

knative-prow-robot requested review from dprotaso, markusthoemmes and mattmoor September 17, 2020 01:46

mattmoor assigned dprotaso Sep 17, 2020

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 Sep 17, 2020

dprotaso reviewed Sep 17, 2020

View reviewed changes

joshuawilson force-pushed the service-schema branch from 45c6ae7 to 879d6db Compare September 21, 2020 18:57

markusthoemmes reviewed Sep 23, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2020

joshuawilson added a commit to joshuawilson/serving that referenced this pull request Oct 21, 2020

update the Service schema with v1 schema (knative#9436)

ca6e103

joshuawilson force-pushed the service-schema branch from 75c6ed1 to 2e785c4 Compare October 21, 2020 17:33

joshuawilson added a commit to joshuawilson/serving that referenced this pull request Oct 22, 2020

update the Service schema, add high level fields (knative#9436)

a33af73

joshuawilson force-pushed the service-schema branch from 2e785c4 to a4f758e Compare October 22, 2020 16:42

joshuawilson added a commit to joshuawilson/serving that referenced this pull request Oct 23, 2020

update the Service schema, add high level fields (knative#9436)

fac59c1

joshuawilson force-pushed the service-schema branch from 47b5e74 to fac59c1 Compare October 23, 2020 14:41

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 Oct 23, 2020

update the Service schema, add high level fields (knative#9436)

76ec532

joshuawilson force-pushed the service-schema branch from fac59c1 to 76ec532 Compare October 27, 2020 04:52

markusthoemmes reviewed Oct 27, 2020

View reviewed changes

add type to each element in the service schema

b29aeb0

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 Oct 27, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2020

knative-prow-robot merged commit 3e6dae5 into knative:master Oct 27, 2020

joshuawilson deleted the service-schema branch October 27, 2020 15:35

update the Service schema with v1 schema #9436

update the Service schema with v1 schema #9436

Uh oh!

Conversation

joshuawilson commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Sep 16, 2020

Uh oh!

knative-prow-robot commented Sep 16, 2020

Uh oh!

nak3 commented Sep 17, 2020

Uh oh!

dprotaso commented Sep 17, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuawilson Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Sep 17, 2020

Uh oh!

codecov bot commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

knative-test-reporter-robot commented Sep 22, 2020

Uh oh!

knative-test-reporter-robot commented Sep 22, 2020

Uh oh!

joshuawilson commented Sep 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Sep 23, 2020

Uh oh!

markusthoemmes commented Oct 7, 2020

Uh oh!

joshuawilson commented Oct 7, 2020

Uh oh!

rawc0der commented Oct 20, 2020

Uh oh!

joshuawilson commented Sep 16, 2020 •

edited

Loading

joshuawilson Sep 17, 2020 •

edited

Loading

dprotaso Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

knative-prow-robot commented Oct 27, 2020 •

edited

Loading