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

SHIP-0034: Label propagation #86

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pinolo
Copy link

@Pinolo Pinolo commented May 16, 2022

No description provided.

@openshift-ci openshift-ci bot requested review from imjasonh and sbose78 May 16, 2022 01:19
@openshift-ci
Copy link

openshift-ci bot commented May 16, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign saschaschwarze0 after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschaschwarze0 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

@Pinolo
Copy link
Author

Pinolo commented May 16, 2022

/assign @SaschaSchwarze0 (I'm not sure I should assign it now, as I understand it, the EP approval is a process that happens before it's complete and ready)

@SaschaSchwarze0 SaschaSchwarze0 changed the title Drafting the Labels propagation EP SHIP-0033: Label propagation Jun 19, 2022
@SaschaSchwarze0 SaschaSchwarze0 changed the title SHIP-0033: Label propagation SHIP-0034: Label propagation Jun 19, 2022
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.

Hi @Pinolo, thank you for your contribution. That's very valuable. I must say sorry. We were quite busy with v0.10 and cdCon preparation recently.

Please rename your file, I renamed your PR already. You now have SHIP 0034.

Please mention that doumentation will be needed.


The implementation requires the [BuilderStrategy interface](../../pkg/apis/build/v1alpha1/buildstrategy.go) to be extended with a `GetLabels` functions that is implemented in the [BuildStrategy](../../pkg/apis/build/v1alpha1/buildstrategy_types.go) and [ClusterBuildStrategy](../../pkg/apis/build/v1alpha1/clusterbuildstrategy_types.go) types by returning the object's labels.

It also requires `GetLabels` functions to be implemented in the [Build](../../pkg/apis/build/v1alpha1/build_types.go) and [BuildRun](../../pkg/apis/build/v1alpha1/buildrun_types.go) types.
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed because the Build and BuildRun type are Kubernetes objects which have those functions available by default.


### Implementation Notes

The implementation requires the [BuilderStrategy interface](../../pkg/apis/build/v1alpha1/buildstrategy.go) to be extended with a `GetLabels` functions that is implemented in the [BuildStrategy](../../pkg/apis/build/v1alpha1/buildstrategy_types.go) and [ClusterBuildStrategy](../../pkg/apis/build/v1alpha1/clusterbuildstrategy_types.go) types by returning the object'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.

You only need to define the function on the interface. The implementation is already available on the Kubernetes generated types.

Comment on lines +100 to +102
The assignment of the `TaskRun` labels needs to be done in the [generate_taskrun.go](../../pkg/reconciler/buildrun/resources/taskrun.go) file in the `GenerateTaskRun` function. The labels from the build strategy need to be copied to the `TaskRun` except those with reserved prefixes mentioned under [Proposal](#proposal).

Making sure the assignment takes place in the desired order (first labels from `BuilderStrategy`, then from `Build`, and finally from `BuildRun`) will allow cascading overrides.
Copy link
Member

Choose a reason for hiding this comment

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

You should merge these two paragraphs to make it more obvious. Currently the first paragraph mentions that labels only from the build strategy need to be copied. Your second paragraph discussed the override behavior.

Comment on lines +56 to +63
Shipwright resources administrators can define labels in the metadata, as with all Kubernetes objects, see the [Labels](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/) topic in the Kubernetes documentation.

As we did with annotations, we reserve certain label prefixes that are either used by Kubernetes or Tekton or Shipwright's controller:

* `?kubernetes.io`
* `?k8s.io`
* `tekton.dev`
* `*.shipwright.io`
Copy link
Member

Choose a reason for hiding this comment

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

I think you should here discuss the differences between labels and annotations. Conceptionally. You already link to the Kubernetes documentation about labels. There's a topic about annotations as well. The difference is clearly stated. While labels are described as Labels enable users to map their own organizational structures onto system objects in a loosely coupled fashion, without requiring clients to store these mappings., annotations (https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/) have a broader scope inclusing Directives from the end-user to the implementations to modify behavior or engage non-standard features.

Based on Kubernetes' definition, one can easily conclude that labels are safe to be propagated because they cannot be used to possibly enable unwanted behavior.

The caveat/risk you can mention is that implementations are out there which want to modify behavior, but also require the filtering capabilities of labels - and therefore use labels for behavior. Istio is an example, https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#controlling-the-injection-policy.

And that's where it becomes interesting. I personally still think that we can do propagation also from Build and BuildRun.

Our general strategy on that is that we delegate such things to policy management solutions (though we don't have a SHIP for this, like Tekton has, https://github.com/tektoncd/community/blob/main/teps/0035-document-tekton-position-around-policy-authentication-authorization.md).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the insightful digression. I see your point in adding a little background about different purpose for labels and annotations, and I will update the document accordingly.

Regarding the caveat/risk discussion, I see that I actually jumped to a conclusion (to protect shipwright's own dependencies labels) which is quite arbitrary. Considering the main purpose of labels (filtering), I believe use cases may arise where users want to add/override tekton or even k8s key prefixes. Also, the Istio situation is quite tricky, but maybe we don't want to handle it, and delegate it to policy management, as you suggest, possibly in a documented way.

As for shipwright's own labels, I don't know…

Could a solomonic resolution be to allow adding prefixes to "protected" patterns, while disallowing overrides?


### Test Plan

TBD
Copy link
Member

Choose a reason for hiding this comment

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

I think unit and integration tests are necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants