Add support for conversion webhook 🍸 #2413

vdemeester · 2020-04-16T11:58:25Z

Changes

This adds a new webhook controller : ConversionController.
The HubVersion is the stored version, and Zygotes stores the types
of supported versions.

The CRDs are updated to setup the conversion strategy to webhook,
pointing to the tekton-pipelines-webhook.

An update in the webhook clusterrole is required for customresourcedefinitions.

Closes #2047

/cc @sbwsg @bobcatfish @mattmoor

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add support for conversion webhook for tektoncd/pipeline CRDs

tekton-robot · 2020-04-16T11:58:28Z

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: mattmoor.

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

In response to this:

Changes

This adds a new webhook controller : ConversionController.
The HubVersion is the stored version, and Zygotes stores the types
of supported versions.

The CRDs are updated to setup the conversion strategy to webhook,
pointing to the tekton-pipelines-webhook.

An update in the webhook clusterrole is required for customresourcedefinitions.

Closes #2047

/cc @sbwsg @bobcatfish @mattmoor

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)

Includes docs (if user facing)

Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes
Add support for conversion webhook for tektoncd/pipeline CRDs

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.

vdemeester · 2020-04-16T13:15:53Z

/test pull-tekton-pipeline-build-tests

vdemeester · 2020-04-16T13:16:10Z

/test pull-tekton-pipeline-integration-tests

mattmoor

cc @dprotaso

This generally LGTM, but calling in the big guns :)

mattmoor · 2020-04-16T13:58:32Z

cmd/webhook/main.go

+
+		// A function that infuses the context passed to ConvertTo/ConvertFrom/SetDefaults with custom metadata
+		func(ctx context.Context) context.Context {
+			return ctx


Did you want the ctx stuff here too?

I followed what was in knative/serving 😝

vdemeester · 2020-04-17T09:16:20Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T10:40:32Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T11:31:59Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T12:07:33Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T12:37:05Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T14:18:21Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-04-20T15:44:05Z

/test pull-tekton-pipeline-integration-tests

dprotaso · 2020-04-20T20:24:40Z

cmd/webhook/main.go

@@ -98,7 +103,7 @@ func NewValidationAdmissionController(ctx context.Context, cmw configmap.Watcher

 		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
 		func(ctx context.Context) context.Context {
-			return ctx
+			return contexts.WithUpgradeViaDefaulting(store.ToContext(ctx))


contexts.WithUpgradeViaDefaulting

Are you using this upgrade marker for anything?

vdemeester · 2020-04-22T08:26:55Z

The error is legit in this PR, see #2468

vdemeester · 2020-04-22T08:40:09Z

/hold

vdemeester · 2020-04-22T13:33:13Z

yay it is green 🌞

bobcatfish

I think all of my feedback is just asking for some more detail in comments - I think it'll be worthwhile for folks like me who don't really know what's going on :D

but i also understand if you want to merge as is and add that afterward - would like to see it tho if possible!

/approve

bobcatfish · 2020-04-22T13:38:35Z

cmd/webhook/main.go

@@ -124,6 +129,68 @@ func NewConfigValidationController(ctx context.Context, cmw configmap.Watcher) *
 	)
 }

+func NewConversionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	// nolint: golint


why do we not want to lint this?

😅 well, I need to find a better name than v1beta1_ for the variable.. and I was lazy 😅

I am not sure what variable name to use to be honest 🙃

bobcatfish · 2020-04-22T13:39:49Z

cmd/webhook/main.go

+		// The path on which to serve the webhook
+		"/resource-conversion",
+
+		// Specify the types of custom resource definitions that should be converted


so the keys are the types to convert TO and the zygotes are the version to convert FROM is that right? im not 100% clear either way tho so maybe some more detail in the comment might help?

bobcatfish · 2020-04-22T13:40:22Z

config/200-clusterrole.yaml

@@ -63,6 +63,12 @@ apiVersion: rbac.authorization.k8s.io/v1
 metadata:
  name: tekton-pipelines-webhook-cluster-access
 rules:
+  # The webhook needs to be able to list and update customresourcedefinitions,
+  # mainly to update the webhook certificates.ku


ku i think is a typo?

It is 🤦‍♂️

bobcatfish · 2020-04-22T13:40:33Z

config/200-clusterrole.yaml

+  - apiGroups: ["apiextensions.k8s.io"]
+    resources: ["customresourcedefinitions", "customresourcedefinitions/status"]
+    verbs: ["get", "list", "update", "patch", "watch"]
+    # verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]


maybe we can remove this line?

Indeed 😅

bobcatfish · 2020-04-22T13:43:07Z

config/300-taskrun.yaml

+      # this is a work around so we don't need to flush out the
+      # schema for each version at this time
+      #
+      # see issue: https://github.com/knative/serving/issues/912


is it possible to get a bit more detail here on how this works? looking at the issue it's not immediately clear to me, and im wondering how x-kubernetes-preserve-unknown-fields affect unknown fields / typos (making me think of tektoncd/triggers#526)

Right preserveUnknownFields and this one seems to be required to work with k8s 1.18 and the conversion webhook somehow.

Updated the comments 👼

bobcatfish · 2020-04-22T13:44:06Z

pkg/apis/pipeline/register.go

+		Group:    GroupName,
+		Resource: "conditions",
+	}
+)


im not 100% clear on why these are required now or where they are used? does the conversion webhook automatically detect these?

We do here : https://github.com/tektoncd/pipeline/pull/2413/files#diff-58de452513b7e8d8d3cfea23eb4ae6a8R170 (and we might do at other parts of the code). It makes it a bit easier to get conditions.tekton.dev or pipelineruns.tekton.dev in code without hard coding those.

tekton-robot · 2020-04-22T13:45:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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

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

vdemeester · 2020-04-23T13:53:33Z

/hold cancel

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

This adds a new webhook controller : ConversionController. The `HubVersion` is the stored version, and Zygotes stores the types of supported versions. The CRDs are updated to setup the conversion strategy to webhook, pointing to the tekton-pipelines-webhook. An update in the webhook clusterrole is required for customresourcedefinitions. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester · 2020-04-27T13:03:24Z

ping @sbwsg @bobcatfish @imjasonh this is ready for review and updated

ghost · 2020-04-27T19:28:44Z

/lgtm

/meow rocketship

tekton-robot · 2020-04-27T19:28:46Z

@sbwsg:

In response to this:

/lgtm

/meow rocketship

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.

tekton-robot requested a review from a user April 16, 2020 11:58

tekton-robot requested a review from bobcatfish April 16, 2020 11:58

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 16, 2020

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

vdemeester force-pushed the 2047-conversion-webhook branch from 8186199 to cb1960c Compare April 16, 2020 12:35

mattmoor reviewed Apr 16, 2020

View reviewed changes

vdemeester mentioned this pull request Apr 17, 2020

Split reconcile into prepare and reconcile #2421

Merged

1 task

vdemeester force-pushed the 2047-conversion-webhook branch from cb1960c to b49e956 Compare April 20, 2020 13:38

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 20, 2020

dprotaso reviewed Apr 20, 2020

View reviewed changes

vdemeester force-pushed the 2047-conversion-webhook branch from b49e956 to da932a2 Compare April 22, 2020 08:07

vdemeester mentioned this pull request Apr 22, 2020

Fix PipelineTask timeout not correctly set 💦 #2468

Merged

3 tasks

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

vdemeester force-pushed the 2047-conversion-webhook branch from da932a2 to f3a8be7 Compare April 22, 2020 12:42

bobcatfish reviewed Apr 22, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020

vdemeester force-pushed the 2047-conversion-webhook branch from f3a8be7 to 0dbd57e Compare April 23, 2020 14:01

vdemeester mentioned this pull request Apr 23, 2020

Refactor to the remote image library and task references #2395

Merged

2 tasks

vdemeester added 2 commits April 24, 2020 09:47

Bump knative.dev to release-0.13 🏑

9bab0be

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester force-pushed the 2047-conversion-webhook branch from 0dbd57e to 82a934d Compare April 24, 2020 07:48

tekton-robot assigned ghost Apr 27, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020

tekton-robot merged commit 8213475 into tektoncd:master Apr 27, 2020

vdemeester deleted the 2047-conversion-webhook branch April 28, 2020 08:24

afrittoli added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 30, 2020

Add support for conversion webhook 🍸 #2413

Add support for conversion webhook 🍸 #2413

Conversation

vdemeester commented Apr 16, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Apr 16, 2020

Changes

Submitter Checklist

Reviewer Notes

Release Notes

vdemeester commented Apr 16, 2020

vdemeester commented Apr 16, 2020

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Apr 17, 2020

vdemeester commented Apr 20, 2020

vdemeester commented Apr 20, 2020

vdemeester commented Apr 20, 2020

vdemeester commented Apr 20, 2020

vdemeester commented Apr 20, 2020

vdemeester commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Apr 22, 2020

vdemeester commented Apr 22, 2020

vdemeester commented Apr 22, 2020

bobcatfish left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 22, 2020

vdemeester commented Apr 23, 2020

vdemeester commented Apr 27, 2020

ghost commented Apr 27, 2020

tekton-robot commented Apr 27, 2020

vdemeester commented Apr 16, 2020 •

edited

Loading