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
add "app.kubernetes.io/name" to gateway.podlabels #50540
base: master
Are you sure you want to change the base?
add "app.kubernetes.io/name" to gateway.podlabels #50540
Conversation
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
😊 Welcome @lukmdo! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app
label can take effect too for the telemetry, see https://istio.io/latest/docs/ops/deployment/requirements/#pod-requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, useful for telemetry - but I would use the standard label for version too.
/ok-to-test |
I'll add a DNM which @costinm can remove after he reviews the additional changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few comments on readability/structure.
@@ -24,9 +24,6 @@ spec: | |||
{{- end }} | |||
labels: | |||
sidecar.istio.io/inject: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usinggateway.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
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 |
@costinm thank you and I belive I applied/replied to all. I left some comments intentionally open for you to review. |
Please provide a description of this PR:
Two commits to squash or keep the main only