CORENET-7054: Use TLS profile to render CLI args for deployed components#3043
CORENET-7054: Use TLS profile to render CLI args for deployed components#3043tpantelis wants to merge 7 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tpantelis: This pull request references CORENET-7054 which is a valid jira issue. 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 |
|
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 (23)
🚧 Files skipped from review as they are similar to previous changes (18)
Summary by CodeRabbit
WalkthroughThis PR propagates TLS profile data into multiple network render paths, updates manifests and scripts to conditionally emit TLS min-version/cipher-suite arguments, normalizes TLS profile cipher handling, and expands rendering tests to verify the new output. ChangesTLS profile rendering updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 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@v0.8.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 17405 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 |
|
[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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindata/network/node-identity/managed/node-identity.yaml (1)
138-154: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing line continuation breaks the webhook exec command when TLS profile is enabled.
Line 148 (
--loglevel="${LOGLEVEL}") has no trailing\, but the TLS args block is appended right after it. When.UseTLSProfileis true, the rendered script becomes two separate shell statements — bash will try to execute--tls-min-version=...as a command name and fail (script runs withset -xe), crashing the webhook container. The self-hosted template avoids this by placing the TLS block before the unconditional last argument. Apply the same ordering here.🐛 Proposed fix
--extra-allowed-user="system:serviceaccount:openshift-ovn-kubernetes:ovn-kubernetes-control-plane" \ --pod-admission-conditions="/var/run/ovnkube-identity-config/additional-pod-admission-cond.json" \ - --loglevel="${LOGLEVEL}" {{- if .UseTLSProfile }} --tls-min-version={{.TLSMinVersion}} \ {{- if .TLSCipherSuites }} --tls-cipher-suites={{.TLSCipherSuites}} \ {{- end }} {{- end }} + --loglevel="${LOGLEVEL}"🤖 Prompt for 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. In `@bindata/network/node-identity/managed/node-identity.yaml` around lines 138 - 154, The exec command in the ovnkube-identity container template is broken when UseTLSProfile is enabled because the unconditional --loglevel argument ends the shell line before the TLS flags, so the inserted TLS block is treated as a separate command. Update the command in node-identity.yaml to keep the TLS arguments part of the same exec invocation by reordering them so the TLS block appears before the final unconditional argument, matching the self-hosted template and preserving the line continuations around the webhook exec command.
🧹 Nitpick comments (3)
pkg/network/ovn_kubernetes_test.go (1)
197-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: callback reassigns outer-scope
objs/errinstead of using local variables.Purely cosmetic since
objs/erraren't read again after this block, but shadowing with local variables inside the closure would be clearer.♻️ Optional cleanup
func(t *testing.T, tlsProfile bootstrap.TLSProfile) string { testBootstrap := *bootstrapResult testBootstrap.TLSProfile = tlsProfile - objs, _, err = renderOVNKubernetes(config, &testBootstrap, manifestDirOvn, fakeClient, featureGatesCNO) - g.Expect(err).NotTo(HaveOccurred()) + objs, _, err := renderOVNKubernetes(config, &testBootstrap, manifestDirOvn, fakeClient, featureGatesCNO) + g.Expect(err).NotTo(HaveOccurred())🤖 Prompt for 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. In `@pkg/network/ovn_kubernetes_test.go` around lines 197 - 215, The callback in testTLSArgRendering is reusing the outer-scope objs and err variables from renderOVNKubernetes, which makes the closure less clear. Update the closure to use local variables for the renderOVNKubernetes result and error handling, then continue using those locals when finding the ConfigMap and extracting ovnkube-lib.sh. Keep the change scoped to the test helper call site in ovn_kubernetes_test.go.pkg/network/node_identity_test.go (1)
103-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only validates substring presence, not shell-script validity.
findOvnkubeIdentityExecand the TLS assertions checkContainSubstringagainst the raw rendered text, so a missing line-continuation\(see the managednode-identity.yamlbug) wouldn't be caught by this suite. Consider a lightweight syntax check (e.g.,bash -non the extracted script, if feasible in unit tests) for the rendered command blocks to catch broken continuations.Also applies to: 160-165, 265-274
🤖 Prompt for 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. In `@pkg/network/node_identity_test.go` around lines 103 - 109, The current TLS rendering checks for the webhook command only verify substrings via findOvnkubeIdentityExec and related TLS assertions, so they can miss broken shell syntax such as a missing line-continuation. Update the tests around testTLSArgRendering and the other affected cases to validate the extracted script as shell, preferably by adding a lightweight syntax check on the rendered command block (for example via bash -n or an equivalent parsing step) in addition to the existing content assertions.bindata/kube-proxy/kube-proxy.yaml (1)
147-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTLS conditional rendering verified correct.
Traced all three branches (
UseTLSProfiletrue+ciphers, true+no-ciphers for TLS1.3, and false/fallback) — shell line continuations render correctly in each case, and fallback cipher list matches the prior hardcoded default.kube-rbac-proxysupports both--tls-min-versionand--tls-cipher-suitesflags as used here.One maintainability note: this exact 7-line conditional block is duplicated verbatim in
bindata/network/network-metrics/001-daemonset.yaml(and will likely recur in the other sibling renderers in this stack). Consider whether the render pipeline supports a shared template partial (Go templatedefine/templateaction) to avoid drift if the TLS flag logic changes later.🤖 Prompt for 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. In `@bindata/kube-proxy/kube-proxy.yaml` around lines 147 - 154, The TLS flag rendering block is duplicated in multiple kube-proxy-related templates, which risks drift if the logic changes. Refactor the repeated `UseTLSProfile` / `TLSMinVersion` / `TLSCipherSuites` conditional into a shared Go template partial using `define`/`template`, then reuse that partial from this renderer and the sibling manifests. Keep the rendered shell line continuations and fallback cipher list identical by centralizing the logic in one place.
🤖 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/network/bootstrap_test.go`:
- Around line 89-91: The failure message in the bootstrap test has a stale
expected TLS version, so update the t.Errorf call in the TLS assertion to report
the same expected value used in the if condition within bootstrap_test.go. Use
the existing result.TLSProfile.Spec.MinTLSVersion check as the source of truth
and make the logged expected version consistent with configv1.VersionTLS11 so
failures point to the correct value.
---
Outside diff comments:
In `@bindata/network/node-identity/managed/node-identity.yaml`:
- Around line 138-154: The exec command in the ovnkube-identity container
template is broken when UseTLSProfile is enabled because the unconditional
--loglevel argument ends the shell line before the TLS flags, so the inserted
TLS block is treated as a separate command. Update the command in
node-identity.yaml to keep the TLS arguments part of the same exec invocation by
reordering them so the TLS block appears before the final unconditional
argument, matching the self-hosted template and preserving the line
continuations around the webhook exec command.
---
Nitpick comments:
In `@bindata/kube-proxy/kube-proxy.yaml`:
- Around line 147-154: The TLS flag rendering block is duplicated in multiple
kube-proxy-related templates, which risks drift if the logic changes. Refactor
the repeated `UseTLSProfile` / `TLSMinVersion` / `TLSCipherSuites` conditional
into a shared Go template partial using `define`/`template`, then reuse that
partial from this renderer and the sibling manifests. Keep the rendered shell
line continuations and fallback cipher list identical by centralizing the logic
in one place.
In `@pkg/network/node_identity_test.go`:
- Around line 103-109: The current TLS rendering checks for the webhook command
only verify substrings via findOvnkubeIdentityExec and related TLS assertions,
so they can miss broken shell syntax such as a missing line-continuation. Update
the tests around testTLSArgRendering and the other affected cases to validate
the extracted script as shell, preferably by adding a lightweight syntax check
on the rendered command block (for example via bash -n or an equivalent parsing
step) in addition to the existing content assertions.
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 197-215: The callback in testTLSArgRendering is reusing the
outer-scope objs and err variables from renderOVNKubernetes, which makes the
closure less clear. Update the closure to use local variables for the
renderOVNKubernetes result and error handling, then continue using those locals
when finding the ConfigMap and extracting ovnkube-lib.sh. Keep the change scoped
to the test helper call site in ovn_kubernetes_test.go.
🪄 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: 0b82c24d-12c4-476b-91a0-23123dd70a38
📒 Files selected for processing (23)
bindata/kube-proxy/kube-proxy.yamlbindata/network/frr-k8s/frr-k8s.yamlbindata/network/frr-k8s/node-status-cleaner.yamlbindata/network/multus-admission-controller/admission-controller.yamlbindata/network/network-metrics/001-daemonset.yamlbindata/network/node-identity/managed/node-identity.yamlbindata/network/node-identity/self-hosted/node-identity.yamlbindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yamlpkg/network/bootstrap_test.gopkg/network/kube_proxy.gopkg/network/kube_proxy_test.gopkg/network/multus.gopkg/network/multus_admission_controller.gopkg/network/multus_admission_controller_test.gopkg/network/network_metrics_test.gopkg/network/node_identity.gopkg/network/node_identity_test.gopkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.gopkg/network/render.gopkg/network/render_test.gopkg/network/tls.go
This commit adds rendering of TLS CLI args based on the TLS profile to the webhook container command of the network-node-identity Deployment for both managed and self-hosted environments. Changes: - Add TLS profile configuration to renderNetworkNodeIdentity - Update yaml manifests to include TLS CLI arguments when configured Also added unit tests for the renderNetworkNodeIdentity function to verify correct rendering of the DaemonSet (self-hosted) and the Deployment (HyperShift) configurations, TLS argument handling based on adherence policies, and proper behavior when network node identity is disabled. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
This commit adds rendering of TLS CLI args based on the TLS profile to the multus-admission-controller Deployment. Changes: - Add --tls-min-version and --tls-cipher-suites flags to the webhook container for both HyperShift and non-HyperShift deployments - Add --tls-min-version and --tls-cipher-suites flags to kube-rbac-proxy with fallback to hardcoded ciphers when TLS profile is not honored - Call addTLSInfoToRenderData in renderMultusAdmissonControllerConfig to populate TLS template data - Add comprehensive unit tests for TLS argument rendering in both HyperShift and non-HyperShift modes Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Replace inline kube-rbac-proxy script code with the shared start-rbac-proxy-node() function from the ovnkube-script-lib ConfigMap. This reduces code duplication and ensures consistent behavior across all OVN Kubernetes components. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Add TLS profile support to the start-rbac-proxy-node() function in the ovnkube-script-lib ConfigMap. This function is used by kube-rbac-proxy containers in ovnkube-node and ovnkube-control-plane pods. Changes: - Add TLS template code to start-rbac-proxy-node() function in 008-script-lib.yaml - Add addTLSInfoToRenderData() call in renderOVNKubernetes() function - Add TLS test to TestRenderOVNKubernetes using testTLSArgRendering helper Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
- Update bindata/kube-proxy/kube-proxy.yaml to use TLS profile template variables (UseTLSProfile, TLSMinVersion, TLSCipherSuites) in the kube-rbac-proxy container's exec command - Add call to addTLSInfoToRenderData in renderStandaloneKubeProxy to populate TLS profile data in the render context - Add unit test in TestRenderKubeProxy to verify TLS CLI arguments are correctly rendered based on TLS profile adherence policy Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
- Update bindata/network/network-metrics/001-daemonset.yaml to use TLS profile template variables (UseTLSProfile, TLSMinVersion, TLSCipherSuites) in the kube-rbac-proxy container's args - Update renderNetworkMetricsDaemon to accept bootstrapResult parameter and add call to addTLSInfoToRenderData to populate TLS profile data - Update renderMultus to pass bootstrapResult to renderNetworkMetricsDaemon - Add unit test in TestRenderNetworkMetricsDaemon to verify TLS CLI arguments are correctly rendered based on TLS profile adherence policy Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
This commit adds rendering of TLS CLI args based on the TLS profile to the three FRR components that serve HTTPS endpoints: 1. controller container (port 9140) - Kubernetes controller for FRR 2. frr-metrics container (port 9141) - Prometheus metrics exporter 3. frr-k8s-statuscleaner container (port 9123) - Webhook for status cleanup Changes: - Add conditional template code to render --tls-min-version and --tls-cipher-suites CLI arguments when UseTLSProfile is true - Update renderAdditionalRoutingCapabilities to accept bootstrapResult parameter and call addTLSInfoToRenderData - Add comprehensive unit tests in Test_renderFRRRoutingCapabilities using the testTLSArgRendering helper function Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
force-pushed
the
render_tls_args
branch
from
274ce30 to
ac5e5ce
Compare
|
@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. |
2 participants
See commits for details