WINC-1988: TLS profile adherence by mansikulkarni96 · Pull Request #4340 · openshift/windows-machine-config-operator

mansikulkarni96 · 2026-07-17T03:57:13Z

Add TLS strict obedience for PQC readiness
Fetch TLS configuration from cluster APIServer and apply to metrics server.
Watch for TLS profile changes and restart pod to reload config.
Uses SecurityProfileWatcher from controller-runtime-common following the pattern from machine-api-operator. Satisfies OpenShift 5.0 TLS.
Strict Obedience requirement for centralized TLS management.

openshift-ci · 2026-07-17T03:57:17Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-07-17T03:57:31Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (2)

do-not-merge/work-in-progress
do-not-merge/hold

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 22148a94-0136-4639-9055-721d92803294

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-07-17T03:57:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mansikulkarni96

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mansikulkarni96]

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

openshift-ci-robot · 2026-07-17T04:22:42Z

@mansikulkarni96: This pull request references WINC-1988 which is a valid jira issue.

Details

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

mansikulkarni96 · 2026-07-17T13:59:46Z

@code-rabbit review

mansikulkarni96 · 2026-07-17T16:37:04Z

@code-rabbit full review

mansikulkarni96 · 2026-07-17T16:38:17Z

/test aws-e2e-operator

jrvaldes · 2026-07-18T21:25:28Z

/test lint

openshift-ci · 2026-07-18T21:38:24Z

@mansikulkarni96: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/aws-e2e-operator	`527f1be`	link	true	`/test aws-e2e-operator`
ci/prow/lint	`527f1be`	link	true	`/test lint`

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

jrvaldes

@mansikulkarni96 thanks for working on this, mostly LGTM.

PTAL at the comments.

jrvaldes · 2026-07-18T22:01:01Z

 	// get cluster configuration
 	ctx := ctrl.SetupSignalHandler()
+
+	// Fetch TLS configuration from APIServer for PQC readiness (OpenShift 4.22 requirement)


(OpenShift 4.22 requirement)

does this mean the feature must be backported?

no this requirement is moved to 5.0, updated.

jrvaldes · 2026-07-18T22:01:45Z

 	ctx := ctrl.SetupSignalHandler()
+
+	// Fetch TLS configuration from APIServer for PQC readiness (OpenShift 4.22 requirement)
+	setupLog.Info("fetching TLS configuration from cluster APIServer")


consider debug log for this entry

jrvaldes · 2026-07-18T22:06:56Z

+
+	// Convert TLS profile to config function
+	tlsConfigFn, unsupportedCiphers := tlspkg.NewTLSConfigFromProfile(tlsProfile)
+	if len(unsupportedCiphers) > 0 {


should this log a WARN instead?

jrvaldes · 2026-07-19T02:34:00Z

+	}
+
+	setupLog.Info("TLS configuration loaded",
+		"minVersion", tlsProfile.MinTLSVersion,


windows-exporter may need min_version and cipher_suites on it TLS configuration
as it serve metrics over HTTPS

windows-machine-config-operator/pkg/internal/windows-exporter-webconfig.yaml

Line 1 in 740351c

tls_server_config:

not a blocker, consider a separate PR/jira for this.

jrvaldes · 2026-07-19T02:37:14Z

 	//       cache type.
 	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
 		Scheme: scheme,
 		Metrics: metricsserver.Options{


the proposal only sets MinVersion and CipherSuites. It does not set NextProtos. Without this, the metrics server falls back to Go's default
ALPN behavior (which is implementation-defined per Go version).

The doc reference explicitly states:
▎ Set NextProtos / ALPN yourself — the cluster profile does not set it.

https://docs.google.com/document/d/1NoHP2nZdg-xUkcB2y2BtI8469-iicdSG7xJwRad4kF0/edit?tab=t.d6wz8lyxdzg1#heading=h.y3v2qj7yid7t

controller-runtime's metrics server already sets NextProtos: []string{"h2"} in server.go:279 before calling our TLSOpts function. Since NewTLSConfigFromProfile only sets MinVersion and CipherSuites, the
existing NextProtos is preserved. No additional setting needed on our side.

jrvaldes · 2026-07-19T02:40:50Z

+			setupLog.Info("TLS security profile changed, pod will restart",
+				"oldMinVersion", oldProfile.MinTLSVersion,
+				"newMinVersion", newProfile.MinTLSVersion)
+			os.Exit(0) // Graceful exit - Kubernetes will restart pod


os.Exit does not run deferred functions. All ongoing node configuration, FTP transfers, Windows service management is abandoned
without cleanup. This can leave Windows nodes partially configured.

consider using context cancellation, which allows mgr.Start(ctx) to return cleanly, running all deferred cleanup registered with the manager.

jrvaldes · 2026-07-19T02:44:21Z

 			SecureServing:  true,
 			FilterProvider: filters.WithAuthenticationAndAuthorization,
+			TLSOpts:        []func(*tls.Config){tlsConfigFn}, // Apply cluster TLS profile
 		},


curve preferences PR merged, consider setting CurvePreferences to include X25519MLKEM768 for post-quantum key exchange

NE-2751: Implement enhancement in OpenShift API to support for TLS curves in TLSProfile api#2583

only the enhancement merged, implementation is pending https://github.com/openshift/controller-runtime-common/pull/22/changes

jrvaldes · 2026-07-19T02:48:04Z

+	// Watches for TLS profile or adherence policy changes and restarts pod to reload config
+	setupLog.Info("setting up TLS security profile watcher")
+	tlsWatcher := &tlspkg.SecurityProfileWatcher{
+		Client:                    mgr.GetClient(),


The PR creates cl, err := client.New(cfg, client.Options{}) exclusively for the initial TLS fetch, then discards it. The manager creates its own
internal client. The early client uses no cache (direct API server calls) which is correct for the pre-start fetch, but the two clients represent different connection pools.

Consider passing the pre-fetched values as parameters rather than creating a second client

Fetch TLS configuration from cluster APIServer and apply to metrics server. Watch for TLS profile changes and restart pod to reload config. Uses SecurityProfileWatcher from controller-runtime-common following the pattern from machine-api-operator. Satisfies OpenShift 5.0 TLS Strict Obedience requirement for centralized TLS management.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2026

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2026

mansikulkarni96 changed the title ~~TLS profile adherence~~ WINC-1988: TLS profile adherence Jul 17, 2026

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 17, 2026

mansikulkarni96 requested a review from jrvaldes July 17, 2026 16:38

jrvaldes requested changes Jul 19, 2026

View reviewed changes

mansikulkarni96 force-pushed the tls_profile branch from dc4efd2 to 41196ff Compare July 24, 2026 21:11

mansikulkarni96 requested a review from jrvaldes July 31, 2026 18:20

mansikulkarni96 added 2 commits July 31, 2026 14:21

[vendor] controller-runtime-common

41253d6

mansikulkarni96 force-pushed the tls_profile branch from 41196ff to 41253d6 Compare July 31, 2026 18:39

Uh oh!

Conversation

mansikulkarni96 commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

openshift-ci Bot commented Jul 17, 2026

Uh oh!

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

Uh oh!

Uh oh!

mansikulkarni96 commented Jul 17, 2026

Uh oh!

mansikulkarni96 commented Jul 17, 2026

Uh oh!

mansikulkarni96 commented Jul 17, 2026

Uh oh!

jrvaldes commented Jul 18, 2026

Uh oh!

openshift-ci Bot commented Jul 18, 2026

Uh oh!

jrvaldes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mansikulkarni96 commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

openshift-ci-robot commented Jul 17, 2026 •

edited by openshift-ci Bot

Loading