Open
Conversation
mtodor
force-pushed
the
mtodor/ROX-33250-add-self-signed-certs
branch
from
4b190eb to
2769128
Compare
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
E2E Test ResultsCommit: 814901f |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds optional Central CA certificate support: Helm helpers and templates to supply or reference a CA secret, configmap/deployment changes to expose a CA file, a new CentralConfig CA path field, and client code to load, validate, and apply CA certs for TLS verification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
🧹 Nitpick comments (1)
internal/client/client_test.go (1)
238-245: Assert the loaded CA is actually present in the pool.These success-path tests only prove that
loadCACertPoolreturned no error and a non-nil pool. They would still pass if the PEM contents were silently ignored, so they do not fully protect the new behavior.♻️ Example assertion pattern
pool, err := loadCACertPool(path) require.NoError(t, err) require.NotNil(t, pool) +expected := x509.NewCertPool() +require.True(t, expected.AppendCertsFromPEM(certPEM)) +assert.True(t, expected.Equal(pool))That same assertion shape should work for the mixed-PEM case too, since the private key block is expected to be ignored.
Also applies to: 560-582
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client_test.go` around lines 238 - 245, The tests currently only check for non-nil pool; update TestLoadCACertPool_ValidCACert (and the mixed-PEM test) to assert the actual CA cert was added to the pool: parse the generated PEM from generateTestCACert (or re-read the file written by writeTestFile) into an x509.Certificate, then verify that the pool returned by loadCACertPool contains that certificate (e.g., by checking pool.Subjects() includes the cert.RawSubject or otherwise matching the cert's Raw bytes). Apply the same presence assertion in the mixed-PEM test where the private key block should be ignored and only the CA cert should be present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/client/client.go`:
- Around line 243-250: The tlsConfig() currently loads and validates
c.config.CACertPath even when c.config.InsecureSkipTLSVerify is true; update
tlsConfig() to check c.config.InsecureSkipTLSVerify first and skip calling
loadCACertPool or setting tlsCfg.RootCAs when InsecureSkipTLSVerify is true
(honor the config warning), while still setting tlsCfg.InsecureSkipVerify =
true; ensure loadCACertPool is only invoked when InsecureSkipTLSVerify is false
and c.config.CACertPath != "" to avoid failing connection setup due to
missing/invalid CA files.
---
Nitpick comments:
In `@internal/client/client_test.go`:
- Around line 238-245: The tests currently only check for non-nil pool; update
TestLoadCACertPool_ValidCACert (and the mixed-PEM test) to assert the actual CA
cert was added to the pool: parse the generated PEM from generateTestCACert (or
re-read the file written by writeTestFile) into an x509.Certificate, then verify
that the pool returned by loadCACertPool contains that certificate (e.g., by
checking pool.Subjects() includes the cert.RawSubject or otherwise matching the
cert's Raw bytes). Apply the same presence assertion in the mixed-PEM test where
the private key block should be ignored and only the CA cert should be present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 02d8cbab-e2ee-4748-ba24-e2372f220670
📒 Files selected for processing (9)
charts/stackrox-mcp/templates/_helpers.tplcharts/stackrox-mcp/templates/central-ca-secret.yamlcharts/stackrox-mcp/templates/configmap.yamlcharts/stackrox-mcp/templates/deployment.yamlcharts/stackrox-mcp/values.yamlinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.go
mtodor
force-pushed
the
mtodor/ROX-33250-add-self-signed-certs
branch
from
2769128 to
814901f
Compare
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 `@charts/stackrox-mcp/templates/_helpers.tpl`:
- Around line 87-93: The template stackrox-mcp.centralCASecretName can produce
names >63 chars when appending "-central-ca" to include "stackrox-mcp.fullname";
truncate the fullname portion before adding the suffix so the total DNS label
length stays <=63. Update the else branch to take include
"stackrox-mcp.fullname" . and pipe it through a truncation (e.g., sprig's trunc)
to 52 characters (63 - len("-central-ca") = 52), then append "-central-ca" so
the resulting secret name always fits Kubernetes DNS label limits.
In `@internal/client/client_test.go`:
- Around line 545-558: The loop using conn.GetState and conn.WaitForStateChange
can exit due to timeout and still satisfy assert.NotEqual, hiding regressions;
modify the loop in the test to record whether connectivity.TransientFailure was
actually observed (e.g., set a boolean observedTransientFailure when state ==
connectivity.TransientFailure) and after the loop assert that
observedTransientFailure is true (or call t.Fatalf on timeout) instead of only
asserting conn.GetState() != connectivity.Ready; reference conn.GetState,
conn.WaitForStateChange, connectivity.TransientFailure, and connectivity.Ready
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 42f20684-e724-47bc-9ba9-f8bd8f33a92a
📒 Files selected for processing (9)
charts/stackrox-mcp/templates/_helpers.tplcharts/stackrox-mcp/templates/central-ca-secret.yamlcharts/stackrox-mcp/templates/configmap.yamlcharts/stackrox-mcp/templates/deployment.yamlcharts/stackrox-mcp/values.yamlinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.go
Comment on lines
+87
to
+93
| {{- define "stackrox-mcp.centralCASecretName" -}} | ||
| {{- if .Values.centralCACert.existingSecretName }} | ||
| {{- .Values.centralCACert.existingSecretName }} | ||
| {{- else }} | ||
| {{- include "stackrox-mcp.fullname" . }}-central-ca | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Truncate CA secret name after suffixing to avoid invalid Kubernetes names.
If stackrox-mcp.fullname is already near 63 chars, adding -central-ca can exceed the DNS label limit and break installs for long release names.
🔧 Proposed fix
{{- define "stackrox-mcp.centralCASecretName" -}}
{{- if .Values.centralCACert.existingSecretName }}
{{- .Values.centralCACert.existingSecretName }}
{{- else }}
-{{- include "stackrox-mcp.fullname" . }}-central-ca
+{{- printf "%s-central-ca" (include "stackrox-mcp.fullname" .) | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}- name: {{ include "stackrox-mcp.fullname" . }}-central-ca
+ name: {{ include "stackrox-mcp.centralCASecretName" . }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- define "stackrox-mcp.centralCASecretName" -}} | |
| {{- if .Values.centralCACert.existingSecretName }} | |
| {{- .Values.centralCACert.existingSecretName }} | |
| {{- else }} | |
| {{- include "stackrox-mcp.fullname" . }}-central-ca | |
| {{- end }} | |
| {{- end }} | |
| {{- define "stackrox-mcp.centralCASecretName" -}} | |
| {{- if .Values.centralCACert.existingSecretName }} | |
| {{- .Values.centralCACert.existingSecretName }} | |
| {{- else }} | |
| {{- printf "%s-central-ca" (include "stackrox-mcp.fullname" .) | trunc 63 | trimSuffix "-" }} | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/stackrox-mcp/templates/_helpers.tpl` around lines 87 - 93, The
template stackrox-mcp.centralCASecretName can produce names >63 chars when
appending "-central-ca" to include "stackrox-mcp.fullname"; truncate the
fullname portion before adding the suffix so the total DNS label length stays
<=63. Update the else branch to take include "stackrox-mcp.fullname" . and pipe
it through a truncation (e.g., sprig's trunc) to 52 characters (63 -
len("-central-ca") = 52), then append "-central-ca" so the resulting secret name
always fits Kubernetes DNS label limits.
Comment on lines
+545
to
+558
| for { | ||
| state := conn.GetState() | ||
| if state == connectivity.TransientFailure { | ||
| break | ||
| } | ||
|
|
||
| if !conn.WaitForStateChange(ctx, state) { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| assert.NotEqual(t, connectivity.Ready, conn.GetState(), | ||
| "connection must not reach Ready state without CA cert for self-signed server") | ||
| } |
There was a problem hiding this comment.
Negative TLS test can pass without proving handshake rejection
Line 551 breaks on timeout and still passes with NotEqual(Ready), which can hide regressions (e.g., connection never actually attempted/failed). The test should assert it observed TransientFailure (or explicitly fail on timeout) to validate the rejection path.
Suggested fix
- for {
+ reachedTransientFailure := false
+ for {
state := conn.GetState()
if state == connectivity.TransientFailure {
+ reachedTransientFailure = true
break
}
if !conn.WaitForStateChange(ctx, state) {
- break
+ t.Fatalf("timeout waiting for TransientFailure; last state: %s", state)
}
}
- assert.NotEqual(t, connectivity.Ready, conn.GetState(),
- "connection must not reach Ready state without CA cert for self-signed server")
+ assert.True(t, reachedTransientFailure,
+ "expected TransientFailure without CA cert for self-signed server")
+ assert.NotEqual(t, connectivity.Ready, conn.GetState(),
+ "connection must not reach Ready state without CA cert for self-signed server")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/client_test.go` around lines 545 - 558, The loop using
conn.GetState and conn.WaitForStateChange can exit due to timeout and still
satisfy assert.NotEqual, hiding regressions; modify the loop in the test to
record whether connectivity.TransientFailure was actually observed (e.g., set a
boolean observedTransientFailure when state == connectivity.TransientFailure)
and after the loop assert that observedTransientFailure is true (or call
t.Fatalf on timeout) instead of only asserting conn.GetState() !=
connectivity.Ready; reference conn.GetState, conn.WaitForStateChange,
connectivity.TransientFailure, and connectivity.Ready when making the change.
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
2 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.
Description
This PR is adding support for self-signed certificates. In some cases customers are using own certificates and this feature will support certificate validation. Otherwise it would be required to use insecure TLS.
Validation
AI-assisted development
In this case different environment was used and different organization of work.