restricting tekton.dev labels in metadata #2891

pritidesai · 2020-07-02T22:37:24Z

Changes

Tekton resources are automatically assigned certain labels including the ones with tekton.dev. The labels are created and propagated from one resource to another using this group name. We should restrict possibility of overwriting those labels otherwise it could result in conflicting resources.

Result of the discussion in PR 2826

On hold until we confirm this is something we want to restrict 🤔

This will break folks using these labels in metadata, see an example in catalog.

/cc @vdemeester

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Restricting user defined tekton.dev/ labels.

tekton-robot · 2020-07-02T22:37:54Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

pritidesai · 2020-07-02T22:38:08Z

/kind misc
/hold

tekton-robot · 2020-07-02T22:40:20Z

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%	88.9%	5.6

vdemeester · 2020-07-03T14:15:38Z

This will break folks using these labels in metadata, see an example in catalog.

Interesting, those catalog task shouldn't use that label anyway as it is supposed to be set by the controller 🤔

pritidesai · 2020-07-06T19:00:16Z

This will break folks using these labels in metadata, see an example in catalog.

Interesting, those catalog task shouldn't use that label anyway as it is supposed to be set by the controller 🤔

Yup, indeed, @bobcatfish @sbwsg WDYT?

dlorenc · 2020-07-15T22:36:27Z

Yup, indeed, @bobcatfish @sbwsg WDYT?

Ref #2946 - we should probably document this there too

dlorenc · 2020-07-15T22:41:45Z

Yup, indeed, @bobcatfish @sbwsg WDYT?

Is it actually "used" there? The catalog reorg happened so I had to hunt them down (the links changed). Here it is now: https://github.com/tektoncd/catalog/blob/d7e5c6db536d79bd3c205caaf075a50f5c9dee7e/task/github-close-issue/0.1/README.md

They only really appear in the READMEs for examples on creating a TaskRun. My wild guess is that someone created the taskrun with the CLI or something, then dumped it as YAML to generate the docs and kept that label.

It's only in 3 places:

dlorenc · 2020-07-15T22:42:22Z

pkg/apis/validate/metadata.go

+
+ // metadata must not contain any label with tekton.dev/*
+ for k := range meta.GetLabels() {
+ if strings.HasPrefix(k, pipeline.GroupName) {


Do we care/have an opinion on things like "*.tekton.dev" as well?

This is meant to be Tekton internal, created and utilized by the pipeline controller itself, better to enforce rather than relying on Task/Pipeline/TaskRun/PipelineRun authors to follow recommendations, thoughts?

@dlorenc good point. I would think we should reserve tekton.dev and most likely pipelines.tekton.dev (future-proof 😅 ), but given some proposals, we shouldn't restrict all the *.tekton.dev 🙃

+1 on saving the extra namespace. In chains right now we're using "chains.tekton.dev", but only in an annotation.

This PR is restricting labels, annotations have all the freedom 😉

tekton-robot · 2020-08-17T09:12:12Z

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.

vdemeester · 2020-09-15T12:08:37Z

/remove-lifecycle rotten

vdemeester · 2020-09-15T12:08:52Z

cc @tektoncd/core-maintainers

ghost · 2020-09-15T14:08:27Z

/approve

tekton-robot · 2020-09-15T14:08:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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:

~~pkg/apis/OWNERS~~ [sbwsg]

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

dibyom · 2020-09-15T20:03:24Z

Tekton resources are automatically assigned certain labels including the ones with tekton.dev. The labels are created and propagated from one resource to another using this group name. We should restrict possibility of overwriting those labels otherwise it could result in conflicting resources.

Sounds good to me!

vdemeester · 2020-09-16T09:57:15Z

/retest

Tekton resources are automatically assigned certain labels including the ones with tekton.dev. The labels are created and propagated from one resource to another using this group name. We should restrict possibility of overwriting those labels otherwise it could result in conflicting resources.

afrittoli · 2020-09-24T06:57:17Z

/test pull-tekton-pipeline-integration-tests

afrittoli · 2020-09-24T06:59:14Z

pkg/apis/validate/metadata.go

+ for k := range meta.GetLabels() {
+ if strings.HasPrefix(k, pipeline.GroupName) {
+ return &apis.FieldError{
+ Message: "Invalid label key: can not change tekton resource name",


NIT: the error message may be confusing if someone is not trying to change the resource name. I think it would be better to say that the tekton.dev/ labels are reserved for internal use.

afrittoli · 2020-09-24T07:00:37Z

We do use tekton.dev labels in plumbing, so I need to change those before we install this :)

afrittoli

Thanks for this!
I would update the error message, but otherwise it looks good to me.
I think we can reserve extra namespace in further PRs if we decide to?
@pritidesai is the "hold" still needed on this one?

pritidesai · 2020-09-25T15:56:47Z

thanks. Nope, no more hold needed.
/hold cancel
/test pull-tekton-pipeline-integration-tests

pritidesai · 2020-09-25T17:43:18Z

We do use tekton.dev labels in plumbing, so I need to change those before we install this :)

Could this be causing integration failure? 🤔 Integration tests failure is consistent with these changes in terms of number of failing Tests:

162 failed / 9 succeeded

pritidesai · 2020-09-25T18:04:25Z

16 test examples showing this error:

TestExamples/v1alpha1/pipelineruns/conditional-pipelinerun-with-same-condition-refer: examples_test.go:57: Failed waiting for pipeline run done: timed out waiting for the condition

pritidesai · 2020-09-25T19:38:17Z

This is actually a legit failure 😢 not related to infra or timeout.

Taskrun and Pipelinerun controller injects these labels which are then going through the same validation routine which validates user specified labels. Will update this PR to separate this validation calls next week, putting it on hold until then.

/hold

vdemeester · 2020-10-07T09:32:18Z

/lgtm cancel

tekton-robot · 2021-01-05T10:16: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.

vdemeester · 2021-01-05T10:33:02Z

/remove-lifecycle stale
@tektoncd/core-maintainers what should we do here ?

tekton-robot · 2021-03-11T15:39:22Z

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%	88.9%	5.6

tekton-robot · 2021-03-11T16:41:09Z

@pritidesai: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-unit-tests	`beaf288`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-integration-tests	`beaf288`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tekton-robot · 2021-03-11T16:41:09Z

@pritidesai: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-unit-tests	`beaf288`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-integration-tests	`beaf288`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tekton-robot · 2021-04-24T06:57:11Z

@pritidesai: PR needs rebase.

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.

vdemeester · 2021-05-03T14:07:16Z

@pritidesai @tektoncd/core-maintainers gentle ping 👼🏼

pritidesai · 2021-05-03T21:17:24Z

thanks @vdemeester its a long overdue, will look into it this week 🙏

tekton-robot · 2021-10-15T00:45:32Z

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.

pritidesai · 2021-11-09T19:46:28Z

closing this for now, will open a new PR once I have a breather to implement this. I have created an issue capturing the PR discussion here.

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 2, 2020

tekton-robot requested review from dlorenc and a user July 2, 2020 22:37

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 2, 2020

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Jul 2, 2020

pritidesai mentioned this pull request Jul 2, 2020

adding metadata to taskSpec in PipelineTask #2826

Merged

4 tasks

pritidesai changed the title ~~restricting tekton.dev in metadata~~ restricting tekton.dev labels in metadata Jul 2, 2020

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 3, 2020

dlorenc reviewed Jul 15, 2020

View reviewed changes

pritidesai mentioned this pull request Jul 16, 2020

Rewrites the "Labels" documentation for clarity and flow. #2946

Merged

4 tasks

tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 17, 2020

tekton-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 15, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2020

afrittoli reviewed Sep 24, 2020

View reviewed changes

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2020

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2020

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

tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 5, 2021

Base automatically changed from master to main March 11, 2021 15:28

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2021

pritidesai mentioned this pull request Aug 13, 2021

cleaning up label keys 🧹 #4154

Merged

5 tasks

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

pritidesai mentioned this pull request Nov 9, 2021

restricting tekton.dev labels in metadata #4366

Open

pritidesai closed this Nov 9, 2021

restricting tekton.dev labels in metadata #2891

restricting tekton.dev labels in metadata #2891

Conversation

pritidesai commented Jul 2, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jul 2, 2020

pritidesai commented Jul 2, 2020 • edited Loading

tekton-robot commented Jul 2, 2020

vdemeester commented Jul 3, 2020

pritidesai commented Jul 6, 2020

dlorenc commented Jul 15, 2020

dlorenc commented Jul 15, 2020

dlorenc Jul 15, 2020

Choose a reason for hiding this comment

pritidesai Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

vdemeester Jul 16, 2020

Choose a reason for hiding this comment

dlorenc Jul 16, 2020

Choose a reason for hiding this comment

pritidesai Jul 18, 2020

Choose a reason for hiding this comment

tekton-robot commented Aug 17, 2020

vdemeester commented Sep 15, 2020

vdemeester commented Sep 15, 2020

ghost commented Sep 15, 2020

tekton-robot commented Sep 15, 2020

dibyom commented Sep 15, 2020

vdemeester commented Sep 16, 2020

afrittoli commented Sep 24, 2020

afrittoli Sep 24, 2020

Choose a reason for hiding this comment

afrittoli commented Sep 24, 2020

afrittoli left a comment

Choose a reason for hiding this comment

pritidesai commented Sep 25, 2020

pritidesai commented Sep 25, 2020

pritidesai commented Sep 25, 2020

pritidesai commented Sep 25, 2020

vdemeester commented Oct 7, 2020

tekton-robot commented Jan 5, 2021

vdemeester commented Jan 5, 2021

tekton-robot commented Mar 11, 2021

tekton-robot commented Mar 11, 2021

tekton-robot commented Mar 11, 2021

tekton-robot commented Apr 24, 2021

vdemeester commented May 3, 2021

pritidesai commented May 3, 2021

tekton-robot commented Oct 15, 2021

pritidesai commented Nov 9, 2021

pritidesai commented Jul 2, 2020 •

edited

Loading

pritidesai commented Jul 2, 2020 •

edited

Loading

pritidesai Jul 16, 2020 •

edited

Loading