CORENET-7054: Use TLS profile to render CLI args for deployed components#3043
Open
tpantelis wants to merge 12 commits into
Open
CORENET-7054: Use TLS profile to render CLI args for deployed components#3043tpantelis wants to merge 12 commits into
tpantelis wants to merge 12 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@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. |
Contributor
Author
|
/cc @danwinship |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR propagates TLS profile data through network render paths, conditionally emits TLS arguments in generated manifests and scripts, normalizes cipher handling, and adds rendering tests for kube-proxy, Multus, network metrics, node identity, OVN-Kubernetes, and FRR components. ChangesTLS profile rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BootstrapResult
participant NetworkRenderers
participant TLSProfileData
participant GeneratedManifests
BootstrapResult->>NetworkRenderers: provide TLS profile
NetworkRenderers->>TLSProfileData: populate render data
TLSProfileData->>GeneratedManifests: render TLS flags
GeneratedManifests->>GeneratedManifests: start component with TLS settings
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 20 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
tpantelis
force-pushed
the
render_tls_args
branch
from
July 6, 2026 14:08
274ce30 to
ac5e5ce
Compare
Contributor
Author
|
/test frrk8s-e2e |
Contributor
|
[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 |
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/network/ovn_kubernetes_test.go`:
- Around line 224-226: In the testBootstrap setup, update the
OVNKubernetesConfig pointer before assigning HyperShiftConfig so the closure
does not mutate the shared object from bootstrapResult. Create a local copy of
the pointed-to OVN configuration, reassign testBootstrap.OVN.OVNKubernetesConfig
to that copy, then apply the existing HyperShiftConfig assignment.
🪄 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: a7cab110-67e0-4769-a616-ae93be9ff2c5
📒 Files selected for processing (2)
bindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yamlpkg/network/ovn_kubernetes_test.go
tpantelis
force-pushed
the
render_tls_args
branch
from
July 15, 2026 15:41
d8dcaea to
560c19a
Compare
danwinship
reviewed
Jul 15, 2026
| } | ||
|
|
||
| // TLS 1.3 cipher suites are not configurable in Go - all TLS 1.3 ciphers are always enabled. | ||
| // Clear the cipher list for TLS 1.3 as some components are strict and fail if ciphers are provided with min version 1.3. |
Contributor
There was a problem hiding this comment.
The changes here (and in bootstrap_test.go) aren't really specific to this commit ("Use TLS profile to render CLI args in the node identity webhook") right? It would be better to do them as an separate commit.
Contributor
Author
There was a problem hiding this comment.
They are specific - node identity et al require IANA names.
Contributor
There was a problem hiding this comment.
Right, it's the "et al" that I'm talking about; these changes aren't specific to node identity, they're a prereq for the entire branch. Right? Like: if we remove the node-identity feature from CNO/OCP in the future, we would still want to keep these changes to toTLSProfile() and TestBootstrap(), right? So given that, it would be clearer to split these changes (and the testutil_test.go stuff) into a separate setup commit before any of the component-specific changes.
Contributor
Author
| - --tls-cipher-suites={{.TLSCipherSuites}} | ||
| {{- end }} | ||
| {{- else }} | ||
| - --tls-cipher-suites=TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 |
Contributor
There was a problem hiding this comment.
Hm... These old defaults seem like something we're probably going to need to revisit later on?
Contributor
Author
There was a problem hiding this comment.
I'm not sure. They're not weak ciphers as they've passed prior TLS scanning. They're needed for legacy adherence now but, at some point, I imagine adherence will be changed to strict by default and at least the Intermediate profile will be the default.
| }) | ||
| } | ||
|
|
||
| func mustFindRenderedObj[T any](t *testing.T, objs []*uns.Unstructured, kind, name string, objType T) T { |
Contributor
There was a problem hiding this comment.
This shouldn't be in node_identity_test.go if it's going to be used by tests in other files later.
You can make a file like helpers_test.go or utils_test.go with helper functions that are only used by unit tests.
Contributor
Author
There was a problem hiding this comment.
Moved to existing testutil_test.go.
| t.Run("should successfully render the network-node-identity DaemonSet", func(t *testing.T) { | ||
| networkConfig, bootstrapResult, client := setupTest(t) | ||
| daemonSet := mustFindRenderedObj(t, assertRenderSuccess(t, networkConfig, bootstrapResult, client), | ||
| "DaemonSet", "network-node-identity", &appsv1.DaemonSet{}) |
Contributor
There was a problem hiding this comment.
This is a weird mix of generic and non-generic calling conventions. You should be able to get rid of the objType arg and invoke this as
daemonSet := mustFindRenderedObj[*appsv1.DaemonSet](t, assertRenderSuccess(t, networkConfig, bootstrapResult, client),
"DaemonSet", "network-node-identity")
or failing that, get rid of the genericness and have it invoked as
var daemonSet appsv1.DaemonSet
mustFindRenderedObj(t, assertRenderSuccess(t, networkConfig, bootstrapResult, client),
"DaemonSet", "network-node-identity", &daemonSet)
| return mustFindRenderedObj(t, objs, "Deployment", "multus-admission-controller", &appsv1.Deployment{}) | ||
| } | ||
|
|
||
| func findMultusWebhookExec(t *testing.T, objs []*unstructured.Unstructured) string { |
Contributor
There was a problem hiding this comment.
There are a few variants of this function and maybe part of it could be helper-ized?
Contributor
Author
There was a problem hiding this comment.
Refactored common code to findExecCommand function.
| --tls-private-key-file=${TLS_PK} \ | ||
| --tls-cert-file=${TLS_CERT} | ||
| . /ovnkube-lib/ovnkube-lib.sh || exit 1 | ||
| start-rbac-proxy-node ovn-control-plane-metrics 9108 29108 /etc/pki/tls/metrics-cert/tls.key /etc/pki/tls/metrics-cert/tls.crt |
Contributor
There was a problem hiding this comment.
the "-node" in "start-rbac-proxy-node" means "for ovnkube-node", so you should remove that from the name if it's being used more generically. (There are also a couple of comments in the function that refer specifically to ovnkube-node things that need to be made more generic.) (Also don't forget to fix the name in this commit message and the next one.)
Contributor
Author
There was a problem hiding this comment.
Renamed to start-rbac-proxy to reflect its generic usage.
tpantelis
force-pushed
the
render_tls_args
branch
from
July 15, 2026 18:29
560c19a to
bbce4ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node_identity_test.go`:
- Line 36: Remove the outer-scope NewWithT(t) instances in
pkg/network/node_identity_test.go at lines 36-36 and 175-175; create local
Gomega objects inside assertRenderSuccess and every affected t.Run block,
including the inner t.Run blocks at lines 175-175, so assertions use each
subtest’s testing context.
In `@pkg/network/tls.go`:
- Around line 72-83: Update the cipher normalization loop in the TLS profile
handling to avoid mutating the potentially shared profileSpec.Ciphers backing
array: construct a new cipher slice and reassign it after processing. Preserve
valid IANA names, replace each OpenSSL name with every result from
crypto.OpenSSLToIANACipherSuites, and retain the original value when no mapping
exists, including one-to-many mappings.
🪄 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: 57a04c1a-5c8b-4508-87ce-73c46b3fff5e
📒 Files selected for processing (27)
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/managed/ovnkube-control-plane.yamlbindata/network/ovn-kubernetes/managed/ovnkube-node.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-node.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/testutil_test.gopkg/network/tls.go
🚧 Files skipped from review as they are similar to previous changes (17)
- bindata/network/node-identity/managed/node-identity.yaml
- bindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yaml
- bindata/network/multus-admission-controller/admission-controller.yaml
- pkg/network/kube_proxy_test.go
- bindata/kube-proxy/kube-proxy.yaml
- bindata/network/node-identity/self-hosted/node-identity.yaml
- pkg/network/render.go
- bindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yaml
- pkg/network/bootstrap_test.go
- pkg/network/multus_admission_controller.go
- pkg/network/ovn_kubernetes.go
- pkg/network/network_metrics_test.go
- pkg/network/multus_admission_controller_test.go
- pkg/network/kube_proxy.go
- pkg/network/multus.go
- pkg/network/render_test.go
- pkg/network/ovn_kubernetes_test.go
tpantelis
force-pushed
the
render_tls_args
branch
from
July 15, 2026 23:37
bbce4ff to
ba9cef8
Compare
tpantelis
force-pushed
the
render_tls_args
branch
from
July 17, 2026 12:23
ba9cef8 to
0bc74f5
Compare
This commit enhances the TLS profile handling to support different cipher name formats and TLS version requirements: 1. TLS 1.3 cipher suite handling: - Clear cipher list when MinTLSVersion is TLS 1.3 - TLS 1.3 cipher suites are not configurable in Go (all are enabled) - Some components fail if ciphers are provided with TLS 1.3 2. Cipher name conversion: - Convert OpenSSL cipher names to IANA names - OCP pre-defined profiles use OpenSSL names - Components accepting cipher suite args expect IANA names - Support both formats in custom user profiles Also updated tests to verify the cipher name conversion logic handles both IANA and OpenSSL formatted cipher names correctly. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
This commit adds helper functions to testutil_test.go that are used by component rendering tests to verify rendered Kubernetes objects: - mustFindRenderedObj: Finds and converts unstructured objects using generics - mustFindContainer: Finds a container by name in a container list - findExecCommand: Extracts exec command strings from container command args These utilities facilitate testing of rendered manifests and container configurations across multiple components. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
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() function (renamed from start-rbac-proxy-node to reflect its generic usage) 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() 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() 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>
In HyperShift deployments, the ovnkube-control-plane serves metrics on port 9108 using TLS. This commit adds --tls-min-version and --tls-cipher-suites CLI arguments to the ovnkube command to ensure it respects the cluster's TLS security profile. The managed/ovnkube-control-plane.yaml template now conditionally renders these TLS arguments based on the UseTLSProfile flag, similar to how kube-rbac-proxy is configured in non-HyperShift deployments. Added unit test coverage to TestRenderOVNKubernetes to verify the rendering. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
force-pushed
the
render_tls_args
branch
from
July 17, 2026 12:24
0bc74f5 to
6ad012f
Compare
Render TLS CLI arguments (--tls-min-version and --tls-cipher-suites) for the network-check-source deployment based on the cluster's TLS security profile. This follows the same pattern as other components in the CNO. Updates renderNetworkDiagnostics to accept bootstrapResult and adds TLS data to the render context. Includes unit tests to verify the TLS args are correctly rendered in the deployment manifest. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
force-pushed
the
render_tls_args
branch
from
July 17, 2026 17:56
cbf9d4e to
390ed4a
Compare
Migrate the check-endpoints command from library-go's controllercmd framework to controller-runtime to support TLS configuration for the metrics server. This change: - Adds --tls-min-version and --tls-cipher-suites CLI flags - Uses cliflag parsers (TLSVersion, TLSCipherSuites) for validation - Applies CLI TLS settings first, then crypto.SecureTLSConfig() for defaults - Migrates metrics registration from legacyregistry to controller-runtime registry - Adds serviceability profiling support (BehaviorOnPanic, Profile, StartProfiler) The controller-runtime migration enables direct TLS customization via metricsserver.Options, which was not possible with library-go's controllercmd framework. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
force-pushed
the
render_tls_args
branch
from
July 17, 2026 18:19
390ed4a to
7451017
Compare
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See commits for details