Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass join token suggestedLabels to app server labels during install.sh #50720

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Pass join token suggestedLabels to app server labels during install.sh #50720

merged 4 commits into from
Jan 9, 2025
+27 −0

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Jan 3, 2025 •
edited
Loading

part of #46976

Passes suggested labels stored from join token to app server installation

Tested, going through the add app flow, adding labels, and running download script (UI changes will be in another PR):

$ tctl get app_server
kind: app_server
metadata:
  expires: "2025-01-02T23:26:07Z"
  name: grafaana
spec:
  app:
    kind: app
    metadata:
      labels:
        apple: pie     # custom label
        hello: world   # custom label
        teleport.dev/origin: config-file
        teleport.internal/resource-id: 321a00ee-0b4f-42b8-91f7-2d67ca738ea1
      name: grafaana
    spec:
      insecure_skip_verify: false
      public_addr: grafaana.kimlisa28.cloud.gravitational.io
      uri: http://localhost:3000
    version: v3
  host_id: 5a2b4c34-997a-459e-a7ba-da9669829c07
  hostname: Lisas-MacBook-Pro.local
  proxy_ids:
  - 44f96969-6e88-4376-96b1-2b7ab1f3c183
  - 0ca0dc22-7602-40c1-82d4-2a0860cde2ac
  rotation:
    current_id: ""
    last_rotated: "0001-01-01T00:00:00Z"
    schedule:
      standby: "0001-01-01T00:00:00Z"
      update_clients: "0001-01-01T00:00:00Z"
      update_servers: "0001-01-01T00:00:00Z"
    started: "0001-01-01T00:00:00Z"
  version: 16.4.6
version: v3

web ui label rendering
image

zmb3
zmb3 reviewed Jan 3, 2025
View reviewed changes
lib/web/join_tokens.go Outdated
Comment on lines 630 to 633
var buf bytes.Buffer
// If app install mode is requested but parameters are blank for some reason,
// we need to return an error.
var appServerResourceLabels []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: move this declaration up so that the comment about app install mode stays next to the code that checks settings.appInstallMode.

Suggested change
var buf bytes.Buffer
// If app install mode is requested but parameters are blank for some reason,
// we need to return an error.
var appServerResourceLabels []string
var buf bytes.Buffer, appServerResourceLabels []string
// If app install mode is requested but parameters are blank for some reason,
// we need to return an error.

lib/web/scripts/node-join/install.sh
@@ -463,6 +463,9 @@ app_service:
- name: "${APP_NAME}"
uri: "${APP_URI}"
public_addr: ${APP_PUBLIC_ADDR}
labels:{{range $index, $line := .appServerResourceLabels}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need a conditional here to handle the case when labels is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have to, this is valid labels: (no values), i also tested it without adding labels to double check

Copy link
Contributor

Choose a reason for hiding this comment

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

Some day (🤞) I will convert this script to go (the same thing we did for Server Auto Discovery scripts), and it will be much simpler to do these things

rosstimothy
rosstimothy reviewed Jan 6, 2025
View reviewed changes
lib/web/join_tokens.go Outdated
@@ -637,6 +638,12 @@ func getJoinScript(ctx context.Context, settings scriptSettings, m nodeAPIGetter
if !appURIPattern.MatchString(settings.appURI) {
return "", trace.BadParameter("appURI %q contains invalid characters", settings.appURI)
}

suggestedLabels := token.GetSuggestedLabels()
appServerResourceLabels, err = scripts.MarshalLabelsYAML(suggestedLabels, 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 6? What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its extra space to add with yaml list, but only applies when there are multi values for a key

so if labels is only env: ['dev'] yaml string is like below and the number doesn't have any affect (like this part of the code won't run)

env: dev

but if labels is env: ['dev', 'prod'] yaml string is like below (with the extra space applied before the hyphen)

env:
  - dev
  - prod

with that said though... this number won't get used because we prevent the web UI user from adding multi values for a key (the script will error out if you let it)

lib/web/join_tokens_test.go Outdated Show resolved Hide resolved
@kimlisa kimlisa force-pushed the lisa/labels-for-app branch 2 times, most recently from 4d48a07 to 9253787 Compare January 7, 2025 07:28
@kimlisa kimlisa added the no-changelog Indicates that a PR does not require a changelog entry label Jan 7, 2025
@kimlisa kimlisa requested review from marcoandredinis and removed request for vapopov January 7, 2025 20:10
@kimlisa kimlisa mentioned this pull request Jan 8, 2025
Open
1 task
marcoandredinis
marcoandredinis approved these changes Jan 8, 2025
View reviewed changes
lib/web/scripts/node-join/install.sh
@@ -463,6 +463,9 @@ app_service:
- name: "${APP_NAME}"
uri: "${APP_URI}"
public_addr: ${APP_PUBLIC_ADDR}
labels:{{range $index, $line := .appServerResourceLabels}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some day (🤞) I will convert this script to go (the same thing we did for Server Auto Discovery scripts), and it will be much simpler to do these things

@kimlisa kimlisa force-pushed the lisa/labels-for-app branch from 9253787 to 2a4e548 Compare January 9, 2025 22:28
@kimlisa kimlisa enabled auto-merge January 9, 2025 22:29
@kimlisa kimlisa added this pull request to the merge queue Jan 9, 2025
Merged via the queue into master with commit 4ee850e Jan 9, 2025
43 checks passed
@kimlisa kimlisa deleted the lisa/labels-for-app branch January 9, 2025 23:34
@public-teleport-github-review-bot
Copy link

@kimlisa See the table below for backport results.

Branch Result
branch/v17 Create PR

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

@zmb3 zmb3 zmb3 left review comments

@rosstimothy rosstimothy rosstimothy approved these changes

@marcoandredinis marcoandredinis marcoandredinis approved these changes

Assignees
No one assigned
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kimlisa @marcoandredinis @zmb3 @rosstimothy