refactor(controller): drive policy-server metrics port via CLI flag#1744
refactor(controller): drive policy-server metrics port via CLI flag#1744officialasishkumar wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the controller-wide default PolicyServer metrics Service port configuration from an environment variable to a dedicated CLI flag, and updates tests/docs/charts accordingly.
Changes:
- Add
--policy-server-metrics-portCLI flag (with range validation) to set the controller-wide default metrics Service port. - Remove the
KUBEWARDEN_POLICY_SERVER_SERVICES_METRICS_PORTenv var constant and related wiring. - Update CRD docs, controller comments, Helm templates, and chart tests to reference the CLI flag instead of the env var.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/policyserver_controller_service_test.go | Updates test descriptions/assertion messages to reflect CLI-flag-based priority. |
| internal/controller/policyserver_controller.go | Updates reconciler field documentation to reference CLI flag. |
| internal/constants/constants.go | Removes the env var name constant used for metrics port configuration. |
| docs/crds/CRD-docs-for-docs-repo.md | Updates CRD field docs to reference the CLI flag default. |
| docs/crds/CRD-docs-for-docs-repo.adoc | Updates CRD field docs to reference the CLI flag default. |
| cmd/controller/main.go | Introduces CLI flag + validation and removes env var parsing logic. |
| charts/kubewarden-crds/templates/crds/policies.kubewarden.io_policyservers.yaml | Updates CRD schema description to reference CLI flag default. |
| charts/kubewarden-controller/tests/telemetry_configuration_test.yaml | Updates Helm chart tests to assert args-based flag instead of env var. |
| charts/kubewarden-controller/templates/deployment.yaml | Switches Helm deployment from env var injection to CLI flag argument. |
| api/policies/v1/policyserver_types.go | Updates API field comment to reference CLI flag default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| //nolint:funlen,gocognit // Avoid splitting the main function in multiple functions to avoid changing the retcode logic for metrics shutdown | ||
| //nolint:funlen // Avoid splitting the main function in multiple functions to avoid changing the retcode logic for metrics shutdown |
| // Validate --policy-server-metrics-port range. | ||
| if int64(*policyServerMetricsPortFlag) < minAllowedPort || | ||
| int64(*policyServerMetricsPortFlag) > maxAllowedPort { | ||
| setupLog.Error( | ||
| errors.New("port must be between 1 and 65535"), | ||
| "invalid policy server metrics port", | ||
| "flag", "--policy-server-metrics-port", | ||
| "value", *policyServerMetricsPortFlag, | ||
| "min", minAllowedPort, | ||
| "max", maxAllowedPort, | ||
| ) | ||
| retcode = 1 | ||
| return | ||
| } | ||
| policyServerMetricsPort := int32(*policyServerMetricsPortFlag) |
There was a problem hiding this comment.
Added the deprecated env-var fallback with migration warnings in 0bae37d; explicit --policy-server-metrics-port still takes precedence.
The controller exposed exactly one piece of its configuration through an
environment variable, `KUBEWARDEN_POLICY_SERVER_SERVICES_METRICS_PORT`,
while every other knob already lived behind a `flag.*` CLI flag. The
mismatch made the documentation harder to keep consistent and forced the
Helm chart to use a different mechanism for this single setting.
This commit moves the configuration onto the existing CLI surface:
- `cmd/controller/main.go` registers a new
`--policy-server-metrics-port` integer flag (default
`constants.PolicyServerMetricsPort`, range `1`-`65535`, validated with
the same pattern as `--webhook-server-port`), and drops the
`os.Getenv` block plus the now-unused `strconv` import. The unused
`gocognit` nolint on `main()` is removed because trimming the env-var
parser brings the function under the threshold.
- `internal/constants/constants.go` drops the
`PolicyServerMetricsPortEnvVar` constant; nothing else referenced it.
- The PolicyServer CRD doc comment in `api/policies/v1/policyserver_types.go`
now points users at the new flag, and the generated CRD YAML
(`charts/kubewarden-crds/templates/crds/policies.kubewarden.io_policyservers.yaml`)
and CRD docs (`docs/crds/CRD-docs-for-docs-repo.{adoc,md}`) are
regenerated.
- `charts/kubewarden-controller/templates/deployment.yaml` emits
`--policy-server-metrics-port=...` inside the existing sidecar-mode
args block (where the env var was set previously), and drops the
matching `env:` entry.
- The two helm-unittest assertions in
`charts/kubewarden-controller/tests/telemetry_configuration_test.yaml`
that pinned the env-var name are rewritten as `args:` assertions for
the new flag.
- The comments in `internal/controller/policyserver_controller.go` and
`internal/controller/policyserver_controller_service_test.go` are
refreshed to describe the priority chain in terms of the CLI flag.
Operators who run the controller binary directly with the old env var
set will need to switch to `--policy-server-metrics-port=<n>`; this
breaking change is intentional and matches the request in the issue.
The Helm chart upgrade is transparent because the chart already drives
both ends.
Closes kubewarden#787
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
a63cbd8 to
0bae37d
Compare
| {{- if or .Values.telemetry.metrics .Values.telemetry.tracing }} | ||
| {{- if and (eq .Values.telemetry.mode "sidecar") }} | ||
| - --enable-otel-sidecar | ||
| {{- if .Values.telemetry.metrics }} |
There was a problem hiding this comment.
If you migrating to the CLI, please ensure the helm chart template follow the same "if"
There was a problem hiding this comment.
Updated in f3190f5. The template now uses the same and .Values.telemetry.metrics (eq .Values.telemetry.mode "sidecar") condition for --policy-server-metrics-port, with Helm unittest coverage for sidecar tracing-only mode.
| policyServerMetricsPortFlag := flag.Int("policy-server-metrics-port", | ||
| constants.PolicyServerMetricsPort, | ||
| "The default port exposed by every PolicyServer metrics Service. "+ | ||
| "Per-PolicyServer overrides (spec.metricsPort) always take priority.") |
There was a problem hiding this comment.
Please, follow the other CLI flags parttern. Declare the var above and use it in the IntVar call.
There was a problem hiding this comment.
Updated in f3190f5. policyServerMetricsPort is now declared with the other local flag variables and registered through flag.IntVar, matching the existing pattern.
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
e2bfc81 to
f3190f5
Compare
Summary
Closes #787.
The controller exposed exactly one piece of its configuration through an environment variable,
KUBEWARDEN_POLICY_SERVER_SERVICES_METRICS_PORT, while every other knob already lived behind aflag.*CLI flag. The mismatch made the documentation harder to keep consistent and forced theHelm chart to use a different mechanism for this single setting.
This PR moves the configuration onto the existing CLI surface as a new
--policy-server-metrics-portinteger flag and updates everything downstream of that change.What changes
cmd/controller/main.go--policy-server-metrics-portflag (defaultconstants.PolicyServerMetricsPort, validated against the same[1, 65535]range as--webhook-server-port). Drops theos.Getenvblock plus thestrconvimport. Removes the now-unusedgocognitnolint onmain().internal/constants/constants.goPolicyServerMetricsPortEnvVar.api/policies/v1/policyserver_types.gometricsPortnow points users at the new flag.charts/kubewarden-crds/templates/crds/policies.kubewarden.io_policyservers.yamldocs/crds/CRD-docs-for-docs-repo.{adoc,md}charts/kubewarden-controller/templates/deployment.yaml--policy-server-metrics-port=...inside the existing sidecar-mode args block (where the env var was set previously) and drops the matchingenv:entry.charts/kubewarden-controller/tests/telemetry_configuration_test.yamlargs:assertions for the new flag.internal/controller/policyserver_controller.goPolicyServerMetricsPortnow describes the CLI flag origin.internal/controller/policyserver_controller_service_test.goCompatibility note
Operators who run the controller binary directly with the old env var set will need to switch to
--policy-server-metrics-port=<n>. This breaking change matches the request in the issue. TheHelm chart upgrade is transparent because the chart drives both ends.
Test plan
go build ./...builds cleanlygo vet ./...cleangolangci-lint run ./cmd/... ./internal/... ./api/...reports zero issuesmake test-gopasses all 58 envtest controller specsmake helm-unittestpasses for all three charts (62 + 25 + 5 tests). Thetelemetry configurationsuite still validates that the sidecar-mode flag is presentand that non-sidecar mode does not emit it.
make generateis idempotent (the regenerated CRD chart and CRD docs only differ inthe comment line that switched from env-var name to CLI flag name).