Disallow unknown fields from resources struct #1081

vdemeester · 2019-07-16T08:00:17Z

Changes

Before this change, it is possible to passe any additionnal field (in
the yaml/json form) with Tekton resources. This is now disallowed.

Closes #297

cc @bobcatfish

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

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 and TaskRun to build and release this image

Release Notes

Unknown fields in resource YAML/JSON will now be rejected when running "kubectl apply"

Before this change, it is possible to passe any additionnal field (in the yaml/json form) with Tekton resources. This is now disallowed. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2019-07-16T08:00:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

vdemeester · 2019-07-16T08:01:19Z

/hold
/cc @bobcatfish @abayer @dlorenc @sbwsg

vdemeester · 2019-07-16T09:13:10Z

examples/pipelineruns/pipelinerun.yaml

-    clusters:
-    - name: targetCluster
-      description: Not yet used, kubectl command below should use this cluster
+    #clusters:


Wasn't sure where this comes from. I am guessing we should remove this altogether most likely 👼

oh wow! yeah feel free to remove it, this is leftover from a really old version of the spec XD

forgot to update before removing the hold 😅

Could you perhaps have a test, which actually validates this behavior. Would feel better when not only the happy-paths are tested.

@abergmeier yes, working on it (once I land somewhere 🤭)

ghost · 2019-07-16T11:37:07Z

This is great. I have definitely been confused by this behaviour before.

/lgtm

bobcatfish · 2019-07-16T12:16:28Z

Closes #297

ermagherd so cool! I'm glad we can fix this!!

bobcatfish

This is great!!!

I wonder if we need to cherry-pick it into 0.5.2 however. If you feel strongly I'm fine with doing it, but considering that we have to keep fixing 0.5 I feel like it'd be good to minimize the risk in stuff we cherry pick into the release and this kinda feels like a new feature?

/lgtm
/meow space

bobcatfish · 2019-07-16T12:17:07Z

cmd/webhook/main.go

-		Logger: logger,
-
+		Logger:                logger,
+		DisallowUnknownFields: true,


For my own curiosity: is this a feature that got added somewhere recently, or something we could have been using all along?

and p.s. if it's not new, do you happen to know why ppl looking at this issue before thought we couldn't easily fix it? (#297 (comment))

It was added with knative/pkg#338, so ~4month ago 😉

bobcatfish · 2019-07-16T12:17:17Z

examples/pipelineruns/clustertask-pipelinerun.yaml

@@ -27,6 +27,4 @@ metadata:
 spec:
  pipelineRef:
    name: sample-pipeline-cluster-task-4
-  trigger:
-    type: manual


bobcatfish · 2019-07-16T12:17:37Z

examples/pipelineruns/pipelinerun.yaml

-    clusters:
-    - name: targetCluster
-      description: Not yet used, kubectl command below should use this cluster
+    #clusters:


oh wow! yeah feel free to remove it, this is leftover from a really old version of the spec XD

tekton-robot · 2019-07-16T12:19:41Z

@bobcatfish:

In response to this:

This is great!!!

I wonder if we need to cherry-pick it into 0.5.2 however. If you feel strongly I'm fine with doing it, but considering that we have to keep fixing 0.5 I feel like it'd be good to minimize the risk in stuff we cherry pick into the release and this kinda feels like a new feature?

/lgtm
/meow space

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.

ghost · 2019-07-16T12:21:47Z

Looking at this once more, minor suggestion for the release note: At the moment it reads Unknown field in resources are now disabled. I suggest updating this to something a bit more specific like Unknown fields in resource YAML/JSON will now be rejected when running "kubectl apply". This feels almost like a breaking change to me since yamls that used to work now may not in the next version so I wonder if some stronger language may help message that.

vdemeester · 2019-07-16T12:28:06Z

Interesting failure 😓

I0716 11:40:40.841] >>>> taskrun list-file
I0716 11:40:45.162] Tailing logs for taskrun: list-file...
I0716 12:18:23.709] panic: interface conversion: runtime.Object is nil, not *v1.Pod
I0716 12:18:23.710] 
I0716 12:18:23.710] goroutine 39 [running]:
I0716 12:18:23.710] github.com/tektoncd/pipeline/test/logs/pod.(*Watcher).Start.func1(0x1208620, 0xc000048060, 0x11f89c0, 0xc000416ba0, 0xc0004167b0)
I0716 12:18:23.710] 	/go/src/github.com/tektoncd/pipeline/test/logs/pod/watcher.go:59 +0x19f
I0716 12:18:23.711] created by github.com/tektoncd/pipeline/test/logs/pod.(*Watcher).Start
I0716 12:18:23.711] 	/go/src/github.com/tektoncd/pipeline/test/logs/pod/watcher.go:51 +0x285
I0716 12:18:23.715] exit status 2
I0716 12:18:23.742] >>>> taskrun multiple-build-push-kaniko-run

@sbwsg thx 😻 I'll update 😉

vdemeester · 2019-07-16T12:34:16Z

I0716 12:20:51.287] {"level":"error","logger":"controller.taskrun-controller","caller":"taskrun/taskrun.go:238","msg":"Failed to validate taskrun \"list-file\": invalid input resources: TaskRun's declared resources didn't match usage in Task: Didn't provide required values: [rules]","knative.dev/controller":"taskrun-controller","stacktrace":"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun.(*Reconciler).reconcile\n\t/go/src/github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/taskrun.go:238\ngitpro.ttaallkk.top/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun.(*Reconciler).Reconcile\n\t/go/src/github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/taskrun.go:125\ngitpro.ttaallkk.top/tektoncd/pipeline/vendor/github.com/knative/pkg/controller.(*Impl).processNextWorkItem\n\t/go/src/github.com/tektoncd/pipeline/vendor/github.com/knative/pkg/controller/controller.go:330\ngitpro.ttaallkk.top/tektoncd/pipeline/vendor/github.com/knative/pkg/controller.(*Impl).Run.func1\n\t/go/src/github.com/tektoncd/pipeline/vendor/github.com/knative/pkg/controller/controller.go:282"}

Interesting that it failed there but not when applying 🤔 🤦‍♂️
Preparing a follow-up, as we don't validate embedded task specs.

Now that the webhook disallow unknown fields, some examples fails where they didn't before — which is a good thing. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

ghost · 2019-07-16T13:19:12Z

/lgtm

vdemeester · 2019-07-16T13:26:18Z

/hold cancel

Disallow unknown fields from resources struct

44d5041

Before this change, it is possible to passe any additionnal field (in the yaml/json form) with Tekton resources. This is now disallowed. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot requested review from dlorenc and imjasonh July 16, 2019 08:00

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

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2019

tekton-robot requested review from abayer, bobcatfish and a user July 16, 2019 08:01

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2019

vdemeester commented Jul 16, 2019

View reviewed changes

vdemeester force-pushed the webhook-disallow-unknow-fields branch 3 times, most recently from 66e640b to d229fb2 Compare July 16, 2019 11:18

vdemeester added this to the Pipelines 0.5 🐱 milestone Jul 16, 2019

tekton-robot assigned ghost Jul 16, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

bobcatfish reviewed Jul 16, 2019

View reviewed changes

tekton-robot assigned bobcatfish Jul 16, 2019

Fix examples yaml that were not valid

8068575

Now that the webhook disallow unknown fields, some examples fails where they didn't before — which is a good thing. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester force-pushed the webhook-disallow-unknow-fields branch from d229fb2 to 8068575 Compare July 16, 2019 12:35

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2019

vdemeester mentioned this pull request Jul 16, 2019

Add validation of embedded TaskSpec in a TaskRun #1084

Merged

3 tasks

tekton-robot merged commit 744df2d into tektoncd:master Jul 16, 2019

vdemeester deleted the webhook-disallow-unknow-fields branch July 16, 2019 13:28

vdemeester mentioned this pull request Jul 16, 2019

Clusters is not used nor allowed, let's remove it for now. #1086

Merged

3 tasks

vdemeester removed this from the Pipelines 0.5 🐱 milestone Jul 16, 2019

vdemeester mentioned this pull request Jul 17, 2019

Error: resource name may not be empty #1096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow unknown fields from resources struct #1081

Disallow unknown fields from resources struct #1081

vdemeester commented Jul 16, 2019 •

edited

Loading

tekton-robot commented Jul 16, 2019

vdemeester commented Jul 16, 2019

vdemeester Jul 16, 2019

bobcatfish Jul 16, 2019

vdemeester Jul 16, 2019

abergmeier Jul 17, 2019

vdemeester Jul 17, 2019

ghost commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

bobcatfish left a comment

bobcatfish Jul 16, 2019

bobcatfish Jul 16, 2019

vdemeester Jul 16, 2019

bobcatfish Jul 16, 2019

bobcatfish Jul 16, 2019

tekton-robot commented Jul 16, 2019

ghost commented Jul 16, 2019 •

edited by ghost

Loading

vdemeester commented Jul 16, 2019

vdemeester commented Jul 16, 2019

ghost commented Jul 16, 2019

vdemeester commented Jul 16, 2019

Disallow unknown fields from resources struct #1081

Disallow unknown fields from resources struct #1081

Conversation

vdemeester commented Jul 16, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 16, 2019

vdemeester commented Jul 16, 2019

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

ghost commented Jul 16, 2019

bobcatfish commented Jul 16, 2019

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

tekton-robot commented Jul 16, 2019

ghost commented Jul 16, 2019 • edited by ghost Loading

vdemeester commented Jul 16, 2019

vdemeester commented Jul 16, 2019

ghost commented Jul 16, 2019

vdemeester commented Jul 16, 2019

vdemeester commented Jul 16, 2019 •

edited

Loading

ghost commented Jul 16, 2019 •

edited by ghost

Loading