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.

Sign up for GitHub

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

Jump to bottom

625 user control over pod labels #1069

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

625 user control over pod labels #1069

wants to merge 6 commits into from

Conversation

Pinolo
Copy link

@Pinolo Pinolo commented May 24, 2022 •
edited

Changes

  • Allows passing labels from ClusterBuildStrategy, BuildStrategy, Build, BuildRun, on to TaskRun and its pod
  • Overlapping labels in "lower" levels override "upper" levels
  • Some labels prefixes are reserved, i.e. not propagated

Fixes #625

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

  • Developers and admins are now able to set labels in ClusterBuildStrategy, BuildStrategy, Build, BuildRun, that will be propagated to the pods running Tekton pipelines
  • Overlapping labels in "lower" levels override "upper" levels, i.e. if you set the same label key in both BuildStrategy resource and Build resource, the one in Build resource will be used
  • Reserved label key prefixes:
    • k8s.io
    • kubernetes.io
    • tekton.dev
    • shipwright.io
Developers and admins are now able to set labels in ClusterBuildStrategy, BuildStrategy, Build, BuildRun, that will be propagated to the pods running Tekton pipelines. Some reserved label key prefixes will not propagate:
- k8s.io
- kubernetes.io
- tekton.dev
- shipwright.io

@openshift-ci openshift-ci bot 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 May 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 24, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign otaviof after the PR has been reviewed.
You can assign the PR to them by writing /assign @otaviof in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note 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 May 25, 2022
@Pinolo
Copy link
Author

Pinolo commented May 25, 2022

/kind feature

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label May 25, 2022
@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.11.0 milestone Jun 1, 2022
SaschaSchwarze0
SaschaSchwarze0 requested changes Jun 19, 2022
View reviewed changes
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. We should wait for the discussions in the ship to be completed. Beside that, implementation looks mostly good. Also the right set of documentation is included. I think you should add an integration test as well.

docs/buildstrategies.md
- The Kubernetes [Network Traffic Shaping](https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/#support-traffic-shaping) feature looks for the `kubernetes.io/ingress-bandwidth` and `kubernetes.io/egress-bandwidth` annotations to limit the network bandwidth the `Pod` is allowed to use.
- The [AppArmor profile of a container](https://kubernetes.io/docs/tutorials/clusters/apparmor/) is defined using the `container.apparmor.security.beta.kubernetes.io/<container_name>` annotation.

Since you can define labels for all of BuildStrategy/ClusterBuildStrategy, Build and BuildRun resources, if you define the same label key for different resources in the same build "pipeline", only the value in the last definition stage will be used (e.g. if you define `someproj.io/label: value01` in a Build and `someproj.io/label: value02` in a BuildRun that references such Build, `value02` will be used).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention here that annotations cannot be overriden.

pkg/apis/build/v1alpha1/build_types.go
Comment on lines +278 to +280
func (b Build) GetLabels() map[string]string {
return b.Labels
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed because Kubernetes already provides that function on ObjectMeta.

pkg/apis/build/v1alpha1/buildrun_types.go
Comment on lines +275 to +278
// GetLabels returns the labels of the BuildRun
func (br *BuildRun) GetLabels() map[string]string {
return br.Labels
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed because Kubernetes already provides that function on ObjectMeta.

pkg/apis/build/v1alpha1/buildstrategy_types.go
Comment on lines +87 to +90
// GetLabels returns the labels the build strategy
func (s BuildStrategy) GetLabels() map[string]string {
return s.Labels
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed because Kubernetes already provides that function on ObjectMeta.

pkg/apis/build/v1alpha1/clusterbuildstrategy_types.go
Comment on lines +88 to +91
// GetLabels returns the labels of the build strategy
func (s ClusterBuildStrategy) GetLabels() map[string]string {
return s.Labels
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed because Kubernetes already provides that function on ObjectMeta.

pkg/reconciler/buildrun/resources/taskrun.go
@@ -322,6 +322,32 @@ func GenerateTaskRun(
expectedTaskRun.Labels[label] = value
}

reserved, err := regexp.Compile(reservedLabels)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regular expression should be compiled only once.

@binnn6 binnn6 mentioned this pull request Nov 11, 2022
Open
4 tasks
@SaschaSchwarze0 SaschaSchwarze0 removed this from the release-v0.13.0 milestone Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paolomainardi paolomainardi paolomainardi left review comments

@SaschaSchwarze0 SaschaSchwarze0 SaschaSchwarze0 requested changes

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@shahulsonhal shahulsonhal Awaiting requested review from shahulsonhal

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. release-note Label for when a PR has specified a release note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide user control over pod label
4 participants
@Pinolo @paolomainardi @SaschaSchwarze0 @alessiopiazza