Show and modify routing rules from the UI

[prakhar10]

Description

1. Adding a way to display routing rules and also modify them from the UI
2. Minor change to show a user icon with a gear if user is ADMIN or a regular user icon if its a regular USER on the top right corner of the UI
3. Add config option to disable UI pages

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(X) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* 

[willmostly]

This looks like a nice improvement, thank you! I reviewed the Java half, and will try to find someone for the typescript.

My biggest concern here is not with the code, but with allowing users to supply input that will be written to disk on the server, then executed by the jeasy rules engine. We should take a close look at jeasy and mvel to understand what security vulnerabilities we might be introducing. At minimum we should log all of the updates so that they can be audited. @wendigo and @ebyhr wdyt?

[willmostly]

as the name of this method suggests, this updates an existing rule. It does not appear that this change adds support for deleting an existing rule or adding a new one. I guess this may be due to the UI changes required, but would it be difficult to add this functionality?

[prakhar10]

I don't think it would be difficult. I thought of initially going with only updates and in a separate PR i will put Add and Delete functionality. Will that be ok? Or should I add it in this PR itself?

[willmostly]

that plan sounds fine to me, thanks!

[prakhar10]

This looks like a nice improvement, thank you! I reviewed the Java half, and will try to find someone for the typescript.

My biggest concern here is not with the code, but with allowing users to supply input that will be written to disk on the server, then executed by the jeasy rules engine. We should take a close look at jeasy and mvel to understand what security vulnerabilities we might be introducing. At minimum we should log all of the updates so that they can be audited. @wendigo and @ebyhr wdyt?

This functionality will be accessible only for users with ADMIN privileges. Regular users will have this as Read-Only. I also have a functionality implemented in our internal repo where we are auditing changes being made to the Clusters from the UI. I can raise a PR for that as well and will add functionality to audit routing rules as well. I was thinking of getting this reviewed first and then from comments and suggestions I will raise the Audit logs PR.

[willmostly]

this is looking good, we're almost there. @mosabua and I discussed the need to support this for clustered deployments. We should also provide a way to disable this feature in case someone doesn't want to bother setting up a shared filesystem

[willmostly]

@prakhar10 please squash these commits

[prakhar10]

this is looking good, we're almost there. @mosabua and I discussed the need to support this for clustered deployments. We should also provide a way to disable this feature in case someone doesn't want to bother setting up a shared filesystem

So this means provide an option or a config to show/disable the Routing Rules option in the left panel of the UI right?

[mosabua]

this is looking good, we're almost there. @mosabua and I discussed the need to support this for clustered deployments. We should also provide a way to disable this feature in case someone doesn't want to bother setting up a shared filesystem

So this means provide an option or a config to show/disable the Routing Rules option in the left panel of the UI right?

I think for now we can just say in the docs that you must provide shared storage for that feature to work and in terms of disabling it could be just a config option option .. I think over time we will need various config options for the UI anyway

[prakhar10]

@mosabua @willmostly can you please review?

[mosabua]

I assume that @prakhar10 and @willmostly are collaborating on this currently .. ping me if review or anything else is needed.

[prakhar10]

I need to make changes according to @ebyhr 's comments. So i will get that done by this weekend.

[prakhar10]

@ebyhr can you review now please?

[prakhar10]

@mosabua can you please review?

[prakhar10]

@ebyhr can you take a look now?

[prakhar10]

@willmostly @ebyhr can you review now please?

[willmostly]

The method definition includes

@Consumes(MediaType.APPLICATION_JSON)

Does this curl command work without

-H 'Content-Type: application/json'

?

[prakhar10]

Let me check

[prakhar10]

It does not work without it, I will add it to the documentation.

[willmostly]

Should these methods return the bare type instead of wrapping them in a Response, similar to the Trino UI? E.g. https://github.com/trinodb/trino/blob/bb80ff8be6073bc970501825e2e7f304b3b9d643/core/trino-main/src/main/java/io/trino/server/ui/ClusterResource.java#L51

[prakhar10]

I tried to keep it similar to other methods and also here -

trino-gateway/webapp/src/api/base.ts

Line 6 in 64dcbd5

async get(url: string, params: Record<string, any> = {}): Promise<any> {

we are checking the response status and then extracting the json body, so to not make any changes to this code I kept it as is.

[willmostly]

Ok in that case

[willmostly]

Suggested change

	For this feature to work with multiple replicas of the Trino Gateway, you will need to provide a shared storage for the routing rules file. If multiple replicas are used with local storage, then rules will get out of sync when updated.
	For this feature to work with multiple replicas of the Trino Gateway, you will need to provide a shared storage that supports file locking for the routing rules file. If multiple replicas are used with local storage, then rules will get out of sync when updated.

[willmostly]

Suggested change

	List<RoutingRule> currentRoutingRulesList = new ArrayList<>();
	List<RoutingRule> currentRoutingRulesList = getRoutingRules();

also remove the the logic below to read rules

[prakhar10]

I was doing this earlier but changed it due to this review comment - #433 (comment)

[willmostly]

You could use an immutable currentRoutingRulesList in this method if you move your replacement logic to the write phase. For example, delete

           for (int i = 0; i < currentRoutingRulesList.size(); i++) {
                if (currentRoutingRulesList.get(i).name().equals(routingRule.name())) {
                    currentRoutingRulesList.set(i, routingRule);
                    break;
                }
            }

then modify the loop where you build yamlWriter and updatedRoutingRulesBuilder to

            for (RoutingRule rule : currentRoutingRulesList) {
                if (rule.name().equals(routingRule.name())) {
                   yamlContent.append(yamlWriter.writeValueAsString(routingRule));
                   updatedRoutingRulesBuilder.add(routingRule);
               } else {
                   yamlContent.append(yamlWriter.writeValueAsString(rule));
                   updatedRoutingRulesBuilder.add(rule);
               }
            }

Please double check the code - I wrote it in the GH text box :). If this change causes some issue then please disregard the suggestion, we can clean up the code in a follow up.

[willmostly]

Lock the file before it is read. Otherwise you allow a sequence like

1. Gateway 1 reads rules
2. Gateway 2 reads rules
3. Gateway 1 locks rules, writes rules, releases lock.
4. Gateway 2 locks rules, writes rules, releases lock.

Which results in the changes by Gateway 1 being lost.

[willmostly]

Please add tests that use the REST API

[prakhar10]

@willmostly sorry I didn't understand this, can you provide an examples or please help me understand?

[prakhar10]

Discussed with Will, working on this.

[willmostly]

priority should not be nullable. It may be defaulted to 0.

[willmostly]

Set description to "" if it is null