Allow dot character in resource names #3893

psschwei · 2021-04-19T19:53:48Z

Changes

Fixes #3891 to allow dot (.) characters in resource names.

The basic approach here was to update pkg/apis/validate/metadata.go so that it checks if Tekton resource names meet the criteria of DNS Subdomain names (. is a valid character in a DNS subdomain). It does this by using the IsDNS1123Subdomain() function from k8s.io/apimachinery/pkg/util/validation.

There had previously been a 63 character limit on resource names that, to my understanding, was put in place due to the fact that Tekton pod names are composed based on pipelineRun + pipeline + pipelineTask. For reference, a DNS subdomain can be up to 253 characters in length.

Some updates were made to the tests as well:

tests that checked for failure if a . was found in the resource name were updated to instead check for a comma
several tests used camelCase in the metadata.name field -- capital letters are not valid for metadata.name, and the new validation method will throw errors on any resource name in camelCase. I've updated the tests to use all lowercase letters for resource names. As this PR has been steadily increasing in size, I'll add new tests for camelCase in a separate PR
examples/v1beta1/pipelineruns/pipelinerun.yaml was updated to include resource names with a . in it, in order to catch any future issues that may arise with resource names containing a .

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Allow resource names to contain the dot character (".") 
Resource names now validated using the common `validation.IsDNS1123Subdomain()` function

tekton-robot · 2021-04-19T19:56:23Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-19T20:15:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-19T20:26:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-19T20:27:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-19T20:43:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-19T21:08:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

pkg/apis/pipeline/v1alpha1/condition_validation_test.go

pritidesai · 2021-04-19T21:51:15Z

@imjasonh after dropping the restriction for a dot character, the metadata_validation.go would have a single check validating the length of the resource name. Is there any more validation needed here or should the length validation be part of metadata_validation.go?

imjasonh · 2021-04-19T22:01:44Z

@imjasonh after dropping the restriction for a dot character, the metadata_validation.go would have a single check validating the length of the resource name. Is there any more validation needed here or should the length validation be part of metadata_validation.go?

Keeping it in a shared function seems useful, just to document what validation means right now. I don't feel that strongly though.

ghost · 2021-04-20T12:48:24Z

Could I request we also add an example yaml in examples/v1beta1/pipelineruns that includes a task with the new name format and a pipeline with the new name format, referenced by taskRef and pipelineRef? This way we're exercising the new naming format end-to-end and if we accidentally break taskRef pointing to names with "." in future we'd catch it early.

afrittoli · 2021-04-20T14:18:49Z

Could I request we also add an example yaml in examples/v1beta1/pipelineruns that includes a task with the new name format and a pipeline with the new name format, referenced by taskRef and pipelineRef? This way we're exercising the new naming format end-to-end and if we accidentally break taskRef pointing to names with "." in future we'd catch it early.

Good idea!
Would it be acceptable to use the dot in the name of some existing example, to avoid adding an entire new E2E tests for testing this alone?

psschwei · 2021-04-20T14:32:30Z

I can do either option (add a new example or edit an existing one)... if we go with the latter, would the main pipelinerun.yaml one be a suitable candidate? Thinking I would update to use unit.tests task and demo.pipeline pipeline...

ghost · 2021-04-20T14:49:02Z

Absolutely, updating an existing one to use the format sounds good to me too.

Edit: the main pipelinerun.yaml would work great for this.

psschwei · 2021-04-20T15:31:34Z

/test pull-tekton-pipeline-integration-tests

tekton-robot · 2021-04-20T15:33:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

ghost · 2021-04-20T15:52:49Z

The integration tests are failing due to a version change in the cluster that gets spun up for the tests to run in. Folks in slack are exploring the problem atm.

psschwei · 2021-04-21T20:42:37Z

/retest

psschwei · 2021-04-21T21:54:45Z

So the integration tests are failing with the following error:

   Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid resource name "unit.tests": must be a valid DNS label: metadata.name
        Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid resource name "demo.pipeline": must be a valid DNS label: metadata.name

Looking closer at the Kubernetes docs, while a . is valid for DNS Subdomain names, it is not a valid character for DNS Label names.

Assuming that the metadata.name for Pipelines and Tasks are DNS Labels rather than Subdomains (based on the fact that existing code was limiting names to <= 63 characters), it turns out that . may not be a valid character after all.

/meow

tekton-robot · 2021-04-21T21:54:47Z

@psschwei:

In response to this:

So the integration tests are failing with the following error:
  Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid resource name "unit.tests": must be a valid DNS label: metadata.name
       Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid resource name "demo.pipeline": must be a valid DNS label: metadata.name
Looking closer at the Kubernetes docs, while a . is valid for DNS Subdomain names, it is not a valid character for DNS Label names.

Assuming that the metadata.name for Pipelines and Tasks are DNS Labels rather than Subdomains (based on the fact that existing code was limiting names to <= 63 characters), it turns out that . may not be a valid character after all.

/meow

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 · 2021-04-22T00:06:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.0%	-0.7
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-22T00:10:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.0%	-0.7
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-22T00:12:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	97.8%	97.0%	-0.7
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	75.0%	-8.3

tekton-robot · 2021-04-22T00:19:11Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	50.0%	-33.3

tekton-robot · 2021-04-22T00:26:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/condition_validation.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	95.5%	90.9%	-4.5
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2
pkg/apis/pipeline/v1alpha1/taskrun_validation.go	89.8%	88.1%	-1.7
pkg/apis/validate/metadata.go	83.3%	50.0%	-33.3

tekton-robot · 2021-04-22T01:01:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2

tekton-robot · 2021-04-22T01:02:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2

psschwei · 2021-04-22T01:35:46Z

PR description has been updated with more detail. This should be ready to go now.
/hold cancel
/meow

tekton-robot · 2021-04-22T01:35:47Z

@psschwei:

In response to this:

PR description has been updated with more detail. This should be ready to go now.
/hold cancel
/meow

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.

pkg/apis/validate/metadata.go

tekton-robot · 2021-04-22T13:07:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2

tekton-robot · 2021-04-22T13:08:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2

vdemeester

One comment on the DNS1123 validation.

pkg/apis/validate/metadata.go

psschwei · 2021-04-22T13:16:59Z

One comment on the DNS1123 validation.
Does this mean that we allowed name like fooBar and now we don't ? Or was it already not allowed by the kubernetes api server ?

It appears the api server blocked it:

$ cat echo-hello-world.yaml 
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: echo-Hello-world
spec:
  steps:
    - name: echo
      image: ubuntu
      command:
        - echo
      args:
        - "Hello World"
$ k apply -f echo-hello-world.yaml -n paul
The Task "echo-Hello-world" is invalid: metadata.name: Invalid value: "echo-Hello-world": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

(tested on Pipelines v0.15.2)

vdemeester · 2021-04-22T13:21:42Z

@psschwei indeed, it seems it is either the api-server or the generic validation knative.dev/pkg provides us.
/lgtm

psschwei · 2021-04-23T00:15:54Z

/test pull-tekton-pipeline-go-coverage

tekton-robot · 2021-04-23T00:17:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/run_validation.go	93.8%	87.5%	-6.2

pritidesai · 2021-04-23T17:42:17Z

thanks @psschwei for all the work which started as a simple change 😜

/approve

tekton-robot · 2021-04-23T17:42:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai

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

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

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 19, 2021

tekton-robot requested review from bobcatfish and dlorenc April 19, 2021 19:53

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 19, 2021

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 19, 2021

psschwei force-pushed the main branch from f540835 to d297d22 Compare April 19, 2021 20:13

psschwei force-pushed the main branch from 01ee11d to 338077b Compare April 19, 2021 20:24

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2021

psschwei force-pushed the main branch from f172c20 to ac92457 Compare April 19, 2021 20:41

psschwei force-pushed the main branch from 6c177d7 to 460d058 Compare April 19, 2021 21:05

imjasonh reviewed Apr 19, 2021

View reviewed changes

pkg/apis/pipeline/v1alpha1/condition_validation_test.go Show resolved Hide resolved

psschwei force-pushed the main branch from 3e2470e to c79bc78 Compare April 20, 2021 15:31

psschwei force-pushed the main branch from 0732313 to 2ce1061 Compare April 22, 2021 01:00

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2021

pritidesai reviewed Apr 22, 2021

View reviewed changes

pkg/apis/validate/metadata.go Show resolved Hide resolved

allow dot character in resource names

8ec16a9

psschwei force-pushed the main branch from 98efb88 to 8ec16a9 Compare April 22, 2021 13:04

vdemeester reviewed Apr 22, 2021

View reviewed changes

pkg/apis/validate/metadata.go Show resolved Hide resolved

tekton-robot assigned vdemeester Apr 22, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2021

tekton-robot merged commit 3dc52df into tektoncd:main Apr 23, 2021

Allow dot character in resource names #3893

Allow dot character in resource names #3893

Conversation

psschwei commented Apr 19, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 19, 2021

tekton-robot commented Apr 19, 2021

tekton-robot commented Apr 19, 2021

tekton-robot commented Apr 19, 2021

tekton-robot commented Apr 19, 2021

tekton-robot commented Apr 19, 2021

pritidesai commented Apr 19, 2021

imjasonh commented Apr 19, 2021

ghost commented Apr 20, 2021

afrittoli commented Apr 20, 2021

psschwei commented Apr 20, 2021

ghost commented Apr 20, 2021 • edited by ghost Loading

psschwei commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

ghost commented Apr 20, 2021

psschwei commented Apr 21, 2021

psschwei commented Apr 21, 2021

tekton-robot commented Apr 21, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

psschwei commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

tekton-robot commented Apr 22, 2021

vdemeester left a comment

Choose a reason for hiding this comment

psschwei commented Apr 22, 2021 • edited Loading

vdemeester commented Apr 22, 2021

psschwei commented Apr 23, 2021

tekton-robot commented Apr 23, 2021

pritidesai commented Apr 23, 2021

tekton-robot commented Apr 23, 2021

psschwei commented Apr 19, 2021 •

edited

Loading

ghost commented Apr 20, 2021 •

edited by ghost

Loading

psschwei commented Apr 22, 2021 •

edited

Loading