improve register resiliency and idempotency - #463
Conversation
1e46278 to
9ebe927
Compare
25be2ea to
9ea37de
Compare
| 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") |
|
|
||
| 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{ |
| const MissingAPIKeyOrgIDMessage = "org id missing, please login again; run 'brev login --api-key <api-key>'" | ||
|
|
There was a problem hiding this comment.
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.")) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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(), ®); err != nil {
return nil, breverrors.WrapAndTrace(err)
}
if err := reg.validate(); err != nil {
return nil, err
}
return ®, 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},
})
Makes device registration and deregistration resilient and idempotent, and wires up the previously-unused LinuxUser cache for SSH grants.
Register
Deregister
LinuxUser cache