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

Any non-empty Kind with empty APIVersion in TaskRef is a Task #6459

Closed
jerop opened this issue Mar 29, 2023 · 8 comments · Fixed by #6505
Closed

Any non-empty Kind with empty APIVersion in TaskRef is a Task #6459

jerop opened this issue Mar 29, 2023 · 8 comments · Fixed by #6505
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@jerop
Copy link
Member

jerop commented Mar 29, 2023

Expected Behavior

Setting Kind to any non-empty string and APIVersion to an empty string in a TaskRef will be considered a validation error.

Note that I don't expect it to be considered a Custom Task because APIVersion is empty.

Actual Behavior

Setting Kind to any non-empty string and APIVersion to an empty string in a TaskRef will be considered a Task, and attempt to create a TaskRun. This is especially a problem if Kind is not set to "Task".

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: pr-
spec:
  pipelineSpec:
    tasks:
      - name: greeting
        taskRef:
          kind: Example
          name: foo
---
apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  name: pr-6whfx
spec:
  pipelineSpec:
    tasks:
    - name: greeting
      taskRef:
        kind: Example
        name: foo
status:
  completionTime: "2023-03-29T16:34:42Z"
  conditions:
  - lastTransitionTime: "2023-03-29T16:34:42Z"
    message: 'Pipeline default/pr-6whfx can't be Run; it contains Tasks that don't exist: Couldn't retrieve Task "foo": failed to get task: tasks.tekton.dev "foo" not found'
    reason: CouldntGetTask
    status: "False"
    type: Succeeded
  pipelineSpec:
    tasks:
    - name: greeting
      taskRef:
        kind: Example
        name: foo

Steps to Reproduce the Problem

  1. Create a PipelineRun with a TaskRef where the Kind is set to a non-empty string that's not "Task"
  2. Observe the PipelineRun status
@jerop jerop added the kind/bug Categorizes issue or PR as related to a bug. label Mar 29, 2023
@jerop jerop changed the title Non-empty Kind field with empty APIVersion is considered a Task Any non-empty Kind field with empty APIVersion is considered a Task Mar 29, 2023
@jerop jerop changed the title Any non-empty Kind field with empty APIVersion is considered a Task Any non-empty Kind with empty APIVersion in TaskRef is considered a Task Mar 29, 2023
@jerop jerop changed the title Any non-empty Kind with empty APIVersion in TaskRef is considered a Task Any non-empty Kind with empty APIVersion in TaskRef is a Task Mar 29, 2023
@jerop
Copy link
Member Author

jerop commented Mar 29, 2023

We could require that APIVersion has to specified if Kind is specified, then we create a CustomRun - see #6457

@jerop
Copy link
Member Author

jerop commented Apr 3, 2023

cc @pritidesai

@jerop jerop added this to the Pipelines v0.47 milestone Apr 4, 2023
@Yongxuanzhang
Copy link
Member

/assign

@Yongxuanzhang
Copy link
Member

Can we say if TaskRef.Kind is not empty and not NamespacedTaskKind("Task"), and api version is not set, we should return validation error? It may make sense to set Kind to "Task"?

@jerop
Copy link
Member Author

jerop commented Apr 6, 2023

Can we say if TaskRef.Kind is not empty and not NamespacedTaskKind("Task"), and api version is not set, we should return validation error?

Yes, it should be a validation error

It may make sense to set Kind to "Task"?

No, let's not change the user-provided value in "Kind" to "Task" 🙏🏾

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Apr 6, 2023

Can we say if TaskRef.Kind is not empty and not NamespacedTaskKind("Task"), and api version is not set, we should return validation error?

Yes, it should be a validation error

It may make sense to set Kind to "Task"?

No, let's not change the user-provided value in "Kind" to "Task" 🙏🏾

Sorry I didn't mean we should change that. This issue says "Setting Kind to any non-empty string and APIVersion to an empty string in a TaskRef will be considered a validation error." But if it is set to "Task" by user then it should be a valid case without apiversion?

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 6, 2023
This commit closes tektoncd#6459. For a custometask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error.

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 10, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error.

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
@jerop
Copy link
Member Author

jerop commented Apr 10, 2023

But if it is set to "Task" by user then it should be a valid case without apiversion?

yes, let's leave that as a valid case, especially given that we default to setting kind to task, so users see that in the spec in the status

@QuanZhang-William
Copy link
Member

/assign

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 19, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error.

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 19, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error. TaskRun's taskRef validation will be
handled separately in tektoncd#6557

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 20, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error. TaskRun's taskRef validation will be
handled separately in tektoncd#6557

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Apr 25, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error. TaskRun's taskRef validation will be
handled separately in tektoncd#6557

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
tekton-robot pushed a commit that referenced this issue Apr 25, 2023
This commit closes #6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error. TaskRun's taskRef validation will be
handled separately in #6557

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
EmmaMunley pushed a commit to EmmaMunley/pipeline that referenced this issue Apr 28, 2023
This commit closes tektoncd#6459. For a customtask reference in a pipelinetask,
if the Kind is non-empty and not "Task" or "ClusterTask", then it should
be a Custom task and api version should be set. If not then validation
webhook should return error. TaskRun's taskRef validation will be
handled separately in tektoncd#6557

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
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.

4 participants