Incorrect capitalization in fields treated differently b/w first creation + updates #526

bobcatfish · 2020-04-07T16:31:25Z

Expected Behavior

When I make a change to a TriggerTemplate it should change!

Actual Behavior

When I apply a TriggerTemplate and subsequently try to make changes to it, the changes are never reflected in the state of the TriggerTemplate! I have to delete the TriggerTemplate and re-apply it to see the change.

Steps to Reproduce the Problem

Apply a Trigger template such as:

---
apiVersion: triggers.tekton.dev/v1alpha1
kind: TriggerTemplate
metadata:
  name: unused-triggertemplate
spec:
  resourceTemplates:
  - apiVersion: tekton.dev/v1beta1
    kind: PipelineRun
    metadata:
      name: sad-pr-$(uid)
    spec:
      pipelineSpec:
        tasks:
        - name: be-sad
          taskSpec:
            steps:
            - name: sad
              image: ubuntu
              script: |
                #/bin/bash
                set -ex
                echo "happy"

Make a change to the template and apply, e.g.:

---
apiVersion: triggers.tekton.dev/v1alpha1
kind: TriggerTemplate
metadata:
  name: unused-triggertemplate
spec:
  resourceTemplates:
  - apiVersion: tekton.dev/v1beta1
    kind: PipelineRun
    metadata:
      name: sad-pr-$(uid)
    spec:
      pipelineSpec:
        tasks:
        - name: be-sad
          taskSpec:
            steps:
            - name: sad
              image: ubuntu
              script: |
                #/bin/bash
                set -ex
                echo "preplexed or is it perplex"

Retrieve the TriggerTemplate with kubectl - and note that it still uses the first value (i.e. "happy" in the case above) but bizarrely the last-applied-configuration does reflect the values we tried to change (i.e. "perplexed" above), e.g:

k get triggertemplates -o yaml unused-triggertemplate

apiVersion: triggers.tekton.dev/v1alpha1
kind: TriggerTemplate
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"triggers.tekton.dev/v1alpha1","kind":"TriggerTemplate","metadata":{"annotations":{},"name":"unused-triggertemplate","namespace":"default"},"spec":{"resourceTemplates":[{"apiVersion":"tekton.dev/v1beta1","kind":"PipelineRun","metadata":{"name":"sad-pr-$(uid)"},"spec":{"pipelineSpec":{"tasks":[{"name":"be-sad","taskSpec":{"steps":[{"image":"ubuntu","name":"sad","script":"#/bin/bash\nset -ex\necho \"preplexed or is it perplex\"\n"}]}}]}}}]}}
  creationTimestamp: "2020-04-07T16:25:09Z"
  generation: 1
  name: unused-triggertemplate
  namespace: default
  resourceVersion: "20694699"
  selfLink: /apis/triggers.tekton.dev/v1alpha1/namespaces/default/triggertemplates/unused-triggertemplate
  uid: 808ade7d-acc1-41f9-9f36-599e88b444f2
spec:
  resourcetemplates:
  - apiVersion: tekton.dev/v1beta1
    kind: PipelineRun
    metadata:
      name: sad-pr-$(uid)
    spec:
      pipelineSpec:
        tasks:
        - name: be-sad
          taskSpec:
            steps:
            - image: ubuntu
              name: sad
              script: |
                #/bin/bash
                set -ex
                echo "happy"

Note that I am not even using an eventlistener or referring to unused-triggertemplate anywhere! At first I thought the event listener might be doing this but it's not involved at all afaik!

(I am using the latest triggers from HEAD b44648c)

Additional Info

My only guess for what could be causing this is a webhook? I'm kinda stumped tho!

Also this might be something I'm not understanding that I need to do differently and not a bug at all? However I haven't seen this behavior with any other CRDs so not sure!

The text was updated successfully, but these errors were encountered:

bobcatfish · 2020-04-07T16:31:58Z

p.s. I thought this was just me until I paired with @sbwsg and he ran into it as well

dibyom · 2020-04-07T17:06:32Z

Does this only happen if you change something in the resourceTemplate. If this happens when you change something in the metadata/params as well, then this is definitely very bizarre

ghost · 2020-04-07T17:27:27Z

Ah, interesting! So... If I add a new param to the trigger template then I see this update straight away after applying it.

However, if I rename a workspace (for example) in the resourceTemplate and apply it then that change ends up in the kubectl.kubernetes.io/last-applied-configuration JSON but does not appear in the resourceTemplate itself.

bobcatfish · 2020-04-07T17:32:49Z

This is wild! If I change a param and a resource template field at the same time, the param change takes and the resource template field only changes in the last-updated-configuration :O

@dibyom sounds like this makes sense to you somehow??

dibyom · 2020-04-07T18:18:49Z

My hunch is that it has something to do with just the resourceTemplates since they get stored as "rawMessage"/"unstructured" unlike the regular fields

dibyom · 2020-04-07T18:18:54Z

/kind bug

dibyom · 2020-04-08T19:29:51Z

probably related: kubernetes/kubernetes#64612

bobcatfish · 2020-04-08T19:33:31Z

In the repro case we were using resourceTemplates instead of resourcetemplates 😅 Looks like if you create a CRD with a field having the wrong case, it gets automatically corrected, but apply afterward doesnt do that b/c it just generates a patch containing the field with the wrong capitalization which is then IGNORED X'D

It's possible that open API validation will help here, some related issues from pipeline:

Request to add validation: Add structural OpenAPI schema to Tekton CRDs pipeline#1461
PR to add the validation, Add CRD OpenAPI validation for Pipeline, PipelineResource, Task pipeline#1179 which made us concerned we'd have a mismatch between our validation + the schema
WIP to generate this automatically wip: Generate validate OpenAPIV3Schema 🏫 pipeline#1776

bobcatfish · 2020-04-08T19:34:03Z

@dibyom tried adding openapi validation, but it seemed to be ignored unless we disabled the webhook 😨

ghost · 2020-04-08T19:51:55Z

Wow, great sleuthing!

dibyom · 2020-04-08T20:37:51Z

Expanding on @bobcatfish 's comment above, here is what I tried:

Add openAPI schema validation to config/300-triggertemplates.yaml:

  validation:
    openAPIV3Schema:
      properties:
        spec:
          required:
          - resourcetemplates

Applied the CRD and then tried to create a TriggerTemplate with a resourceTemplates field. This should have failed due to the above validation but did not i.e. the template got created. Interestingly, the if you get the tempalte, the field is now all lower-case resourcetemplate.
I removed our webooks by running k delete -f config/500-webhooks.yaml. Now the apiSchema validation seemed to work and we cannot create the trigger template anymore: spec.resourcetemplates in body is required

I don't know enough about the webhooks workflow, but it seems like the knative webhook field name checks should be case sensitive?

(Also, @bobcatfish could repro this on Pipelines as well!)

tekton-robot · 2020-08-13T23:34:14Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

tekton-robot · 2020-08-13T23:34:14Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot · 2020-08-13T23:34:14Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot · 2020-08-13T23:34:16Z

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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 · 2020-11-17T21:18:07Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

ghost · 2021-11-01T19:35:45Z

/label priority/important-longterm

dibyom · 2021-11-17T20:00:05Z

Verified that this is still an issue. I think adding openAPI schemas would be the ideal long term way of solving this so I'm going to try to add openAPI schemas for Triggers

dibyom · 2021-11-17T22:00:00Z

Update: I was able to generate a go file that contains the openAPI structs along with a bunch of OpenAPI violations. Now I'm not sure how to get these into the CRD yaml in some automated fashion.

go run k8s.io/kube-openapi/cmd/openapi-gen \
    --input-dirs ./pkg/apis/triggers/v1beta1,knative.dev/pkg/apis,knative.dev/pkg/apis/duck/v1beta1 \
    --output-package ./pkg/apis/triggers/v1beta1 -o ./ \
    --go-header-file hack/boilerplate/boilerplate.go.txt

Vincent's branch came in handy: tektoncd/pipeline@main...vdemeester:schema-gen

My branch is here: https://github.com/tektoncd/triggers/compare/main...dibyom:openapi?expand=1

dibyom · 2021-11-17T22:44:39Z

Ok, it kinda works 🎉

 controller-gen \
  schemapatch:manifests=config/resources,generateEmbeddedObjectMeta=true \
  output:dir=config/ \
  paths=./pkg/apis/...

Main issue is with the URL type:

 controller-gen \
  schemapatch:manifests=config/resources,generateEmbeddedObjectMeta=true \
  output:dir=config/ \
  paths=./pkg/apis/...
/usr/local/opt/go/libexec/src/net/url/url.go:359:2: encountered struct field "Scheme" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:360:2: encountered struct field "Opaque" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:361:2: encountered struct field "User" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:362:2: encountered struct field "Host" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:363:2: encountered struct field "Path" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:364:2: encountered struct field "RawPath" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:365:2: encountered struct field "ForceQuery" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:366:2: encountered struct field "RawQuery" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:367:2: encountered struct field "Fragment" without JSON tag in type "URL"
/usr/local/opt/go/libexec/src/net/url/url.go:368:2: encountered struct field "RawFragment" without JSON tag in type "URL"
Error: not all generators ran successfully
run `controller-gen schemapatch:manifests=config/resources,generateEmbeddedObjectMeta=true output:dir=config/ paths=./pkg/apis/... -w` to see all available markers, or `controller-gen schemapatch:manifests=config/resources,generateEmbeddedObjectMeta=true output:dir=config/ paths=./pkg/apis/... -h` for usage

dibyom · 2021-11-17T22:50:01Z

k apply -f config/300-eventlistener.yaml
The CustomResourceDefinition "eventlisteners.triggers.tekton.dev" is invalid: metadata.annotations: Too long: must have at most 262144 bytes

😞

(looks like we need some custom schemapatch after all)

EDIT: Custom schemapatch works. Remaining work is:

Correcting schemapatch file to allow only the podTemplate fields we allow
Verifying each CRD can be applied
Verifying this actually fixes the problem in this issue
Fixing up the scripts/additional dependencies that I added.

This commit adds a hack/update-schemas.sh script that will generate OpenAPI specs for v1beta1 Triggers CRDs and then patch the CRD definitions in the config/ folder with these specs. To enable these, we now have stub CRD definitions for all types in config/resources that this script uses to generate the fully formed CRDs with OpenAPI schema. Since the EventListener allows PodSpec types, we use the hack/schema-config.yaml file to filter down the subset of `PodSpec` and related fields that we allow in Triggers. Addresses tektoncd#526 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

dibyom · 2021-11-22T18:52:12Z

So, the OpenAPI fix works but this does mean a BREAKING CHANGE if we merge the PR. The old behavior where we allowed wrong cases (e.g. resourceTemplates) will now throw an error.

I wonder if there is a way to enable the schemas without this breaking change for now (while we announce and deprecate the old behavior).

dibyom · 2021-11-24T18:37:49Z

From today's WG:

The inconsistent handling of case sensitivity is a bug but fixing it is a BREAKING change.
We'll add OpenAPI schemas but with the option to preserve unknown fields for now which keep the behavior consistent.
We'll add a Deprecation Notice for this behavior and will start validating schemas using OpenAPI 9 months after the Deprecation Notice is sent out.

dibyom · 2021-11-29T18:26:21Z

We need to add x-kubernetes-embedded-resource: true to fix resourceTemplate validation
Suggestion from @wlynch - Return a list of warnings to the user from admission controller (while also allowing unknown fields)
does not seem to work by just adding it to the type as a comment. Alternative to try - implementing the interface: https://github.com/kubernetes/kube-openapi/tree/master/pkg/generators

lbernick · 2021-12-13T20:41:24Z

may be fixed by addressing #1271

tekton-robot · 2022-03-13T20:42:28Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot · 2022-06-19T20:13:07Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot · 2022-07-19T21:10:56Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

khrm · 2022-07-20T11:05:51Z

Is this resolved and can be closed? @dibyom

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 7, 2020

bobcatfish mentioned this issue Apr 8, 2020

Test GCS Tasks tektoncd/catalog#262

Merged

3 tasks

bobcatfish mentioned this issue Apr 22, 2020

Add support for conversion webhook 🍸 tektoncd/pipeline#2413

Merged

3 tasks

tekton-robot closed this as completed Aug 13, 2020

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 13, 2020

tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 13, 2020

dibyom reopened this Aug 14, 2020

dibyom removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 19, 2020

bobcatfish mentioned this issue Oct 15, 2020

v0.17.0 Pipeline can't be Run; it has an invalid spec: missing field(s): tasks[1].when[0] tektoncd/pipeline#3382

Closed

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 17, 2020

tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 15, 2021

bobcatfish added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Nov 1, 2021

bobcatfish changed the title ~~TriggerTemplates getting overwritten??~~ Incorrect capitalization in fields treated differently b/w first creation + updates Nov 1, 2021

dibyom removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 2, 2021

dibyom self-assigned this Nov 8, 2021

dibyom mentioned this issue Nov 22, 2021

Generate OpenAPI spec for Trigger CRDs #1265

Closed

4 tasks

bobcatfish mentioned this issue Feb 16, 2022

Create a Quickstart to run a Tekton task with Minikube tektoncd/website#298

Merged

2 tasks

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2022

dibyom removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 21, 2022

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 19, 2022

tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 19, 2022

khrm removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 20, 2022

dibyom closed this as completed Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect capitalization in fields treated differently b/w first creation + updates #526

Incorrect capitalization in fields treated differently b/w first creation + updates #526

bobcatfish commented Apr 7, 2020

bobcatfish commented Apr 7, 2020

dibyom commented Apr 7, 2020 •

edited

Loading

ghost commented Apr 7, 2020

bobcatfish commented Apr 7, 2020

dibyom commented Apr 7, 2020

dibyom commented Apr 7, 2020

dibyom commented Apr 8, 2020

bobcatfish commented Apr 8, 2020

bobcatfish commented Apr 8, 2020

ghost commented Apr 8, 2020

dibyom commented Apr 8, 2020 •

edited

Loading

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Nov 17, 2020

ghost commented Nov 1, 2021

dibyom commented Nov 17, 2021

dibyom commented Nov 17, 2021 •

edited

Loading

dibyom commented Nov 17, 2021

dibyom commented Nov 17, 2021 •

edited

Loading

dibyom commented Nov 22, 2021 •

edited

Loading

dibyom commented Nov 24, 2021

dibyom commented Nov 29, 2021 •

edited

Loading

lbernick commented Dec 13, 2021

tekton-robot commented Mar 13, 2022

tekton-robot commented Jun 19, 2022

tekton-robot commented Jul 19, 2022

khrm commented Jul 20, 2022

Incorrect capitalization in fields treated differently b/w first creation + updates #526

Incorrect capitalization in fields treated differently b/w first creation + updates #526

Comments

bobcatfish commented Apr 7, 2020

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Additional Info

bobcatfish commented Apr 7, 2020

dibyom commented Apr 7, 2020 • edited Loading

ghost commented Apr 7, 2020

bobcatfish commented Apr 7, 2020

dibyom commented Apr 7, 2020

dibyom commented Apr 7, 2020

dibyom commented Apr 8, 2020

bobcatfish commented Apr 8, 2020

bobcatfish commented Apr 8, 2020

ghost commented Apr 8, 2020

dibyom commented Apr 8, 2020 • edited Loading

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Nov 17, 2020

ghost commented Nov 1, 2021

dibyom commented Nov 17, 2021

dibyom commented Nov 17, 2021 • edited Loading

dibyom commented Nov 17, 2021

dibyom commented Nov 17, 2021 • edited Loading

dibyom commented Nov 22, 2021 • edited Loading

dibyom commented Nov 24, 2021

dibyom commented Nov 29, 2021 • edited Loading

lbernick commented Dec 13, 2021

tekton-robot commented Mar 13, 2022

tekton-robot commented Jun 19, 2022

tekton-robot commented Jul 19, 2022

khrm commented Jul 20, 2022

dibyom commented Apr 7, 2020 •

edited

Loading

dibyom commented Apr 8, 2020 •

edited

Loading

dibyom commented Nov 17, 2021 •

edited

Loading

dibyom commented Nov 17, 2021 •

edited

Loading

dibyom commented Nov 22, 2021 •

edited

Loading

dibyom commented Nov 29, 2021 •

edited

Loading