Skip to content

Improve error message if data dir on tbot and tctl not available for permissions#14282

Merged
stevenGravy merged 15 commits intomasterfrom
stevenGravy/permissions
Jul 25, 2022
Merged

Improve error message if data dir on tbot and tctl not available for permissions#14282
stevenGravy merged 15 commits intomasterfrom
stevenGravy/permissions

Conversation

@stevenGravy
Copy link
Copy Markdown
Contributor

This matches the error message given in teleport start for tctl and tbot when not running with access to data dir. This matches to similar message when running teleport start

Currently:

$ tctl users ls                     
# ERROR: lstat /private/var/lib/teleport/host_uuid: permission denied`

$ tbot start --auth-server=server
# ERROR: stat /var/lib/teleport/bot: permission denied

Now

$ tctl users ls
ERROR: Teleport does not have permission to read Teleport host UUID file at /var/lib/teleport/host_uuid. Ensure that you are running as a user with appropriate permissions.
lstat /private/var/lib/teleport/host_uuid: permission denied

$ tbot start --auth-server=server
ERRO [TBOT]      Teleport does not have permission to write to: /var/lib/teleport/bot. Ensure that you are running as a user with appropriate permissions. config/destination_directory.go:132
ERROR: stat /var/lib/teleport/bot: permission denied

@github-actions github-actions Bot requested review from r0mant and russjones July 9, 2022 20:38
@github-actions github-actions Bot added machine-id tctl tctl - Teleport admin tool labels Jul 9, 2022
@stevenGravy stevenGravy enabled auto-merge (squash) July 9, 2022 23:20
@stevenGravy stevenGravy changed the title Improve error message if data dir on tbot and tctl not available Improve error message if data dir on tbot and tctl not available for permissions Jul 9, 2022
@stevenGravy stevenGravy requested a review from zmb3 July 11, 2022 12:02
@stevenGravy
Copy link
Copy Markdown
Contributor Author

@zmb3 added you as you were part of the original error message review on #10044

Comment thread tool/tctl/common/tctl.go
return nil, trace.Wrap(err, fmt.Sprintf("Could not load Teleport host UUID file at %s. "+
"Please make sure that Teleport is up and running prior to using tctl.",
filepath.Join(cfg.DataDir, utils.HostUUIDFile)))
} else if errors.Is(err, fs.ErrPermission) {
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.

A couple comments:

  1. Why the inconsistency? In tbot we are logging an enhanced error message, but returning the original error unmodified. In tctl we are wrapping the original error with a better message. Can we be consistent and take the same approach in both places?
  2. You shouldn't need to use fmt.Sprintf here. trace.Wrap already supports this: trace.Wrap(err, "this is a message: %v", x)

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.

thanks @zmb3 I've made these consistent approaches and removed the unnecessary fmt.Sprintf. Please check when you can.

log.Infof("Created directory %q", p)
} else if err != nil {
if errors.Is(err, fs.ErrPermission) {
log.Errorf("Teleport does not have permission to write to: %v. Ensure that you are running as a user with appropriate permissions.", p)
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.

This might confuse a user, as you'll log the error here, but also return a [different] error, which will eventually also be logged.

In general you should handle an error either by logging it or returning it, but not both.

@github-actions github-actions Bot requested a review from zmb3 July 12, 2022 16:00
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

We're nearly there.

Comment thread lib/tbot/config/destination_directory.go Outdated
Comment thread lib/tbot/config/destination_directory.go Outdated
} else if err != nil {
//This error permission can occur if unable to read into the data dir
if errors.Is(err, fs.ErrPermission) {
return trace.Wrap(err, "Teleport does not have permission to write to: %v. Ensure that you are running as a user with appropriate permissions.", p)
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.

In this case, we weren't trying to write, so the error message is not correct.

I would say something like does not have permission to access %v.

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.

@zmb3 changed error message and applied other suggestions

Comment thread lib/tbot/config/destination_directory.go Outdated
stevenGravy and others added 6 commits July 18, 2022 10:37
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
@github-actions github-actions Bot removed the request for review from r0mant July 25, 2022 18:44
@github-actions github-actions Bot removed the request for review from russjones July 25, 2022 18:44
@stevenGravy stevenGravy merged commit c59cc21 into master Jul 25, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@stevenGravy See the table below for backport results.

Branch Result
branch/v10 Create PR
branch/v9 Create PR

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

Labels

machine-id tctl tctl - Teleport admin tool ux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants