Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faulty Remote Resources are accepted by Remote Resolution #7951

Closed
chitrangpatel opened this issue May 16, 2024 · 1 comment · Fixed by #7952
Closed

Faulty Remote Resources are accepted by Remote Resolution #7951

chitrangpatel opened this issue May 16, 2024 · 1 comment · Fixed by #7952
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@chitrangpatel
Copy link
Member

chitrangpatel commented May 16, 2024

Expected Behavior

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  name: ref-0001
spec:
  pipelineSpec:
    tasks:
    - name: task1
      taskSpec:
        steps:
        - name: step1
          image: ubuntu
          script: |
            echo "Hello World from a TaskSpec!"
    - name: task2
      taskRef:
        resolver: git
        params:
        - name: url
          value: <git-repo>
        - name: revision
          value: main
        - name: pathInRepo
          value: bad-task.yaml
---
# where bad-task looks like:
apiVersion: tekton.dev/v1
kind: Task
metadata:
  name: bad-task
spec:
  steps:
    - image: ubuntu
       foo: bar

Notice that the field foo is not supposed to exist in the above Task.

At runtime, a validation error is expected when the remotely referenced resource is inspected.
In fact, if we kubectl apply the bad yaml to the cluster, we do get a webhook validation error.

Actual Behavior

The remote resolution discards the bad field foo and runs the rest of it so that error is not caught and the Task runs as if there was nothing wrong.

Issue

We use the universal deserializer to unmarshal the bytes to the pipeline struct.

obj, _, err := scheme.Codecs.UniversalDeserializer().Decode(data, nil, nil)

The CodecFactory that is used here does not have serializer.EnableStrict as an argument. Hence, the unmarshilling process zaps the bad field.

Solution

The following diff in the resolver code does the magic:

+       serializer "k8s.io/apimachinery/pkg/runtime/serializer"


@@ -97,7 +98,8 @@ func ResolvedRequest(resolved resolutioncommon.ResolvedResource, err error) (run
-       obj, _, err := scheme.Codecs.UniversalDeserializer().Decode(data, nil, nil)
+       codecs := serializer.NewCodecFactory(scheme.Scheme, serializer.EnableStrict)
+       obj, _, err := codecs.UniversalDeserializer().Decode(data, nil, nil)

@chitrangpatel chitrangpatel added the kind/bug Categorizes issue or PR as related to a bug. label May 16, 2024
@chitrangpatel
Copy link
Member Author

@tektoncd/core-maintainers PTAL here 🙏 I think this is a bug that we should fix for consistent behavior and catch bad resources during remote resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant