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

config: UI configuration block with Vault/Consul links #11555

Merged
merged 4 commits into from
Nov 24, 2021
Merged

config: UI configuration block with Vault/Consul links #11555

merged 4 commits into from
Nov 24, 2021

Conversation

tgross
Copy link
Member

@tgross tgross commented Nov 22, 2021 •
edited
Loading

For #5860

Add ui block to agent configuration to enable/disable the web UI and
provide the web UI with links to Vault/Consul.

This PR should probably wait pending on the RFC I've circulated earlier today. But this is what it looks like with this configuration block:

ui {
  enabled = true
  consul {
    base_url = "https://consul.example.com:8500/ui"
  }
  vault {
    base_url = "https://vault.example.com:8200/ui"
  }
}

It shows up like the following in the API:

$ curl -s localhost:4646/v1/agent/self | jq '.config.UI'
{
  "Consul": {
    "BaseURL": "https://consul.example.com:8500/ui"
  },
  "Enabled": true,
  "Vault": {
    "BaseURL": "https://vault.example.com:8200/ui"
  }
}

@tgross tgross added this to the 1.2.2 milestone Nov 22, 2021
@tgross tgross self-assigned this Nov 22, 2021
@tgross tgross marked this pull request as ready for review November 22, 2021 19:30
ChaiWithJai
ChaiWithJai reviewed Nov 22, 2021
View reviewed changes
Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

I shouldn't be approving this. But, great job!

lgfa29
lgfa29 approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

LGTM. A few comments and suggestions, but none are blockers.

command/agent/http.go
s.mux.Handle("/ui/", http.StripPrefix("/ui/", s.handleUI(http.FileServer(&UIAssetWrapper{FileSystem: assetFS()}))))
s.logger.Debug("UI is enabled")
} else {
// Write the stubHTML
s.mux.HandleFunc("/ui/", func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(stubHTML))
Copy link
Contributor

Choose a reason for hiding this comment

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

stubHTML is only set when building without the ui build tag. This results in the /ui endpoint to return a 200 and a empty page.

Looking at Vault and Consul, they each do different things. Vault returns 404 response code and a an error message:
image

Consul returns a 200 and generic content body:
image

Personally I would say Vault's seems to be the most correct approach, but no strong feelings. I think we could move this handler registration to the else clause below and add a generic 404 body content in the handleRootFallthrough handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'd intentionally not touched the existing behavior for the fallthrough, but a 404 does sound a lot nicer. Will do.

Copy link
Member Author

@tgross tgross Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, having tried it just now I don't think that's very user friendly. If you go to http://localhost:4646 you end up getting a redirect to http://localhost:4646/ui with just the browser's 404 page. Having some stub content there and filling it out so that it does something more similar to Consul seems nicer.

(If the UI is disabled in the build, you get an explanation of that from https://github.com/hashicorp/nomad/blob/main/command/agent/stub_asset.go)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 8006bd6

website/content/docs/configuration/ui.mdx Outdated Show resolved Hide resolved
website/content/docs/configuration/ui.mdx Outdated Show resolved Hide resolved
website/data/docs-nav-data.json Outdated Show resolved Hide resolved
jrasell
jrasell approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

LGTM!

tgross and others added 4 commits November 24, 2021 10:50
@tgross
config: UI configuration block with Vault/Consul links
1d78a29 
Add `ui` block to agent configuration to enable/disable the web UI and
provide the web UI with links to Vault/Consul.
@tgross @lgfa29
Apply suggestions from code review
4999304 
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
@tgross
website ordering fixup
57f045d
@tgross
display message in stub when UI is disabled
8006bd6
@vercel vercel bot temporarily deployed to Preview – nomad November 24, 2021 16:05 Inactive
@tgross tgross merged commit 1160817 into main Nov 24, 2021
@tgross tgross deleted the f-ui-config-block branch November 24, 2021 16:20
@tgross
Copy link
Member Author

tgross commented Nov 24, 2021

I realized just as I pressed the squash button that the branch name f-ui-* got filtered from running all the unit tests. The new test here does pass and this change is fairly safe, but I'll keep an eye on the next build on main just to make sure we didn't miss anything 😊

$ go test ./nomad/structs/config -run TestUIConfig_Merge
ok      github.com/hashicorp/nomad/nomad/structs/config 0.004s

tgross added a commit that referenced this pull request Nov 24, 2021
@tgross
config: fix test-only failures in UI handler setup
2d4977a 
The `TestHTTPServer_Limits_Error` test never starts the agent so it
had an incomplete configuration, which caused panics in the test. Fix
the configuration.

The PR #11555 had a branch name like `f-ui-*` which caused CI to skip
the unit tests over the HTTP handler setup, so this wasn't caught in
PR review.
@tgross tgross mentioned this pull request Nov 24, 2021
Merged
tgross added a commit that referenced this pull request Nov 24, 2021
@tgross
config: fix test-only failures in UI handler setup
e64f1a9 
The `TestHTTPServer_Limits_Error` test never starts the agent so it
had an incomplete configuration, which caused panics in the test. Fix
the configuration.

The PR #11555 had a branch name like `f-ui-*` which caused CI to skip
the unit tests over the HTTP handler setup, so this wasn't caught in
PR review.
tgross added a commit that referenced this pull request Nov 24, 2021
@tgross
config: fix test-only failures in UI handler setup (#11571)
5b418a1 
The `TestHTTPServer_Limits_Error` test never starts the agent so it
had an incomplete configuration, which caused panics in the test. Fix
the configuration.

The PR #11555 had a branch name like `f-ui-*` which caused CI to skip
the unit tests over the HTTP handler setup, so this wasn't caught in
PR review.
tgross added a commit that referenced this pull request Dec 14, 2021
@tgross
Revert "config: UI configuration block with Vault/Consul links (#11555)"
9a4f846 
This reverts commit 1160817.
lgfa29 added a commit that referenced this pull request Dec 18, 2021
@lgfa29
changelog: add entries for #11555, #11557, and #11687
2f1cc57
@lgfa29 lgfa29 mentioned this pull request Dec 18, 2021
Merged
lgfa29 added a commit that referenced this pull request Dec 20, 2021
@lgfa29
changelog: add entries for #11555, #11557, and #11687 (#11706)
84ef826
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChaiWithJai ChaiWithJai ChaiWithJai left review comments

@lgfa29 lgfa29 lgfa29 approved these changes

@jrasell jrasell jrasell approved these changes

@jazzyfresh jazzyfresh Awaiting requested review from jazzyfresh

Assignees

@tgross tgross

Labels
theme/config theme/ui
Projects
None yet
Milestone
1.2.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tgross @lgfa29 @jrasell @ChaiWithJai