✨ Add pkg/tls package with ConfigMap-driven TLS profile support - #217
Merged
openshift-merge-bot[bot] merged 11 commits intoMar 24, 2026
Conversation
|
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:
WalkthroughAdds a new pkg/tls package for parsing/applying TLS profiles (cipher-suite map, TLSConfig type, ConfigMap load/watch helpers, conversion utilities, and README), a ConfigMap watcher in pkg/watcher, and comprehensive unit tests for both packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (7)
pkg/watcher/watcher.go (1)
147-163: Hash function is suitable for change detection but not cryptographically secure.The
hashConfigMapDatafunction creates a deterministic string representation rather than a cryptographic hash. This is appropriate for change detection (comparing equality), but the function name "hash" could be misleading. Consider renaming toserializeConfigMapDataor documenting that this returns a canonical string representation for comparison, not a cryptographic digest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/watcher/watcher.go` around lines 147 - 163, The function hashConfigMapData is a deterministic serializer, not a cryptographic hash; rename it to serializeConfigMapData (or similarly clear name) and update all call sites (hashConfigMapData -> serializeConfigMapData) to avoid misleading terminology, and/or update the function comment to state explicitly that it returns a canonical string representation for equality/change detection (not a cryptographic digest) so callers and future readers are not confused; adjust any tests or docs referencing hashConfigMapData accordingly.pkg/tls/tls_test.go (1)
648-665: Consider using synchronization primitives instead oftime.Sleepfor test reliability.The watcher tests rely on
time.Sleepfor synchronization (50ms, 200ms, 500ms delays). This can cause flakiness in CI environments with variable timing. Consider using channels or condition variables to signal when the watcher has processed events.♻️ Example using a channel for synchronization
t.Run("watcher calls onChangeFn when ConfigMap is updated", func(t *testing.T) { // ... setup ... changeChan := make(chan struct{}, 1) onChangeFn := func() { select { case changeChan <- struct{}{}: default: } cancel() } // ... start watcher ... // Wait for informer sync with timeout select { case <-time.After(2 * time.Second): t.Fatal("timeout waiting for informer sync") case <-func() chan struct{} { // Poll until ready or use informer's HasSynced ch := make(chan struct{}) go func() { time.Sleep(200 * time.Millisecond) // Initial sync close(ch) }() return ch }(): } // Update ConfigMap... // Wait for change notification select { case <-changeChan: // Success case <-time.After(2 * time.Second): t.Errorf("onChangeFn should be called when ConfigMap is updated") } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/tls_test.go` around lines 648 - 665, Replace the brittle time.Sleep calls with explicit synchronization: create a buffered channel (e.g., changeChan) and modify onChangeFn to signal that channel instead of toggling changeCalled; use the informer's HasSynced (or an equivalent ready channel) to wait for informer sync with a select+timeout instead of sleeping, then perform the update via client.CoreV1().ConfigMaps.Update and wait on changeChan with a timeout to assert the watcher fired. Update all occurrences of time.Sleep in this test to use these mechanisms and remove the changeCalled boolean check in favor of receiving from changeChan.pkg/tls/config.go (2)
158-166: Consider adding a reverse lookup map forcipherIDToNameefficiency.The current implementation iterates over the entire
cipherMapfor each cipher ID lookup, resulting in O(n) complexity per call. ForCipherSuitesToStringwith multiple cipher suites, this compounds. A pre-built reverse map would provide O(1) lookups.♻️ Proposed optimization with reverse map
In
cipher.go:+// reverseCipherMap maps Go cipher suite IDs to OpenSSL-style names +var reverseCipherMap = func() map[uint16]string { + m := make(map[uint16]string, len(cipherMap)) + for name, id := range cipherMap { + m[id] = name + } + return m +}()In
config.go:// cipherIDToName converts a cipher suite ID to its OpenSSL-style name func cipherIDToName(id uint16) string { - for name, suiteID := range cipherMap { - if suiteID == id { - return name - } - } - return "" + return reverseCipherMap[id] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/config.go` around lines 158 - 166, Replace the O(n) loop in cipherIDToName by adding a prebuilt reverse map (e.g., cipherNameByID map[uint16]string) built once at package init (or immediately after cipherMap is defined) and use it for O(1) lookups; update cipherIDToName to check cipherNameByID[id] and return the name if present (empty string otherwise). Ensure the reverse map is populated from the existing cipherMap (iterating once) and left immutable so no further concurrency guards are required, and update any callers like CipherSuitesToString to rely on the faster cipherIDToName.
82-111: Inconsistent handling of unsupported ciphers between flags and ConfigMap.
TLSConfigFromFlagsreturns an error when unsupported cipher suites are provided (lines 104-106), butparseTLSConfigFromConfigMapinconfigmap.goonly logs a warning and continues with the supported ciphers. This inconsistency could confuse users—flag-based config is strict while ConfigMap-based config is lenient.Consider aligning the behavior: either both should error, or both should warn and continue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/config.go` around lines 82 - 111, The behavior is inconsistent: TLSConfigFromFlags currently errors on unsupported cipher suites while parseTLSConfigFromConfigMap only warns; make them consistent by updating parseTLSConfigFromConfigMap to return an error for unsupported ciphers (matching TLSConfigFromFlags). Locate parseTLSConfigFromConfigMap and its use of parseCipherSuites, change its handling so that if parseCipherSuites returns any unsupported suites it returns an error (propagate a descriptive error like "unsupported cipher suites: %v"); update parseTLSConfigFromConfigMap's signature/return to include error and adjust its callers to handle the error accordingly.pkg/tls/configmap.go (2)
54-55: PassingnilascancelFuncrelies ononChangeFnalways being set.The code passes
nilascancelFunctoNewConfigMapWatcherand immediately setsonChangeFunc. This is safe here becauseonChangeFnis validated non-nil at line 37-39. However, this creates an implicit contract that's not enforced by the watcher API itself—ifSetOnChangeFuncwere missed,triggerRestart()would silently do nothing.Consider adding a comment explaining this design decision, or pass a defensive fallback:
💡 Option: Add clarifying comment
- w := watcher.NewConfigMapWatcher(client, namespace, ConfigMapName, nil, initData) + // Pass nil for cancelFunc since we always use onChangeFunc for graceful restarts. + // The onChangeFn validation above (line 37-39) ensures triggerRestart() will always have a callback. + w := watcher.NewConfigMapWatcher(client, namespace, ConfigMapName, nil, initData)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/configmap.go` around lines 54 - 55, The call to NewConfigMapWatcher currently passes nil for cancelFunc which relies on the subsequent SetOnChangeFunc(onChangeFn) call and a non-nil onChangeFn invariant; make this explicit by either (a) adding a short clarifying comment next to the NewConfigMapWatcher call that documents the invariant that onChangeFn is validated non-nil and SetOnChangeFunc must always be called, or (b) more defensively supply a small fallback cancel function (e.g., cancelFuncFallback) instead of nil so the watcher API never receives nil; update the call site where NewConfigMapWatcher(client, namespace, ConfigMapName, nil, initData) is invoked and reference onChangeFn, SetOnChangeFunc and triggerRestart in the comment or when creating the fallback to make the contract clear.
22-22: Consider using structured logging consistently.Line 22 uses
klog.V(4).Infof(printf-style), while line 59 usesklog.FromContext(ctx).Error(structured logging). For consistency and better log querying, consider using structured logging throughout.♻️ Proposed fix for consistent structured logging
- klog.V(4).Infof("ConfigMap %s/%s not found, using default TLS config", namespace, ConfigMapName) + klog.FromContext(ctx).V(4).Info("ConfigMap not found, using default TLS config", + "namespace", namespace, + "configmap", ConfigMapName)Similarly for line 91 in
parseTLSConfigFromConfigMap(thoughctxis not available there, a logger could be passed or the existing style retained for that function).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/configmap.go` at line 22, The log at the ConfigMap-not-found site uses printf-style klog.V(4).Infof; change it to structured logging (use klog.FromContext(ctx).WithValues("namespace", namespace, "configMap", ConfigMapName).Info("ConfigMap not found, using default TLS config")) so logs are consistent with the klog.FromContext(ctx).Error usage elsewhere; for parseTLSConfigFromConfigMap (where ctx isn't available) either pass a logger into that function or explicitly document/keep its current style, but prefer accepting a klog.Logger parameter and using logger.WithValues(...).Info to make both locations use structured logging.pkg/tls/cipher.go (1)
35-35: Consider removing the weak 3DES cipher suite.
DES-CBC3-SHA(3DES) is vulnerable to the SWEET32 attack (CVE-2016-2183) and is considered cryptographically weak. Modern security guidelines recommend disabling 3DES. If this cipher is included for legacy compatibility, consider documenting that rationale explicitly.🔒 Proposed fix to remove weak cipher
- "DES-CBC3-SHA": tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, + // "DES-CBC3-SHA": tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, // Disabled: vulnerable to SWEET32 attack (CVE-2016-2183)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/cipher.go` at line 35, The map entry mapping "DES-CBC3-SHA" to tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA in pkg/tls/cipher.go is a weak 3DES cipher (SWEET32); remove this map element to disable 3DES support (or if legacy compatibility is required, keep it but add a clear comment explaining the risk and why it must remain). Locate the cipher suite mapping (the key "DES-CBC3-SHA" and value tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA) and delete that line from the cipher list; if retaining it, prepend a comment referencing CVE-2016-2183 and an explicit justification for preservation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/watcher/watcher.go`:
- Around line 114-121: The DeleteFunc currently asserts obj.(*corev1.ConfigMap)
and will panic when a cache.DeletedFinalStateUnknown tombstone is received;
update DeleteFunc to detect tombstones (cache.DeletedFinalStateUnknown), unwrap
the tombstone to get the actual object, then type-assert that result to
*corev1.ConfigMap before using cm.Name and calling w.triggerRestart(); also log
or ignore cases where the tombstone payload isn't a ConfigMap to avoid panics
(references: DeleteFunc, cache.DeletedFinalStateUnknown, corev1.ConfigMap,
w.configMapName, w.triggerRestart, logger.Info).
- Around line 81-93: The AddFunc handler accesses and mutates the watcher
struct's initialHash without synchronization, which can race with other informer
callbacks; protect this by adding a sync.Once field (e.g., initOnce) or a mutex
on the watcher struct and use it when initializing initialHash inside the
AddFunc branch (where initialHash == "" is checked) so the baseline is set
exactly once; keep the existing else branch that compares cmHash and calls
w.triggerRestart(), but read initialHash under the same synchronization to avoid
races.
---
Nitpick comments:
In `@pkg/tls/cipher.go`:
- Line 35: The map entry mapping "DES-CBC3-SHA" to
tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA in pkg/tls/cipher.go is a weak 3DES cipher
(SWEET32); remove this map element to disable 3DES support (or if legacy
compatibility is required, keep it but add a clear comment explaining the risk
and why it must remain). Locate the cipher suite mapping (the key "DES-CBC3-SHA"
and value tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA) and delete that line from the
cipher list; if retaining it, prepend a comment referencing CVE-2016-2183 and an
explicit justification for preservation.
In `@pkg/tls/config.go`:
- Around line 158-166: Replace the O(n) loop in cipherIDToName by adding a
prebuilt reverse map (e.g., cipherNameByID map[uint16]string) built once at
package init (or immediately after cipherMap is defined) and use it for O(1)
lookups; update cipherIDToName to check cipherNameByID[id] and return the name
if present (empty string otherwise). Ensure the reverse map is populated from
the existing cipherMap (iterating once) and left immutable so no further
concurrency guards are required, and update any callers like
CipherSuitesToString to rely on the faster cipherIDToName.
- Around line 82-111: The behavior is inconsistent: TLSConfigFromFlags currently
errors on unsupported cipher suites while parseTLSConfigFromConfigMap only
warns; make them consistent by updating parseTLSConfigFromConfigMap to return an
error for unsupported ciphers (matching TLSConfigFromFlags). Locate
parseTLSConfigFromConfigMap and its use of parseCipherSuites, change its
handling so that if parseCipherSuites returns any unsupported suites it returns
an error (propagate a descriptive error like "unsupported cipher suites: %v");
update parseTLSConfigFromConfigMap's signature/return to include error and
adjust its callers to handle the error accordingly.
In `@pkg/tls/configmap.go`:
- Around line 54-55: The call to NewConfigMapWatcher currently passes nil for
cancelFunc which relies on the subsequent SetOnChangeFunc(onChangeFn) call and a
non-nil onChangeFn invariant; make this explicit by either (a) adding a short
clarifying comment next to the NewConfigMapWatcher call that documents the
invariant that onChangeFn is validated non-nil and SetOnChangeFunc must always
be called, or (b) more defensively supply a small fallback cancel function
(e.g., cancelFuncFallback) instead of nil so the watcher API never receives nil;
update the call site where NewConfigMapWatcher(client, namespace, ConfigMapName,
nil, initData) is invoked and reference onChangeFn, SetOnChangeFunc and
triggerRestart in the comment or when creating the fallback to make the contract
clear.
- Line 22: The log at the ConfigMap-not-found site uses printf-style
klog.V(4).Infof; change it to structured logging (use
klog.FromContext(ctx).WithValues("namespace", namespace, "configMap",
ConfigMapName).Info("ConfigMap not found, using default TLS config")) so logs
are consistent with the klog.FromContext(ctx).Error usage elsewhere; for
parseTLSConfigFromConfigMap (where ctx isn't available) either pass a logger
into that function or explicitly document/keep its current style, but prefer
accepting a klog.Logger parameter and using logger.WithValues(...).Info to make
both locations use structured logging.
In `@pkg/tls/tls_test.go`:
- Around line 648-665: Replace the brittle time.Sleep calls with explicit
synchronization: create a buffered channel (e.g., changeChan) and modify
onChangeFn to signal that channel instead of toggling changeCalled; use the
informer's HasSynced (or an equivalent ready channel) to wait for informer sync
with a select+timeout instead of sleeping, then perform the update via
client.CoreV1().ConfigMaps.Update and wait on changeChan with a timeout to
assert the watcher fired. Update all occurrences of time.Sleep in this test to
use these mechanisms and remove the changeCalled boolean check in favor of
receiving from changeChan.
In `@pkg/watcher/watcher.go`:
- Around line 147-163: The function hashConfigMapData is a deterministic
serializer, not a cryptographic hash; rename it to serializeConfigMapData (or
similarly clear name) and update all call sites (hashConfigMapData ->
serializeConfigMapData) to avoid misleading terminology, and/or update the
function comment to state explicitly that it returns a canonical string
representation for equality/change detection (not a cryptographic digest) so
callers and future readers are not confused; adjust any tests or docs
referencing hashConfigMapData accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 964da0da-41b2-40b8-bc50-772b89e7fc19
📒 Files selected for processing (5)
pkg/tls/cipher.gopkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.gopkg/watcher/watcher.go
zhujian7
force-pushed
the
feat/add-tls-watcher-packages
branch
from
March 20, 2026 07:57
6a328aa to
191fc9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/watcher/watcher_test.go (1)
31-36: Remove unusedwantSamefield from test cases.
wantSameis declared but never used, which makes the table intent unclear.🧹 Proposed cleanup
tests := []struct { name string data map[string]string wantHash string - wantSame string // another data map that must produce the same hash (optional) }{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/watcher/watcher_test.go` around lines 31 - 36, The table-driven tests in watcher_test.go declare an unused field wantSame in the tests slice literal which should be removed; update the tests variable (remove the wantSame field from the anonymous struct and delete any wantSame entries in each test case) and adjust any test logic referencing wantSame so only name, data and wantHash remain (look for the tests variable and any Test* functions using it) to clean up the unused field.pkg/tls/configmap.go (1)
86-94: Consider behavior when all specified ciphers are unsupported.If a user specifies cipher suites but all of them are unsupported,
cipherSuiteswill be an empty slice andcfg.CipherSuiteswill benil(empty slice assigned). This silently falls back to Go's default cipher suite selection after logging a warning.This may be intentional, but consider whether this edge case warrants an error or at least a more prominent warning indicating that no ciphers from the configuration could be applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/configmap.go` around lines 86 - 94, When parsing cipher suites from the ConfigMap (cipherSuitesStr -> parseCipherSuites), handle the case where the user provided values but none are supported: after calling parseCipherSuites check if cipherSuitesStr != "" && len(cipherSuites) == 0 and instead of quietly assigning cfg.CipherSuites, surface a stronger signal (for example return or propagate an error, or change the log to klog.Errorf and include cm.Namespace/cm.Name and the unsupported list) so callers know no configured ciphers could be applied; update the code around parseCipherSuites, cfg.CipherSuites, and the klog.Warningf call accordingly.pkg/tls/tls_test.go (1)
648-665: Consider using channels for more deterministic watcher tests.The
time.Sleepcalls (200ms for informer sync, 500ms for event processing) may cause flakiness under CI load. Consider using a channel-based approach for more reliable synchronization:♻️ Example approach
changeCalled := false +changeCh := make(chan struct{}, 1) onChangeFn := func() { changeCalled = true + changeCh <- struct{}{} cancel() // Cancel context to stop the watcher } // ... -// Wait for the event to be processed -time.Sleep(500 * time.Millisecond) +// Wait for the event to be processed +select { +case <-changeCh: +case <-time.After(2 * time.Second): + t.Fatal("timeout waiting for onChangeFn to be called") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tls/tls_test.go` around lines 648 - 665, Replace the brittle time.Sleep-based waits with channel-based synchronization: remove the 200ms and 500ms sleeps and instead wait for the informer to be fully synced (use the informer/sharedInformerFactory HasSynced or WaitForCacheSync) with a short loop or context timeout, and have the onChangeFn signal a channel (e.g., make a changesCh and send a token when onChangeFn runs and sets changeCalled) then after performing client.CoreV1().ConfigMaps(...).Update(ctx, updatedCM, metav1.UpdateOptions{}) wait on that channel with a timeout to deterministically assert the update was processed; use ConfigMapKeyMinVersion/updatedCM and changeCalled/onChangeFn names to locate the update and the callback to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/watcher/watcher_test.go`:
- Line 336: The test's cancelWrapper currently only signals the channel but
doesn't call the real cancellation, so update the closure assigned to
cancelWrapper to call the actual cancel function (e.g., invoke cancel() inside
the wrapper) so the test validates the cancellation side-effect and fallback
semantics; apply the same change to the other occurrences noted around the
cancelWrapper usage (lines ~351-356) so each wrapper both signals cancelCalled
and calls cancel().
- Around line 140-145: Replace fragile 200ms hardcoded waits in the select
blocks that assert "no trigger" by introducing a shared timeout (e.g., a
package-level const noTriggerTimeout = 500*time.Millisecond or a helper
waitNoTrigger(triggered <-chan struct{}) error) and use that constant/helper in
the select that reads from the triggered channel (the select blocks around the
variable triggered and the similar selects at the other occurrences). Update all
occurrences (the select at the shown snippet and the selects referenced at
165-170, 223-228, 318-323) to use this shared timeout to stabilize CI timing.
---
Nitpick comments:
In `@pkg/tls/configmap.go`:
- Around line 86-94: When parsing cipher suites from the ConfigMap
(cipherSuitesStr -> parseCipherSuites), handle the case where the user provided
values but none are supported: after calling parseCipherSuites check if
cipherSuitesStr != "" && len(cipherSuites) == 0 and instead of quietly assigning
cfg.CipherSuites, surface a stronger signal (for example return or propagate an
error, or change the log to klog.Errorf and include cm.Namespace/cm.Name and the
unsupported list) so callers know no configured ciphers could be applied; update
the code around parseCipherSuites, cfg.CipherSuites, and the klog.Warningf call
accordingly.
In `@pkg/tls/tls_test.go`:
- Around line 648-665: Replace the brittle time.Sleep-based waits with
channel-based synchronization: remove the 200ms and 500ms sleeps and instead
wait for the informer to be fully synced (use the informer/sharedInformerFactory
HasSynced or WaitForCacheSync) with a short loop or context timeout, and have
the onChangeFn signal a channel (e.g., make a changesCh and send a token when
onChangeFn runs and sets changeCalled) then after performing
client.CoreV1().ConfigMaps(...).Update(ctx, updatedCM, metav1.UpdateOptions{})
wait on that channel with a timeout to deterministically assert the update was
processed; use ConfigMapKeyMinVersion/updatedCM and changeCalled/onChangeFn
names to locate the update and the callback to modify.
In `@pkg/watcher/watcher_test.go`:
- Around line 31-36: The table-driven tests in watcher_test.go declare an unused
field wantSame in the tests slice literal which should be removed; update the
tests variable (remove the wantSame field from the anonymous struct and delete
any wantSame entries in each test case) and adjust any test logic referencing
wantSame so only name, data and wantHash remain (look for the tests variable and
any Test* functions using it) to clean up the unused field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 814dd36f-a410-40bd-a185-b605e82bcaa9
📒 Files selected for processing (4)
pkg/tls/cipher.gopkg/tls/configmap.gopkg/tls/tls_test.gopkg/watcher/watcher_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tls/cipher.go
Member
Author
|
@zhujian7: GitHub didn't allow me to request PR reviews from the following users: shivansh-source. Note that only open-cluster-management-io members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tls/config.go`:
- Around line 147-155: ConfigToFunc should handle a nil TLSConfig to avoid
panics when composed with ConfigFromFlags; update ConfigToFunc (the function
that returns func(*tls.Config)) to check if tlsCfg == nil and return a no-op
func that leaves the provided *tls.Config unchanged, otherwise keep the existing
logic (setting MinVersion, MaxVersion, CipherSuites). Add a regression test that
calls ConfigFromFlags (when both flags are unset) and then calls the returned
function from ConfigToFunc to ensure no nil dereference occurs and the
tls.Config remains valid.
In `@pkg/tls/configmap.go`:
- Around line 41-55: The watcher is being seeded with a synthesized initData
built from the normalized TLSConfig which can differ from the raw ConfigMap and
trigger a spurious change; instead, fetch the ConfigMap raw data first and pass
that to watcher.NewConfigMapWatcher when the ConfigMap exists (use the client to
Get the ConfigMap named ConfigMapName in namespace and set initData = cm.Data),
and only synthesize defaults (using LoadTLSConfigFromConfigMap /
GetDefaultTLSConfig and VersionToString/CipherSuitesToString) when the ConfigMap
is not found; update the block around LoadTLSConfigFromConfigMap, initData and
the call to watcher.NewConfigMapWatcher (and keep using onChangeFn) so the
initial seed uses the original cm.Data.
In `@pkg/tls/tls_test.go`:
- Around line 310-319: The test is missing negative assertions: in the
ConfigToFunc test (using variables tc and config) add an else branch for
tc.expectCipherSuites so when it's false assert len(config.CipherSuites) == 0
(to ensure CipherSuites wasn't populated), and add a check when
tc.expectMaxVersion == 0 that config.MaxVersion == 0 (to ensure MaxVersion
wasn't set); update the block after the existing checks that reference
tc.expectCipherSuites, tc.expectMaxVersion, expectedCipherCount, and config to
include these negative assertions.
- Around line 536-570: The tests use an unsynchronized shared boolean
(changeCalled) and Sleep to observe the informer callback, causing data races
and flaky timing; update each subtest that calls StartTLSConfigMapWatcher to
replace changeCalled and Sleep with a buffered event channel (e.g., changed :=
make(chan struct{},1)) and make onChangeFn send into that channel
(non-blocking), then use select with a time.After timeout to assert "did not
fire" or "did fire" deterministically; reference the StartTLSConfigMapWatcher
call and the onChangeFn callback when making this change and apply the same
pattern to all four subtests to remove races and fixed sleeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc0b113-b26a-4a85-a23b-14d7abd4369e
📒 Files selected for processing (6)
pkg/tls/README.mdpkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.gopkg/watcher/watcher.gopkg/watcher/watcher_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/watcher/watcher_test.go
- pkg/tls/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/watcher/watcher.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tls/configmap.go`:
- Around line 52-85: The startup path seeds initData and tlsCfg after calling
client.CoreV1().ConfigMaps(...).Get(...), but if the ConfigMap existed at that
Get and was deleted before the informer syncs, ConfigMapWatcher.Start(ctx) can
return without invoking AddFunc/DeleteFunc and the process will keep the stale
tlsCfg; update the logic after w.Start(ctx) (or inside watcher.Start flow) to
re-check the ConfigMap presence post-sync when initData produced a non-empty
hash: call client.CoreV1().ConfigMaps(namespace).Get(ctx, ConfigMapName,
metav1.GetOptions{}) again (or use the watcher's lister/Get), and if it returns
NotFound, invoke the onChangeFn with nil/empty data (or trigger the same delete
handling path) so tlsCfg is cleared/rewritten to default; add a regression test
that simulates ConfigMap present at initial Get, deleted before informer sync,
and asserts the delete handling runs and tlsCfg is not left stale.
- Around line 111-118: The current ConfigMap parsing path (parseCipherSuites ->
cfg.CipherSuites) only warns on unsupported cipher names and will silently leave
cfg.CipherSuites empty if all names are invalid; change this to fail closed:
after calling parseCipherSuites in the ConfigMap handling code, if the input
string is non-empty and parseCipherSuites returns zero supported suites, return
an error (or propagate an error) instead of only logging a warning so the
invalid policy is rejected (matching ConfigFromFlags behavior); update the code
paths that call this code to handle the error and add a regression test that
supplies a ConfigMap with only unsupported cipher names and asserts the function
returns an error (and that valid lists still set cfg.CipherSuites).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df97c7c3-be85-4104-83bd-6f7e00c1b6a5
📒 Files selected for processing (3)
pkg/tls/config.gopkg/tls/configmap.gopkg/tls/tls_test.go
Comment on lines
+52
to
+85
| cm, err := client.CoreV1().ConfigMaps(namespace).Get(ctx, ConfigMapName, metav1.GetOptions{}) | ||
| if err != nil && !errors.IsNotFound(err) { | ||
| return nil, fmt.Errorf("failed to get ConfigMap %s/%s: %w", namespace, ConfigMapName, err) | ||
| } | ||
|
|
||
| var initData map[string]string | ||
| var tlsCfg *TLSConfig | ||
|
|
||
| if err == nil { | ||
| initData = cm.Data | ||
| tlsCfg, err = parseTLSConfigFromConfigMap(cm) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| tlsCfg = GetDefaultTLSConfig() | ||
| initData = map[string]string{ | ||
| ConfigMapKeyMinVersion: VersionToString(tlsCfg.MinVersion), | ||
| } | ||
| if len(tlsCfg.CipherSuites) > 0 { | ||
| initData[ConfigMapKeyCipherSuites] = CipherSuitesToString(tlsCfg.CipherSuites) | ||
| } | ||
| } | ||
|
|
||
| w := watcher.NewConfigMapWatcher(client, namespace, ConfigMapName, nil, initData) | ||
| w.SetOnChangeFunc(onChangeFn) | ||
|
|
||
| // Start blocks until the informer cache is synced, then returns while the | ||
| // informer goroutines continue running until ctx is cancelled. Calling it | ||
| // synchronously guarantees that the watcher is ready when this function returns, | ||
| // so callers can safely create/update/delete the ConfigMap immediately after. | ||
| if err := w.Start(ctx); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Handle the “present at startup Get, missing at first informer sync” case.
If the ConfigMap exists at Line 52 but is deleted before the informer ever observes it, pkg/watcher/watcher.go never runs AddFunc or DeleteFunc, so w.Start(ctx) returns and the process keeps using the stale tlsCfg until some later change happens. Please add a post-sync absence check when startup seeded a non-empty hash, and cover it with a regression test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tls/configmap.go` around lines 52 - 85, The startup path seeds initData
and tlsCfg after calling client.CoreV1().ConfigMaps(...).Get(...), but if the
ConfigMap existed at that Get and was deleted before the informer syncs,
ConfigMapWatcher.Start(ctx) can return without invoking AddFunc/DeleteFunc and
the process will keep the stale tlsCfg; update the logic after w.Start(ctx) (or
inside watcher.Start flow) to re-check the ConfigMap presence post-sync when
initData produced a non-empty hash: call
client.CoreV1().ConfigMaps(namespace).Get(ctx, ConfigMapName,
metav1.GetOptions{}) again (or use the watcher's lister/Get), and if it returns
NotFound, invoke the onChangeFn with nil/empty data (or trigger the same delete
handling path) so tlsCfg is cleared/rewritten to default; add a regression test
that simulates ConfigMap present at initial Get, deleted before informer sync,
and asserts the delete handling runs and tlsCfg is not left stale.
zhujian7
force-pushed
the
feat/add-tls-watcher-packages
branch
from
March 20, 2026 13:27
9f33bf3 to
8122606
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
|
LGTM |
Member
I got this wrong, openshift using OpenSSL-style, it's not equal to go's standard tls names. The mapping is required. PR looks good to me. |
qiujian16
reviewed
Mar 24, 2026
| // returns while the informer continues running until ctx is canceled. | ||
| // onChangeFn is called when the ConfigMap changes. | ||
| // It returns the TLSConfig active at startup so callers can apply it immediately. | ||
| func StartTLSConfigMapWatcher(ctx context.Context, client kubernetes.Interface, namespace string, onChangeFn func()) (*TLSConfig, error) { |
Member
There was a problem hiding this comment.
why don't we just build a controller for this using base controller, and we can read from the lister.
|
|
||
| _, err := cmInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
| AddFunc: func(obj interface{}) { | ||
| cm := obj.(*corev1.ConfigMap) |
Member
There was a problem hiding this comment.
I feel it will be much easier to implement if using controller. We do not need to customize handler, just check if the hash is set and if it is changed.
The only question is what if configmap is missing and created later? how it can be differed from a start when configmap already exists?
pkg/tls provides TLS configuration helpers for OCM components: - parse TLS version and cipher suites from flags or a ConfigMap - build crypto/tls.Config and controller-runtime TLSOpts functions - Options type with pflag integration and priority-based config loading (flags > ConfigMap > defaults) pkg/watcher provides a generic ConfigMapWatcher that triggers graceful shutdown (via context cancellation) or a custom callback when a watched ConfigMap is created, updated, or deleted. The initial hash is seeded from the config in use at startup so any drift detected on first sync also triggers a restart. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
Remove exported symbols that are not called by either consumer: - options.go deleted (Options, NewOptions, AddFlags, GetTLSConfig, StartConfigMapWatcher, GetTLSConfigForServer — none used) - GetSupportedCipherSuites deleted from cipher.go Make internal-only helpers unexported: - ParseTLSVersion → parseTLSVersion - ParseCipherSuites → parseCipherSuites - DefaultMinTLSVersion → defaultMinTLSVersion - ParseTLSConfigFromConfigMap → parseTLSConfigFromConfigMap - HashConfigMapData → hashConfigMapData (watcher package) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
StartTLSConfigMapWatcher combines ConfigMap loading, watcher seeding, and background goroutine startup into a single call. It: - loads the current TLS config (falling back to defaults if absent) - seeds the watcher with the effective config so mid-startup changes are detected - starts the watcher in a background goroutine - returns the active TLSConfig so callers can apply it immediately - rejects a nil onChangeFn to prevent silent no-ops Add tls_test.go with 96.9% statement coverage across all exported functions and the StartTLSConfigMapWatcher watcher integration. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
Tests cover: - hashConfigMapData: nil/empty, single/multi-key sorted output, order independence, collision resistance, different values - NewConfigMapWatcher: field and initialHash initialization - Start AddFunc: no initData sets baseline without trigger, matching initData no-ops, differing initData triggers - Start UpdateFunc: same data no-ops, changed data triggers - Start DeleteFunc: always triggers - Wrong CM name is ignored - cancelFunc is called when no onChangeFunc is set - Pre-cancelled context returns error Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
Rename exported functions whose names begin with the package name (tls.*TLS...) to avoid the revive 'exported' stutter warning: TLSConfigFromFlags -> ConfigFromFlags TLSVersionToString -> VersionToString TLSConfigToFunc -> ConfigToFunc Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
pkg/watcher/watcher.go: - Use sync.Once to protect one-time initialisation of initialHash in AddFunc, eliminating the potential data race - Handle cache.DeletedFinalStateUnknown tombstones in DeleteFunc to avoid a panic when the informer reconnects after a disconnect pkg/watcher/watcher_test.go: - Replace hard-coded 200ms waits with a noEventWait constant (500ms) for more stable CI behaviour - cancelWrapper in TestStart_CancelFuncCalledWhenNoOnChangeFunc now calls the real cancel function; uses a dedicated watcher context so the test select does not race between cancelCalled and ctx.Done() pkg/tls/config.go + callers: - Rename TLSConfigFromFlags -> ConfigFromFlags - Rename TLSVersionToString -> VersionToString - Rename TLSConfigToFunc -> ConfigToFunc to fix revive stutter lint errors (tls.TLS*) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
Documents the ConfigMap format, three use cases (flag-based config, ConfigMap watcher, and operator-to-component flag forwarding), and an API reference table for all exported functions and constants. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
pkg/tls/config.go: - ConfigToFunc: guard against nil TLSConfig (returned by ConfigFromFlags when no flags are set) by returning a no-op func instead of panicking pkg/tls/configmap.go: - StartTLSConfigMapWatcher: seed watcher with raw cm.Data instead of re-serializing through VersionToString/CipherSuitesToString. The watcher hashes raw cm.Data, so normalization differences (e.g. "TLSv1.2" vs "VersionTLS12", or an injected empty "cipherSuites" key) produce a mismatched hash and trigger a spurious restart loop on every startup. - StartTLSConfigMapWatcher: when no CM exists at startup, seed with only non-empty default fields (minTLSVersion only) so a CM created later with non-defaults triggers onChangeFn, while a CM with the same defaults does not. - StartTLSConfigMapWatcher: call w.Start synchronously so it blocks until the informer cache is synced before returning, guaranteeing the watcher is ready when the function returns and removing the goroutine-induced race. pkg/tls/tls_test.go: - ConfigToFunc: add nil TLSConfig test case and negative assertions for CipherSuites and MaxVersion not being set when not expected - StartTLSConfigMapWatcher: replace bool + time.Sleep (data race) with buffered channel + select for race-safe, deterministic event assertions - StartTLSConfigMapWatcher: add test verifying that a CM created with non-default TLS version triggers restart when no CM existed at startup Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com>
Replace the custom ConfigMapWatcher (pkg/watcher) with a controller built on pkg/basecontroller/factory. The new implementation: - Eliminates the startup race-window workaround (hash seeding via sync.Once) by seeding lastHash from the warm lister after WaitForCacheSync, before starting the controller goroutine - Removes the custom Add/Update/Delete event handlers — Sync() reads from the lister and compares hashes in one place - Deletes pkg/watcher entirely (it was only used by pkg/tls) Signed-off-by: Jia Zhu <jiazhu@redhat.com> Signed-off-by: zhujian <jiazhu@redhat.com>
If cipherSuites is non-empty but every name is unknown, the previous code silently left cfg.CipherSuites nil, causing ConfigToFunc to fall back to Go's default suites instead of rejecting the invalid policy. Return an error when no supported cipher suite is found, consistent with ConfigFromFlags behavior. Add a regression test. Signed-off-by: zhujian <jiazhu@redhat.com>
zhujian7
force-pushed
the
feat/add-tls-watcher-packages
branch
from
March 24, 2026 10:01
8122606 to
70a6cc7
Compare
Signed-off-by: zhujian <jiazhu@redhat.com>
zhujian7
changed the title
Member
Author
|
@qiujian16 PTAL |
Member
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 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 |
openshift-merge-bot
Bot
merged commit Mar 24, 2026
9cbb733
into
open-cluster-management-io:main
10 checks passed
13 tasks
This was referenced Mar 30, 2026
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.
Summary
pkg/tlspackage with helpers for parsing and applying TLS profile configuration:LoadTLSConfigFromConfigMap— one-shot load from theocm-tls-profileConfigMapStartTLSConfigMapWatcher— loads the active TLS config and starts a background controller that callsonChangeFnwhen the ConfigMap is created, updated, or deletedConfigFromFlags,ConfigToFunc,GetDefaultTLSConfig,VersionToString,CipherSuitesToString— flag parsing and conversion helpersStartTLSConfigMapWatcheris built onpkg/basecontroller/factorywith a ConfigMap lister, which eliminates the need for a separate watcher package and removes the startup race-window workaround that was previously needed to seed an initial hashparseTLSConfigFromConfigMap: return an error when every configured cipher suite is unsupported (rather than silently falling back to Go's defaults)ConfigToFuncnil guard so it composes safely withConfigFromFlagswhen no flags are setpkg/tlsdocumenting ConfigMap format, supported values, and usage examplesRelated issue(s)
Fixes #open-cluster-management-io/ocm#1443