-
Notifications
You must be signed in to change notification settings - Fork 150
feat(recommended labels) #4327
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
base: master
Are you sure you want to change the base?
feat(recommended labels) #4327
Conversation
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) 2022-2024 Tigera, Inc. All rights reserved. | |||
| // Copyright (c) 2022-2025 Tigera, Inc. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol on Dec 29th...
628c1c9 to
1cbb0a6
Compare
| "app.kubernetes.io/instance": "tigera-secure", | ||
| "app.kubernetes.io/managed-by": "tigera-operator", | ||
| "app.kubernetes.io/name": "tigera-external-prometheus", | ||
| "app.kubernetes.io/part-of": "TigeraSecureEnterprise", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoud this just be Calico always? I think that makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do CalicoEnterprise with the enterprise variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, although I think from a users perspective "Calico" is probably the most useful. Not sure it adds value for a user to have this change between product variants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either of your suggestions are nicer than TigeraSecureEnterprise. My take would be that:
- Either we hardcode "calico"
- Or - since we don't really like TigeraSecureEnterprise - we deprecate Variant enum value TigeraSecureEnterprise in favour of CalicoEnterprise as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hard code Calico for now IMO - the use-case of allow-listing a set of components means a singular value here is probably better.
We should look at getting rid of TSE separately IMO.
| Expect(serviceMonitor.Spec.Endpoints).To(HaveLen(1)) | ||
| // Verify that the default settings are propagated. | ||
| Expect(serviceMonitor.Labels).To(Equal(map[string]string{render.AppLabelName: monitor.TigeraExternalPrometheus})) | ||
| Expect(serviceMonitor.Labels).To(Equal(map[string]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about app.kubernetes.io/version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're including version we need to differentiate in one of the other labels enterprise vs open source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unless it was app.kubernetes.io/version: v3.30-enterprise or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We spoke about this during the dev grooming meetings (IIRC twice even) and we concluded that today we don't have a very good way of ensuring they are all correct, that doing it correct would take a lot of work and so we decided to defer this label for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't recall those (or maybe I just missed them). How would they be incorrect? Don't we have the version compiled into operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we chatted only briefly on the approach before debating if there would be value in having it. In the end we basically concluded that it was low ROI and worth skipping until a user asks for it with a valid use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the use-cases for all of the other labels?
Feel free to go ahead without it, I'm just a bit confused as to the motivations for some of these labels but not others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a user that wants to block resources being deployed using a webhook if they do not belong to allow-listed components. Previously they would filter resources on the expected name prefix such as "tigera-" or "calico-". Labels would be a better way of implementing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely that's only an argument for adding the app.kubernetes.io/component label then, and not all of the others? Or maybe "app.kubernetes.io/part-of"?
54bfbbc to
0c939f5
Compare
feat(recommended labels): Set recommended labels as per
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
For example:
The version label was deliberately skipped as discussed in our operator grooming meetings.