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.

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

[vtctld] Add 'enable_vtctld_ui' flag to vtctld to explicitly enable (or disable) the vtctld2 UI #9614

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Feb 1, 2022

Description

This PR closes #9194 by adding a flag to enable/disable the vtctld2 UI. This default value is true, which means that the vtctld2 UI will continue to be enabled by default. In other words, nothing will change unless operators explicitly opt out of the vtctld2 UI.

With -enable_vtctld_ui=false, any requests to any of the following vtctld URLs will 404:

  • /app/
  • /app/**/*
  • Any other unrecognized path, as those all get redirected to /app/ by this handler

This flag does not affect the HTTP API that the vtctld serves on /api/ (used by the web app).

On nomenclature.

I went back and forth for far too long on whether the flag should be enable_ (and default to true) or disable_ (and default to false).

Ultimately, I went with enable_ given the next step will be to disable the UI by default, meaning this flag will default to false and Vitess operators will have to explicitly opt-in to enable the UI. (We can discuss the timing and communication around this next step in the forthcoming RFC for removing the vtctld2 codebase.)

That said, I'm happy to switch this up if another approach would make more sense. :)

What it looks like

An easy way to test this out is to update examples/local/scripts/vtctld-up.sh with the flag as follows:

vtctld \
 $TOPOLOGY_FLAGS \
 -enable_vtctld_ui=false \
# ... other vtctld flags go here

(Do note there is no space before the false value as described in the flag package.)

Then, all requests to http://localhost:15000/app/ (and any subroutes) will 404.

Screen Shot 2022-02-02 at 12 18 36 PM

One can also verify that /api/ requests still work:

$ curl http://localhost:15000/api/keyspaces/
[
  "commerce"
]

Related Issue(s)

Closes #9194

Checklist

  • Should this PR be backported? No, I think it's fine for this to wait for 14.0 unless folks feel strongly that it should be backported.
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Noted above.

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg doeg added release notes Component: web UI Type: Enhancement Logical improvement (somewhere between a bug and feature) and removed Backport me! labels Feb 1, 2022
@@ -200,3 +170,41 @@ func InitVtctld(ts *topo.Server) error {

return nil
}

func webAppHandler(w http.ResponseWriter, r *http.Request) {
if !*enableUI {
Copy link
Contributor Author

@doeg doeg Feb 2, 2022

Choose a reason for hiding this comment

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

This if statement to check the flag is the only new thing in this function. The rest is extracted from above without any changes.

@doeg doeg marked this pull request as ready for review February 2, 2022 17:35
@doeg doeg requested a review from ajm188 as a code owner February 2, 2022 17:35
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

I approve of this change 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: web UI Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to optionally disable the vtctld2 UI
2 participants