Drop roxctl dependency, use Central REST API for CRS#234
Open
porridge wants to merge 7 commits into
Open
Conversation
Replace the roxctl subprocess call for generating Cluster Registration Secrets with a direct HTTP POST to Central's REST API endpoint (/v1/cluster-init/crs). This eliminates the requirement for users to install a matching roxctl binary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/deployer/crs.go (2)
136-136: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet an explicit
MinVersionon the TLS config.Consider pinning
MinVersion: tls.VersionTLS12(orTLS13) rather than relying on the implicit default, to make the security posture explicit and future-proof.🔒 Proposed change
- tlsConfig := &tls.Config{} + tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/crs.go` at line 136, The TLS configuration in crs.go is created with the default settings, so make the security baseline explicit by setting MinVersion on the tls.Config used in the deployment flow. Update the tlsConfig initialization in the relevant CRS deployment path to pin a minimum version such as tls.VersionTLS12 (or TLS13 if supported), keeping the change localized to the tlsConfig setup so the behavior is clear and future-proof.Source: Linters/SAST tools
143-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
CertPool.AppendCertsFromPEMand guard against an empty pool.The manual
pem.Decode/ParseCertificateloop will error out on any non-CERTIFICATEPEM block, and if the file yields no certificates it silently produces an empty pool that is then set asRootCAs, causing all TLS verification to fail rather than falling back.AppendCertsFromPEMhandles mixed/extra blocks and lets you detect the empty case.♻️ Proposed refactor
if d.roxCACertFile != "" { pemData, err := os.ReadFile(d.roxCACertFile) if err != nil { return nil, fmt.Errorf("reading CA cert file %s: %w", d.roxCACertFile, err) } pool := x509.NewCertPool() - for block, rest := pem.Decode(pemData); block != nil; block, rest = pem.Decode(rest) { - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, fmt.Errorf("parsing CA certificate from %s: %w", d.roxCACertFile, err) - } - pool.AddCert(cert) - } + if !pool.AppendCertsFromPEM(pemData) { + return nil, fmt.Errorf("no valid certificates found in CA cert file %s", d.roxCACertFile) + } tlsConfig.RootCAs = pool }If you drop the manual loop, the
crypto/x509andencoding/pemimports may need adjustment (x509is still used forNewCertPool).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/crs.go` around lines 143 - 151, Replace the manual PEM parsing in the CA certificate setup with x509.CertPool.AppendCertsFromPEM in the certificate-loading path inside the deployer logic. In the code that builds tlsConfig.RootCAs, load the PEM data into a new CertPool, append all certs in one pass, and explicitly handle the case where no certificates were added so you don’t assign an empty RootCAs pool. Keep the existing x509.NewCertPool usage as needed and remove any now-unused pem handling in internal/deployer/crs.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/deployer/crs.go`:
- Line 136: The TLS configuration in crs.go is created with the default
settings, so make the security baseline explicit by setting MinVersion on the
tls.Config used in the deployment flow. Update the tlsConfig initialization in
the relevant CRS deployment path to pin a minimum version such as
tls.VersionTLS12 (or TLS13 if supported), keeping the change localized to the
tlsConfig setup so the behavior is clear and future-proof.
- Around line 143-151: Replace the manual PEM parsing in the CA certificate
setup with x509.CertPool.AppendCertsFromPEM in the certificate-loading path
inside the deployer logic. In the code that builds tlsConfig.RootCAs, load the
PEM data into a new CertPool, append all certs in one pass, and explicitly
handle the case where no certificates were added so you don’t assign an empty
RootCAs pool. Keep the existing x509.NewCertPool usage as needed and remove any
now-unused pem handling in internal/deployer/crs.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e908d08-6f1d-43cc-84ee-1fe4cb942595
📒 Files selected for processing (5)
.github/workflows/e2e-tests.ymlREADME.mdinternal/deployer/crs.gointernal/deployer/deployer.gointernal/deployer/roxctl.go
💤 Files with no reviewable changes (3)
- README.md
- .github/workflows/e2e-tests.yml
- internal/deployer/roxctl.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/errorhelpers/format.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo: missing space in package comment.
✏️ Proposed fix
-// Package errorhelpers was copied fromhttps://github.com/stackrox/stackrox/tree/master/pkg/errorhelpers +// Package errorhelpers was copied from https://github.com/stackrox/stackrox/tree/master/pkg/errorhelpers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/errorhelpers/format.go` at line 1, The package comment for errorhelpers has a typo because it is missing a space in the copied source reference. Update the comment at the top of the file so the text is properly spaced and reads naturally, keeping the package comment aligned with the errorhelpers package declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/errorhelpers/format.go`:
- Line 1: The package comment for errorhelpers has a typo because it is missing
a space in the copied source reference. Update the comment at the top of the
file so the text is properly spaced and reads naturally, keeping the package
comment aligned with the errorhelpers package declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 2165fa01-34ec-41a2-bef2-e0a384e9f4c4
📒 Files selected for processing (3)
go.modinternal/deployer/crs.gointernal/errorhelpers/format.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/deployer/crs.go
Contributor
Author
|
@mclasmeier one question I have: should we keep |
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
1 participant
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
roxctlsubprocess call for CRS generation with a direct HTTP POST to Central's REST API (/v1/cluster-init/crs), using Basic auth and proper TLS (CA cert when available, system pool otherwise)internal/deployer/roxctl.go(now unused)roxctlfrom required tools check, CI workflow, and README prerequisitesThe custom TLS code is needed to prevent errors such as:
Closes #160
Resolves: ROX-35666
🤖 Partially Generated with Claude Code
Tested in CI:
Also tested manually with the CA cert from wrong central:
Summary by CodeRabbit
New Features
roxieCLI and direct service API calls.Bug Fixes
kubectlduring setup.Documentation
roxieCLI requirement.