Skip to content

improve register resiliency and idempotency - #463

Open
patelspratik wants to merge 1 commit into
mainfrom
pr1-register-resiliency
Open

improve register resiliency and idempotency#463
patelspratik wants to merge 1 commit into
mainfrom
pr1-register-resiliency

Conversation

@patelspratik

@patelspratik patelspratik commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Makes device registration and deregistration resilient and idempotent, and wires up the previously-unused LinuxUser cache for SSH grants.

Register

  • Pending-registration record is written before AddNode, so a crash or timeout mid-registration is recoverable
  • Re-running brev register with an incomplete registration resumes it, reusing the same device ID (AddNode is idempotent on device_id)
  • Already-registered machines now report node connectivity, check/start the Brev tunnel, and direct the user to deregister first — instead of hard-failing
  • Org mismatch between the existing registration and the requested org produces an actionable error
  • Interactive and non-interactive modes standardized; --ssh-port deprecated in favor of brev enable-ssh

Deregister

  • Handles pending registrations: skips RemoveNode when the backend never confirmed the node, recovers the node by device ID where possible
  • ListNodes failure during pending cleanup is non-fatal

LinuxUser cache

  • Removed GetCachedLinuxUser/SaveCachedLinuxUser (present since BRE2-818, never wired) in favor of the picker we currently have

@patelspratik
patelspratik force-pushed the pr1-register-resiliency branch 4 times, most recently from 1e46278 to 9ebe927 Compare August 27, 2026 18:23
@patelspratik patelspratik changed the title Pr1 register resiliency improve register resiliency and idempotency Aug 27, 2026
@patelspratik
patelspratik force-pushed the pr1-register-resiliency branch from 25be2ea to 9ea37de Compare August 27, 2026 21:00
@patelspratik
patelspratik marked this pull request as ready for review August 27, 2026 21:00
@patelspratik
patelspratik requested a review from a team as a code owner August 27, 2026 21:00
cmd.Flags().StringVarP(&nameFlag, "name", "n", "", "device name (required when using non-interactive mode)")
cmd.Flags().IntVarP(&sshPort, "ssh-port", "p", 0, "SSH port (if ssh access is desired)")
cmd.Flags().BoolVar(&approveFlag, "approve", false, "skip all confirmation prompts (assume yes)")
_ = cmd.Flags().MarkDeprecated("ssh-port", "use 'brev enable-ssh' after registration to enable SSH access")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice


func findNodeByDeviceID(ctx context.Context, s externalnode.TokenProvider, deps deregisterDeps, orgID, deviceID string) (string, error) {
client := deps.nodeClients.NewNodeClient(s, config.GlobalConfig.GetBrevPublicAPIURL())
resp, err := client.ListNodes(ctx, connect.NewRequest(&nodev1.ListNodesRequest{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Paginate?

Comment thread pkg/auth/auth.go
Comment on lines +105 to 106
const MissingAPIKeyOrgIDMessage = "org id missing, please login again; run 'brev login --api-key <api-key>'"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the old error message was more correct here (specifically the guidance to use --org-id <org-id>)?

}
}
if externalNodeID == "" {
t.Vprintf(" %s\n", t.Yellow("No registered node to remove (pending registration); cleaning up local state."))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like this will happen if:

  • the node truly wasn't there, or
  • we had some failure in looking up the node

For the second case, I wonder if we actually shouldn't clean up local state so that we can retry the deregister or understand that a new registration shouldn't be allowed (or maybe we just abort the deregister altogether).

t.Vprint("")
return nil
org := &entity.Organization{ID: pending.OrgID, Name: pending.OrgName}
return runRegisterSteps(ctx, t, s, pending.DisplayName, org, deps, pending.DeviceID)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can do this later, but in the spirit of idempotency I wonder if we could skip certain steps here (like re-installing netbird, re-collecting hardware profile, etc.)

type RegistrationStore interface {
Save(reg *DeviceRegistration) error
Load() (*DeviceRegistration, error)
Load(includeAll bool) (*DeviceRegistration, error)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This kind of blew up the usage and leads to lots of Load(false) -- could we keep "Load" as always loading everything + performing the validation it does today, then add a helper for the relatively rare cases where we care only for non-pending registrations?

e.g.:

func (s *FileRegistrationStore) Load() (*DeviceRegistration, error) {
  if exists, err := s.Exists(); err != nil {
    return nil, breverrors.WrapAndTrace(err)
  } else if !exists {
    return nil, breverrors.New("device registration not found, run 'brev register' first")
  }

  var reg DeviceRegistration
    if err := files.ReadJSON(files.AppFs, s.path(), &reg); err != nil {
      return nil, breverrors.WrapAndTrace(err)
    }
    if err := reg.validate(); err != nil {
      return nil, err
  }

  return &reg, nil
}

func (r *DeviceRegistration) validate() error {
  switch r.Status {
  case "":
      if r.ExternalNodeID == "" || r.OrgID == "" {
          return breverrors.New("malformed registration")
      }
      r.Status = RegistrationStatusRegistered

  case RegistrationStatusPending:
      if r.DisplayName == "" || r.OrgID == "" || r.DeviceID == "" {
          return breverrors.New("malformed pending registration")
      }
      if r.ExternalNodeID != "" {
          return breverrors.New("pending registration unexpectedly has a node ID")
      }

  case RegistrationStatusRegistered:
      if r.ExternalNodeID == "" || r.OrgID == "" {
          return breverrors.New("malformed registered registration")
      }

    default:
      return fmt.Errorf("unknown registration status %q", r.Status)
  }

  return nil
}

And then if we need to report an error for pending:

reg, err := store.Load()
if err != nil {
  return nil, err
}
if reg.Status == RegistrationStatusPending {
  return nil, breverrors.New("device registration is incomplete; re-run 'brev register' to finish")
}

or even a helper function that is super explicit:

reg, err := LoadRegistrations(LoadRegistrations{
    AllowedStates: []string{RegistrationStatusPending, RegistrationStatusRegistersd},
})

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