Fix Istio sidecar injection by moving from annotations to labels #3044

madmecodes · 2025-03-08T14:01:27Z

Pull Request Template for Kubeflow Manifests

✏️ Summary of Changes

This PR fixes Istio sidecar injection by moving control from annotations to labels across all manifests, following Istio best practices documentation.

📦 Dependencies

No dependencies on other PRs.

🐛 Related Issues

Fixes #2798

✅ Contributor Checklist

I have tested these changes with kustomize. See Installation Prerequisites.
All commits are signed-off to satisfy the DCO check.

You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

Signed-off-by: madmecodes <[email protected]>

google-oss-prow · 2025-03-08T14:01:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

juliusvonkohout · 2025-03-08T15:21:39Z

Hello, for everything that has "upstream" in its path you also need to create a PR in the respective upstream repository where we synchronize the manifests from. For example kubeflow/pipelines, kubeflow/kubeflow. Please check out the synchronization scripts in /hack to see where what comes from. But leave this PR here open as well, we need it for all the integration tests. And please rebase (not merge) from master soon.

juliusvonkohout · 2025-03-08T15:22:53Z

/ok-to-test

juliusvonkohout · 2025-03-08T15:32:37Z

/retest

juliusvonkohout · 2025-03-08T15:38:26Z

@madmecodes Thank you for the PR.
Some test seem to fail consistently with your changes

https://github.com/kubeflow/manifests/actions/runs/13738306849/job/38426089031?pr=3044

Run if grep -q 'No YAML files have changed in this PR.' changed_files_in_PR.txt; then
common/knative/knative-serving/base/patches/sidecar-injection.yaml
tests/gh-actions/kf-objects/training_operator_job.yaml
  Error: 18:13 [indentation] wrong indentation: expected 10 but found 12
  Error: 22:17 [indentation] wrong indentation: expected 14 but found 16
  Error: 34:13 [indentation] wrong indentation: expected 10 but found 12
  Error: 38:17 [indentation] wrong indentation: expected 14 but found 16
  Error: 40:31 [new-line-at-end-of-file] no new line character at the end of file
Error: Process completed with exit code 1.

https://github.com/kubeflow/manifests/actions/runs/13738306848/job/38426088943?pr=3044

 Error: accumulating resources: accumulation err='accumulating resources from '../../base': '/home/runner/work/manifests/manifests/apps/spark/spark-operator/base' must resolve to a file': recursed accumulation of path '/home/runner/work/manifests/manifests/apps/spark/spark-operator/base': trouble configuring builtin PatchTransformer with config: `
patch: |-
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: spark-operator-webhook
  spec:
    template:
      metadata:
      labels:
          sidecar.istio.io/inject: "false"
        annotations:
          sidecar.istio.io/inject: "false"
target:
  kind: Deployment
  name: spark-operator-controller
`: unable to parse SM or JSON patch from [patch: "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n  name: spark-operator-webhook\nspec:\n  template:\n    metadata:\n    labels:\n        sidecar.istio.io/inject: \"false\"\n      annotations:\n        sidecar.istio.io/inject: \"false\""]
error: no objects passed to apply
make: *** [Makefile:17: test] Error 1

https://github.com/kubeflow/manifests/actions/runs/13738306844/job/38425965179?pr=3044

juliusvonkohout · 2025-03-08T15:42:49Z

apps/pipeline/upstream/base/metadata/base/metadata-envoy-deployment.yaml

@@ -13,6 +13,7 @@ spec:
    metadata:
      labels:
        component: metadata-envoy
+        sidecar.istio.io/inject: "false"
      annotations:
        sidecar.istio.io/inject: "false"


here its now there twice as annotation and label

apps/pipeline/upstream/base/installs/multi-user/pipelines-profile-controller/deployment.yaml

apps/pipeline/upstream/base/metadata/overlays/db/metadata-db-deployment.yaml

apps/pipeline/upstream/base/metadata/overlays/postgres/metadata-db-deployment.yaml

apps/pipeline/upstream/third-party/metacontroller/base/stateful-set.yaml

apps/spark/spark-operator/base/kustomization.yaml

common/knative/knative-serving/base/patches/sidecar-injection.yaml

madmecodes · 2025-03-08T16:01:30Z

@juliusvonkohout Thanks for the feedback.

Is there a way to run these tests locally before submitting PRs? I'm thinking of merging to my fork first to trigger the GitHub Actions pipeline, or is there another recommended approach to test?

That would help me address issues proactively and ensure the changes work correctly with your CI pipeline.

I'll fix the following:

The YAML indentation issues in the two files mentioned
The syntax error in the spark-operator kustomization.yaml
Remove the duplicate directives - I'll properly migrate from annotations to labels without keeping both

Regarding upstream repos, I'll look into the hack/ directory scripts to identify which upstream repositories need corresponding PRs. Should I start with those after fixing this PR?

juliusvonkohout · 2025-03-10T11:32:38Z

@juliusvonkohout Thanks for the feedback.

Is there a way to run these tests locally before submitting PRs? I'm thinking of merging to my fork first to trigger the GitHub Actions pipeline, or is there another recommended approach to test?

That would help me address issues proactively and ensure the changes work correctly with your CI pipeline.

I'll fix the following:

The YAML indentation issues in the two files mentioned The syntax error in the spark-operator kustomization.yaml Remove the duplicate directives - I'll properly migrate from annotations to labels without keeping both

Regarding upstream repos, I'll look into the hack/ directory scripts to identify which upstream repositories need corresponding PRs. Should I start with those after fixing this PR?

Yes, did you check out https://github.com/kubeflow/manifests#pre-commit-hooks?

And also all other tests / gha workflows are bash and python that you can run locally.

It is also fine to use the ci/cd for testing. And yes we need the upstream PRs in parallel and merge them first.

Signed-off-by: madmecodes <[email protected]>

juliusvonkohout · 2025-03-11T10:20:25Z

Are you talking about

manifests/apps/spark/spark-operator/base/kustomization.yaml

Line 60 in c01c446

name: spark-operator-webhook

? Yes i think that needs to change. By the way spark-operator is missing a synchronization script in /scripts. Right now we just have the makefile https://github.com/kubeflow/manifests/blob/master/apps/spark/Makefile and need to transform it to have it similar as for the other components https://github.com/kubeflow/manifests/blob/master/scripts/synchronize-model-registry-manifests.sh. That is also something you could work on.

madmecodes · 2025-03-12T05:32:40Z

i have updated the PR

juliusvonkohout · 2025-03-12T23:42:38Z

i have updated the PR

Thank you, it looks good now. Now you have to create the corresponding PRs for the manifests in the upstream repositories.

madmecodes · 2025-03-14T13:55:39Z

i have updated the PR

Thank you, it looks good now. Now you have to create the corresponding PRs for the manifests in the upstream repositories.

Upstream PRs like this right?
Created for

Katib Fix Istio sidecar injection by moving from annotations to labels katib#2527
model-registry Fix Istio sidecar injection by moving from annotations to labels model-registry#869
pipeline Fix Istio sidecar injection by moving from annotations to labels pipelines#11750
kubeflow fix: Fix Istio sidecar injection by moving from annotations to labels kubeflow#7708

All upstream files PR done, but i cant find upstream for this file
https://github.com/kubeflow/manifests/blob/master/apps/training-operator/upstream/base/deployment.yaml

juliusvonkohout · 2025-03-14T17:55:46Z

Thank you very much, you can take a look at https://github.com/kubeflow/manifests/blob/master/scripts/synchronize-training-operator-manifests.sh to see where it is coming from.

madmecodes · 2025-03-14T18:02:46Z

Thank you very much, you can take a look at https://github.com/kubeflow/manifests/blob/master/scripts/synchronize-training-operator-manifests.sh to see where it is coming from.

training-operator is being renamed to trainer, i guess, okay i will look into it

madmecodes · 2025-03-15T12:59:05Z

Thank you very much, you can take a look at https://github.com/kubeflow/manifests/blob/master/scripts/synchronize-training-operator-manifests.sh to see where it is coming from.

I recheked, but there is indeed no deployment file in trainer upstream, i did wildcard search also and couldn't find
"sidecar.istio.io/inject": "false" in any file in trainer or training-operator upstream repository.

juliusvonkohout · 2025-03-16T13:55:37Z

Thank you very much, you can take a look at https://github.com/kubeflow/manifests/blob/master/scripts/synchronize-training-operator-manifests.sh to see where it is coming from.

I recheked, but there is indeed no deployment file in trainer upstream, i did wildcard search also and couldn't find "sidecar.istio.io/inject": "false" in any file in trainer or training-operator upstream repository.

no worries, there is a lot of restructureing going on. Lets just get the oter PRs merged then. I started the CI/CD and tagged the maintainers for merging.

Fix Istio sidecar injection by moving from annotations to labels

a8ab234

Signed-off-by: madmecodes <[email protected]>

google-oss-prow bot requested review from gkcalat, juliusvonkohout and tarilabs March 8, 2025 14:01

google-oss-prow bot added the size/M label Mar 8, 2025

google-oss-prow bot added the ok-to-test label Mar 8, 2025