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

Support "canonical" handling of app and version labels #8122

Merged
merged 19 commits into from
Feb 12, 2025

Conversation

jshaughn
Copy link
Collaborator

@jshaughn jshaughn commented Feb 5, 2025

Closes #7603

Operator PR: kiali/kiali-operator#879

Historically, Kiali supports a single set of app/version label names and the Kiali defaults to the original Istio label names:
app, version.
Over time the labeling schemes have evolved, and many users prefer the k8s label names of:
app.kubernetes.io/name, app.kubernetes.io/version
And to handle this mixed approach in telemetry, Istio introduced the idea of a canonical app and version, and with it introduced the label names:
service.istio.io/canonical-name, service.istio.io/canonical-revision

When setting the canonical value, the order of preference is:

  1. service.istio.io/canonical-name, service.istio.io/canonical-revision
  2. app.kubernetes.io/name, app.kubernetes.io/version
  3. app, version

What has resulted for many users is a mixed labeling scheme. And as such, Kiali's single scheme, as defined in the Kiali CR, is limiting to those users, where certain objects may be missed.

This PR relieves that limitation by allowing for a mixed scheme by default. The Kiali CR now defaults to unset values for istio_labels.app_label_name and istio_labels.version_label_name, and will accept any of the three schemes above. For users with a consistent scheme, they can still set the CR to their scheme (one of the above, or something custom).

Note that this PR, and therefore Kiali, does not support a mix of app and version label names. For example, if labeling a deployment with app, it is assumed that the version label name would be version. Although, it is valid to label the same object with multiple schemes. The same service could be labeled with both app=foo and app.kubernetes.io/name=foo. Note that when determining the app label value used on the service, the canonical order of preference is applied. So, in the [unrecommended] situation where the same service is labeled with both app=foo and app.kubernetes.io/name=bar, bar would be the preferred value.

The crux of the PR is in config.go, where we now determine the active label names when the config is set, and a set of utilities that do the handling. It is now very rare to find in the code a direct access of config.IstioLabels.AppLabelName, for example. In general all access is done via the utilities. On the client, there are analogous utilities found in ServerConfig.ts.

To test:
Using the operator and server prs, install kiali and bookinfo. With the defaults, you will see some different behavior. For example, in overview you may see a higher number of applications listed for istio-system, because we are now picking up apps not labeled with app, but with other labeling schemes. You can play with mixed labeling to convince yourself it is working. Then, update the CR to set a single scheme. After the restart you should lose sight of objects not labeled in that way.

Note:
PR also includes a fix to hack/istio/multicluster/install-multi-primary.sh to better handle DORP=podman

@jshaughn jshaughn self-assigned this Feb 5, 2025
@jshaughn jshaughn added test: back-end/integration PR adds/updates back-end tests (unit and/or integration) requires docs PR Requires kiali.io or other documentation updates requires operator PR It requires update in operator code labels Feb 5, 2025
Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

In all of the unit tests, you have now added this:

	conf.IstioLabels.AppLabelName = "app"
	conf.IstioLabels.VersionLabelName = "version"

Why don't you have some of these tests set those to empty string? That will exercise your new code. And since all of the tests don't have the canonical or kubernetes.io labels, the tests should all pass still.

@jshaughn
Copy link
Collaborator Author

jshaughn commented Feb 5, 2025

Why don't you have some of these tests set those to empty string? That will exercise your new code. And since all of the tests don't have the canonical or kubernetes.io labels, the tests should all pass still.

Yes, this is a Draft PR, I'm not done coding but I wanted to start running CI. When all of the current tests are passing I'll write dedicated tests for the new use cases.

I originally thought the same as you, that the tests will still pass. But its not true because the way the tests are written is that they access the serverConfig settings directly. I didn't want to convert all of it so instead I used the existing tests to prove that things still work if the user DOES set the fields explicitly in the config. The new default will be that the config is unset, and that is what the pending tests will verify.

@jshaughn jshaughn force-pushed the kiali7603 branch 5 times, most recently from 0ac161a to cc87bf6 Compare February 8, 2025 17:02
@jshaughn jshaughn marked this pull request as ready for review February 8, 2025 17:54
@jshaughn jshaughn requested a review from jmazzitelli February 11, 2025 18:23
@jmazzitelli
Copy link
Collaborator

jmazzitelli commented Feb 11, 2025

How I tested:

  1. installed bookinfo - I see it all normally
  2. create mazz namespace and label it with istio-injection=enabled
  3. Create a test app like this:
$ kubectl apply -f - <<EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-app
  namespace: mazz
spec:
  replicas: 1
  selector:
    matchLabels:
      foo: bar
  template:
    metadata:
      labels:
        foo: bar
        app.kubernetes.io/name: test-app
        app.kubernetes.io/version: v1
    spec:
      containers:
        - name: test-app
          image: nginx
          ports:
            - containerPort: 80
EOF

See test-app show up in the Applications list.

Now edit the Kiali CR so the istio_labels has app_label_name=app and version_label_name=version.

After Kiali server pod restarts, see the test-app no longer in the applications list.

Now edit that test Deployment so it has these labels:

        app.kubernetes.io/name: test-app
        app: tttest-app
        app.kubernetes.io/version: v1
        version: vvv1

Now see the test app show up in the Applications list again (proving that the istio_labels did filter only on "app/version"

business/dashboards.go Show resolved Hide resolved
business/services.go Outdated Show resolved Hide resolved
business/workloads.go Show resolved Hide resolved
frontend/public/locales/zh/translation.json Show resolved Hide resolved
- replace some literal instances of "app" as a label name
- where it was correct, replace with a constant
- consolidate some label constants into config
@jshaughn
Copy link
Collaborator Author

@jmazzitelli I pushed a change for your requested fix. I also pushed a few more changes due to finding explicit uses of "app" as a label. Explicitly using "app" is OK when we are working with Istio components, where we should rely on it. I made this more clear by creating a constant. I replaced some other Istio literals with constants, so the file count when up with the refactor.

Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

looks good... molecule tests pass, too

@jshaughn jshaughn merged commit 6050ee6 into kiali:master Feb 12, 2025
10 checks passed
@jshaughn jshaughn deleted the kiali7603 branch February 12, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires docs PR Requires kiali.io or other documentation updates requires operator PR It requires update in operator code test: back-end/integration PR adds/updates back-end tests (unit and/or integration)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add AppLabelNameSet and VersionLabelNameSet Inputs
2 participants