-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(server): Add ability to disable the UI #26682
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
base: master
Are you sure you want to change the base?
Conversation
For controlled environments, users may want to disable the UI (enabled by default)
Reviewer's GuideAdds a configurable server flag to enable or disable the Presto web UI and conditionally binds UI routes to either the full UI or a minimal "UI disabled" page, including configuration plumbing and tests. Sequence diagram for handling /ui requests with configurable web UIsequenceDiagram
actor "User" as User
participant "Browser" as Browser
participant "PrestoServer" as Server
participant "CoordinatorModule" as CoordModule
participant "StaticResourceHandler" as StaticHandler
User->>Browser: "Enter /ui URL"
Browser->>Server: "HTTP GET /ui"
Server->>CoordModule: "Resolve route for /ui"
alt "Web UI enabled (webUIEnabled = true)"
CoordModule->>StaticHandler: "Bind /ui to \"webapp\" resources with welcome \"index.html\""
CoordModule->>StaticHandler: "Bind /ui/dev and /tableau when enabled"
Server->>Browser: "Serve full UI index.html from \"webapp/index.html\""
Browser->>User: "Display full Presto UI"
else "Web UI disabled (webUIEnabled = false)"
CoordModule->>StaticHandler: "Bind /ui to \"nowebapp\" resources with welcome \"index.html\""
Server->>Browser: "Serve minimal disabled page from \"nowebapp/index.html\""
Browser->>User: "Display message: \"The Presto UI has been disabled for this environment\""
end
Class diagram for updated server configuration and coordinator moduleclassDiagram
class ServerConfig {
- Duration clusterStatsExpirationDuration
- boolean nestedDataSerializationEnabled
- Duration clusterResourceGroupStateInfoExpirationDuration
- boolean webUIEnabled = true
+ boolean isResourceManager()
+ boolean isCoordinator()
+ boolean isCatalogServer()
+ boolean isWebUIEnabled()
+ ServerConfig setWebUIEnabled(boolean webUIEnabled)
}
class CoordinatorModule {
- String DEFAULT_WEBUI_CSP
- boolean isWebUIEnabled
+ CoordinatorModule(boolean webUIEnabled)
+ void setup(Binder binder)
+ static HttpResourceBinding webUIBinder(Binder binder, String path, String classPathResourceBase)
}
class ServerMainModule {
+ void configure()
}
ServerMainModule --> ServerConfig : reads webUIEnabled flag
ServerMainModule --> CoordinatorModule : creates with isWebUIEnabled()
CoordinatorModule --> HttpResourceBinding : uses for /ui route binding
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Changing CoordinatorModule from a no-arg constructor to requiring a boolean may break existing code/tests that instantiate it directly; consider adding an overloaded no-arg constructor that defaults to
webUIEnabled = trueto preserve backward compatibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing CoordinatorModule from a no-arg constructor to requiring a boolean may break existing code/tests that instantiate it directly; consider adding an overloaded no-arg constructor that defaults to `webUIEnabled = true` to preserve backward compatibility.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| else if (serverConfig.isCoordinator()) { | ||
| install(new CoordinatorModule()); | ||
| install(new CoordinatorModule(serverConfig.isWebUIEnabled())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can bind ServerConfig in CoordinatorModule and check for UI config there?
ServerConfig severConfig = buildConfigObject(ServerConfig.class);
| webUIBinder(binder, "/tableau", "webapp/tableau"); | ||
| } | ||
| else { | ||
| webUIBinder(binder, "/ui", "nowebapp").withWelcomeFile("index.html"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we disable /dev & /tableau as well otherwise they will have 404 when webUI is disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense.
Description
For controlled environments, users may want to disable the UI
The UI is enabled by default
Impact
No impact
Test Plan
Contributor checklist
Release Notes