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

Show and modify routing rules from the UI #433

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Show and modify routing rules from the UI #433

merged 1 commit into from
Feb 10, 2025

Conversation

prakhar10
Copy link
Member

@prakhar10 prakhar10 commented Aug 9, 2024 •
edited
Loading

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

Screenshot 2024-08-08 at 8 22 16 PM

Screenshot 2024-08-08 at 8 22 35 PM

Screenshot 2024-08-08 at 8 23 06 PM

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:

*

@cla-bot cla-bot bot added the cla-signed label Aug 9, 2024
willmostly
willmostly requested changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

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?

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java Outdated
Comment on lines 483 to 486
for (int i = 0; i < routingRulesList.size(); i++) {
if (routingRulesList.get(i).name().equals(routingRules.name())) {
routingRulesList.set(i, routingRules);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

that plan sounds fine to me, thanks!

@prakhar10
Copy link
Member Author

prakhar10 commented Aug 16, 2024 •
edited
Loading

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
willmostly requested changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@prakhar10 please squash these commits

@prakhar10
Copy link
Member Author

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
Copy link
Member

mosabua commented Sep 3, 2024

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
Copy link
Member Author

@mosabua @willmostly can you please review?

@mosabua
Copy link
Member

mosabua commented Oct 3, 2024

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

@prakhar10
Copy link
Member Author

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

ebyhr
ebyhr reviewed Oct 9, 2024
View reviewed changes
@prakhar10
Copy link
Member Author

@ebyhr can you review now please?

@prakhar10
Copy link
Member Author

@mosabua can you please review?

ebyhr
ebyhr requested changes Oct 27, 2024
View reviewed changes
@prakhar10
Copy link
Member Author

@ebyhr can you take a look now?

ebyhr
ebyhr reviewed Nov 19, 2024
View reviewed changes
@prakhar10
Copy link
Member Author

@willmostly @ebyhr can you review now please?

willmostly
willmostly requested changes Jan 7, 2025
View reviewed changes
gateway-ha/src/main/java/io/trino/gateway/ha/domain/RoutingRule.java
requireNonNull(name, "name is null");
actions = ImmutableList.copyOf(actions);
requireNonNull(condition, "condition is null");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Set description to "" if it is null

Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to set an empty value by default? It worth leaving a code comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the semantics of an empty value are more appropriate than null. The routing rule comes from a web form or an API call. If a description is not supplied, then I would say that the user intends it to be blank, not that the description is unknown to them.

@prakhar10 prakhar10 requested a review from willmostly January 27, 2025 18:59
@prakhar10 prakhar10 requested a review from ebyhr January 29, 2025 05:01
willmostly
willmostly approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @prakhar10 !

ebyhr
ebyhr reviewed Feb 4, 2025
View reviewed changes
gateway-ha/src/main/java/io/trino/gateway/ha/domain/RoutingRule.java
requireNonNull(name, "name is null");
actions = ImmutableList.copyOf(actions);
requireNonNull(condition, "condition is null");
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to set an empty value by default? It worth leaving a code comment.

@prakhar10 prakhar10 force-pushed the routing-rules-ui branch 2 times, most recently from 534372c to b835337 Compare February 5, 2025 21:33
@prakhar10 prakhar10 requested a review from ebyhr February 7, 2025 01:37
@willmostly willmostly merged commit 4f77442 into trinodb:main Feb 10, 2025
2 checks passed
@github-actions github-actions bot added this to the 14 milestone Feb 10, 2025
@willmostly
Copy link
Contributor

@mosabua I recommend we just use the PR title as the release note:

Show and modify routing rules from the UI

Any cleanup can be done in a follow up

@mosabua mosabua mentioned this pull request Feb 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua left review comments

@xkrogen xkrogen xkrogen left review comments

@Chaho12 Chaho12 Chaho12 left review comments

@ytwp ytwp ytwp left review comments

@willmostly willmostly willmostly approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees

@prakhar10 prakhar10

Labels
cla-signed
Milestone
14
Development

Successfully merging this pull request may close these issues.
7 participants
@prakhar10 @willmostly @mosabua @ebyhr @xkrogen @Chaho12 @ytwp