Reorganize tctl commands to not require an auth client by default #48894

vapopov · 2024-11-13T14:37:18Z

In this PR added ability to declare tctl commands which not required auth client such as version command

zmb3 · 2024-11-13T15:06:25Z

I don't love this approach but I'm struggling to come up with a better idea at the moment.. 🤔

vapopov · 2024-11-13T16:08:49Z

I don't love this approach but I'm struggling to come up with a better idea at the moment.. 🤔

@zmb3 I tried couple other options but they also look not perfect, like to make auth client initialization as lazy loading so only commands which requires auth going to init connection, and another one - not return error if auth client failed to init, so in commands client might be nil

TryRun(ctx context.Context, cmd string, client *authclient.Client)

rosstimothy

I agree with Zac, I'm not particularly fond of splitting up the commands. We could potentially expand on your suggestion to have the client be lazily constructed and instead of

TryRun(ctx context.Context, cmd string, client *authclient.Client)

we could do

TryRun(ctx context.Context, cmd string, clientFunc func() *authclient.Client)

In that scenario, the logic of building the client can stay where it is, but only be executed if the command requires an authenticated connection to the cluster.

tool/tctl/common/tctl.go

tool/tctl/common/cmds.go

…tctl-commands

vapopov · 2024-11-15T20:44:25Z

I've refactored to have separate auth client initialization after command matching
commands that do not require an authentication client: tctl version|top|fido2|touchid|webauthnwin

zmb3

I'm liking this approach better. 👍

tool/tctl/common/acl_command.go

zmb3 · 2024-11-20T21:08:07Z

tool/tctl/common/auth_command.go

 	switch cmd {
 	case a.authGenerate.FullCommand():
-		err = a.GenerateKeys(ctx, client)
+		client, clientClose, err := clientFunc(ctx)


There's a lot of repeated code here. Would it be simpler to run the clientFunc prior to the switch?

In this case we init auth client (start checking profile folder, establish connection, request ping) before actual matching the command has to be run. We need to identify what command it is by FullCommand() and then each command decides if auth client is required

tool/tctl/common/client/auth.go

tool/tctl/common/config/profile.go

Replace logrus to use slog

…tctl-commands

vapopov · 2024-12-02T17:07:31Z

would appreciate of review of this PR

Joerger

LGTM, approach seems reasonable. Just left a suggestion for the naming.

tool/tctl/common/access_request_command.go

…tctl-commands

vapopov · 2024-12-09T16:24:08Z

one more review by any chance

hugoShaka

LGTM once comments are resolved

tool/tctl/common/client/auth.go

tool/tctl/common/admin_action_test.go

hugoShaka · 2024-12-11T19:21:31Z

tool/tctl/common/fido2.go

+func (c *fido2Command) TryRun(ctx context.Context, cmd string, _ commonclient.InitFunc) (match bool, err error) {
+	return c.impl.TryRun(ctx, cmd)


This PR makes the fido2 command work even without a valid client, nice :)

tool/tctl/common/auth_command.go

tool/tctl/common/plugin/plugins_command.go

…lient Code review changes

…tctl-commands

…8894) * Reorganize tctl commands to have commands not required auth client * Replace auth client with lazy loading approach * Fix linter warning * Replace camel case in import alias Replace logrus to use slog * Rename close function * Refactor plugin commands to use interface of auth client and plugin client Code review changes * Refactor workload identity commands * Add access to global config for the commands * Add test checking all tctl commands match process * Fix golangci-lint warnings

vapopov requested review from hugoShaka and rosstimothy November 13, 2024 14:37

github-actions bot added the size/sm label Nov 13, 2024

github-actions bot requested a review from Joerger November 13, 2024 14:37

github-actions bot added tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Nov 13, 2024

github-actions bot requested a review from r0mant November 13, 2024 14:37

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from ceef55e to 3d57ff8 Compare November 13, 2024 14:37

Reorganize tctl commands to have commands not required auth client

fa9f372

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 3d57ff8 to fa9f372 Compare November 13, 2024 14:39

rosstimothy reviewed Nov 14, 2024

View reviewed changes

tool/tctl/common/tctl.go Outdated Show resolved Hide resolved

tool/tctl/common/tctl.go Outdated Show resolved Hide resolved

tool/tctl/common/cmds.go Outdated Show resolved Hide resolved

vapopov added 2 commits November 15, 2024 14:55

Replace auth client with lazy loading approach

af7baa1

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

7b32cfc

…tctl-commands

vapopov added the no-changelog Indicates that a PR does not require a changelog entry label Nov 15, 2024

Fix linter warning

e7d0081

vapopov requested review from rosstimothy and zmb3 November 18, 2024 21:50

zmb3 reviewed Nov 20, 2024

View reviewed changes

vapopov added 2 commits November 21, 2024 12:38

Replace camel case in import alias

0ea5a68

Replace logrus to use slog

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

b6fe948

…tctl-commands

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 7240ab5 to b6fe948 Compare November 21, 2024 19:02

vapopov requested a review from zmb3 November 25, 2024 19:44

Joerger approved these changes Dec 2, 2024

View reviewed changes

tool/tctl/common/access_request_command.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

c68f24f

…tctl-commands

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 0569781 to c68f24f Compare December 6, 2024 01:34

Rename close function

352fd16

Joerger approved these changes Dec 9, 2024

View reviewed changes

vapopov requested a review from espadolini December 11, 2024 16:56

hugoShaka approved these changes Dec 11, 2024

View reviewed changes

public-teleport-github-review-bot bot removed request for r0mant and espadolini December 11, 2024 20:18

vapopov added 5 commits December 16, 2024 22:04

Refactor plugin commands to use interface of auth client and plugin c…

1b50d46

…lient Code review changes

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

8d8f135

…tctl-commands

Refactor workload identity commands

ecbbbf5

Add access to global config for the commands

8047189

Add test checking all tctl commands match process

4b1e596

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 70123e6 to 4b1e596 Compare December 17, 2024 19:52

Fix golangci-lint warnings

cebf1b0

vapopov changed the title ~~Reorganize tctl commands to have commands not required auth client~~ Reorganize tctl commands to not require an auth client by default Dec 17, 2024

vapopov added this pull request to the merge queue Dec 17, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024

vapopov added this pull request to the merge queue Dec 17, 2024

Merged via the queue into master with commit 45f29a8 Dec 17, 2024
42 checks passed

vapopov deleted the vapopov/reorgonize-tctl-commands branch December 17, 2024 21:13

vapopov mentioned this pull request Jan 10, 2025

[v17] Reorganize tctl commands to not require an auth client by default #50966

Open

vapopov mentioned this pull request Jan 11, 2025

[v16] Reorganize tctl commands to not require an auth client by default #50968

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize tctl commands to not require an auth client by default #48894

Reorganize tctl commands to not require an auth client by default #48894

vapopov commented Nov 13, 2024

zmb3 commented Nov 13, 2024

vapopov commented Nov 13, 2024 •

edited

Loading

rosstimothy left a comment

vapopov commented Nov 15, 2024

zmb3 left a comment

zmb3 Nov 20, 2024

vapopov Nov 20, 2024

vapopov commented Dec 2, 2024

Joerger left a comment

vapopov commented Dec 9, 2024

hugoShaka left a comment

hugoShaka Dec 11, 2024

		func (c *fido2Command) TryRun(ctx context.Context, cmd string, _ commonclient.InitFunc) (match bool, err error) {
		return c.impl.TryRun(ctx, cmd)

Reorganize tctl commands to not require an auth client by default #48894

Reorganize tctl commands to not require an auth client by default #48894

Conversation

vapopov commented Nov 13, 2024

zmb3 commented Nov 13, 2024

vapopov commented Nov 13, 2024 • edited Loading

rosstimothy left a comment

Choose a reason for hiding this comment

vapopov commented Nov 15, 2024

zmb3 left a comment

Choose a reason for hiding this comment

zmb3 Nov 20, 2024

Choose a reason for hiding this comment

vapopov Nov 20, 2024

Choose a reason for hiding this comment

vapopov commented Dec 2, 2024

Joerger left a comment

Choose a reason for hiding this comment

vapopov commented Dec 9, 2024

hugoShaka left a comment

Choose a reason for hiding this comment

hugoShaka Dec 11, 2024

Choose a reason for hiding this comment

vapopov commented Nov 13, 2024 •

edited

Loading