Add Suggested Labels to Provision Tokens by marcoandredinis · Pull Request #15114 · gravitational/teleport

marcoandredinis · 2022-08-02T11:06:46Z

For Teleport Discover, the user will be able to test connecting to a
resource right after adding it.

The flow should look like this:

User selects the resource type - in this case Server/Nodes
WebUI generates a new Token
WebUI shows the sudo bash .../<token>/install.sh to the user
When the user runs this command, and assuming everything works out,
the WebUI should be able to receive the Server/Node that was
generated from that specific Token.

To achieve this, here's a more detailed flow, which this PR implements

User selects the resource type - in this case Server/Nodes
WebUI generates a new Token
This generates a Provision Token which contains a RefResourceID
WebUI receives back that ID
WebUI shows the sudo bash .../<token>/install.sh to the user
This generates a script which includes setting the labels as part of
the teleport configure command:
$ teleport configure ... --labels teleport.internal/resource-id=<refResourceID> ...
User runs the provided command on the target host
WebUI should be able to query the Servers/Nodes which contain that
specific <resource-id> and allow the user to connect to it.

Demo:

espadolini · 2022-08-03T09:00:21Z

Should this be teleport.dev/resource-id instead? Or are we deprecating teleport.dev and standardizing on teleport.internal instead?

I think the idea is to keep both and filter out teleport.internal labels from the UI (not sure if that's the case now).

So, the WebUI doesn't filter teleport.dev either

I'm not sure which one we should use, but after looking into this code

teleport/tool/tctl/common/collection.go

Line 804 in 54616ac

func stripInternalTeleportLabels(verbose bool, labels map[string]string) string {

I would say we should use teleport.dev/ and create an issue to filter out these labels when displaying labels in the UI

I'd say teleport.internal/ should be hidden, teleport.dev/ is also used for stuff like automatic labels from LDAP for desktop access - so I'm ok with teleport.internal/resource-id.

What I'm not ok with is that one bot label using teleport.internal/kebab-case while desktop access uses teleport.dev/snake_case, but that's not something that can be fixed here (or ever? 😭)

I'll keep it as teleport.internal/ then.

As for case, it seems teleport.internal/ uses kebab case and teleport.dev/ uses snake case
At least they are consistent within the namespace 😂
Although the teleport.internal/ only has one occurrence, so we can't really say if that's the rule or the exception 😅

espadolini · 2022-08-03T09:11:29Z

Can we have a generic suggested_labels map field instead? It could be useful in the future, maybe to add labels from the web UI, to be embedded in the script ("suggested" or "initial" because they're not enforced and can be changed from the node's config file, as opposed to some hypothetical centrally managed and enforced labeling on the cluster side).

Something like this?
bebe142

Can you please take a look?

I wonder if we should do this in the backend or let the UI/client set the labels freely 🤔

IMO, I think we should do this in the backend as opposed to let the UI generate the ID and set the label
The ID and label would be used when generating the script, and I'm not sure we want to have that coming from a client

However, given the current flow I start to doubt if that's the correct approach

r0mant

lgtm with @espadolini's comments

r0mant · 2022-08-03T22:24:27Z

I think the idea is to keep both and filter out teleport.internal labels from the UI (not sure if that's the case now).

marcoandredinis · 2022-08-05T11:19:23Z

I had to change a couple of thing in the script to ensure we support multiple values in a single label and shellcheck does not report any issue

For reference, here's the reports from shellcheck

In ./lib/web/scripts/node-join/install.sh line 44:
[ ! -z "$JOIN_METHOD" ] && JOIN_METHOD_FLAG="--join-method ${JOIN_METHOD}"
  ^-- SC2236 (style): Use -n instead of ! -z.

In ./lib/web/scripts/node-join/install.sh line 49:
[ -n "$LABELS" ] && LABELS_FLAG="--labels \"${LABELS}\""
                                 ^---------^ SC2089 (warning): Quotes/backslashes will be treated literally. Use an array.

In ./lib/web/scripts/node-join/install.sh line 448:
      ${LABELS_FLAG} \
      ^------------^ SC2128 (warning): Expanding an array without an index only gives the first element

In ./lib/web/scripts/node-join/install.sh line 448:
      ${LABELS_FLAG[@]} \
      ^---------------^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.

I tested again and it worked even with a space between the label values

For Teleport Discover, the user will be able to test connecting to a resource right after adding it. The flow should look like this: - User selects the resource type - in this case Server/Nodes - WebAPI generates a new Token - WebAPI shows the `sudo bash .../<token>/install.sh` to the user - When the user runs this command, and assuming everything works out, the WebAPI should be able to receive the Server/Node that was generated from that specific Token. To achieve this, here's a more detailed flow: - User selects the resource type - in this case Server/Nodes - WebUI generates a new Token This generates a Provision Token which contains a RefResourceID WebUI receives that token and stores it - WebUI shows the `sudo bash .../<token>/install.sh` to the user This generates a script which includes setting the labels as part of the `teleport configure` command: `$ teleport configure ... --labels teleport.internal/resource-id=abc ...` - User runs the provided command on the target host - WebAPI should be able to query the Servers/Nodes which contain that specific `<resource-id>` and allow the user to connect to it.

github-actions · 2022-08-08T17:07:27Z

@marcoandredinis See the table below for backport results.

Branch	Result
branch/v10	Failed

Add Suggested Labels to Provision Tokens (#15114) For Teleport Discover, the user will be able to test connecting to a resource right after adding it. The flow should look like this: - User selects the resource type - in this case Server/Nodes - WebUI generates a new Token - WebUI shows the `sudo bash .../<token>/install.sh` to the user - When the user runs this command, and assuming everything works out, the WebUI should be able to receive the Server/Node that was generated from that specific Token. To achieve this, here's a more detailed flow, which this PR implements - User selects the resource type - in this case Server/Nodes - WebUI generates a new Token This generates a Provision Token which contains a RefResourceID WebUI receives back that ID - WebUI shows the `sudo bash .../<token>/install.sh` to the user This generates a script which includes setting the labels as part of the `teleport configure` command: `$ teleport configure ... --labels teleport.internal/resource-id=<refResourceID> ...` - User runs the provided command on the target host - WebUI should be able to query the Servers/Nodes which contain that specific `<resource-id>` and allow the user to connect to it. Demo: ![image](https://user-images.githubusercontent.com/689271/182440692-97c75ae0-1e14-4f76-b9ff-d41060c73ed0.png)

Recently we added a way to add labels on newly added nodes based on the token. #15114 Each token now has a list of SuggestedLabels, which are used to feed that list. However, if that list is empty, the generated script would trigger the following error when running the `teleport node configure ...` command: `teleport: error: unexpected` This happens because the command is generating an empty argument `""` when running the `teleport node configure ...` command. So it looks like this: ```bash ${TELEPORT_BINARY_DIR}/teleport node configure \ --token token \ joinmethod \ --ca-pin pin \ --auth-server host:port \ "" \ --output someport ``` That empty argument breaks things. So, in order to fix it, we are going to change the default value when no labels are provided. Instead of an empty string, we'll use an empty array. Demo (teleport node configure message removed for brev No label ```bash $ LABELS_FLAG=(); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .s sh_service.labels $f enabled: "yes" commands: - name: hostname command: [hostname] period: 1m0s ``` Single label ```bash $ LABELS_FLAG=(--labels x=y); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f enabled: "yes" labels: x: "y" commands: - name: hostname command: [hostname] period: 1m0s ``` Multiple labels ```bash $ LABELS_FLAG=(--labels x=y,dev=prod); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f enabled: "yes" labels: dev: prod x: "y" commands: - name: hostname command: [hostname] period: 1m0s ```

github-actions Bot requested review from Joerger and fspmarshall August 2, 2022 11:07

marcoandredinis added backport/branch/v10 discover Issues related to Teleport Discover labels Aug 2, 2022

marcoandredinis requested review from kimlisa and ryanclark August 2, 2022 11:13

marcoandredinis marked this pull request as draft August 2, 2022 17:04

marcoandredinis force-pushed the marco/discover_label_join_token branch 2 times, most recently from 6fc0458 to 263846b Compare August 2, 2022 17:42

marcoandredinis marked this pull request as ready for review August 2, 2022 17:50

github-actions Bot requested review from capnspacehook and espadolini August 2, 2022 17:50

marcoandredinis force-pushed the marco/discover_label_join_token branch 4 times, most recently from 751e7d2 to 155c16b Compare August 3, 2022 08:41

espadolini reviewed Aug 3, 2022

View reviewed changes

r0mant reviewed Aug 3, 2022

View reviewed changes

marcoandredinis force-pushed the marco/discover_label_join_token branch from bebe142 to 6ebaf0b Compare August 4, 2022 16:33

kimlisa mentioned this pull request Aug 5, 2022

[Discover] Add resource discovery polling and static OS logins gravitational/webapps#1088

Merged

1 task

marcoandredinis force-pushed the marco/discover_label_join_token branch from 6ebaf0b to c4fd961 Compare August 5, 2022 07:28

espadolini approved these changes Aug 5, 2022

View reviewed changes

marcoandredinis force-pushed the marco/discover_label_join_token branch from c4fd961 to 89055eb Compare August 5, 2022 10:02

marcoandredinis changed the title ~~Add internal resource id to Provision Tokens~~ Add Suggested Labels to Provision Tokens Aug 8, 2022

marcoandredinis force-pushed the marco/discover_label_join_token branch from bf3c955 to 39bce53 Compare August 8, 2022 07:36

Joerger approved these changes Aug 8, 2022

View reviewed changes

github-actions Bot removed request for capnspacehook, fspmarshall and ryanclark August 8, 2022 16:40

marcoandredinis added 3 commits August 8, 2022 17:43

Replace RefResourceID with SuggestedLabels

80bceb7

fix sh issues reported by shellcheck

c3a7a37

marcoandredinis force-pushed the marco/discover_label_join_token branch from 39bce53 to c3a7a37 Compare August 8, 2022 16:43

marcoandredinis requested a review from r0mant August 8, 2022 16:45

r0mant approved these changes Aug 8, 2022

View reviewed changes

marcoandredinis enabled auto-merge (squash) August 8, 2022 16:50

marcoandredinis merged commit 117d7d2 into master Aug 8, 2022

marcoandredinis deleted the marco/discover_label_join_token branch August 8, 2022 20:27

marcoandredinis mentioned this pull request Aug 8, 2022

[v10] Add Suggested Labels to Provision Tokens (#15114) #15319

Merged

marcoandredinis mentioned this pull request Aug 22, 2022

NodeJoin script: fix when no labels are provided #15709

Merged

r0mant mentioned this pull request Sep 28, 2022

Discover: Web UI should be able to query registered Kube agent #16823

Closed

Conversation

marcoandredinis commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

espadolini Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

r0mant Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

marcoandredinis Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

espadolini Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

marcoandredinis Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

espadolini Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

marcoandredinis Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

marcoandredinis Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

r0mant Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcoandredinis commented Aug 5, 2022

Uh oh!

github-actions Bot commented Aug 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcoandredinis commented Aug 2, 2022 •

edited

Loading

marcoandredinis Aug 4, 2022 •

edited

Loading

marcoandredinis Aug 4, 2022 •

edited

Loading