label all Cilium resources with "app.kubernetes.io/part-of: cilium"

[cyclinder]

Signed-off-by: cyclinder qifeng.guo@daocloud.io

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

Fixes: #20088

label all Cilium resources with "app.kubernetes.io/part-of: cilium"

[cyclinder]

I'm not sure if I need to label resources of the type configMap,RBAC..

[gandro]

Thanks for the PR. Overall I think you got the relevant resources, though I don't think we should also make it part of e.g. the service selectors.

[ldelossa]

Is the idea here to eventually deprecate the k8s-app=cilium label?

[gandro]

Is the idea here to eventually deprecate the k8s-app=cilium label?

I don't think so. k8s-app uses different values for each component (e.g. k8s-app: cilium vs k8s-app: hubble-relay), which we also use for service selectors, while part-of is the same for all components of the cilium Helm chart and can be used e.g. when fetching all relevant resources for a sysdump.

[ldelossa]

Yes but if we begin following the "recommended" labels, k8s-app would equate to app.kubernetes.io/name or app.kubernetes.io/component. See: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

[ldelossa]

@gandro and @cyclinder Given that this moves us to one of the recommended k8s labels, I'd also be pretty happy if we moved k8s-app to the recommended ones suggested above as well. I do not think we should remove the k8s-app label just yet, but migrate away from it and towards the set of recommended lables. How do you both feel about this?

[tommyp1ckles]

I'm not sure if I need to label resources of the type configMap,RBAC..

Is there a reason those might be excluded?

[tommyp1ckles]

We use k8s-app for all the deployment/ds matchLabels selectors - these fields are immutable so we would probably want to put that behind a flag/deprecation.

[cyclinder]

@gandro and @cyclinder Given that this moves us to one of the recommended k8s labels, I'd also be pretty happy if we moved k8s-app to the recommended ones suggested above as well. I do not think we should remove the k8s-app label just yet, but migrate away from it and towards the set of recommended lables. How do you both feel about this?

Thanks for the review! I totally agree.

We shouldn't remove k8s-app right now (maybe we should still keep it), but at least we should labels all cilium resources with app.kubernetes.io/name or app.kubernetes.io/component according to recommended" labels, just like k8s-app.

[cyclinder]

Is there a reason those might be excluded?

Since I see no labels for these resources at the moment

[gandro]

On whether or not to deprecate the k8s-app labels: While I'm not against removing them, I believe cilium sysdump is currently relying on them.

So if we decide to remove them, then we need to figure out a migration strategy for cilium sysdump that works with both old and new versions of Cilium.

[ldelossa]

@gandro and @cyclinder I do agree. We do not want to deprecate k8s-app until we can safely do so, especially with its usage in cilium sysdump. That being said, we can add the recommended labels like mentioned above. There is a bit of value add here. Right now I have a kind cluster and I see that the 'agent' pods use k8s-app=cilium while the operators do not.

Ideally, we can do something like:

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-agent

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-operator

Tho, I'm not sure if s/name/component or not?

[cyclinder]

Update:

• Label all Cilium resources with app.kubernetes.io/part-of: cilium (But not including configMap,RBAC.etc, Since these resources do not have any label), The meaning of app.kubernetes.io/part-of is just to label which resources belong to cilium, and not be used for selector and matchLabels.
• The label of app.kubernetes.io/name can be equal to k8s-app, which is used to label different components in cilium, and which also used for selector and matchLabels, Just like k8s-app do. In addition to, I used app.kubernetes.io/name instead of name .

@ldelossa @gandro

[cyclinder]

@gandro and @cyclinder I do agree. We do not want to deprecate k8s-app until we can safely do so, especially with its usage in cilium sysdump. That being said, we can add the recommended labels like mentioned above. There is a bit of value add here. Right now I have a kind cluster and I see that the 'agent' pods use k8s-app=cilium while the operators do not.

Ideally, we can do something like:

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-agent

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-operator

Tho, I'm not sure if s/name/component or not?

I think the both is ok, I would prefer name :)

[gandro]

@gandro and @cyclinder I do agree. We do not want to deprecate k8s-app until we can safely do so, especially with its usage in cilium sysdump. That being said, we can add the recommended labels like mentioned above. There is a bit of value add here. Right now I have a kind cluster and I see that the 'agent' pods use k8s-app=cilium while the operators do not.

Ideally, we can do something like:

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-agent

app.kubernetes.io/part-of: cilium
app.kubernetes.io/name: cilium-operator

Tho, I'm not sure if s/name/component or not?

I like this proposal. I'm happy with name.

@cyclinder Would you be willing to change this PR to reflect that?

[tommyp1ckles]

Changes lgtm, I am still a bit concerned over changing the immutable matchLabels. For example, if I try to do a helm upgrade with this branch I get:

Error: UPGRADE FAILED: cannot patch "cilium" with kind DaemonSet: DaemonSet.apps "cilium" is invalid: [spec.template.metadata.labels: Invalid value: map[string]string{"app.kubernetes.io/name":"cilium", "app.kubernetes.io/part-of":"cilium-agent", "k8s-app":"cilium"}: selectordoes not match templatelabels, spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/name":"cilium-agent", "k8s-app":"cilium"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable] && cannot patch "cilium-operator" with kind Deployment: Deployment.apps "cilium-operator" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/name":"cilium-operator", "io.cilium/app":"operator"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Not sure what the best solution for this is, perhaps to have way to specify custom matchLabel selectors in values.yaml.

[ldelossa]

This looks good to me, contingent on dealing with the existing feedback. Thanks for adding to the scope of this PR.

[cyclinder]

I'm sorry to not responding for a long time since I was busy with work these days.

@cyclinder Would you be willing to change this PR to reflect that?

Hi @gandro , If my understanding is right, the current PR already reflects these changes, Right?

[cyclinder]

Changes lgtm, I am still a bit concerned over changing the immutable matchLabels. For example, if I try to do a helm upgrade with this branch I get:

Error: UPGRADE FAILED: cannot patch "cilium" with kind DaemonSet: DaemonSet.apps "cilium" is invalid: [spec.template.metadata.labels: Invalid value: map[string]string{"app.kubernetes.io/name":"cilium", "app.kubernetes.io/part-of":"cilium-agent", "k8s-app":"cilium"}: selectordoes not match templatelabels, spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/name":"cilium-agent", "k8s-app":"cilium"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable] && cannot patch "cilium-operator" with kind Deployment: Deployment.apps "cilium-operator" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/name":"cilium-operator", "io.cilium/app":"operator"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Not sure what the best solution for this is, perhaps to have way to specify custom matchLabel selectors in values.yaml.

Thanks! Good catch!

I'll test it on my local machine, and find a best solution for this.

[cyclinder]

Update:

As before, I added two labels to each cilium resource, app.kubernetes.io/name and app.kubernetes.io/part-of, and only when UseNewMatchLabel= true, then app.kubernetes.io/name is one of the matchLabel.

We set the default value of UseNewMatchLabel to true in values.yaml , and we expect that when cilium is installed, app.kubernetes.io/namewill be one of the matchLabel. We may failed when updating chart because matchLabel is immutable, so we need to update it by --set UseNewMatchLabel=false.

[aanm]

/test

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 61.413s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

[tommyp1ckles]

LGTM, thanks you!

[gandro]

Thanks. Given the discussion, the well-known identity change can be simplified again. Let's not add the additional identities.

[gandro]

Thanks. This should be good to go form my side I think. Let's run CI again.

[gandro]

@cyclinder It seems like your signed-off-by line in the commit is not properly formatted. From the checkpatch CI (here):

  Error: ERROR:BAD_SIGN_OFF: Unrecognized email address: 'cyclinder qifeng.guo@daocloud.io'
  #7: 
  Signed-off-by: cyclinder qifeng.guo@daocloud.io
  
  Error: ERROR:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'cyclinder <qifeng.guo@daocloud.io>'

It's missing the < and > around your email address. You can add this via git CLI by using git commit --amend -s

[cyclinder]

@gandro Thank you very much for your help!

[gandro]

/test

[cyclinder]

I have no idea about this CI failed(gke-stable). Can this CI job be retested?

[gandro]

/test-gke

Yeah, looks like a provisioning issue (no k8s1 label node). GKE is currently known to be flaky and not required to merge a PR at the moment. I restarted the test anyways.

Link: https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/8910/testReport/junit/Suite-k8s-1/22/K8sDemosTest_Tests_Star_Wars_Demo/

[cyclinder]

Thanks, Does this PR require a review? This is my first contribution to cilium :) , Can you tell me about the requirements for PR merged?

[gandro]

I prefer to have another code review from the policy CODEOWNERs (e.g. Chris), to ensure the well-known identity change is valid.

[christarazi]

LGTM for well-known identities. Given that the security identity value will not change (just the labels that map to the identity value), then this makes it safe for upgrading / downgrading. I've added the upgrade-impact label just in case we need to review any PRs in the future regarding any changes on upgrade.

[gandro]

Marking this ready to merge. The GKE failure is unrelated.