Unused workspaces in PipelineRun

[jimmyjones2]

If I create a PipelineRun containing a workspace that is unused by the Pipeline, there are no errors and it completes successfully. I can't find this behaviour documented.

I've got a fairly generic process that creates PipelineRuns, but depending on the Pipeline chosen, some of the workspaces may be unused. It is safe to rely on this behaviour?

Example:

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: clone-read
  namespace: default
spec:
  description: |
    This pipeline clones a git repo, then echoes the README file to the stout.
  params:
  - description: The git repo URL to clone from.
    name: repo-url
    type: string
  tasks:
  - name: fetch-source
    params:
    - name: url
      value: $(params.repo-url)
    taskRef:
      kind: Task
      name: git-clone
    workspaces:
    - name: output
      workspace: shared-data
  - name: show-readme
    runAfter:
    - fetch-source
    taskRef:
      kind: Task
      name: show-readme
    workspaces:
    - name: source
      workspace: shared-data
  workspaces:
  - description: |
      This workspace contains the cloned repo files, so they can be read by the
      next task.
    name: shared-data
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: clone-read-run-
  namespace: default
spec:
  params:
  - name: repo-url
    value: https://github.com/tektoncd/website.git
  pipelineRef:
    name: clone-read
  podTemplate:
    securityContext:
      fsGroup: 65532
  serviceAccountName: default
  timeout: 1h0m0s
  workspaces:
  - name: foo
    configMap:
      name: foo
  - name: shared-data
    volumeClaimTemplate:
      metadata:
        creationTimestamp: null
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 1Gi
      status: {}

[pritidesai]

There is no such validation which restricts adding unused workspaces in pipelineRun.spec.workspaces. We validate workspaces for isExist() i.e. a workspace defined in a pipeline must exist in pipelineRun. And there is a validation of Unique() among the workspaces defined in pipelineRun.spec.workspaces.

isExist():

pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

Lines 450 to 465 in 7b23f93

	func ValidateWorkspaceBindings(p *v1.PipelineSpec, pr *v1.PipelineRun) error {
	pipelineRunWorkspaces := make(map[string]v1.WorkspaceBinding)
	for _, binding := range pr.Spec.Workspaces {
	pipelineRunWorkspaces[binding.Name] = binding
	}
	for _, ws := range p.Workspaces {
	if ws.Optional {
	continue
	}
	if _, ok := pipelineRunWorkspaces[ws.Name]; !ok {
	return fmt.Errorf("pipeline requires workspace with name %q be provided by pipelinerun", ws.Name)
	}
	}
	return nil
	}

Unique:

pipeline/pkg/apis/pipeline/v1/pipelinerun_validation.go

Lines 98 to 107 in 7b23f93

	if ps.Workspaces != nil {
	wsNames := make(map[string]int)
	for idx, ws := range ps.Workspaces {
	errs = errs.Also(ws.Validate(ctx).ViaFieldIndex("workspaces", idx))
	if prevIdx, alreadyExists := wsNames[ws.Name]; alreadyExists {
	errs = errs.Also(apis.ErrGeneric(fmt.Sprintf("workspace %q provided by pipelinerun more than once, at index %d and %d", ws.Name, prevIdx, idx), "name").ViaFieldIndex("workspaces", idx))
	}
	wsNames[ws.Name] = idx
	}
	}

It might be accidental to allow extra workspaces but I do not foresee restricting such specification.

@tektoncd/core-collaborators shall we document this behavior and allow specifying any number of workspaces in pipelineRun.spec.workspaces?

[lbernick]

This seems like a bug to me. I'm not sure there's a good reason to support specifying extra workspaces in the pipelinerun, and I would expect most pipelinerun authors to prefer fail-fast behavior, i.e. rejecting pipelineruns with extra workspaces since they're most likely misconfigured.

[jerop]

It seems supporting extra Workspaces was intentional for the same reason that we added support for extra Parameters - #2513:

Imagine you have a system creating PipelineRuns automatically, and each PipelineRun uses a different Pipeline which has a different set of parameters. The system has a number of parameters it knows how to provide, e.g. the service account and url of a bucket the Pipelines can push to (but not all Pipelines need to push to a bucket).

It would be very convenient if the system creating the PipelineRuns could provide all the parameters it has available to all PipelineRuns, and the Pipelines could use the ones they need and ignore the rest.

While I see the value of fail-fast as @lbernick suggested, I think we should keep and document this behavior as @pritidesai mentioned because some users could be depending on it for the above use case.

Here is the documentation for extra Parameters for reference:

pipeline/docs/pipelineruns.md

Lines 269 to 272 in 21828f3

	You can pass in extra `Parameters` if needed depending on your use cases. An example use
	case is when your CI system autogenerates `PipelineRuns` and it has `Parameters` it wants to
	provide to all `PipelineRuns`. Because you can pass in extra `Parameters`, you don't have to
	go through the complexity of checking each `Pipeline` and providing only the required params.

[jimmyjones2]

Fixed in #6917