-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: conditionally render insecureSkipTLSVerify in APIService template #1727
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
fix: conditionally render insecureSkipTLSVerify in APIService template #1727
Conversation
|
|
|
This issue is currently awaiting triage. If metrics-server contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
|
Welcome @pawl! |
|
Hi @pawl. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. DetailsInstructions 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-sigs/prow repository. |
ea8a608 to
28ed655
Compare
stevehipwell
left a comment
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.
Thanks for the PR @pawl, I've added a comment on the change. Could you also add an entry to the chart CHANGELOG.
| {{- if .Values.apiService.insecureSkipTLSVerify }} | ||
| insecureSkipTLSVerify: {{ .Values.apiService.insecureSkipTLSVerify }} | ||
| {{- end }} |
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.
| {{- if .Values.apiService.insecureSkipTLSVerify }} | |
| insecureSkipTLSVerify: {{ .Values.apiService.insecureSkipTLSVerify }} | |
| {{- end }} | |
| {{- with .Values.apiService.insecureSkipTLSVerify }} | |
| insecureSkipTLSVerify: {{ . }} | |
| {{- end }} |
This would be more idiomatic to the chart.
3fb9eb7 to
ffd0928
Compare
| {{- with .Values.apiService.insecureSkipTLSVerify }} | ||
| insecureSkipTLSVerify: {{ . }} | ||
| {{- end }} |
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.
| {{- with .Values.apiService.insecureSkipTLSVerify }} | |
| insecureSkipTLSVerify: {{ . }} | |
| {{- end }} | |
| {{- with .Values.apiService.insecureSkipTLSVerify }} | |
| insecureSkipTLSVerify: {{ . }} | |
| {{- end }} |
I suspect GitHub didn't get the formatting correct in my last comment, but the indentation needs fixing.
charts/metrics-server/CHANGELOG.md
Outdated
| ### Fixed | ||
|
|
||
| - Conditionally render `insecureSkipTLSVerify` field in APIService template to prevent GitOps sync drift when value is `false`. ([#1727](https://github.com/kubernetes-sigs/metrics-server/pull/1727)) _@pawl_ | ||
|
|
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.
This needs to be put under the [UNRELEASED] heading. While you're there could you replace the TBC from the 3.13.0 release heading with 2025-07-22 as this hasn't been updated yet.
Only render insecureSkipTLSVerify field when true to prevent GitOps sync drift. Kubernetes omits this field when false (API default), causing ArgoCD to show resources as OutOfSync.
ffd0928 to
85338e9
Compare
|
Thanks for the prompt changes @pawl. /ok-to-test |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pawl, stevehipwell The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR fixes GitOps sync drift caused by the APIService template unconditionally rendering the
insecureSkipTLSVerifyfield.Problem: When
apiService.insecureSkipTLSVerify: falseis set in values, Kubernetes omits this field from live resources (sincefalseis the API default), but the Helm template always renders it explicitly. This causes a continuous diff between desired state (Helm template) and live state (Kubernetes API), leading GitOps tools like ArgoCD to show the APIService as "OutOfSync".Solution: Implement conditional rendering using
{{- if .Values.apiService.insecureSkipTLSVerify }}to only include the field when the value istrue. This matches the approach used by other projects like KEDA that encountered the same issue.Which issue(s) this PR fixes:
Fixes #1725
Testing:
The existing chart CI will verify this change works correctly:
chart-testinginsecureSkipTLSVerify: falsein multiple scenarios:ci/tls-certManager-values.yaml(cert-manager TLS)ci/tls-helm-values.yaml(helm-generated TLS)ci/tls-existingSecret-values.yaml(existing secret TLS, set dynamically)Release note: