add "app.kubernetes.io/name" to gateway.podlabels #50540

lukmdo · 2024-04-18T22:56:05Z

Please provide a description of this PR:

Two commits to squash or keep the main only

main (separate commit)

add "app.kubernetes.io/name" to gateway.podLabels

Goal: to have matching "app.kubernetes.io/name" label and value for on deployment and pod for metrics.

Using .Values.labels for that will lead to duplicate "app.kubernetes.io/name" labels.

optional (cleanup as separate commit)

move istio.io/rev label to gateway.podLabels (not-functional change)

Goal: to have matching "app.kubernetes.io/name" label and value for on `deployment` and `pod` for metrics. Using `.Values.labels` for that will lead to duplicate "app.kubernetes.io/name" labels.

this is not-functional change only optional cleanup

istio-policy-bot · 2024-04-18T22:56:09Z

😊 Welcome @lukmdo! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

linux-foundation-easycla · 2024-04-18T22:56:11Z

The committers listed above are authorized under a signed CLA.

✅ login: lukmdo / name: Lukasz Dobrzanski (f32ab97, 0457dfc, 3ac9a50, 281c532, 5f32127)

istio-testing · 2024-04-18T22:56:16Z

Hi @lukmdo. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hanxiaop

app label can take effect too for the telemetry, see https://istio.io/latest/docs/ops/deployment/requirements/#pod-requirements

manifests/charts/gateway/templates/_helpers.tpl

costinm

Looks good, useful for telemetry - but I would use the standard label for version too.

istio#50540 (review)

istio#50540 (review) help managing common labels

lukmdo · 2024-04-19T14:45:29Z

Looks good, useful for telemetry - but I would use the standard label for version too.

thank you for feedback

@costinm I added separate new commits

281c532 review: add app.kubernetes.io/version to gateway.podLabels
3ac9a50 review: (opt cleanup) use gateway.podLabels in gateway.labels

ericvn · 2024-04-23T15:11:42Z

/ok-to-test

ericvn · 2024-04-23T15:13:42Z

I'll add a DNM which @costinm can remove after he reviews the additional changes.

costinm

Left few comments on readability/structure.

costinm · 2024-04-23T15:56:59Z

manifests/charts/gateway/templates/deployment.yaml

@@ -24,9 +24,6 @@ spec:
      {{- end }}
      labels:
        sidecar.istio.io/inject: "true"


Both inject and rev are part of the "select injector" - they should stick together, very confusing otherwise.

Worth also adding a comment for future contributors on the purpose of the pair ( matching the webhook injector ).

Thank you - applied by addding sidecarInjectionLabels with note and doc link

please see comment https://github.com/istio/istio/pull/50540/files#r1582266135

good to resolve from my side - @costinm I am open for your feedback

costinm · 2024-04-23T15:58:02Z

manifests/charts/gateway/templates/deployment.yaml

@@ -24,9 +24,6 @@ spec:
      {{- end }}
      labels:
        sidecar.istio.io/inject: "true"
-        {{- with .Values.revision }}
-        istio.io/rev: {{ . | quote }}
-        {{- end }}
        {{- include "gateway.podLabels" . | nindent 8 }}


Also as cosmetic/readability: is the gateway.podLabels template used in multiple places ? If not - it would easier to read if it was in this file, instead of having a redirection and template complexity.

gateway.podLabels is only used here and in gateway.labels

I agree naming here may be misleading as the current gateway.podLabels is more:

gateway.podBaseLabels

gateway.commonLabels

(no injection labels)

I decided to apply your comments in 5f32127
by:

add sidecarInjectionLabels with note and doc link

remove the gateway.podLabels template. If it feels easier to read and maintain, may consider using gateway.commonLabels

{{- define "gateway.commonLabels" -}} {{ include "gateway.selectorLabels" }} app.kubernetes.io/name: {{ include "gateway.name" . }} app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} {{- range $key, $val := .Values.labels }} {{- if and (ne $key "app") (ne $key "istio") }} {{ $key | quote }}: {{ $val | quote }} {{- end }} {{- end }} {{- end }}

This is good to resolve from my side - @costinm I am open to your feedback

manifests/charts/gateway/templates/_helpers.tpl

lukmdo · 2024-04-28T18:40:07Z

See:

Helm Diff: master/pr50540

--- helm.master.yaml	2024-04-28 18:57:22
+++ helm.pr50540.yaml	2024-04-28 19:10:10
@@ -1,188 +1,190 @@
 ---
 # Source: gateway/templates/serviceaccount.yaml
 apiVersion: v1
 kind: ServiceAccount
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
 ---
 # Source: gateway/templates/role.yaml
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
   annotations:
     {}
 rules:
 - apiGroups: [""]
   resources: ["secrets"]
   verbs: ["get", "watch", "list"]
 ---
 # Source: gateway/templates/role.yaml
 apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
   annotations:
     {}
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: Role
   name: istio-gateway
 subjects:
 - kind: ServiceAccount
   name: istio-gateway
 ---
 # Source: gateway/templates/service.yaml
 apiVersion: v1
 kind: Service
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
   annotations:
     {}
 spec:
   type: LoadBalancer
   ports:
     - name: status-port
       port: 15021
       protocol: TCP
       targetPort: 15021
     - name: http2
       port: 80
       protocol: TCP
       targetPort: 80
     - name: https
       port: 443
       protocol: TCP
       targetPort: 443
   selector:
     app: istio-gateway
     istio: gateway
 ---
 # Source: gateway/templates/deployment.yaml
 apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
   annotations:
     {}
 spec:
   selector:
     matchLabels:
       app: istio-gateway
       istio: gateway
   template:
     metadata:
       annotations:
         inject.istio.io/templates: gateway
         prometheus.io/path: /stats/prometheus
         prometheus.io/port: "15020"
         prometheus.io/scrape: "true"
         sidecar.istio.io/inject: "true"
       labels:
         sidecar.istio.io/inject: "true"
         app: istio-gateway
         istio: gateway
+        app.kubernetes.io/name: istio-gateway
+        app.kubernetes.io/version: "1.0.0"
     spec:
       serviceAccountName: istio-gateway
       securityContext:
         # Safe since 1.22: https://github.com/kubernetes/kubernetes/pull/103326
         sysctls:
         - name: net.ipv4.ip_unprivileged_port_start
           value: "0"
       containers:
         - name: istio-proxy
           # "auto" will be populated at runtime by the mutating webhook. See https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#customizing-injection
           image: auto
           securityContext:
             capabilities:
               drop:
               - ALL
             allowPrivilegeEscalation: false
             privileged: false
             readOnlyRootFilesystem: true
             runAsUser: 1337
             runAsGroup: 1337
             runAsNonRoot: true
           env:
           ports:
           - containerPort: 15090
             protocol: TCP
             name: http-envoy-prom
           resources:
             limits:
               cpu: 2000m
               memory: 1024Mi
             requests:
               cpu: 100m
               memory: 128Mi
       terminationGracePeriodSeconds: 30
 ---
 # Source: gateway/templates/hpa.yaml
 apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
   name: istio-gateway
   namespace: remote-ag-stg
   labels:
     helm.sh/chart: gateway-1.0.0
     app: istio-gateway
     istio: gateway
+    app.kubernetes.io/name: istio-gateway
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/managed-by: Helm
-    app.kubernetes.io/name: istio-gateway
   annotations:
     {}
 spec:
   scaleTargetRef:
     apiVersion: apps/v1
     kind: Deployment
     name: istio-gateway
   minReplicas: 1
   maxReplicas: 5
   metrics:
     - type: Resource
       resource:
         name: cpu
         target:
           averageUtilization: 80
           type: Utilization

lukmdo added 2 commits April 18, 2024 23:36

add "app.kubernetes.io/name" to gateway.podLabels

f32ab97

Goal: to have matching "app.kubernetes.io/name" label and value for on `deployment` and `pod` for metrics. Using `.Values.labels` for that will lead to duplicate "app.kubernetes.io/name" labels.

opt: (cleanup) move istio.io/rev label to gateway.podLabels

0457dfc

this is not-functional change only optional cleanup

lukmdo requested a review from a team as a code owner April 18, 2024 22:56

istio-policy-bot added area/environments release-notes-none Indicates a PR that does not require release notes. labels Apr 18, 2024

istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels Apr 18, 2024

hanxiaop reviewed Apr 19, 2024

View reviewed changes

costinm reviewed Apr 19, 2024

View reviewed changes

manifests/charts/gateway/templates/_helpers.tpl Outdated Show resolved Hide resolved

costinm approved these changes Apr 19, 2024

View reviewed changes

lukmdo added 2 commits April 19, 2024 15:32

review: add app.kubernetes.io/version to gateway.podLabels

281c532

istio#50540 (review)

review: (opt cleanup) use gateway.podLabels in gateway.labels

3ac9a50

istio#50540 (review) help managing common labels

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 19, 2024

lukmdo requested a review from costinm April 19, 2024 14:51

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Apr 23, 2024

ericvn added the do-not-merge Block automatic merging of a PR. label Apr 23, 2024

costinm reviewed Apr 23, 2024

View reviewed changes

review: add gateway.sidecarInjectionLabels, drop gateway.podLabels

5f32127

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 28, 2024

lukmdo requested a review from costinm April 28, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "app.kubernetes.io/name" to gateway.podlabels #50540

add "app.kubernetes.io/name" to gateway.podlabels #50540

lukmdo commented Apr 18, 2024 •

edited by istio-policy-bot

istio-policy-bot commented Apr 18, 2024

linux-foundation-easycla bot commented Apr 18, 2024 •

edited

istio-testing commented Apr 18, 2024

hanxiaop left a comment

costinm left a comment

lukmdo commented Apr 19, 2024

ericvn commented Apr 23, 2024

ericvn commented Apr 23, 2024

costinm left a comment

costinm Apr 23, 2024

lukmdo Apr 28, 2024 •

edited

costinm Apr 23, 2024

lukmdo Apr 28, 2024 •

edited

lukmdo commented Apr 28, 2024

add "app.kubernetes.io/name" to gateway.podlabels #50540

Are you sure you want to change the base?

add "app.kubernetes.io/name" to gateway.podlabels #50540

Conversation

lukmdo commented Apr 18, 2024 • edited by istio-policy-bot

istio-policy-bot commented Apr 18, 2024

linux-foundation-easycla bot commented Apr 18, 2024 • edited

istio-testing commented Apr 18, 2024

hanxiaop left a comment

Choose a reason for hiding this comment

costinm left a comment

Choose a reason for hiding this comment

lukmdo commented Apr 19, 2024

ericvn commented Apr 23, 2024

ericvn commented Apr 23, 2024

costinm left a comment

Choose a reason for hiding this comment

costinm Apr 23, 2024

Choose a reason for hiding this comment

lukmdo Apr 28, 2024 • edited

Choose a reason for hiding this comment

costinm Apr 23, 2024

Choose a reason for hiding this comment

lukmdo Apr 28, 2024 • edited

Choose a reason for hiding this comment

lukmdo commented Apr 28, 2024

lukmdo commented Apr 18, 2024 •

edited by istio-policy-bot

linux-foundation-easycla bot commented Apr 18, 2024 •

edited

lukmdo Apr 28, 2024 •

edited

lukmdo Apr 28, 2024 •

edited