CAIB: fetch server from jumpstarter#136
Conversation
evakhoni
changed the title
evakhoni
changed the title
📝 WalkthroughWalkthroughAdds Jumpstarter-based server discovery as a fallback for resolving the default CAIB server; exposes JumpstarterEndpoint, DeriveServerFromJumpstarter, and DefaultServerWithDerive; updates many CLI commands to use the new resolver, updates login semantics to allow no-arg derivation, and adds tests for derivation and health checks. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Env as Environment
participant Store as SavedConfig
participant FS as Filesystem
participant Jump as JumpstarterConfig
participant API as BuildAPI
User->>CLI: invoke command (e.g., run, login, build)
CLI->>Env: query CAIB_SERVER
alt CAIB_SERVER set
Env-->>CLI: return env URL
else
CLI->>Store: Read saved server URL
alt saved present
Store-->>CLI: return saved URL
else
CLI->>FS: read ~/.config/jumpstarter/config.yaml
FS-->>Jump: load client alias & client.yaml endpoint
Jump->>API: derive Build API URL (namespace/route)
API-->>Jump: HTTP GET /v1/healthz (insecure TLS client, timeout)
alt 200 OK
Jump->>Store: Save derived Build API URL
Store-->>CLI: return derived URL
else
API-->>CLI: health check failed -> return empty
end
end
end
CLI-->>User: proceed or return error suggesting caib/jmp login
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 113-122: The two fmt.Printf calls that emit server-resolution
diagnostics (the "could not reach derived Build API server" and "Using Build API
server derived..." messages) should write to stderr instead of stdout to avoid
corrupting machine-readable output; replace fmt.Printf(...) with
fmt.Fprintf(os.Stderr, ...) and import/use os as needed. Keep the current
behavior of persisting via SaveServerURL(apiURL) and the warning that follows
(also write that warning to stderr), and ensure resp.Body.Close() remains
unchanged.
- Around line 111-123: The health-check logic using httpClient.Get(apiURL +
"/v1/healthz") treats any HTTP response as success and ignores resp.Body.Close
errors while printing diagnostics to stdout; update the code that calls
httpClient.Get and inspects resp to check resp.StatusCode and only proceed when
it's in the 2xx range, handle or log errors returned by resp.Body.Close, and
replace fmt.Printf diagnostic messages with fmt.Fprintf(os.Stderr, ...) for
warnings and errors (including the SaveServerURL(apiURL) failure message),
ensuring you only call SaveServerURL(apiURL) and return apiURL when the status
code indicates success.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/caib/config/config.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/main.go
evakhoni
force-pushed
the
login-from-jumpstarter
branch
from
c22656f to
565df38
Compare
|
general approach is fine, this is what i discussed with bella |
|
Looks good from my side. The only thing I’m wondering about is the case where we’re using multiple environments and different Jumpstarter clients. Once we switch to a new client, the server won’t be refreshed automatically. Maybe we can add a flag like |
i'd rather not pile on this, it is more likely they switch to an instance where the operator doesn't exist |
yep, ideally if/once we'll have an option to discover the build api endpoint cluster-side, and add it to jumpstarter config for caib to read it directly, this would be nice and easy as making caib check for default jmp client, either by default or on demand by triggering caib login. generally I like the idea of both being sync (unless we have a valid use case where we definitely don't want them to 🤔) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/caib/config/config.go (1)
117-124:⚠️ Potential issue | 🟠 MajorHealth-check handling is still too narrow (repeated finding).
Line 122 still ignores
Close()errors, and Line 123 only accepts200 OKrather than any2xx. This can both mask cleanup errors and reject otherwise healthy endpoints.🔧 Proposed fix
resp, err := httpClient.Get(apiURL + "/v1/healthz") if err != nil { fmt.Fprintf(os.Stderr, "Warning: Jumpstarter config found, but could not reach derived Build API server %s.\n", apiURL) return "" } - _ = resp.Body.Close() - if resp.StatusCode != http.StatusOK { + defer func() { + if cerr := resp.Body.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to close healthz response body: %v\n", cerr) + } + }() + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { fmt.Fprintf(os.Stderr, "Warning: Jumpstarter config found, but derived Build API server %s returned HTTP %d.\n", apiURL, resp.StatusCode) return "" }#!/bin/bash set -euo pipefail # Inspect current health-check implementation. cat -n cmd/caib/config/config.go | sed -n '115,126p' # Inspect errcheck settings to see whether blank identifier ignores are treated as failures. fd -HI 'golangci*.yml' 'golangci*.yaml' '.golangci.yml' '.golangci.yaml' | while read -r f; do echo "== $f ==" rg -n 'errcheck|blank' "$f" || true done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 117 - 124, The health-check treats non-200 codes as failures and ignores Close() errors; change the logic so you accept any 2xx as healthy and handle Close() errors instead of discarding them. Specifically, in the block that calls httpClient.Get(apiURL + "/v1/healthz") replace the blank-assignment _ = resp.Body.Close() with a proper check (e.g., if cerr := resp.Body.Close(); cerr != nil { fmt.Fprintf(os.Stderr, "Warning: ... close error: %v\n", cerr) }) and change the status check from resp.StatusCode != http.StatusOK to something like if resp.StatusCode/100 != 2 { fmt.Fprintf(os.Stderr, "Warning: Jumpstarter config found, but derived Build API server %s returned HTTP %d.\n", apiURL, resp.StatusCode); return "" } so any 2xx succeeds; keep references to httpClient.Get, resp.Body.Close, and resp.StatusCode when making these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 98-112: The host extraction logic using strings.LastIndex on
grpcEndpoint is brittle; change the parsing to first parse grpcEndpoint with
url.Parse (to strip scheme), then use net.SplitHostPort on the parsed URL's Host
to separate host and port (handle missing port by checking error and using the
Host as-is), and finally strip IPv6 brackets via strings.Trim(host, "[]") before
applying the existing baseDomain logic that checks strings.Index(host, ".apps.")
and the fallback SplitN; update the code around the grpcEndpoint/host variable
where host is set to perform these steps so IPv6, scheme-prefixed URLs, and
explicit ports are handled correctly.
---
Duplicate comments:
In `@cmd/caib/config/config.go`:
- Around line 117-124: The health-check treats non-200 codes as failures and
ignores Close() errors; change the logic so you accept any 2xx as healthy and
handle Close() errors instead of discarding them. Specifically, in the block
that calls httpClient.Get(apiURL + "/v1/healthz") replace the blank-assignment _
= resp.Body.Close() with a proper check (e.g., if cerr := resp.Body.Close();
cerr != nil { fmt.Fprintf(os.Stderr, "Warning: ... close error: %v\n", cerr) })
and change the status check from resp.StatusCode != http.StatusOK to something
like if resp.StatusCode/100 != 2 { fmt.Fprintf(os.Stderr, "Warning: Jumpstarter
config found, but derived Build API server %s returned HTTP %d.\n", apiURL,
resp.StatusCode); return "" } so any 2xx succeeds; keep references to
httpClient.Get, resp.Body.Close, and resp.StatusCode when making these edits.
48598bd to
f130f30
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/caib/config/config.go (2)
124-129: Consider adding TLS MinVersion for defense in depth.While
InsecureSkipVerify: trueis intentional for this connectivity probe (and appropriately marked withnolint:gosec), addingMinVersion: tls.VersionTLS13provides defense in depth. Even when certificate validation is skipped, enforcing modern TLS prevents protocol downgrade attacks that could expose probe traffic.Suggested change
httpClient := &http.Client{ //nolint:gosec Timeout: 5 * time.Second, Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS13, + }, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 124 - 129, The TLS config for the probe uses InsecureSkipVerify in the httpClient's Transport (tls.Config inside http.Transport) but lacks a minimum TLS version; update the tls.Config used in the Transport (where httpClient is created) to include MinVersion: tls.VersionTLS13 to enforce modern TLS (defense-in-depth) while keeping InsecureSkipVerify and the existing nolint:gosec annotations.
22-23: Consider making namespace configurable via environment variable.The hardcoded
buildAPINamespacewill fail for non-default deployments. While the TODO acknowledges this and the health check provides runtime validation, an optional environment variable override (e.g.,CAIB_BUILD_API_NAMESPACE) would give operators flexibility without requiring code changes.Suggested approach
const ( configDir = ".caib" configFile = "cli.json" buildAPIRoutePrefix = "ado-build-api" - buildAPINamespace = "automotive-dev-operator-system" // todo: add dynamic namespace discovery + defaultBuildAPINamespace = "automotive-dev-operator-system" // todo: add dynamic namespace discovery ) + +func getBuildAPINamespace() string { + if ns := os.Getenv("CAIB_BUILD_API_NAMESPACE"); ns != "" { + return ns + } + return defaultBuildAPINamespace +}Then use
getBuildAPINamespace()in the URL construction at line 119.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 22 - 23, Replace the hardcoded buildAPINamespace with a configurable getter: add a getBuildAPINamespace() function that returns the value of the CAIB_BUILD_API_NAMESPACE environment variable if set, otherwise falls back to the existing buildAPINamespace default; update any code that directly references buildAPINamespace (notably the URL construction currently using buildAPINamespace) to call getBuildAPINamespace() so deployments can override the namespace via env var without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/caib/config/config.go`:
- Around line 124-129: The TLS config for the probe uses InsecureSkipVerify in
the httpClient's Transport (tls.Config inside http.Transport) but lacks a
minimum TLS version; update the tls.Config used in the Transport (where
httpClient is created) to include MinVersion: tls.VersionTLS13 to enforce modern
TLS (defense-in-depth) while keeping InsecureSkipVerify and the existing
nolint:gosec annotations.
- Around line 22-23: Replace the hardcoded buildAPINamespace with a configurable
getter: add a getBuildAPINamespace() function that returns the value of the
CAIB_BUILD_API_NAMESPACE environment variable if set, otherwise falls back to
the existing buildAPINamespace default; update any code that directly references
buildAPINamespace (notably the URL construction currently using
buildAPINamespace) to call getBuildAPINamespace() so deployments can override
the namespace via env var without code changes.
8caf47e to
21c2a25
Compare
|
thanks @bkhizgiy |
|
@mangelajo I have added test cases, this one is ready for merging. |
| // Skip derivation at startup when the user is explicitly running 'login', | ||
| // so that 'caib login' without arguments always performs a fresh derivation from the current Jumpstarter config. | ||
| var defaultServer string | ||
| if len(os.Args) < 2 || os.Args[1] != "login" { |
There was a problem hiding this comment.
would this override CAIB_SERVER?
There was a problem hiding this comment.
it will attempt to fetch from jumpstarter config, skipping the env var, and this is intentional. in this case, caib login with no parameters is used to sync caib with the currently selected jumpstarter user. it will attempt to reach the reconstructed endpoint, and if it is able to - it will save it to cli.json similarly to how caib login <url> will save it. if the endpoint is not reachable - it will just print a warning and do nothing to the config file.
with that said, if you have the env set, later on with other caib commands it will choose the env instead of the config, as usual. i did not changed this behavior.
There was a problem hiding this comment.
i think CAIB_SERVER should take precedence
There was a problem hiding this comment.
you mean just using caib login with no params should save CAIB_SERVER into cli.json if available, instead of fetching from jumpstarter? I can do that 🤔
There was a problem hiding this comment.
side note, just seen there's a large changes that created a merge conflict for me, so I'll have to rebase it, this will move things around a bit..
There was a problem hiding this comment.
Hey @bennyz! finally rebased the whole thing, with a few minor changes, and it's ready for a final review.
about CAIB_SERVER precedence, on a second thought this is probably not a good idea in this specific case. first, the use case here is not clear: if the env is set, it will already take precedence over the config file anyway, so we're not gaining anything by also forcing the env into the config file. sounds like an unintended side effect to me, it will cause the env to stick after it been unset.
also, it looks misleading to the user: the command supposed to allow the user to switch the caib server to match the jumpstarter environment, but if in certain cases it will follow jumpstarter, but in other cases it will cause the env to be written to persistent config, without letting the user to choice, seems like an inconsistency and bad ux to me. if a user really wants to write the env to config file, one can simply run caib login $CAIB_SERVER
There was a problem hiding this comment.
I'm not sure I am following. If CAIB_SERVER is set it should be used, we should not attempt to use anything from jumpstarter in that case, the jumpstarter login is a convenience heuristic when nothing is set
I did not re-read yet, if that's the case then it's good
There was a problem hiding this comment.
yeah yeah, if CAIB_SERVER is set it will use it, as usual. this one is only for writing to config file.
it's the same as it always been with caib login <url>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/main.go`:
- Around line 357-362: The code computes a defaultServer variable to avoid
triggering derivation during 'caib login' but later still calls
config.DefaultServer() directly; replace those direct calls with the precomputed
defaultServer so sealed/derivation-sensitive flows use the same value. Locate
usages of config.DefaultServer() in the sealed-command path (the calls
referenced around the code that interacts with defaultServer, e.g., where server
default is passed into command setup or derivation logic) and change them to use
the defaultServer variable (ensuring defaultServer is in scope where used); keep
the existing skip-on-login check and ensure no new calls to
config.DefaultServer() remain in that code path.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/config/suite_test.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/caib/container/container.go
evakhoni
force-pushed
the
login-from-jumpstarter
branch
from
398f280 to
0d7ee1c
Compare
|
@coderabbitai I just pushed some changes, ptal |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/caib/login.go (1)
18-23: Avoid double persistence in the derive-login path.
config.DeriveServerFromJumpstarter()already persists the URL, and thenrunLoginpersists again at Line 62. This is redundant and can report a false failure if only the second write fails.♻️ Suggested adjustment
func runLogin(_ *cobra.Command, args []string) { var server string + shouldSave := true if len(args) == 0 { server = config.DeriveServerFromJumpstarter() if server == "" { handleError(fmt.Errorf("no Jumpstarter config found or derived endpoint unreachable; provide a server URL explicitly")) return } + shouldSave = false // already persisted by DeriveServerFromJumpstarter } else { server = strings.TrimSpace(args[0]) if server == "" { handleError(fmt.Errorf("server URL is required")) return } @@ - if err := config.SaveServerURL(server); err != nil { - handleError(fmt.Errorf("failed to save server URL: %w", err)) - return - } + if shouldSave { + if err := config.SaveServerURL(server); err != nil { + handleError(fmt.Errorf("failed to save server URL: %w", err)) + return + } + } fmt.Printf("Server saved: %s\n", server)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` around lines 18 - 23, The derived server URL is already persisted by config.DeriveServerFromJumpstarter(), so avoid writing it again in runLogin to prevent duplicate persistence and false failure reports; update the call/site that invokes runLogin when args are empty to pass a mode/flag or use a runLogin variant that does not persist, or modify runLogin to accept a boolean like skipPersist (and respect it) so that when you call runLogin after config.DeriveServerFromJumpstarter() you set skipPersist=true; ensure references to config.DeriveServerFromJumpstarter and runLogin are updated accordingly and that only one persistence path performs the save.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 84-90: Validate and sanitize the userCfg.Config.CurrentClient
alias before using it in filepath.Join: ensure the alias variable contains only
an allowed token (e.g., alphanumeric, hyphen, underscore), reject or return an
error if it contains path separators, ".." or absolute paths, or verify alias ==
filepath.Base(alias) and does not include os.PathSeparator; only after
validation use filepath.Join(jmpDir, "clients", alias+".yaml") and call
os.ReadFile. This protects the os.ReadFile call from directory traversal and
ensures the alias cannot escape the clients directory.
In `@cmd/caib/container/logs.go`:
- Around line 78-80: The branch that errors when strings.TrimSpace(serverURL) ==
"" should first attempt to obtain a derived default via
config.DefaultServerWithDerive() before calling handleError; modify the logic
around that check (the block that currently calls handleError on empty
serverURL) to call config.DefaultServerWithDerive(), use its returned value when
non-empty, and only call handleError(fmt.Errorf(...)) if the derived value is
also empty — update references to serverURL accordingly so subsequent code uses
the derived value; this change touches the empty-check logic and the call site
that currently invokes handleError.
In `@cmd/caib/image/image.go`:
- Around line 83-87: The PersistentPreRunE hook is auto-populating
opts.ServerURL (using config.DefaultServerWithDerive()) for all image
subcommands which forces sealed commands to always run remotely; restrict server
URL derivation to only the commands that actually require a server (do not
mutate opts.ServerURL in the global PersistentPreRunE). Move the derivation
logic out of PersistentPreRunE and into the specific subcommands that need it
(or add a whitelist check in the hook using the cobra command name), so that
sealed handlers (sealedcmd functions that call sealedRunViaAPI(), e.g.,
prepare-reseal, reseal, extract-for-signing, inject-signed) see a nil/empty
opts.ServerURL when the user intends local behavior and can thus avoid
unconditional sealedRunViaAPI().
---
Nitpick comments:
In `@cmd/caib/login.go`:
- Around line 18-23: The derived server URL is already persisted by
config.DeriveServerFromJumpstarter(), so avoid writing it again in runLogin to
prevent duplicate persistence and false failure reports; update the call/site
that invokes runLogin when args are empty to pass a mode/flag or use a runLogin
variant that does not persist, or modify runLogin to accept a boolean like
skipPersist (and respect it) so that when you call runLogin after
config.DeriveServerFromJumpstarter() you set skipPersist=true; ensure references
to config.DeriveServerFromJumpstarter and runLogin are updated accordingly and
that only one persistence path performs the save.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c15b735-2a3f-455e-83f5-646d9542adac
📒 Files selected for processing (19)
cmd/caib/authcmd/authcmd.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/logs.gocmd/caib/catalog/add.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/caib/catalog/publish.gocmd/caib/catalog/remove.gocmd/caib/catalog/verify.gocmd/caib/config/config.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/login.gocmd/caib/querycmd/query.gocmd/caib/root.go
✅ Files skipped from review due to trivial changes (1)
- cmd/caib/downloadcmd/download.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/caib/container/build.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/caib/image/image.go (1)
83-87:⚠️ Potential issue | 🟠 MajorGlobal server derivation breaks sealed commands’ local-by-default contract.
At Line 83-Line 86,
PersistentPreRunEauto-fillsopts.ServerURLfor allimagesubcommands. That conflicts with sealed command behavior documented at Line 437-Line 439 and Line 456-Line 457 (“with--serverrun on cluster; otherwise local”). Please scope derivation to server-required subcommands only, and leave sealed commands untouched when--serveris omitted.Suggested fix
- PersistentPreRunE: func(_ *cobra.Command, _ []string) error { + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { if opts.ServerURL != nil && strings.TrimSpace(*opts.ServerURL) == "" { - *opts.ServerURL = config.DefaultServerWithDerive() + switch cmd.Name() { + case "prepare-reseal", "reseal", "extract-for-signing", "inject-signed": + // Preserve local-by-default sealed workflows. + default: + *opts.ServerURL = config.DefaultServerWithDerive() + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/image/image.go` around lines 83 - 87, The PersistentPreRunE is currently auto-deriving opts.ServerURL for every image subcommand; limit that behavior by moving the config.DefaultServerWithDerive logic out of the global PersistentPreRunE and into only those subcommands that require a server (i.e., the non-sealed commands), so sealed/local-by-default commands do not get a derived server when --server is omitted; locate the PersistentPreRunE in image.go and change it to only set opts.ServerURL when the current command is one of the server-required commands (or add per-command PreRunE handlers for commands that require server), using the same config.DefaultServerWithDerive() call and preserving the existing nil/empty checks on opts.ServerURL.
🧹 Nitpick comments (3)
cmd/caib/container/logs.go (1)
79-80: Remove unreachablereturnafterhandleError.At Line 80, the
returnis dead code becausehandleErrorincmd/caib/container/utilities.goterminates the process withos.Exit(1).♻️ Proposed cleanup
if strings.TrimSpace(serverURL) == "" { handleError(fmt.Errorf("server URL required (use --server, CAIB_SERVER, run 'caib login <server-url>' or 'jmp login <endpoint>')")) - return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/container/logs.go` around lines 79 - 80, The return after calling handleError is unreachable because handleError (in cmd/caib/container/utilities.go) exits the process; remove the dead `return` statement following the handleError(fmt.Errorf(...)) call in logs.go to clean up unreachable code and rely on handleError to terminate execution.cmd/caib/authcmd/authcmd.go (1)
128-134: Update error message for consistency with other commands.The error message at line 132 only suggests
'caib login <server-url>', while other commands updated in this PR also mention'jmp login <endpoint>'as an alternative. For a consistent user experience, consider updating this message.♻️ Proposed fix
serverURL := config.DefaultServerWithDerive() if serverURL == "" { fmt.Println(color.RedString("No server configured.")) - fmt.Println("Run 'caib login <server-url>' first.") + fmt.Println("Run 'caib login <server-url>' or 'jmp login <endpoint>' first.") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/authcmd/authcmd.go` around lines 128 - 134, The "No server configured." message in runRefresh should be updated to match other commands by suggesting both login commands; in the runRefresh function update the second printed line (currently "Run 'caib login <server-url>' first.") to include the alternative "jmp login <endpoint>" (e.g., "Run 'caib login <server-url>' or 'jmp login <endpoint>' first.") so the error text is consistent with other command messages.cmd/caib/config/config.go (1)
139-144: Consider addingMinVersionto the TLS configuration.While
InsecureSkipVerify: trueis intentional for this connectivity probe, setting a minimum TLS version is still a good security practice to prevent protocol downgrade attacks during the handshake.🔒 Proposed improvement
client = &http.Client{ //nolint:gosec Timeout: 5 * time.Second, Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, + }, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 139 - 144, The TLS config for the probe sets InsecureSkipVerify but omits a minimum TLS version; update the tls.Config used in the http.Client transport (where client is created and Transport is &http.Transport with TLSClientConfig) to include a MinVersion (e.g., tls.VersionTLS12 or tls.VersionTLS13) to prevent protocol downgrades—add the MinVersion field to the tls.Config initialization alongside InsecureSkipVerify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/caib/image/image.go`:
- Around line 83-87: The PersistentPreRunE is currently auto-deriving
opts.ServerURL for every image subcommand; limit that behavior by moving the
config.DefaultServerWithDerive logic out of the global PersistentPreRunE and
into only those subcommands that require a server (i.e., the non-sealed
commands), so sealed/local-by-default commands do not get a derived server when
--server is omitted; locate the PersistentPreRunE in image.go and change it to
only set opts.ServerURL when the current command is one of the server-required
commands (or add per-command PreRunE handlers for commands that require server),
using the same config.DefaultServerWithDerive() call and preserving the existing
nil/empty checks on opts.ServerURL.
---
Nitpick comments:
In `@cmd/caib/authcmd/authcmd.go`:
- Around line 128-134: The "No server configured." message in runRefresh should
be updated to match other commands by suggesting both login commands; in the
runRefresh function update the second printed line (currently "Run 'caib login
<server-url>' first.") to include the alternative "jmp login <endpoint>" (e.g.,
"Run 'caib login <server-url>' or 'jmp login <endpoint>' first.") so the error
text is consistent with other command messages.
In `@cmd/caib/config/config.go`:
- Around line 139-144: The TLS config for the probe sets InsecureSkipVerify but
omits a minimum TLS version; update the tls.Config used in the http.Client
transport (where client is created and Transport is &http.Transport with
TLSClientConfig) to include a MinVersion (e.g., tls.VersionTLS12 or
tls.VersionTLS13) to prevent protocol downgrades—add the MinVersion field to the
tls.Config initialization alongside InsecureSkipVerify.
In `@cmd/caib/container/logs.go`:
- Around line 79-80: The return after calling handleError is unreachable because
handleError (in cmd/caib/container/utilities.go) exits the process; remove the
dead `return` statement following the handleError(fmt.Errorf(...)) call in
logs.go to clean up unreachable code and rely on handleError to terminate
execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1aa37c6-ea56-4038-8d9a-9e5fa2429e85
📒 Files selected for processing (19)
cmd/caib/authcmd/authcmd.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/logs.gocmd/caib/catalog/add.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/caib/catalog/publish.gocmd/caib/catalog/remove.gocmd/caib/catalog/verify.gocmd/caib/config/config.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/login.gocmd/caib/querycmd/query.gocmd/caib/root.go
evakhoni
force-pushed
the
login-from-jumpstarter
branch
from
0d7ee1c to
bd09e1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/caib/login.go (1)
19-23: Avoid double-saving the server URL in the no-arg login path.Line 19 already uses
DeriveServerFromJumpstarter(), which persists the URL. Line 62 persists again unconditionally. Consider keeping persistence in one place to avoid redundant writes.Refactor option
- if err := config.SaveServerURL(server); err != nil { - handleError(fmt.Errorf("failed to save server URL: %w", err)) - return - } + if len(args) > 0 { + if err := config.SaveServerURL(server); err != nil { + handleError(fmt.Errorf("failed to save server URL: %w", err)) + return + } + }Also applies to: 62-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` around lines 19 - 23, The code currently persists the server URL twice: once inside DeriveServerFromJumpstarter() and again later in the no-arg login path; fix this by tracking whether the server was derived and only persisting when the user provided the server explicitly. Concretely, set a bool (e.g., derived := true) when you call DeriveServerFromJumpstarter() and use that flag to skip the later unconditional config write (the block that saves the server URL around the later save/persist calls), or invert it to only persist when derived == false; reference DeriveServerFromJumpstarter() and the later server-persistence block to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 22-24: The derived Build API host is using buildAPIRoutePrefix and
buildAPINamespace that produce "ado-build-api-automotive-dev-operator-system..."
but the route manifest uses route name "build-api" in namespace "system"; update
the constants or derivation so they match the manifest. Specifically, change
buildAPIRoutePrefix to "build-api" and buildAPINamespace to "system" (or adjust
the hostname construction logic where those constants are used) so the produced
host equals "build-api-system.<baseDomain>" and the fallback flow will match the
route.yaml route name and namespace.
---
Nitpick comments:
In `@cmd/caib/login.go`:
- Around line 19-23: The code currently persists the server URL twice: once
inside DeriveServerFromJumpstarter() and again later in the no-arg login path;
fix this by tracking whether the server was derived and only persisting when the
user provided the server explicitly. Concretely, set a bool (e.g., derived :=
true) when you call DeriveServerFromJumpstarter() and use that flag to skip the
later unconditional config write (the block that saves the server URL around the
later save/persist calls), or invert it to only persist when derived == false;
reference DeriveServerFromJumpstarter() and the later server-persistence block
to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d68eec2-8d7a-4e7e-af9e-0f112e130ace
📒 Files selected for processing (19)
cmd/caib/authcmd/authcmd.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/logs.gocmd/caib/catalog/add.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/caib/catalog/publish.gocmd/caib/catalog/remove.gocmd/caib/catalog/verify.gocmd/caib/config/config.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/login.gocmd/caib/querycmd/query.gocmd/caib/root.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/caib/downloadcmd/download.go
- cmd/caib/container/build.go
- cmd/caib/container/container.go
- cmd/caib/catalog/list.go
- cmd/caib/authcmd/authcmd.go
- cmd/caib/catalog/get.go
- cmd/caib/catalog/verify.go
- cmd/caib/root.go
- cmd/caib/container/logs.go
- cmd/caib/image/image.go
- cmd/caib/catalog/add.go
|
@evakhoni add |
attempting to calculate the server endpoint from jumpstarter config, as a fallback if no login endpoint is availabe. saves to config. Signed-off-by: Evgeni Vakhonin <evakhoni@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
verifying the fetching from jumpstarter config Signed-off-by: Evgeni Vakhonin <evakhoni@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Fix a linter complain about unused variable Mention jmp logn as an option for no server error Harden the tls check remove dead returns after os.Exit Signed-off-by: Evgeni Vakhonin <evakhoni@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
evakhoni
force-pushed
the
login-from-jumpstarter
branch
from
edbe2be to
b8754b8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/caib/config/config_test.go (1)
56-163: Consider adding parser edge-case tests for scheme-prefixed endpoints.A couple of extra cases like
grpc://grpc.lab.apps.example.com:443(success) and bracketed IPv6 forms (expected empty) would lock in the URL parsing behavior more tightly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config_test.go` around lines 56 - 163, Add two edge-case tests to config_test.go: one that writes a scheme-prefixed endpoint via writeJumpstarterConfig (e.g., "grpc://grpc.lab.apps.example.com:443"), sets healthHTTPClient using roundTripFunc to return 200, calls DeriveServerFromJumpstarter and asserts the derived URL equals "https://ado-build-api-automotive-dev-operator-system.apps.example.com" and that the health request was made to expected + "/v1/healthz"; and another that writes a bracketed IPv6 endpoint (e.g., "[::1]:443"), sets healthHTTPClient to track calls, calls DeriveServerFromJumpstarter and asserts it returns empty and that the health check was not called. Ensure you reuse the same helpers and variables (writeJumpstarterConfig, healthHTTPClient, roundTripFunc, DeriveServerFromJumpstarter) and mirror the style and assertions used by the existing tests.cmd/caib/login.go (1)
18-22: Consider avoiding double persistence on the no-arg login path.
config.DeriveServerFromJumpstarter()already persists on success, andrunLoginsaves again afterward. Keeping persistence in one place would reduce duplicate I/O and simplify failure semantics.Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` around lines 18 - 22, When server is derived via config.DeriveServerFromJumpstarter() avoid saving twice: change runLogin to accept a flag (e.g., skipPersist or persist bool) or return an indicator so the caller can skip the extra save; specifically, keep persistence inside DeriveServerFromJumpstarter (since it already persists on success) and update the call site in the no-arg branch that invokes runLogin to pass skipPersist=true (or runLogin(..., persist=false)), and ensure runLogin checks that flag and does not call the redundant save logic. Update the other referenced path (lines 54-55) similarly so you only persist once per successful derivation/login.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/caib/config/config_test.go`:
- Around line 56-163: Add two edge-case tests to config_test.go: one that writes
a scheme-prefixed endpoint via writeJumpstarterConfig (e.g.,
"grpc://grpc.lab.apps.example.com:443"), sets healthHTTPClient using
roundTripFunc to return 200, calls DeriveServerFromJumpstarter and asserts the
derived URL equals
"https://ado-build-api-automotive-dev-operator-system.apps.example.com" and that
the health request was made to expected + "/v1/healthz"; and another that writes
a bracketed IPv6 endpoint (e.g., "[::1]:443"), sets healthHTTPClient to track
calls, calls DeriveServerFromJumpstarter and asserts it returns empty and that
the health check was not called. Ensure you reuse the same helpers and variables
(writeJumpstarterConfig, healthHTTPClient, roundTripFunc,
DeriveServerFromJumpstarter) and mirror the style and assertions used by the
existing tests.
In `@cmd/caib/login.go`:
- Around line 18-22: When server is derived via
config.DeriveServerFromJumpstarter() avoid saving twice: change runLogin to
accept a flag (e.g., skipPersist or persist bool) or return an indicator so the
caller can skip the extra save; specifically, keep persistence inside
DeriveServerFromJumpstarter (since it already persists on success) and update
the call site in the no-arg branch that invokes runLogin to pass
skipPersist=true (or runLogin(..., persist=false)), and ensure runLogin checks
that flag and does not call the redundant save logic. Update the other
referenced path (lines 54-55) similarly so you only persist once per successful
derivation/login.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53894ee1-4911-453b-917a-e9adb1feb99a
📒 Files selected for processing (21)
cmd/caib/authcmd/authcmd.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/logs.gocmd/caib/catalog/add.gocmd/caib/catalog/get.gocmd/caib/catalog/list.gocmd/caib/catalog/publish.gocmd/caib/catalog/remove.gocmd/caib/catalog/verify.gocmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/config/suite_test.gocmd/caib/container/build.gocmd/caib/container/container.gocmd/caib/container/logs.gocmd/caib/downloadcmd/download.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/login.gocmd/caib/querycmd/query.gocmd/caib/root.go
🚧 Files skipped from review as they are similar to previous changes (12)
- cmd/caib/catalog/verify.go
- cmd/caib/downloadcmd/download.go
- cmd/caib/catalog/list.go
- cmd/caib/config/suite_test.go
- cmd/caib/container/build.go
- cmd/caib/flashcmd/flash.go
- cmd/caib/catalog/publish.go
- cmd/caib/root.go
- cmd/caib/buildcmd/logs.go
- cmd/caib/catalog/add.go
- cmd/caib/buildcmd/build.go
- cmd/caib/authcmd/authcmd.go
4 participants
in this PR we attempt to reconstruct caib server url from jumpstarter endpont, in order to allow unify caib login with jumpstarter
current implementation uses hardcoded buildAPINamespace = "automotive-dev-operator-system"
in the future we can extend it to automatic namespace discovery it needed.
the code searches for default jumpstarter config, removes the grpc and namespace part and adds the build api one. it checks if the endpoint answers, and if yes - saves to the config for future use.
tested locally with correct and incorrect config:
Summary by CodeRabbit
New Features
Improvements
Tests