Change default PrometheusMetricsClientAuth to NoClientCert#11813
Change default PrometheusMetricsClientAuth to NoClientCert#11813peppi-lotta wants to merge 1 commit intoprojectcalico:masterfrom
Conversation
Signed-off-by: peppi-lotta <peppi-lotta.saari@est.tech>
There was a problem hiding this comment.
Pull request overview
This PR adjusts Calico’s Prometheus /metrics TLS client authentication default from RequireAndVerifyClientCert to NoClientCert across Felix/Typha config defaults and the corresponding CRD/manifests/docs, aligning behavior with more typical user expectations after TLS support was introduced.
Changes:
- Change Felix and Typha config default for
PrometheusMetricsClientAuthtoNoClientCert. - Update CRD/manifests/OpenAPI/docs text to reflect the new default.
- Update TLS client-auth string conversion behavior/error message and align the metrics server unit test expectations.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
felix/config/config_params.go |
Changes Felix config default for PrometheusMetricsClientAuth to NoClientCert. |
typha/pkg/config/config_params.go |
Changes Typha config default for PrometheusMetricsClientAuth to NoClientCert. |
crypto/pkg/tls/tls.go |
Treats empty client-auth as NoClientCert and adjusts invalid-type error behavior. |
libcalico-go/lib/metricsserver/metrics_server_test.go |
Updates unit test expectation for invalid client-auth error text. |
api/pkg/apis/projectcalico/v3/felixconfig.go |
Updates API field comment to state the new default. |
api/pkg/openapi/generated.openapi.go |
Updates generated OpenAPI description string to the new default. |
felix/docs/config-params.md |
Updates Felix configuration documentation for the new default. |
felix/docs/config-params.json |
Updates generated doc metadata defaults/descriptions for the new default. |
libcalico-go/config/crd/crd.projectcalico.org_felixconfigurations.yaml |
Updates CRD description text to the new default. |
manifests/crds.yaml |
Updates bundled CRD description text to the new default. |
manifests/operator-crds.yaml |
Updates operator CRD description text to the new default. |
manifests/calico.yaml |
Updates manifest CRD description text to the new default. |
manifests/calico-bpf.yaml |
Updates manifest CRD description text to the new default. |
manifests/calico-policy-only.yaml |
Updates manifest CRD description text to the new default. |
manifests/calico-typha.yaml |
Updates manifest CRD description text to the new default. |
manifests/calico-vxlan.yaml |
Updates manifest CRD description text to the new default. |
manifests/canal.yaml |
Updates manifest CRD description text to the new default. |
manifests/flannel-migration/calico.yaml |
Updates manifest CRD description text to the new default. |
| return tls.NoClientCert, nil | ||
| default: | ||
| return tls.RequireAndVerifyClientCert, fmt.Errorf("invalid client authentication type: %s. Defaulting to RequireAndVerifyClientCert", clientAuthType) | ||
| return 0, fmt.Errorf("invalid client authentication type: %s", clientAuthType) |
There was a problem hiding this comment.
In the error case this returns a literal 0 for tls.ClientAuthType. That relies on the underlying numeric value of the enum and can be a footgun if a future caller logs/ignores the error (it would silently behave like whatever value 0 maps to). Return an explicit tls.ClientAuthType constant in the error path (and optionally keep the chosen fallback mentioned in the error text) to make the behavior unambiguous and safer.
| return 0, fmt.Errorf("invalid client authentication type: %s", clientAuthType) | |
| return tls.NoClientCert, fmt.Errorf("invalid client authentication type: %s", clientAuthType) |
Description
Few months ago my PR adding tls to encrypt metrics endpoint merged. I think the
RequireAndVerifyClientCertdefault I set for thePrometheusMetricsClientAuthis too strict. It's more common to haveNoClientCertas a default. I believeNoClientCertis what users will expect the default to be and changing the default will provide a better user experience.Related issues/PRs
#10495
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.