Adding custom labels

[gtrkiller]

This PR focuses on the creation of a custom label for both services and ingresses, to not be dependant on the app.juju.is/created-by= label.

[merkata]

Looks good! I wonder if we shouldn't namespace the label like juju is doing, though that is just food for thought.

[jdkandersson]

I still don't see an upgrade test, maybe I missed it?

[jdkandersson]

Since we are making a change to the behaviour (a label is now a different value), an upgrade test from a version of the charm that used to old label to the new label would help us ensure that we don't cause any issues for any users that upgrade. What do you think?

[mthaddon]

Looks good! I wonder if we shouldn't namespace the label like juju is doing, though that is just food for thought.

Yeah, I kind of agree that the label just being created-by is a big too generic. created-by-juju-application seems better, but looking at https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ this one jumps out as being relevant:

Key                          | Description                                                   | Example | Type
app.kubernetes.io/managed-by | The tool being used to manage the operation of an application | helm    | string

Based on this I'd go with app.kubernetes.io/managed-by=juju-application-{self.app.name} after confirming this label isn't already set. Would be interested in other people's thoughts about this as well.

[gtrkiller]

Regarding the new and old labels, the old label (app.juju.is/created-by=) is still there, since it is created on Juju's side and not by the charm. That means we now have two labels, the juju label and our new custom label. So, is it really necessary to have an upgrade test since the old label hasn't been removed? The ingress and service definition/update takes place only when the config or relations are changed, not in an upgrade event. I tested an upgrade and the ingress worked just fine.

[amandahla]

Looks good! I wonder if we shouldn't namespace the label like juju is doing, though that is just food for thought.

Yeah, I kind of agree that the label just being created-by is a big too generic. created-by-juju-application seems better, but looking at https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ this one jumps out as being relevant:

Key                          | Description                                                   | Example | Type
app.kubernetes.io/managed-by | The tool being used to manage the operation of an application | helm    | string

Based on this I'd go with app.kubernetes.io/managed-by=juju-application-{self.app.name} after confirming this label isn't already set. Would be interested in other people's thoughts about this as well.

I liked this but I think it might conflict here:
https://github.com/juju/juju/blob/develop/caas/kubernetes/provider/constants/labels.go#L19
For namespaces:

mymodel                      Active   4h9m    app.kubernetes.io/managed-by=juju,kubernetes.io/metadata.name=mymodel,model.juju.is/name=mymodel

I'm considering the idea of using the same label "app.juju.is/created-by". So will be applied even if is not done by Juju.

[jdkandersson]

Regarding the new and old labels, the old label (app.juju.is/created-by=) is still there, since it is created on Juju's side and not by the charm. That means we now have two labels, the juju label and our new custom label. So, is it really necessary to have an upgrade test since the old label hasn't been removed? The ingress and service definition/update takes place only when the config or relations are changed, not in an upgrade event. I tested an upgrade and the ingress worked just fine.

I was more thinking that during an upgrade the new custom label may not be present when the charm code assumes it is there - hence the upgrade test. It would be good to see the test methodology and the outcome just to see if anyone has any feedback

[mthaddon]

I'm considering the idea of using the same label "app.juju.is/created-by". So will be applied even if is not done by Juju.

I think this is a great idea. And thanks for finding the issue with my other suggestion. I'd switch my vote to this approach.

[weiiwang01]

Another approach is to create a new label prefix for the Nginx ingress integrator charm. The advantage of this option is that we can create as many labels as we want without having to worry about conflicts.

For example:

nginx-ingress-integrator.charm.canonical.com/managed-by=<ingress-app-name>
nginx-ingress-integrator.charm.canonical.com/created-for=<app-name>

[amandahla]

Another approach is to create a new label prefix for the Nginx ingress integrator charm. The advantage of this option is that we can create as many labels as we want without having to worry about conflicts.

For example:

nginx-ingress-integrator.charm.canonical.com/managed-by=<ingress-app-name>
nginx-ingress-integrator.charm.canonical.com/created-for=<app-name>

I'm afraid that in this case would be redundant (and pretty close to the limit of label size)
Also, I guess that a valid DNS should be used like juju.is

[weiiwang01]

Another approach is to create a new label prefix for the Nginx ingress integrator charm. The advantage of this option is that we can create as many labels as we want without having to worry about conflicts.
For example:

nginx-ingress-integrator.charm.canonical.com/managed-by=<ingress-app-name>
nginx-ingress-integrator.charm.canonical.com/created-for=<app-name>

I'm afraid that in this case would be redundant (and pretty close to the limit of label size) Also, I guess that a valid DNS should be used like juju.is

The limit of 63 characters only applies to the name segment i.e. the segment after /, the prefix segment can have another 253 characters.

And yes, we can change the prefix to nginx-ingress-integrator.charm.juju.is, the exact prefix value is still up to debate.

[weiiwang01]

Another approach is to create a new label prefix for the Nginx ingress integrator charm. The advantage of this option is that we can create as many labels as we want without having to worry about conflicts.
For example:

nginx-ingress-integrator.charm.canonical.com/managed-by=<ingress-app-name>
nginx-ingress-integrator.charm.canonical.com/created-for=<app-name>

I'm afraid that in this case would be redundant (and pretty close to the limit of label size) Also, I guess that a valid DNS should be used like juju.is

The limit of 63 characters only applies to the name segment i.e. the segment after /, the prefix segment can have another 253 characters.

And yes, we can change the prefix to nginx-ingress-integrator.charm.juju.is, the exact prefix value is still up to debate.

I think if we choose the customized prefix solution here, we will create a new pattern / convention. Other charm that manages the Kubernetes resources can all have similar label names like this: <charm-name>.charm.juju.is/<name> if they want to add labels to the resources.

[mthaddon]

There have been a number of comments about this on the discourse thread, but let's go ahead with nginx-ingress-integrator.charm.juju.is/managed-by for now and we can easily update this if changed later.

[github-actions]

Test coverage for e912e7e

Name           Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------
src/charm.py     414     25    166      7    93%   342->344, 439-441, 445-450, 454-455, 459-460, 464-469, 484-486, 591->616, 820->exit, 871, 897->928, 915-926
----------------------------------------------------------
TOTAL            414     25    166      7    93%

Static code analysis report

Run started:2023-02-24 16:52:37.267652

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 3083
  Total lines skipped (#nosec): 3
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 1

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

[mthaddon]

LGTM, thx

[amandahla]

Nice :)