NETOBSERV-2515: Implement dynamic TLS configuration from OpenShift API Server - #2823
Merged
Merged
Conversation
Collaborator
|
@leandroberetta: This pull request references NETOBSERV-2515 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "netobserv-2.0" instead. 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. |
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedToo many files! This PR contains 302 files, which is 202 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (302)
You can disable this status message by setting the ✨ 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 |
leandroberetta
force-pushed
the
netobserv-2515
branch
from
July 7, 2026 14:05
f3949e0 to
40ad9b1
Compare
leandroberetta
force-pushed
the
netobserv-2515
branch
from
July 8, 2026 16:04
a04efb7 to
0aa05a5
Compare
leandroberetta
force-pushed
the
netobserv-2515
branch
from
July 15, 2026 13:02
291cace to
0975bd0
Compare
leandroberetta
marked this pull request as ready for review
July 15, 2026 13:31
jpinsonneau
reviewed
Jul 15, 2026
| ClusterInfo *cluster.Info | ||
| Loki *helper.LokiConfig | ||
| Vendor constants.Vendor | ||
| IsDownstream bool |
Member
There was a problem hiding this comment.
it probably reappeared after a rebasing conflict, @leandroberetta ? I removed it in a PR, which was merged yesterday
| module github.com/netobserv/netobserv-operator | ||
|
|
||
| go 1.25.7 | ||
| go 1.26 |
Member
There was a problem hiding this comment.
any reason bumping that in that PR ? It doesn't seems related 🤔
Member
There was a problem hiding this comment.
that's probably required by openshift/api
Anyway, there are other PR opened that do the same, we'll have to go there
| OnProfileChange: func(_ context.Context, oldSpec, newSpec configv1.TLSProfileSpec) { | ||
| setupLog.Info("TLS profile has changed, initiating graceful shutdown to reload", | ||
| "oldProfile", oldSpec, "newProfile", newSpec) | ||
| os.Exit(0) |
Member
There was a problem hiding this comment.
We should cancel the manager context instead of calling Exit(0) here.
Something like:
ctx, stop := context.WithCancel(ctrl.SetupSignalHandler())
defer stop()
in main function
and then call stop from OnProfileChange / OnAdherencePolicyChange funcs
WDYT ?
| } | ||
|
|
||
| // Return the TLS security profile (may be nil if not set) | ||
| return apiServer.Spec.TLSSecurityProfile, nil |
Member
There was a problem hiding this comment.
So we capture only TLSSecurityProfile and not TLSAdherence ?
Member
Author
There was a problem hiding this comment.
Good point! TLSAdherence controls whether components should strictly follow the configured profile. For now we only read TLSSecurityProfile and apply it when set, which is ok to me but we can add this logic. For OpenShift as it is would be ok, not sure for upstream.
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
jotak
reviewed
Jul 16, 2026
| // On OpenShift with no explicit TLS profile, OCP uses the Intermediate default; we still | ||
| // set up the watcher so that a future explicit profile change is detected and triggers a restart. | ||
| func setupTLSProfileWatcher(mgr *manager.Manager) error { | ||
| if !mgr.ClusterInfo.IsOpenShift() { |
Member
There was a problem hiding this comment.
here and in other places where you check IsOpenShift : what about older versions of openshift? I'm not sure TLS profiles existed back in 4.12, unless it was backported...
This feature started to be mandatory in 4.22, so maybe we can check based on version. Maybe add a UseTLSProfile() bool func in clusterInfo, that returns true if on OpenShift >= 4.22 ?
Member
There was a problem hiding this comment.
hmm I see you added UseTLSProfile() but it's called only here in the watcher, and not in other places where we fetch the tls profile; I don't think it makes sense, because on older openshift, TLS Profile would be fetched once and then unwatched. IMO we should either use it everywhere, or rollback removing UseTLSProfile().
I guess the reason why you don't call it from cluster.NewInfo() is because, at this stage, the openshift version is still unknown, so it's not gonna work (unless doing a bigger refactoring). I'm fine to roll back to your initial code without UseTLSProfile. We must just make sure this code works on older openshift that don't have the TLSProfiles / APIServer APIs.
Member
Author
There was a problem hiding this comment.
Ok, I rollbacked the change. For what I see, the API change was way before 4.12 so we should be good.
leandroberetta
force-pushed
the
netobserv-2515
branch
from
July 16, 2026 14:18
02574e2 to
0cb3156
Compare
leandroberetta
force-pushed
the
netobserv-2515
branch
from
July 16, 2026 14:30
0cb3156 to
8dfbd13
Compare
leandroberetta
force-pushed
the
netobserv-2515
branch
2 times, most recently
from
August 31, 2026 12:19
d7d2b70 to
fbc000b
Compare
Member
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
leandroberetta
force-pushed
the
netobserv-2515
branch
from
August 31, 2026 16:48
fbc000b to
8b11e67
Compare
github-actions
Bot
removed
the
ok-to-test
Member
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
@leandroberetta: The following test 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. |
|
New images: quay.io/netobserv/network-observability-operator:8b11e674
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-8b11e674
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-8b11e674They will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:8b11e674 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-8b11e674Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-8b11e674
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
Member
|
/label qe-approved netobserv-controller-manager goes into crashloopback back and takes time in reconcile but it is at cluster level reconciliation and it takes time to stabilized so it is expected. |
Collaborator
|
@leandroberetta: This pull request references NETOBSERV-2515 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 epic to target either version "5.1.0." or "openshift-5.1.0.", but it targets "netobserv-2.0" instead. 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. |
Member
|
/lgtm |
leandroberetta
force-pushed
the
netobserv-2515
branch
from
September 2, 2026 12:41
8b11e67 to
1dddfb6
Compare
github-actions
Bot
removed
the
ok-to-test
Member
|
/lgtm |
Member
Author
|
/approve |
1 similar comment
Member
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak, leandroberetta The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
5 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.
OpenShift lets cluster admins configure a cluster-wide
TLSSecurityProfileon theAPIServerresource (Old/Intermediate/Modern/Custom). NetObserv Operator watches this setting and propagates the resulting TLS minimum version and cipher suites to all the components it manages: flowlogs-pipeline (FLP), the eBPF agent, and the console plugin(s) (netobserv-plugin/netobserv-plugin-static). When the profile changes, the operator gracefully restarts itself, which triggers a full reconcile of theFlowCollectorand rolls out the new settings to every managed workload.Test Case 0 — No profile configured behaves like Intermediate
Objective: confirm that when
spec.tlsSecurityProfileis absent, the operator behaves as ifIntermediatewere explicitly set — both for the TLS config it actually applies and for the baseline it uses to detect future changes. This matters because it explains an easy false negative: patching the profile toIntermediateas a first step produces no observable change (nothing actually changed from the operator's point of view).Steps
TLS_MIN_VERSIONon any managed pod:Intermediate:Expected Results
OpenShift detected but no TLS profile configured in APIServer, using secure defaults.TLS_MIN_VERSION=771(TLS 1.2, same asIntermediate).Intermediateprofile are treated as identical (no spurious restarts on a no-op change).Test Case 1 — TLS profile propagation on OpenShift
Objective: confirm that changing the cluster TLS profile cascades automatically to all NetObserv-managed components.
Steps
Modern(TLS 1.3 only — chosen because it's guaranteed to differ from the implicit default, so the change is actually observable):Expected Results
oc get pod -n netobserv -l app=netobserv-operatorshows a new pod name / fresh age).AGEclose to the moment the profile was changed (a few seconds apart is normal, since each Deployment/DaemonSet rolls independently once the operator updates its spec).Test Case 3 — Custom TLS profile
Objective: confirm the
Customprofile type (explicitminTLSVersion+cipherslist) is correctly parsed and propagated. This is implemented and unit-tested ininternal/pkg/tlsconfig/config.go(TestComposeTLSConfig_CustomProfile,TestConfigToEnvVars_CustomProfile), including the edge case of a malformed/emptycustomblock.Steps
groupslist):Expected Results
TLS_MIN_VERSION=771(VersionTLS12, per the reference table).TLS_CIPHER_SUITEScontains exactly these 4 decimal IDs (order may vary):52393,52392,49195,49199— corresponding to the 4 OpenSSL cipher names requested, translated to Go's numeric IDs.TLS_CURVE_PREFERENCES=24,29(in that order:secp384r1=24, thenX25519=29Negative sub-case: malformed Custom profile
(
type: Customwith nocustomblock.)Expected: operator logs an error (
custom TLS profile specified but Custom field is nil) and does not crash or leave managed components without a TLS config — it falls back gracefully rather than propagating a broken config. Confirm the operator pod staysRunning(no CrashLoopBackOff) and existing managed pods are not disrupted.Dependencies
netobserv/flowlogs-pipeline#1297
netobserv/netobserv-ebpf-agent#1015
netobserv/netobserv-web-console#1610
Will be addressed in a follow-up:
netobserv/netobserv-cli#552
Checklist