CORENET-7356: Add TLS profile support for network-check-source metrics#3050
CORENET-7356: Add TLS profile support for network-check-source metrics#3050tpantelis wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe check-endpoints CLI now uses controller-runtime for command setup, TLS-protected metrics serving, cancellation, client and informer construction, controller startup, and TLS profile change handling. Its metrics are registered with the controller-runtime registry. ChangesCheck Endpoints Runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CobraCommand
participant CheckEndpointsRuntime
participant TLSProfileAPI
participant MetricsServer
participant Controllers
CobraCommand->>CheckEndpointsRuntime: execute with listen address and pod identity
CheckEndpointsRuntime->>TLSProfileAPI: fetch cluster TLS profile
TLSProfileAPI-->>CheckEndpointsRuntime: return profile
CheckEndpointsRuntime->>MetricsServer: start TLS-protected metrics server
CheckEndpointsRuntime->>Controllers: build clients and start controllers
TLSProfileAPI-->>CheckEndpointsRuntime: report profile or adherence change
CheckEndpointsRuntime->>MetricsServer: cancel shared context
CheckEndpointsRuntime->>Controllers: stop controllers
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17377 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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 |
tpantelis
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@tpantelis: This pull request references CORENET-7356 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @danwinship |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/checkendpoints/cmd.go`:
- Line 169: Update the setup around controller.RegisterMetrics() so its
collectors are registered with controller-runtime’s metrics.Registry, or
explicitly bridge the legacy registry into that registry before serving metrics.
Preserve the existing collector registration behavior while ensuring the
collectors appear on the controller-runtime /metrics endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b60db286-01ee-41d5-9bad-36733b28ea2d
📒 Files selected for processing (1)
pkg/cmd/checkendpoints/cmd.go
a3f09cf to
0aed9d8
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
0aed9d8 to
fcdb6b9
Compare
Use the OCP API server TLS profile to configure TLS options for the metrics server and exit the process on TLS profile changes. library-go's controllercmd framework does not have support for TLS profiles and does not easily allow TLS options to be customized. Therefore the code was migrated to use the controller-runtime Manager API which allows direct customization. The code respects the TLS profile adherence setting and uses the library-go strong defaults when adherence is optional so as not to fall back to weaker Go default ciphers. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
fcdb6b9 to
f05b3c5
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tpantelis The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/pipeline required I don't really understand all of this, but it should fail pretty obviously if something is wrong |
|
Scheduling required tests: Scheduling tests matching the |
|
/assign |
We'll see how many succeed. But the failures I've looked at (with claude's help) are infrastructure-related or caused by other issues not related to this PR. Also this PR changes the |
|
@tpantelis: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
It might be better to just have CNO deploy the pods with |
yeah that's a good idea. We'll still need the |
|
sounds good |
3 participants
Use the OCP API server TLS profile to configure TLS options for the metrics server and exit the process on TLS profile changes.
library-go's controllercmdframework does not have support for TLS profiles and does not easily allow TLS options to be customized. Therefore the code was migrated to use the controller-runtimeManagerAPI which allows direct customization.The code respects the TLS profile adherence setting and uses the
library-gostrong defaults when adherence is optional so as not to fall back to weaker Go default ciphers.