chore(charts): define and use named ports for probes#5775
chore(charts): define and use named ports for probes#5775Skarlso merged 4 commits intoexternal-secrets:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughProbe ports were changed to named ports: readiness -> Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This requires a bit more. Like this: # -- Set this value to 8082 to active liveness probes.
# @schema type: [string, integer]
port: 8082Otherwise this would be a breaking change. Further, you need to run |
|
I ran |
f456472 to
7328be1
Compare
|
I think this is largely sorted. I cannot figure out how to get the README to say |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/charts/external-secrets/README.md (1)
141-146: Documentation reflects the breaking change from values.yaml.The README now documents the liveness probe port as
"metrics"(string, port 8080), which differs from the previous default of8082(integer). This documentation change is contingent on the values.yaml change being finalized.If the values.yaml default is reverted to maintain backward compatibility (as suggested in the review comment on that file), this documentation will need to be updated accordingly to show the numeric default with an explanation that string port names are also supported.
deploy/charts/external-secrets/templates/cert-controller-deployment.yaml (1)
106-123: Probe port references correctly use named ports.The readinessProbe (line 108) and startupProbe (lines 116, 118) correctly reference the named ports
readyandstartuprather than numeric ports. The conditional logic properly handles theuseReadinessProbePortsetting.Note: The readinessProbe port reference is hard-coded to use the
readynamed port. Unlike the startupProbe which has configurable port options, the readinessProbe always uses the port defined incertController.readinessProbe.port(currently 8081). This is acceptable for the cert-controller component.Optional: Consider making readinessProbe port configurable
If there's a future need to allow users to customize which port the readinessProbe uses (similar to the startupProbe's
useReadinessProbePortflag), you could add logic like:readinessProbe: httpGet: {{- if .Values.certController.readinessProbe.useNamedPort }} port: ready {{- else }} port: {{ .Values.certController.readinessProbe.port }} {{- end }} path: /readyzHowever, this is likely unnecessary complexity for the current use case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
deploy/charts/external-secrets/tests/__snapshot__/cert_controller_test.yaml.snapis excluded by!**/*.snapdeploy/charts/external-secrets/tests/__snapshot__/webhook_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
deploy/charts/external-secrets/README.mddeploy/charts/external-secrets/templates/cert-controller-deployment.yamldeploy/charts/external-secrets/templates/webhook-deployment.yamldeploy/charts/external-secrets/tests/cert_controller_test.yamldeploy/charts/external-secrets/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/charts/external-secrets/templates/webhook-deployment.yaml
🔇 Additional comments (2)
deploy/charts/external-secrets/tests/cert_controller_test.yaml (1)
220-255: LGTM! Test assertions correctly validate named port usage.The test updates properly verify:
- Default readinessProbe uses the named port
ready(line 231)- When startup probe is configured with a custom port, a third containerPort named
startupis added (lines 242-247)- The startupProbe references the named port
startup(line 252)The test expectations align with the template implementation.
deploy/charts/external-secrets/templates/cert-controller-deployment.yaml (1)
98-105: Named port implementation is correct for cert-controller.The template properly:
- Defines a containerPort named
readyfor the readiness probe (lines 98-100)- Conditionally adds a containerPort named
startupwhenstartupProbe.enabledis true anduseReadinessProbePortis false (lines 101-105)This aligns with Kubernetes best practices for using named ports in probe configurations.
|
I think I've made coderabbit happy? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/charts/external-secrets/tests/controller_test.yaml (1)
182-215: Consider clarifying test name for precedence behavior.The test name "should customize livenessProbe port under spec.httpGet before spec.port" is slightly ambiguous. The test shows that when both
spec.port: 3030andhttpGet.port: 8080are set,httpGet.port(8080) determines the containerPort.This is correct behavior based on the template logic at line 142-145 of deployment.yaml, which uses
httpGet.portwhen it's not a string. Consider renaming to clarify the precedence, e.g., "should use httpGet.port over spec.port when httpGet.port is numeric".Suggested test name clarification
- - it: should customize livenessProbe port under spec.httpGet before spec.port + - it: should use httpGet.port for containerPort when httpGet.port is numeric (ignoring spec.port)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
deploy/charts/external-secrets/tests/__snapshot__/cert_controller_test.yaml.snapis excluded by!**/*.snapdeploy/charts/external-secrets/tests/__snapshot__/webhook_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
deploy/charts/external-secrets/README.mddeploy/charts/external-secrets/templates/cert-controller-deployment.yamldeploy/charts/external-secrets/templates/deployment.yamldeploy/charts/external-secrets/templates/webhook-deployment.yamldeploy/charts/external-secrets/tests/cert_controller_test.yamldeploy/charts/external-secrets/tests/controller_test.yamldeploy/charts/external-secrets/values.schema.jsondeploy/charts/external-secrets/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/charts/external-secrets/templates/webhook-deployment.yaml
- deploy/charts/external-secrets/README.md
🔇 Additional comments (12)
deploy/charts/external-secrets/values.schema.json (1)
481-483: LGTM! Schema correctly separates numeric port from named port.The new
portproperty underlivenessProbe.specis typed asinteger, which provides the numeric port value for CLI arguments (e.g.,--live-addr), whilehttpGet.portat lines 467-472 accepts bothstringandintegerto support Kubernetes named ports. This separation resolves the past critical issue where named ports broke operator startup.deploy/charts/external-secrets/tests/cert_controller_test.yaml (2)
231-231: LGTM! Readiness probe now correctly uses named port.The test validates that when
startupProbe.enabled: truewith defaultuseReadinessProbePort, the readiness probe uses the named portreadyinstead of a numeric port, aligning with Kubernetes best practices.
242-255: LGTM! Startup port override test validates named port configuration.The test correctly validates:
- A new container port entry with
name: startupatports[2]- The startupProbe using the named
startupport whenuseReadinessProbePort: falseThis ensures proper separation between readiness and startup probes when custom startup ports are configured.
deploy/charts/external-secrets/templates/deployment.yaml (3)
123-128: LGTM! CLI argument now correctly uses numeric port.The conditional logic properly handles the separation:
- When
httpGet.portis a string (e.g.,"live"), it usesspec.port(numeric) for the--live-addrCLI argument- When
httpGet.portis an integer, it uses that value directlyThis resolves the past critical issue where using a named port like
"metrics"for the CLI argument caused operator startup failures.
139-147: LGTM! Container port "live" correctly defined with conditional containerPort.The template properly:
- Only adds the port when
livenessProbe.enabledis true- Uses
spec.portwhenhttpGet.portis a string (named port configuration)- Uses
httpGet.portwhen it's numericThis ensures the container port definition matches the actual listening port regardless of how the probe is configured.
150-150: LGTM! Properly omits internal-only fields from probe spec.Adding
"port"to theomitlist alongside"address"ensures these chart-internal configuration fields don't leak into the Kubernetes livenessProbe spec, which would cause validation errors.deploy/charts/external-secrets/values.yaml (1)
345-346: LGTM! Dual-port configuration properly resolves the past critical issue.The configuration now correctly separates:
spec.port: 8082- Numeric port used for CLI argument--live-addrspec.httpGet.port: live- Named port used in Kubernetes probe specificationThis addresses the previously flagged critical issue where using a named port string for the CLI argument caused operator startup failures. The comment on line 359 clearly documents the expected values.
Also applies to: 359-361
deploy/charts/external-secrets/templates/cert-controller-deployment.yaml (2)
98-105: LGTM! Container ports properly defined with named ports.The template correctly:
- Defines a
readyport for the readiness probe- Conditionally defines a
startupport only whenstartupProbe.enabledanduseReadinessProbePort: falseThis avoids duplicate port definitions and aligns with Kubernetes best practices for named port references.
106-122: LGTM! Probes correctly reference named ports.Both
readinessProbeandstartupProbenow use named port references (readyorstartup) instead of numeric ports, satisfying the Popeye linter requirements and following Kubernetes best practices for probe configuration.deploy/charts/external-secrets/tests/controller_test.yaml (3)
103-124: LGTM! Comprehensive test for default livenessProbe configuration.The test validates:
- The complete livenessProbe spec including
httpGet.port: live(named port)- The container port entry with
containerPort: 8082andname: liveThis ensures the default configuration works correctly with the dual-port setup.
125-148: LGTM! Test validates spec.port override behavior.When
spec.port: 8888is set:
- The
httpGet.portremainslive(named port)- The
containerPortupdates to8888This confirms that
spec.portcontrols the listening port whilehttpGet.portcan remain a named reference.
149-181: LGTM! Test validates spec.httpGet.port override behavior.When
spec.httpGet.port: 8080(integer) is set:
- The
livenessProbe.httpGet.portbecomes8080(numeric)- The
containerPortalso becomes8080This validates the template correctly detects numeric ports and uses them for both the probe and container port.
Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
|
Renamed test per request from coderabbit. |
|
/ok-to-test sha=d583812525513e75a8a2f2703d1f2233d68b945d |
deploy/charts/external-secrets/templates/cert-controller-deployment.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@deploy/charts/external-secrets/values.schema.json`:
- Around line 484-486: The schema entry for livenessProbe.spec.port currently
restricts "port" to integer only; change the JSON Schema for the "port" property
(the same property validated for livenessProbe.spec.port) to accept both string
and integer (e.g., "type": ["string","integer"] or equivalent anyOf with {
"type": "string" } and { "type": "integer" }) so named ports are allowed and it
matches the existing httpGet.port schema.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
deploy/charts/external-secrets/tests/__snapshot__/cert_controller_test.yaml.snapis excluded by!**/*.snapdeploy/charts/external-secrets/tests/__snapshot__/webhook_test.yaml.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
deploy/charts/external-secrets/README.mddeploy/charts/external-secrets/values.schema.jsondeploy/charts/external-secrets/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/charts/external-secrets/README.md
- deploy/charts/external-secrets/values.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T19:14:48.246Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:48.246Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
deploy/charts/external-secrets/values.schema.json
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Are there any remaining blockers for this? |
|
- Add readinessProbe configuration mirroring livenessProbe pattern - Use named port 'live' for httpGet.port (aligned with PR external-secrets#5775) - Reuse 'live' named port for readinessProbe since both probes share the same health server endpoint (/healthz on port 8082) - Add spec.port for actual port number configuration - Update --live-addr to start health server when either probe is enabled - Handle kindOf check for string (named port) vs integer port values Fixes external-secrets#5776 Signed-off-by: AlexOQ <30403857+AlexOQ@users.noreply.github.com>
- Add readinessProbe configuration mirroring livenessProbe pattern - Use named port 'live' for httpGet.port (aligned with PR external-secrets#5775) - Reuse 'live' named port for readinessProbe since both probes share the same health server endpoint (/healthz on port 8082) - Add spec.port for actual port number configuration - Update --live-addr to start health server when either probe is enabled - Handle kindOf check for string (named port) vs integer port values - Add validation guard to fail if both probes enabled with mismatched ports Fixes external-secrets#5776 Signed-off-by: AlexOQ <30403857+AlexOQ@users.noreply.github.com>
- Add readinessProbe configuration mirroring livenessProbe pattern - Use named port 'live' for httpGet.port (aligned with PR external-secrets#5775) - Reuse 'live' named port for readinessProbe since both probes share the same health server endpoint (/healthz on port 8082) - Add spec.port for actual port number configuration - Update --live-addr to start health server when either probe is enabled - Handle kindOf check for string (named port) vs integer port values - Add validation guard to fail if both probes enabled with mismatched ports Fixes external-secrets#5776 Signed-off-by: AlexOQ <30403857+AlexOQ@users.noreply.github.com>
…s#5775) Co-authored-by: Gergely Bräutigam <skarlso777@gmail.com>
Problem Statement
popeyecli.io linter reported that this container used undefined ports for its probes and that they were listed by port number rather than using named ports.
Related Issue
Fixes #...
Proposed Changes
Ensure ports are defined and named following kubernetes best practices.
Checklist
git commit --signoffmake testmake reviewableDefine and use named ports for container probes across the Helm chart.