Skip to content

Drop roxctl dependency [mc]#238

Open
mclasmeier wants to merge 11 commits into
mainfrom
mc/drop-roxctl-dependency
Open

Drop roxctl dependency [mc]#238
mclasmeier wants to merge 11 commits into
mainfrom
mc/drop-roxctl-dependency

Conversation

@mclasmeier

@mclasmeier mclasmeier commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Modified version of #234.

Summary by CodeRabbit

  • New Features

    • CRS generation now connects directly to Central through its REST API.
    • Added support for configured CA certificates and improved TLS verification.
  • Improvements

    • Deployments no longer require the roxctl CLI; only kubectl is required.
    • Updated the Quick start prerequisites to reference the roxie CLI.
  • Bug Fixes

    • Added retry handling for transient Central API errors.
    • Added validation to detect empty CRS responses.

porridge and others added 7 commits July 9, 2026 09:51
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@mclasmeier, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Enterprise

Run ID: b284be39-f6df-4599-a102-792934beee4a

📥 Commits

Reviewing files that changed from the base of the PR and between f6d198c and 170535c.

📒 Files selected for processing (1)
  • internal/deployer/crs.go
📝 Walkthrough

Walkthrough

CRS generation now uses Central’s HTTPS REST API with retries and custom TLS verification instead of roxctl. The deployer no longer requires roxctl, and workflow and README prerequisites now reference roxie. github.com/pkg/errors is a direct dependency.

Changes

Central CRS migration

Layer / File(s) Summary
Central API client and TLS verification
internal/deployer/crs.go, go.mod
Adds Central CRS request/response handling, configurable CA loading, custom certificate verification, and the direct github.com/pkg/errors requirement.
CRS generation and retry flow
internal/deployer/crs.go
Replaces roxctl CRS execution with authenticated Central API requests, response validation, and retries for selected transient errors.
roxctl removal and deployment prerequisites
internal/deployer/deployer.go, internal/deployer/roxctl.go, .github/workflows/e2e-tests.yml, README.md
Removes roxctl execution support, leaves kubectl as the required tool, removes workflow installation, and updates quick-start prerequisites to roxie.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Deployer
  participant CentralHTTPClient
  participant Central
  participant TLSVerifier
  Deployer->>CentralHTTPClient: Create configured client
  Deployer->>Central: POST /v1/cluster-init/crs
  Central->>TLSVerifier: Verify Central certificate
  TLSVerifier-->>Central: Accept configured or Central service CA
  Central-->>Deployer: Return CRS JSON
  Deployer->>Deployer: Trim, validate, or retry CRS generation
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing the roxctl dependency in favor of direct Central calls.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mc/drop-roxctl-dependency

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
internal/deployer/crs.go (1)

97-97: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Optional: set an explicit TLS MinVersion.

The tls.Config omits MinVersion; while the client default is TLS 1.2, pinning MinVersion: tls.VersionTLS12 (or 1.3) makes the security posture explicit.

🤖 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 97, Explicitly set the minimum TLS version
when constructing the tls.Config in the relevant client setup, using MinVersion:
tls.VersionTLS12 (or tls.VersionTLS13 if supported), rather than relying on the
default.

Source: Linters/SAST tools

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

Inline comments:
In `@go.mod`:
- Line 11: Mark github.com/pkg/errors v0.9.1 as an indirect dependency in
go.mod, preserving the existing dependency classification since no Go source
imports it directly.

In `@internal/deployer/crs.go`:
- Around line 79-92: Update the retry/error handling in the CRS generation loop
so retryable errors on the final attempt are recorded and exit the loop instead
of being returned as non-retryable. Use the existing lastErr handling and final
“failed after attempts” return, while preserving immediate returns for genuinely
non-retryable errors; adjust the logic around the retry condition in the CRS
generation method.
- Around line 116-117: Update centralVerifyFunc and its call site in the TLS
setup to accept an explicit hostname argument, passing the request host with its
port removed; use that argument for certificate DNSName validation instead of
reading conf.ServerName from the captured tls.Config.

---

Nitpick comments:
In `@internal/deployer/crs.go`:
- Line 97: Explicitly set the minimum TLS version when constructing the
tls.Config in the relevant client setup, using MinVersion: tls.VersionTLS12 (or
tls.VersionTLS13 if supported), rather than relying on the default.
🪄 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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Enterprise

Run ID: 972615fb-6a05-44b8-9f2e-4c85014d9ac9

📥 Commits

Reviewing files that changed from the base of the PR and between 30a0b90 and f6d198c.

📒 Files selected for processing (6)
  • .github/workflows/e2e-tests.yml
  • README.md
  • go.mod
  • internal/deployer/crs.go
  • internal/deployer/deployer.go
  • internal/deployer/roxctl.go
💤 Files with no reviewable changes (3)
  • internal/deployer/roxctl.go
  • README.md
  • .github/workflows/e2e-tests.yml

Comment thread go.mod Outdated
Comment thread internal/deployer/crs.go
Comment thread internal/deployer/crs.go Outdated
@mclasmeier mclasmeier force-pushed the mc/drop-roxctl-dependency branch from b7105dd to 170535c Compare July 10, 2026 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants