Skip to content

Machine ID: Bot instance management in tctl bots instances#46998

Merged
timothyb89 merged 6 commits intomasterfrom
timothyb89/tctl-bot-instances
Oct 2, 2024
Merged

Machine ID: Bot instance management in tctl bots instances#46998
timothyb89 merged 6 commits intomasterfrom
timothyb89/tctl-bot-instances

Conversation

@timothyb89
Copy link
Copy Markdown
Contributor

This adds new new subcommands to tctl to manage bot instances:

  • tctl bots instances list lists bot instances
  • tctl bots instances show shows a specific bot instance
  • tctl bots instances add adds a new instance to an existing bot

This also adds help text to guide users between the new commands, along with a few aliases for common alternate command names.

changelog: Add new tctl subcommands to manage bot instances

This adds new new subcommands to `tctl` to manage bot instances:
- `tctl bots instances list` lists bot instances
- `tctl bots instances show` shows a specific bot instance
- `tctl bots instances add` adds a new instance to an existing bot

This also adds help text to guide users between the new commands. It
also adds a few aliases for common alternate command names.
@github-actions github-actions Bot added size/md tctl tctl - Teleport admin tool labels Sep 30, 2024
@github-actions github-actions Bot requested review from atburke and avatus September 30, 2024 20:48
Comment thread tool/tctl/common/bots_command.go Outdated
c.botsUpdate.Flag("set-logins", "Sets the bot's logins to the given comma-separated list, replacing any existing logins.").StringVar(&c.setLogins)
c.botsUpdate.Flag("add-logins", "Adds a comma-separated list of logins to an existing bot.").StringVar(&c.addLogins)

c.botsInstance = bots.Command("instance", "Manage bot instances.").Alias("instances")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For tctl bots the plural version is the actual command and the singular version is an alias, but for tctl bots instance we do the reverse.

Not a big deal but might as well be consistent, no?

Comment thread tool/tctl/common/bots_command.go Outdated
Comment thread tool/tctl/common/bots_command.go Outdated
if err := client.UpsertToken(ctx, token); err != nil {
return trace.Wrap(err)
}
} else {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need the else or have all possible code paths in the if been exhausted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can dedent this branch, sure.

Comment thread tool/tctl/common/bots_command.go Outdated
Comment on lines +798 to +801
addr := proxies[0].GetPublicAddr()
if addr == "" {
addr = proxies[0].GetAddr()
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cmp.Or works here too.

Comment thread tool/tctl/common/bots_command.go Outdated

// parseInstanceID converts an instance ID string in the form of
// '[bot name]/[uuid]' to separate bot name and UUID strings.
func parseInstanceID(s string) (string, string, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func parseInstanceID(s string) (string, string, error) {
func parseInstanceID(s string) (name string, uuid string, err error) {

Comment thread tool/tctl/common/bots_command.go Outdated
Comment on lines +867 to +873
parts := strings.Split(s, "/")

if len(parts) != 2 {
return "", "", trace.BadParameter("invalid bot instance syntax, must be: [bot name]/[uuid]")
}

return parts[0], parts[1], nil
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consider using strings.Cut here:

botName, botUUID, ok := strings.Cut(s, "/")
if !ok { /* return bad parameter /* }

return botName, botUUID, nil

Copy link
Copy Markdown
Contributor

@strideynet strideynet left a comment

Choose a reason for hiding this comment

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

Looks really good! Gives us a good basis for what should show in the Bot Instance UI when that gets added.

Comment thread tool/tctl/common/bots_command.go Outdated

// AddBotInstance begins onboarding a new instance of an existing bot.
func (c *BotsCommand) AddBotInstance(ctx context.Context, client *authclient.Client) error {
// A bit of a misnomer but makes the terminology a bit more consistent. This
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.

I tried to think of something better than "tctl bots instances add" but I think it's close enough and not too confusing. I figure the fact that it basically just outputs an example command for delegated join methods makes sense in those scenarios.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, I don't love it but I think it's memorable and at least conceptually similar to existing commands in tctl.

Comment thread tool/tctl/common/bots_command.go
timothyb89 and others added 2 commits October 1, 2024 19:26
@timothyb89 timothyb89 enabled auto-merge October 2, 2024 15:45
@timothyb89 timothyb89 added this pull request to the merge queue Oct 2, 2024
Merged via the queue into master with commit 338968f Oct 2, 2024
@timothyb89 timothyb89 deleted the timothyb89/tctl-bot-instances branch October 2, 2024 23:07
@public-teleport-github-review-bot
Copy link
Copy Markdown

@timothyb89 See the table below for backport results.

Branch Result
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/md tctl tctl - Teleport admin tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants