NETOBSERV-2977: Add TLS support for collector when OpenShift by leandroberetta · Pull Request #552 · netobserv/netobserv-cli

leandroberetta · 2026-08-11T22:18:19Z

Description

Enable TLS for the collector↔agent gRPC connection when running on OpenShift, and make that connection honor the cluster's TLS security profile.

TLS enablement (service-ca)

Enable TLS for the gRPC collector server on OpenShift, using service-ca for automatic cert generation
Conditionally detect OpenShift and annotate the collector Service for cert generation, create a CA ConfigMap with inject-cabundle, and mount certs in the collector pod
Add CA volume and tls.caCertPath to the agent DaemonSet FLP config so agents verify the collector's certificate
On non-OpenShift clusters, everything works without TLS as before

Honor the cluster TLS security profile

Both ends previously hardcoded MinVersion: TLS 1.3. The connection now derives min version / cipher suites / curves from the OpenShift tlsSecurityProfile (apiservers.config.openshift.io/cluster), like every other netobserv component — no hardcoded version
A new resolve-tls subcommand runs as an initContainer on the collector pod: it reads the profile, resolves it, and writes the collector-tls-config ConfigMap. Both the collector container and the agent DaemonSet consume it via envFrom, so a single resolver run drives both ends
When the cluster has no explicit profile, it falls back to the Intermediate preset, mirroring the operator's default (no invented default)
Install order flips on the TLS path (collector first, then agents) so the ConfigMap exists before agents start; agents reference it with optional: false to fail loudly rather than silently downgrade
OpenShift-only, flows/packets only; metrics, --yaml, and non-OCP runs are unchanged

Dependencies

n/a

Checklist

Does the changes in PR need specific configuration or environment set up for testing?
- if so please describe it in PR description.
I have added thorough unit tests for the change.
QE requirements (check 1 from the list):
- Standard QE validation, with pre-merge tests unless stated otherwise.
- Regression tests only (e.g. refactoring with no user-facing change).
- No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

openshift-ci-robot · 2026-08-11T22:18:23Z

openshift-ci · 2026-08-11T22:18:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign memodi for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

DOWNSTREAM_OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jpinsonneau · 2026-08-25T14:35:53Z

 	sigs.k8s.io/yaml v1.6.0 // indirect
 )
+
+replace github.com/netobserv/flowlogs-pipeline => github.com/leandroberetta/flowlogs-pipeline v0.0.0-20260810170916-6c5c94ab0294


Don't forget to remove this :)

jpinsonneau · 2026-08-25T14:39:54Z

that deserve at least a unit test

jpinsonneau · 2026-08-25T14:41:31Z

+  if [[ "$tlsEnabled" == "true" ]]; then
+    cmd="${K8S_CLI_BIN} run -n $namespace collector \\
+    --image=$img --image-pull-policy='Always' --restart='Never' \\
+    --override-type=strategic \\
+    --overrides=$overrides \\
+    --command -- $runCommand"
+  else
+    cmd="${K8S_CLI_BIN} run -n $namespace collector \\
+    --image=$img --image-pull-policy='Always' --restart='Never' \\
+    --overrides=$overrides \\
+    --command -- $runCommand"
+  fi


Suggested change

if [[ "$tlsEnabled" == "true" ]]; then

cmd="${K8S_CLI_BIN} run -n $namespace collector \\

--image=$img --image-pull-policy='Always' --restart='Never' \\

--override-type=strategic \\

--overrides=$overrides \\

--command -- $runCommand"

else

cmd="${K8S_CLI_BIN} run -n $namespace collector \\

--image=$img --image-pull-policy='Always' --restart='Never' \\

--overrides=$overrides \\

--command -- $runCommand"

fi

overrideType=""

if [[ "$tlsEnabled" == "true" ]]; then

overrideType="--override-type=strategic"

fi

cmd="${K8S_CLI_BIN} run -n $namespace collector \

--image=$img --image-pull-policy='Always' --restart='Never' \

$overrideType --overrides=$overrides \

--command -- $runCommand"

jpinsonneau · 2026-08-25T14:43:55Z

We should simplify this to something like:

# Create collector service for flows/packets captures if [[ "$command" = "flows" || "$command" = "packets" ]]; then echo "creating collector service" applyYAML "$collectorServiceYAML" if [[ "$tlsEnabled" == "true" ]]; then echo "creating CA configmap for TLS" createCAConfigMap fi fi if [ "$command" = "flows" ]; then echo "creating flow-capture agents" elif [ "$command" = "packets" ]; then echo "creating packet-capture agents" elif [ "$command" = "metrics" ]; then echo "creating service monitor" applyYAML "$smYAML" echo "creating metric-capture agents:"

jpinsonneau · 2026-08-25T14:46:24Z

+function isOpenShift() {
+  ${K8S_CLI_BIN} get clusterversion version &>/dev/null
+}


You should rely on checkClusterVersion here instead.

Feel free to add a global variable like isOCP in it for your usage 😉

jpinsonneau · 2026-08-25T14:48:12Z

 	golang.org/x/tools v0.45.0 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect
-	google.golang.org/grpc v1.81.1 // indirect
+	google.golang.org/grpc v1.82.0


nit: Is that needed here ?

It's a direct dependency now: collector_tls.go uses grpc.Creds(credentials.NewTLS(...)) to enable TLS on the collector, so grpc is imported directly rather than transitively.

leandroberetta · 2026-09-01T13:50:37Z

@jpinsonneau I addressed the feedback, the only missing one is the dependency update, I'm waiting to merge this: netobserv/flowlogs-pipeline#1297

openshift-ci-robot · 2026-09-02T13:46:14Z

@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.

Details

In response to this:

Description

Enable TLS for the collector↔agent gRPC connection when running on OpenShift, and make that connection honor the cluster's TLS security profile.

TLS enablement (service-ca)

Enable TLS for the gRPC collector server on OpenShift, using service-ca for automatic cert generation

Conditionally detect OpenShift and annotate the collector Service for cert generation, create a CA ConfigMap with inject-cabundle, and mount certs in the collector pod

Add CA volume and tls.caCertPath to the agent DaemonSet FLP config so agents verify the collector's certificate

On non-OpenShift clusters, everything works without TLS as before

Honor the cluster TLS security profile

Both ends previously hardcoded MinVersion: TLS 1.3. The connection now derives min version / cipher suites / curves from the OpenShift tlsSecurityProfile (apiservers.config.openshift.io/cluster), like every other netobserv component — no hardcoded version

A new resolve-tls subcommand runs as an initContainer on the collector pod: it reads the profile, resolves it, and writes the collector-tls-config ConfigMap. Both the collector container and the agent DaemonSet consume it via envFrom, so a single resolver run drives both ends

When the cluster has no explicit profile, it falls back to the Intermediate preset, mirroring the operator's default (no invented default)

Install order flips on the TLS path (collector first, then agents) so the ConfigMap exists before agents start; agents reference it with optional: false to fail loudly rather than silently downgrade

OpenShift-only, flows/packets only; metrics, --yaml, and non-OCP runs are unchanged

Dependencies

n/a

Checklist

Does the changes in PR need specific configuration or environment set up for testing?

if so please describe it in PR description.

I have added thorough unit tests for the change.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

openshift-ci-robot · 2026-09-02T13:55:01Z

@leandroberetta: This pull request references NETOBSERV-2977 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 story to target the "5.1.0" version, but no target version was set.

Details

In response to this:

Description

Enable TLS for the collector↔agent gRPC connection when running on OpenShift, and make that connection honor the cluster's TLS security profile.

TLS enablement (service-ca)

Enable TLS for the gRPC collector server on OpenShift, using service-ca for automatic cert generation

Conditionally detect OpenShift and annotate the collector Service for cert generation, create a CA ConfigMap with inject-cabundle, and mount certs in the collector pod

Add CA volume and tls.caCertPath to the agent DaemonSet FLP config so agents verify the collector's certificate

On non-OpenShift clusters, everything works without TLS as before

Honor the cluster TLS security profile

Both ends previously hardcoded MinVersion: TLS 1.3. The connection now derives min version / cipher suites / curves from the OpenShift tlsSecurityProfile (apiservers.config.openshift.io/cluster), like every other netobserv component — no hardcoded version

A new resolve-tls subcommand runs as an initContainer on the collector pod: it reads the profile, resolves it, and writes the collector-tls-config ConfigMap. Both the collector container and the agent DaemonSet consume it via envFrom, so a single resolver run drives both ends

When the cluster has no explicit profile, it falls back to the Intermediate preset, mirroring the operator's default (no invented default)

Install order flips on the TLS path (collector first, then agents) so the ConfigMap exists before agents start; agents reference it with optional: false to fail loudly rather than silently downgrade

OpenShift-only, flows/packets only; metrics, --yaml, and non-OCP runs are unchanged

Dependencies

n/a

Checklist

Does the changes in PR need specific configuration or environment set up for testing?

if so please describe it in PR description.

I have added thorough unit tests for the change.

QE requirements (check 1 from the list):

Standard QE validation, with pre-merge tests unless stated otherwise.

Regression tests only (e.g. refactoring with no user-facing change).

No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

The gRPC connection between the capture agent (FLP client) and the CLI collector (gRPC server) hardcoded MinVersion: TLS 1.3 on both ends. Every other netobserv component derives its TLS settings from the OpenShift tlsSecurityProfile (apiservers.config.openshift.io/cluster); the CLI now does the same instead of pinning a version. A new `resolve-tls` subcommand runs as an initContainer on the collector pod. It reads the cluster's tlsSecurityProfile, resolves it to concrete min version / cipher suites / curves (falling back to the Intermediate preset when no profile is set, mirroring the operator's default), and writes them into the `collector-tls-config` ConfigMap. Both the collector container and the agent DaemonSet consume that ConfigMap via envFrom, so a single resolver run drives both ends. The collector server applies the resolved settings through flowlogs-pipeline's tlsprofile.Apply. Because the ConfigMap only exists once the collector's init completes, the install order flips on the TLS path: the collector is created and waited on first, then the agents (which reference the ConfigMap with optional:false, failing loudly rather than silently downgrading TLS). This path stays OpenShift-only and applies to flows/packets captures; metrics, --yaml output and non-OCP runs are unchanged. - internal/pkg/tlsresolver: profile resolution + ConfigMap write, with tests - cmd/resolve_tls.go, cmd/root.go: new resolve-tls subcommand - cmd/collector_tls.go: drop hardcoded TLS 1.3, apply resolved profile - res/service-account.yml: RBAC to read apiservers/cluster and write the CM - commands/netobserv, scripts/functions.sh: initContainer, envFrom, ordering - go.mod: add openshift/api and openshift/library-go/pkg/crypto Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The TLS profile resolver maps the OpenShift SecP256r1MLKEM768 / SecP384r1MLKEM1024 groups to their crypto/tls.CurveID constants, which are Go 1.26+. CI (setup-go 1.26), the Dockerfile builder (golang:1.26) and the operator (go 1.26.3) are already on 1.26; only the go.mod directive lagged at 1.25.7, which made govet's stdversion reject those constants and the exhaustive linter reject dropping them. Bump the directive to 1.26.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

agentManifest is set in functions.sh setup() but consumed in commands/netobserv (applied after the collector is ready). shellcheck analyzes each file in isolation, so it flags the assignment as unused; add the same disable directive the file already uses elsewhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Moving the resolve-tls initContainer's namespaced Role and RoleBinding to the end of res/service-account.yml keeps the existing service-account document indices stable, but still adds two documents to every generated capture manifest. Update the positional assertions in the flow, packet and metric YAML e2e tests accordingly: - bump the expected document counts (8 -> 10 for flows/packets, 12 -> 14 for metrics), - assert the new Role (configmaps get/create/update) at index [6] and RoleBinding (ServiceAccount -> Role netobserv-cli) at index [7], - assert the new config.openshift.io/apiservers get rule on the netobserv-cli ClusterRole, - shift the collector Service / DaemonSet and the metric-specific documents to their new indices. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

go.mod requires go >= 1.26.0 (the TLS profile resolver uses the crypto/tls SecP256r1MLKEM768 / SecP384r1MLKEM1024 CurveID constants, added in Go 1.26), but the build root was still rhel-9-release-golang-1.25-openshift-4.21, which ships Go 1.25.12 with GOTOOLCHAIN=local. The netobserv-cli-tests step therefore failed immediately on `go list -m github.com/onsi/ginkgo/v2` with "go.mod requires go >= 1.26.0 (running go 1.25.12; GOTOOLCHAIN=local)". Move to rhel-9-release-golang-1.26-openshift-5.0, the build root already used by network-observability-operator, flowlogs-pipeline and netobserv-ebpf-agent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

StartCommand called pty.Start inside a goroutine and only did `_ = ptmx`, which does not extend the file's lifetime past that goroutine. The runtime finalizer then closed the PTY master, the kernel sent SIGHUP to the foreground process group, and the CLI died about 20s in -- running its EXIT trap, which deletes the daemonset, the collector pod and the namespace. `--max-time` was therefore never honoured in e2e; the whole suite really had a ~20s budget, and StartCommandWait was bumped to 20s to snapshot the output just before the process was killed. That was survivable while the daemonset was created in setup(), a second into the run. With the collector TLS path it is created only once the collector is ready (the agents consume the collector-tls-config ConfigMap written by the resolve-tls initContainer), i.e. about 8s in, leaving ~13s before the SIGHUP -- not enough for the agent pods to report ready, so "Verify all CLI pods are deployed" polled a daemonset that no longer existed until it timed out: 17:33:59.4 create daemonsets/netobserv-cli 17:34:12.4 delete daemonsets/netobserv-cli <- CLI EXIT trap after SIGHUP 17:34:16 test starts seeing NotFound, for 10 minutes Hold the PTY master in a package-level slice and drain it, so the command runs until its own --max-time. Since captures now outlive the spec, add StopStartedCommands and call it from cleanup(): otherwise a lingering CLI would fire its EXIT trap while the next spec is setting up and delete that spec's namespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

codecov · 2026-09-07T20:00:48Z

Codecov Report

❌ Patch coverage is 61.88341% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.31%. Comparing base (d924765) to head (a2b51bb).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/tlsresolver/resolve.go	43.85%	26 Missing and 6 partials ⚠️
e2e/common.go	0.00%	24 Missing ⚠️
internal/pkg/tlsresolver/config.go	85.00%	14 Missing and 1 partial ⚠️
cmd/collector_tls.go	79.41%	5 Missing and 2 partials ⚠️
cmd/resolve_tls.go	0.00%	4 Missing ⚠️
cmd/flow_capture.go	0.00%	1 Missing ⚠️
cmd/mocks.go	0.00%	1 Missing ⚠️
cmd/packet_capture.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
+ Coverage   13.18%   17.31%   +4.13%     
==========================================
  Files          20       24       +4     
  Lines        2443     2656     +213     
==========================================
+ Hits          322      460     +138     
- Misses       2095     2161      +66     
- Partials       26       35       +9

Flag	Coverage Δ
unittests	`17.31% <61.88%> (+4.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/root.go	`28.42% <100.00%> (+0.76%)`	⬆️
cmd/flow_capture.go	`7.59% <0.00%> (ø)`
cmd/mocks.go	`0.00% <0.00%> (ø)`
cmd/packet_capture.go	`0.00% <0.00%> (ø)`
cmd/resolve_tls.go	`0.00% <0.00%> (ø)`
cmd/collector_tls.go	`79.41% <79.41%> (ø)`
internal/pkg/tlsresolver/config.go	`85.00% <85.00%> (ø)`
e2e/common.go	`0.00% <0.00%> (ø)`
internal/pkg/tlsresolver/resolve.go	`43.85% <43.85%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

openshift-ci-robot added the jira/valid-reference label Aug 11, 2026

leandroberetta mentioned this pull request Aug 11, 2026

NETOBSERV-2515: Implement dynamic TLS configuration from OpenShift API Server netobserv/netobserv-operator#2823

Merged

6 tasks

leandroberetta requested review from jotak and jpinsonneau August 20, 2026 14:04

jpinsonneau reviewed Aug 25, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase label Aug 26, 2026

leandroberetta force-pushed the netobserv-2515 branch from f86c30c to 54b1101 Compare August 31, 2026 12:39

openshift-ci Bot removed the needs-rebase label Aug 31, 2026

leandroberetta force-pushed the netobserv-2515 branch from 47414b5 to 565fb2e Compare September 2, 2026 12:15

openshift-ci Bot added the needs-rebase label Sep 2, 2026

leandroberetta force-pushed the netobserv-2515 branch from fd8151c to 8956222 Compare September 2, 2026 13:50

openshift-ci Bot removed the needs-rebase label Sep 2, 2026

leandroberetta changed the title ~~NETOBSERV-2515: Add TLS support for collector when OpenShift~~ NETOBSERV-2977: Add TLS support for collector when OpenShift Sep 2, 2026

openshift-ci Bot added the needs-rebase label Sep 2, 2026

jotak added the ok-to-test label Sep 4, 2026

leandroberetta and others added 7 commits September 7, 2026 11:36

grpc writer with tls in openshift

bcee481

feedback addressed

4a09fda

update flp dependency

8b84fa1

leandroberetta force-pushed the netobserv-2515 branch from 90e2ebe to dce2750 Compare September 7, 2026 14:51

openshift-ci Bot removed the needs-rebase label Sep 7, 2026

github-actions Bot removed the ok-to-test label Sep 7, 2026

leandroberetta and others added 2 commits September 7, 2026 13:30

leandroberetta requested a review from jpinsonneau September 8, 2026 12:50

leandroberetta self-assigned this Sep 8, 2026

leandroberetta added the needs-review Tells that the PR needs a review label Sep 8, 2026

Uh oh!

Conversation

leandroberetta commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci-robot commented Aug 11, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test plan

Dependencies

Checklist

Uh oh!

openshift-ci Bot commented Aug 11, 2026

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

jpinsonneau Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

leandroberetta Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

leandroberetta commented Sep 1, 2026

Uh oh!

openshift-ci-robot commented Sep 2, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

openshift-ci-robot commented Sep 2, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Checklist

Uh oh!

codecov Bot commented Sep 7, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leandroberetta commented Aug 11, 2026 •

edited

Loading

openshift-ci-robot commented Aug 11, 2026 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Sep 2, 2026 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Sep 2, 2026 •

edited by openshift-ci Bot

Loading