CA Paths Fixes #1587
CA Paths Fixes #1587
Conversation
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
4ac1dc6 to
24d183c
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
2 times, most recently
from
635b5eb to
51c594b
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
2 times, most recently
from
45e79d2 to
63580e9
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
b1f1abf to
63580e9
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
63580e9 to
6d65e0c
Compare
WalkthroughThe changes remove a deprecated constant, introduce new constants for certificate bundle file paths, rename a utility function for combining CA bundles, and update reconciler logic to use these new constants and functions. They improve checks for ConfigMap existence and data before mounting CA bundles, adjust mount paths, and enhance AWS SDK HTTP client configuration with a custom transport. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Reconciler
participant Util
participant KubeAPI
User->>Reconciler: Trigger Reconcile()
Reconciler->>Util: CombineCaBundle(ServiceServingCertCAFile)
Util->>Util: Read system cert pool
Util->>Util: Append InjectedBundleCertCAFile
Util->>Util: Append ServiceServingCertCAFile
Util->>Reconciler: Return success or error
alt Success
Reconciler->>Reconciler: Set ApplyCAsToPods = ServiceServingCertCAFile
else Error (not file not found)
Reconciler->>Reconciler: Log error with details, set requeue delay
Reconciler->>Reconciler: Set ApplyCAsToPods = ServiceServingCertCAFile
end
Reconciler->>KubeAPI: Continue reconciliation logic
sequenceDiagram
participant Controller
participant KubeAPI
participant Pod
Controller->>KubeAPI: Check CaBundleConf ConfigMap
alt ConfigMap exists and has data
Controller->>Pod: Mount /etc/ocp-injected-ca-bundle volume
Controller->>Pod: Set NODE_EXTRA_CA_CERTS env var to ApplyCAsToPods
else
Controller->>Pod: Skip CA bundle mount
end
Pod->>Pod: Use CA bundle for TLS connections
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
pkg/system/phase2_creating.go (1)
474-485: 💡 Verification agent🧩 Analysis chain
NODE_EXTRA_CA_CERTSis set unconditionally even when the path is empty
r.UserCertBundlePathis only initialised inReconciler.Reconcile()whenutil.CombineCaBundlesucceeds.
IfCombineCaBundlereturns an os.IsNotExist error (very common when theocp-injected-ca-bundleConfigMap is not mounted into the operator pod),
r.UserCertBundlePathstays the empty string but we still inject it into the pod as the value ofNODE_EXTRA_CA_CERTS.At runtime NodeJS treats an empty value as disable extra certs, not fall back to the system pool, so TLS failures against
service endpoints that present the service-serving CA are still possible.case "NODE_EXTRA_CA_CERTS": - c.Env[j].Value = r.UserCertBundlePath + // Fall back to the service-serving CA when no user bundle is available + if r.UserCertBundlePath == "" { + c.Env[j].Value = util.ServiceServingCertCAFile + } else { + c.Env[j].Value = r.UserCertBundlePath + }This guarantees the container always receives a usable CA file path even when no user bundle exists.
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for CombineCaBundle usage in pkg..." rg -n "CombineCaBundle" -n pkg echo -e "\n🔍 Searching for UserCertBundlePath declaration and assignments..." rg -n "UserCertBundlePath" -n . echo -e "\n🔍 Searching for ServiceServingCertCAFile definition..." rg -n "ServiceServingCertCAFile" -n .Length of output: 1595
🏁 Script executed:
#!/bin/bash echo "🔍 Extracting lines 380-430 from pkg/system/reconciler.go for context" sed -n '380,430p' pkg/system/reconciler.goLength of output: 1919
🏁 Script executed:
#!/bin/bash echo "🔍 Extracting lines 390-430 from pkg/system/phase4_configuring.go for NODE_EXTRA_CA_CERTS handling" sed -n '390,430p' pkg/system/phase4_configuring.goLength of output: 1523
🏁 Script executed:
#!/bin/bash echo "🔍 Displaying CombineCaBundle implementation" rg -n "func CombineCaBundle" -A20 -n pkg/util/util.goLength of output: 748
Ensure NODE_EXTRA_CA_CERTS Always Points to a Valid CA Bundle
r.UserCertBundlePathremains empty wheneverCombineCaBundlereturnsos.IsNotExist(e.g., no injected CA bundle), and we currently inject that empty string intoNODE_EXTRA_CA_CERTS. Node.js treats an emptyNODE_EXTRA_CA_CERTSas “disable extra certs,” not “use system CAs,” leading to TLS failures against service‐serving CAs.Please update both places where
NODE_EXTRA_CA_CERTSis set to fall back toutil.ServiceServingCertCAFilewhenr.UserCertBundlePathis empty:• pkg/system/phase2_creating.go (around line 478)
• pkg/system/phase4_configuring.go (around line 410)Suggested diff for each:
case "NODE_EXTRA_CA_CERTS": - c.Env[j].Value = r.UserCertBundlePath + if r.UserCertBundlePath == "" { + // Fallback to service-serving CA when no injected bundle is available + c.Env[j].Value = util.ServiceServingCertCAFile + } else { + c.Env[j].Value = r.UserCertBundlePath + }This change guarantees every Node.js container receives a usable CA file path.
pkg/system/phase4_configuring.go (1)
510-533:⚠️ Potential issueGuard against missing
ca-bundle.crtkey to prevent pod startup failureThe volume mounts unconditionally reference
ca-bundle.crt.
If the ConfigMap exists but happens to lack that key, the pod will fail to mount and remain inCrashLoopBackOff.-// we want to check that the cm exists and also that it has data in it -if util.KubeCheckQuiet(r.CaBundleConf) && len(r.CaBundleConf.Data) > 0 { +// ensure CM exists *and* contains the expected key with non-empty content +if util.KubeCheckQuiet(r.CaBundleConf) { + if crt, ok := r.CaBundleConf.Data["ca-bundle.crt"]; ok && len(crt) > 0 {(The rest of the block stays the same.)
Failing fast here avoids introducing a hard-to-diagnose scheduling error at runtime.
pkg/util/util.go (1)
155-184: 🛠️ Refactor suggestion
CombineCaBundleshould skip missing files and/or improve error wording
- On a fresh cluster the injected bundle (
/etc/ocp-injected-ca-bundle/ca-bundle.crt) may not exist. Returning an error aborts startup even though the file is optional.- The log message says "Successfully appended …" after the append attempt, regardless of the result of
AppendCertsFromPEM(which could fail silently if the file is empty).A resilient pattern:
for _, certFile := range certFiles { - certs, err := os.ReadFile(certFile) - if err != nil { - return err - } + certs, err := os.ReadFile(certFile) + if os.IsNotExist(err) { + log.Infof("CA bundle %q not found – skipping", certFile) + continue + } else if err != nil { + return err + } // Append our cert to the system pool - if ok := rootCAs.AppendCertsFromPEM(certs); !ok { + if ok := rootCAs.AppendCertsFromPEM(certs); !ok { log.Errorf("Failed to append %q to RootCAs", certFile) return fmt.Errorf("failed to append %q to RootCAs", certFile) } - // Trust the augmented cert pool in our client - log.Infof("Successfully appended %q to RootCAs", certFile) + log.Infof("Appended %q to RootCAs", certFile) }This keeps the operator functional when only one of the files is present.
🧹 Nitpick comments (7)
pkg/system/phase2_creating.go (2)
660-670: Duplicated CA-bundle mounting logicThe identical
util.KubeCheckQuiet(...) && len(...Data) > 0stanza is repeated for both containers.
Extracting it into a helper would avoid divergence in future edits.Not blocking, but worth considering.
704-723: Race-free merge of Volume definitions
util.MergeVolumeListis called after a conditional check that may be executed in two different
containers; if both branches add the same volume concurrently an unnecessary array scan occurs
each reconciliation loop.
Moving the pod-level volume addition to a single, outer block (once per pod) will save work and keep the spec deterministic.Minor optimisation – no action required if low priority.
pkg/system/reconciler.go (1)
70-73: Field initialisation: consider a reliable default
UserCertBundlePathis only set later inReconcile(). Initialising it here to
util.ServiceServingCertCAFilewould avoid the empty-value edge-case noted inphase2_creating.go
and keeps the struct in a self-contained, safe state.pkg/system/phase4_configuring.go (3)
417-417: SkipNODE_EXTRA_CA_CERTSwhen the bundle path is empty
c.Env[j].Value = r.UserCertBundlePathblindly populates the env-var even whenUserCertBundlePathis an empty string.
For NodeJS, an empty value is still treated as “variable present”, which causes an attempted read of""and emits noisyENOENTwarnings on every TLS handshake.case "NODE_EXTRA_CA_CERTS": - c.Env[j].Value = r.UserCertBundlePath + if r.UserCertBundlePath != "" { + c.Env[j].Value = r.UserCertBundlePath + }This keeps the container log clean and avoids surprising behaviour when the ConfigMap was intentionally omitted.
529-531: Avoid hard-coding mount paths
"/etc/ocp-injected-ca-bundle"is duplicated across several files. Consider exporting it once (e.g.util.CaBundleDir) and re-using the constant to keep the path in sync across the codebase.- MountPath: "/etc/ocp-injected-ca-bundle", + MountPath: util.CaBundleDir,
1412-1414: Prefer a secure default transportSwitching from the insecure transport to
GlobalCARefreshingTransportonly whenUserCertBundlePathis set leaves the non-CA-bundle case usingInsecureHTTPTransport, which skips certificate verification entirely.If the intention is “use custom CAs when supplied, otherwise fall back to the system pool”, consider:
-client := &http.Client{ - Transport: util.InsecureHTTPTransport, +transport := util.GlobalCARefreshingTransport +if r.UserCertBundlePath == "" { + transport = util.InsecureHTTPTransport +} +client := &http.Client{ + Transport: transport, Timeout: 10 * time.Second, }This reduces the attack surface while preserving current behaviour.
pkg/util/util.go (1)
132-140: Harden TLS configurationBoth transports omit an explicit minimum TLS version. Relying on Go defaults means a future change could silently lower the bar (e.g., server-side default is still TLS-1.0). Set it explicitly—TLS 1.2 for legacy compatibility or TLS 1.3 if you can.
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, // or tls.VersionTLS13 if legacy clients are irrelevant + },Likewise for
GlobalCARefreshingTransport.
[security]🧰 Tools
🪛 ast-grep (0.38.1)
[warning] 133-133: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 138-138: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: false}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/options/options.go(0 hunks)pkg/system/phase2_creating.go(4 hunks)pkg/system/phase4_configuring.go(4 hunks)pkg/system/reconciler.go(3 hunks)pkg/util/util.go(4 hunks)
💤 Files with no reviewable changes (1)
- pkg/options/options.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/system/phase2_creating.go (1)
pkg/util/util.go (1)
KubeCheckQuiet(620-633)
pkg/system/phase4_configuring.go (1)
pkg/util/util.go (1)
KubeCheckQuiet(620-633)
pkg/system/reconciler.go (1)
pkg/util/util.go (2)
CombineCaBundle(156-185)ServiceServingCertCAFile(89-89)
🪛 ast-grep (0.38.1)
pkg/util/util.go
[warning] 133-133: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: golangci-lint
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
🔇 Additional comments (1)
pkg/system/phase2_creating.go (1)
576-586: Volume mount path changed to a directory – confirm the consuming code reads${path}/ca-bundle.crtMounting the ConfigMap at
/etc/ocp-injected-ca-bundleis fine, but make sure all consumers use the
/etc/ocp-injected-ca-bundle/ca-bundle.crtfile path (not the directory itself).
util.CombineCaBundledoes so viautil.InjectedBundleCertCAFile; please double-check that constant matches the new layout.No change requested – just a verification point.
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
6d65e0c to
6e29f74
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/util/util.go (1)
94-95:CombinedCaBundlePathis still declared but not usedThis constant remains unused as pointed out in the previous review. The
CombineCaBundlefunction only modifies the in-memoryGlobalCARefreshingTransport.TLSClientConfig.RootCAsand never writes to the file path specified by this constant.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/options/options.go(0 hunks)pkg/system/phase2_creating.go(4 hunks)pkg/system/phase4_configuring.go(4 hunks)pkg/system/reconciler.go(3 hunks)pkg/util/util.go(4 hunks)
💤 Files with no reviewable changes (1)
- pkg/options/options.go
✅ Files skipped from review due to trivial changes (1)
- pkg/system/phase2_creating.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/system/reconciler.go
- pkg/system/phase4_configuring.go
🧰 Additional context used
🪛 ast-grep (0.38.1)
pkg/util/util.go
[warning] 133-133: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: cnpg-deployment-test
- GitHub Check: auto-update
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-admission-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (2)
pkg/util/util.go (2)
156-156: Good function rename for clarityThe rename from
AddToRootCAstoCombineCaBundlebetter describes the function's purpose of combining certificate bundles.
163-163: Excellent improvements in implementationTwo positive changes:
- Using the
InjectedBundleCertCAFileconstant improves maintainability- Fixed the typo in the log message ("Successfuly" → "Successfully")
These changes align well with the PR's objective of standardizing CA path handling.
Also applies to: 181-181
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
2490117 to
fa267c2
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
7 times, most recently
from
3513de5 to
e40fc1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/util/util.go (1)
94-95:CombinedCaBundlePathconstant remains unusedThis constant was flagged in previous reviews and still appears to be unused in the codebase. Dead code should be removed to avoid confusion about the actual CA bundle handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/util.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/util/util.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
🪛 ast-grep (0.38.6)
pkg/util/util.go
[warning] 133-133: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 138-138: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-operator-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: golangci-lint
🔇 Additional comments (3)
pkg/util/util.go (3)
155-156: Good function rename for clarityThe rename from
AddToRootCAstoCombineCaBundlebetter reflects the function's purpose of combining CA bundles.
163-163: Good use of constant instead of hardcoded pathReplacing the hardcoded path with the
InjectedBundleCertCAFileconstant improves maintainability and centralization of CA paths.
181-181: Appropriate log level changeThe change from
PrintftoInfofis appropriate for this informational message and maintains consistency with the logging framework.
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
d506daf to
3b9584f
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
3b9584f to
1ad169b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/util/util.go (2)
88-96: CA path constants are well-defined.The new constants provide clear, centralized definitions for CA file paths:
ServiceServingCertCAFilefor the default OpenShift service CAInjectedBundleCertCAFilefor the additional injected CA bundleCombinedCaBundlePathfor the combined CA bundle outputThese constants improve maintainability and consistency across the codebase.
132-140: Comment improvements enhance code clarity.The updated comments for
InsecureHTTPTransportandGlobalCARefreshingTransportprovide clearer descriptions of their certificate verification behavior.
🧹 Nitpick comments (2)
.github/workflows/update-noobaa-core-tag.yml (1)
44-51: Guard against empty commitsIf another race condition (or a manual change) deletes the diff between the “Check for changes” step and the commit step,
git commitwill exit with status 1 (“nothing to commit”) and abort the workflow. Protect against that:git add pkg/options/options.go -git commit -m "chore: update noobaa-core image tag to ${{ github.event.inputs.container_image_tag }}" +if ! git diff --cached --quiet; then + git commit -m "chore: update noobaa-core image tag to ${{ github.event.inputs.container_image_tag }}" +else + echo "Nothing to commit, skipping push" + exit 0 +fi.githooks/pre-commit (1)
18-25: Consider adjusting the linting strategy for staged files.The current implementation has a potential issue where
--new-from-rev="$BASE"might not lint only the staged files, but rather all changed files since the base revision. This could include unstaged changes or miss some staged changes.Consider this alternative approach:
-# Run golangci-lint on staged files -echo "Running golangci-lint on staged files..." -BASE=$(git rev-parse --verify HEAD 2>/dev/null || echo "") -if [ -z "$BASE" ]; then - golangci-lint run $STAGED_FILES # initial commit – lint everything staged -else - golangci-lint run --new-from-rev="$BASE" -fi +# Run golangci-lint on staged files +echo "Running golangci-lint on staged files..." +golangci-lint run $STAGED_FILESThis ensures that only the staged files are linted, regardless of whether it's an initial commit or not.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.githooks/pre-commit(1 hunks).github/workflows/update-noobaa-core-tag.yml(1 hunks)Makefile(2 hunks)deploy/internal/statefulset-core.yaml(1 hunks)deploy/operator.yaml(1 hunks)pkg/bundle/deploy.go(4 hunks)pkg/options/options.go(0 hunks)pkg/system/phase2_creating.go(5 hunks)pkg/system/phase4_configuring.go(4 hunks)pkg/system/reconciler.go(3 hunks)pkg/util/kms/kms_kmip.go(1 hunks)pkg/util/kms/kms_kmip_storage.go(9 hunks)pkg/util/kms/kms_version.go(1 hunks)pkg/util/util.go(4 hunks)
💤 Files with no reviewable changes (1)
- pkg/options/options.go
✅ Files skipped from review due to trivial changes (1)
- pkg/util/kms/kms_kmip.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/system/phase4_configuring.go
- pkg/system/phase2_creating.go
- pkg/system/reconciler.go
🧰 Additional context used
🧠 Learnings (5)
deploy/operator.yaml (2)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1587
File: pkg/system/reconciler.go:287-291
Timestamp: 2025-07-14T10:59:21.959Z
Learning: NooBaa operator design constraint: Multiple NooBaa instances are not supported in the same namespace, only across different namespaces. This means hard-coded resource names like ConfigMaps are acceptable within a namespace scope.
Learnt from: naveenpaul1
PR: noobaa/noobaa-operator#1607
File: pkg/bundle/deploy.go:0-0
Timestamp: 2025-07-02T04:34:47.006Z
Learning: Default values for `METRICS_AUTH_ENABLED` and `VERSION_AUTH_ENABLED` are set up under `noobaa-config` in the NooBaa operator.
pkg/util/kms/kms_version.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
.githooks/pre-commit (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
pkg/bundle/deploy.go (3)
undefined
<retrieved_learning>
Learnt from: Neon-White
PR: #1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
</retrieved_learning>
<retrieved_learning>
Learnt from: Neon-White
PR: #1587
File: pkg/system/reconciler.go:287-291
Timestamp: 2025-07-14T10:59:21.959Z
Learning: NooBaa operator design constraint: Multiple NooBaa instances are not supported in the same namespace, only across different namespaces. This means hard-coded resource names like ConfigMaps are acceptable within a namespace scope.
</retrieved_learning>
<retrieved_learning>
Learnt from: naveenpaul1
PR: #1607
File: pkg/bundle/deploy.go:0-0
Timestamp: 2025-07-02T04:34:47.006Z
Learning: Default values for METRICS_AUTH_ENABLED and VERSION_AUTH_ENABLED are set up under noobaa-config in the NooBaa operator.
</retrieved_learning>
pkg/util/util.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
🧬 Code Graph Analysis (1)
pkg/util/kms/kms_kmip_storage.go (3)
pkg/util/util.go (1)
KubeCheck(571-592)pkg/util/kms/kms_kmip.go (2)
NewKMIPUniqueID(17-17)NewActiveKeyID(18-18)pkg/util/kms/kms_version.go (1)
ActiveRootKey(96-96)
🪛 ast-grep (0.38.6)
pkg/util/util.go
[warning] 133-133: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: golangci-lint
- GitHub Check: run-operator-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (15)
pkg/util/kms/kms_version.go (1)
141-141: LGTM!The type conversion using
toInterfaceMapis appropriate for passing toPutSecret.Likely an incorrect or invalid review comment.
pkg/util/kms/kms_kmip_storage.go (5)
38-40: Good practice: Using a constant for cryptographic length.Defining
cryptographicLength = 256as a constant improves maintainability and makes the expected key size explicit.
213-213: Improved error reporting with additional context.Including
ResultReasonandResultMessageprovides valuable debugging information.
265-276: Well-implemented active key ID handling and validation.The changes properly handle rotating secrets with active key IDs and add appropriate validation for cryptographic length. The error messages now include the secret name for better debugging.
Also applies to: 281-281, 319-321, 332-338
351-352: Correct implementation of rotating key storage pattern.The changes properly handle the active key pattern and store necessary metadata in the secret for future retrieval.
Also applies to: 373-373, 397-398
419-419: Good defensive programming with pre-existence check.Adding
KubeCheckbefore accessing the secret and including the secret name in error messages improves robustness and debuggability.Also applies to: 422-422
.github/workflows/update-noobaa-core-tag.yml (1)
65-75:gh pr checks --watchcan hang indefinitelyIf any check stalls or a required context never reports, this step will block for the full 90-minute timeout. Consider adding
--interval+--waitparameters or your own timeout loop so the workflow fails fast and reports a clear cause.Makefile (1)
122-122: LGTM! Good automation of Git hooks setup.The changes properly automate the installation of Git hooks by:
- Making the
gentarget depend oninstall-hooksto ensure hooks are configured before code generation- Adding proper checks to verify the directory is a Git repository
- Only setting the hooks path if it's not already configured to
.githooks- Using appropriate
.PHONYmarking for the targetThis ensures the pre-commit hook will be automatically activated for developers.
Also applies to: 332-339
pkg/util/util.go (1)
155-185: Function renaming and logic improvements are good.The renaming from
AddToRootCAstoCombineCaBundlebetter reflects the function's purpose. The implementation correctly:
- Uses the new
InjectedBundleCertCAFileconstant instead of hardcoded paths- Maintains the same logic for combining CA bundles
- Provides appropriate error handling and logging
- Updates the global CA pool for
GlobalCARefreshingTransportdeploy/internal/statefulset-core.yaml (1)
55-58: Verify readiness probe change from HTTP GET to TCP socketThe readinessProbe in
deploy/internal/statefulset-core.yamlwas changed from an HTTP GET on/versionto a simple TCP socket check on port 8080. This only confirms the port is open, not that the application is fully ready. Please confirm that:
- The core service no longer exposes or relies on an application-level endpoint at
/version.- A successful TCP connection on port 8080 reliably indicates the service is ready to handle traffic.
- This change aligns with your application’s startup and initialization behavior.
If any of these assumptions do not hold, consider restoring an HTTP-based probe against a lightweight readiness endpoint.
deploy/operator.yaml (1)
57-59: LGTM! Resource requests improve pod scheduling.Adding resource requests alongside the existing limits is a good practice:
- CPU request of 100m (vs 250m limit) allows for reasonable baseline allocation
- Memory request of 256Mi (vs 512Mi limit) provides adequate guaranteed memory
- This helps the Kubernetes scheduler make better placement decisions
- Follows best practices for resource management in cluster environments
pkg/bundle/deploy.go (4)
6209-6211: Good addition of resource requests for better resource management.Adding resource requests is a Kubernetes best practice that helps with:
- Proper pod scheduling based on available resources
- Quality of Service (QoS) class determination
- Resource planning and capacity management
The values (100m CPU, 256Mi memory) are reasonable and appropriately sized relative to the limits.
6151-6151: Verified: SHA256 checksum matches embedded operator YAML
The updatedSha256_deploy_operator_yamlconstant (aa8da1c289a05b3c94b9393b04307d38814a67625ac6a8006dace4d09366f35b) matches the actual SHA256 hash of theFile_deploy_operator_yamlcontent inpkg/bundle/deploy.go. No further action required.
5113-5116: Readiness probe change to TCP is consistentI searched the codebase for other HTTP-based readinessProbe configurations and found none. The switch from an HTTP GET to a TCP socket probe on port 8080 in
pkg/bundle/deploy.go(lines 5113–5116) appears intentional and does not leave any dangling HTTP probes that need coordination.No further changes required.
5057-5057: Manual verification required for SHA256 checksumI wasn’t able to automatically extract the embedded YAML block to compute its hash. Please confirm that the new
Sha256_deploy_internal_statefulset_core_yaml = "40aefcf8dbd77e40dfcbe3db3725b56a832c8a81c282c6ba4adc98ce8a65d470"
at pkg/bundle/deploy.go:5057 exactly matches the SHA-256 digest of theFile_deploy_internal_statefulset_core_yamlcontent.
- File: pkg/bundle/deploy.go
- Line: 5057 (constant
Sha256_deploy_internal_statefulset_core_yaml)
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
1ad169b to
06dbf8e
Compare
Neon-White
force-pushed
the
operator-cert-fixes
branch
2 times, most recently
from
8ef9735 to
22464a2
Compare
- Move the ServiceServingCertCAFile var from options.go to util.go - Fix the MountPath of the OCp injected CA bundle and improve handling of the configmap - Readd the global refreshing transport to the S3 clients in prepareAWSBackingStore - Rename AddToRootCAs to CombineCaBundle for added clarity - Add an explanation about the current certificate flow and its implication on other pods, and their HTTP agents Signed-off-by: Ben <[email protected]>
Neon-White
force-pushed
the
operator-cert-fixes
branch
from
22464a2 to
e1260ff
Compare
This PR picks up where #1570 left off and adds minor finishing touches.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores