Repository navigation
[client] Fix profile regressions in up --profile and status - #6479
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesProfile management via daemon RPC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 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 |
Extract the profile switch/auto-create logic from upFunc into a dedicated switchOrCreateProfile helper. The inlined NotFound-retry branch pushed upFunc over SonarCloud's cognitive complexity threshold (S3776). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Release artifactsBuilt for PR head
GHCR images (amd64)No GHCR images were pushed. This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![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
🤖 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 `@client/cmd/login.go`:
- Around line 247-252: The code has a race condition where another concurrent
process can create the profile between the check and the createProfile call on
line 247. If createProfile fails because the profile already exists, the code
should still attempt switchProfile since it would succeed, rather than returning
the error immediately. Modify the error handling after the createProfile call to
check if the error indicates the profile already exists (e.g., a NotFound or
AlreadyExists error), and if so, proceed to call switchProfile anyway. Only
return the error if switchProfile also fails or if createProfile failed for a
different reason that should be fatal.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 937d7b9a-e868-4d89-94b7-a2cd0730d828
📒 Files selected for processing (2)
client/cmd/login.goclient/cmd/up.go
Don't fail switchOrCreateProfile on a createProfile error: a concurrent run may create the profile between the NotFound check and our create call. Retry the switch regardless and only surface the create error if the switch also fails. Addresses CodeRabbit race-condition feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // createProfile dials the daemon and creates a new profile with the given | ||
| // display name, returning its generated ID. Use addProfileOnDaemon directly | ||
| // when a daemon client is already available to reuse the connection. | ||
| func createProfile(ctx context.Context, profileName, username string) (profilemanager.ID, error) { | ||
| conn, err := DialClientGRPCServer(ctx, daemonAddr) | ||
| if err != nil { | ||
| //nolint | ||
| return "", fmt.Errorf("failed to connect to daemon error: %v\n"+ | ||
| "If the daemon is not running please run: "+ | ||
| "\nnetbird service install \nnetbird service start\n", err) | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| return addProfileOnDaemon(ctx, proto.NewDaemonServiceClient(conn), profileName, username) | ||
| } | ||
|
|
||
| // addProfileOnDaemon issues the AddProfile RPC on an existing daemon client | ||
| // and returns the new profile's ID. It is the single entry point for profile | ||
| // creation, shared by `netbird profile add` and the `netbird up --profile | ||
| // <name>` auto-create path. | ||
| func addProfileOnDaemon(ctx context.Context, client proto.DaemonServiceClient, profileName, username string) (profilemanager.ID, error) { | ||
| resp, err := client.AddProfile(ctx, &proto.AddProfileRequest{ | ||
| ProfileName: profileName, | ||
| Username: username, | ||
| }) | ||
| if err != nil { | ||
| return "", fmt.Errorf("add profile failed: %w", err) |
There was a problem hiding this comment.
Let's move these helper functions to client/cmd/profile.go.
| // switchOrCreateProfile switches the active profile to the one identified by | ||
| // handle, creating it first when it does not exist yet. This restores the | ||
| // pre-0.73 behaviour where `netbird up --profile <name>` auto-creates a | ||
| // missing profile instead of failing. | ||
| func switchOrCreateProfile(ctx context.Context, pm *profilemanager.ProfileManager, handle, username string) error { | ||
| resolvedID, err := switchProfile(ctx, handle, username) | ||
| if err != nil { | ||
| st, ok := gstatus.FromError(err) | ||
| if !ok || st.Code() != codes.NotFound { | ||
| return err | ||
| } | ||
| // Don't fail immediately on a create error: a concurrent run may | ||
| // have created the profile between the NotFound above and this | ||
| // call, in which case the retried switch still succeeds. Only | ||
| // surface the create error if the switch also fails. | ||
| _, createErr := createProfile(ctx, handle, username) | ||
| if resolvedID, err = switchProfile(ctx, handle, username); err != nil { | ||
| if createErr != nil { | ||
| return fmt.Errorf("create profile: %w", createErr) | ||
| } | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if err := pm.SwitchProfile(resolvedID); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Let's move this helper function to client/cmd/up.go.
|
Describe your changes
PR #6367 (migrating profile identity from display name to ID) introduced two regressions in the CLI profile flow. This PR fixes both.
netbird up --profile <name>no longer auto-created a missing profile. Before [client] Migrate profile identity from display name to ID and allow renaming of profiles #6367, switching to a non-existent profile name simply recorded it as the active profile and the config file was created lazily. After the migration, the daemon resolves the supplied handle against existing profiles and returnscodes.NotFound, which broke ephemeral CI workflows relying onnetbird up --profile <name> --setup-key <key>to provision the profile on the fly (reported in discussion netbird up with profile and setup key fails on v0.73.0 #6476).upFuncinclient/cmd/up.gonow detectscodes.NotFoundfromswitchProfile, creates the profile via the daemon's existingAddProfileRPC (createProfilehelper inclient/cmd/login.go), and retries the switch. To make the status code inspectable by the caller,switchProfilenow wraps the daemon error with%winstead of%v.netbird statusshowed an emptyProfile:field for both the default and custom profiles. After [client] Migrate profile identity from display name to ID and allow renaming of profiles #6367,ProfileManager.GetActiveProfilereturns only the profile ID (not the display name), and the CLI — running as the invoking user — cannot read the per-user profile files under/var/lib/netbird/<user>/(root-owned,0700) to resolve the name itself.client/cmd/status.gonow resolves the display name through the daemon's existingGetActiveProfileRPC, which runs as root and maps the active ID to its display name. Added thegetActiveProfileNamehelper and removed the now-unusedprofilemanagerimport.Manually verified:
netbird up --profile <new-name> --setup-key <key>provisions and connects, andnetbird statusshows the correct profile name for both default and custom profiles.Issue ticket number and link
#6476 — regression report for
netbird up --profile+ setup-key failing on v0.73.0 after #6367.Stack
Checklist
Documentation
Select exactly one:
These are regression fixes that restore the documented pre-0.73.0 behavior of
netbird up --profileand thenetbird statusoutput. No public CLI flags, API surface, or configuration options change, so existing documentation remains accurate.Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit
upcommand now switches to a specified profile, and if it doesn’t exist, creates it and then retries activation.statuscommand now pulls the active profile’s display name from the running daemon.