-
Notifications
You must be signed in to change notification settings - Fork 20
Introducing Apache Polaris Console #85
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: main
Are you sure you want to change the base?
Conversation
- Migrate build system to Vite with TypeScript and Tailwind CSS - Reorganize codebase into modular structure (pages/, components/, api/, hooks/, lib/, types/) - Replace legacy components with modern React patterns using Radix UI, TanStack Query, and React Router - Update dependencies and add comprehensive UI component library - Remove deprecated files and consolidate routing and state management
- Update errorHandler to check response.data.error.message first - Use getErrorMessage in Connections page for better error handling - Add missing toast import in GrantCatalogRoleModal
- Remove unused functions, imports, and parameters across codebase - Clean up unused error state in PrincipalsTab - Remove unused type imports
Allow users to configure the realm header name through VITE_POLARIS_REALM_HEADER_NAME environment variable, defaulting to Polaris-Realm. This enables compatibility with custom Polaris server configurations where the realm header name differs from the default.
Change primary color to teal (#01656f) and update all components to use theme colors instead of hardcoded values. Simplify header dropdown menu by removing repetitive user information and keeping only logout action.
…or best practices Implement drag-to-resize functionality for catalog explorer sidebar with width persistence. Extract resize logic into reusable custom hook and move width constants to shared constants file. Refactor component to use proper hooks, memoization, and improved type safety.
Centralize catalog node prefix constant by moving it from component to shared constants file for better maintainability and reusability across the codebase.
|
You can see the current console in action here: https://drive.google.com/file/d/1crPlUyQQPtEAZPO37for7SQvp4NwR_Pj/view?usp=sharing |
flyrain
left a comment
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.
Thanks for working on it! Looks amazing!
| // Always use relative URL to go through the proxy (dev server or production server) | ||
| // This avoids CORS issues by proxying requests through the server | ||
| // The server.ts proxy handles /api routes in production, and Vite handles them in development | ||
| const TOKEN_URL = "/api/catalog/v1/oauth/tokens" |
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.
Can we have a configurable token url for 3-rd party IDPs or different remote Polaris instance? Not a blocker. I'm fine with filing an issue and resolving it later.
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.
@flyrain i have implemented 3 rd party IDPs support in here https://github.com/jbonofre/polaris-tools/tree/keycloak-support i need some help me with testing this one
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.
Yes we have a WiP about that. I will work on it as well.
| formData.append("grant_type", "client_credentials") | ||
| formData.append("client_id", clientId) | ||
| formData.append("client_secret", clientSecret) | ||
| formData.append("scope", "PRINCIPAL_ROLE:ALL") |
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.
I think the scope should be the part of the configuration along with client id and secret. We can provide a default value, but avoid hardcoding it.
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.
Good point. Let's do that.
| }, | ||
|
|
||
| logout: (): void => { | ||
| localStorage.removeItem("polaris_access_token") |
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.
Storing tokens in localStorage has security implications.
Because localStorage is readable by any JavaScript running on the page, an XSS vulnerability would allow an attacker to extract the access token.
Not a blocker for an internal/admin console, but we should at least track this as a follow-up improvement (e.g., memory-only tokens).
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.
I don't think it's a blocker as it's browser side. However I agree that it would be a nice improvement using browser in memory.
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.
Can we file an issue for this? Thanks!
| <!-- Teal background --> | ||
| <rect width="32" height="32" fill="#0F766E"/> | ||
| <!-- White four-pointed star --> | ||
| <path d="M16 4L19.5 12.5L28 16L19.5 19.5L16 28L12.5 19.5L4 16L12.5 12.5L16 4Z" fill="white"/> |
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.
Can we use the one in the main repo to be consistent?
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.
Agree. Let me do that.
| # under the License. | ||
|
|
||
| # Build stage | ||
| FROM node:20-alpine AS builder |
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.
Does it makes sense to use UBI images as well here as that is what we are using for polaris images. In this case, we will be looking at the following:
| realm?: string | ||
| ): Promise<OAuthTokenResponse> => { | ||
| const formData = new URLSearchParams() | ||
| formData.append("grant_type", "client_credentials") |
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.
I would suggest the same for grant_type as what @flyrain points out. There are 2 allowed values for it: https://github.com/apache/polaris/blob/ab421e0f1330d1c95eb9a4f07f9bfab10c7f7e2e/runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/TokenRequestValidator.java#L32
| params.parent = encodeNamespace(parent) | ||
| } | ||
|
|
||
| const response = await apiClient |
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.
NIT: should we consider for adding support for pageSize as if an user create high number of namespace, the UI loading will taking extremely long of time.
console/README.md
Outdated
| Then, you run Polaris Console using: | ||
|
|
||
| ``` | ||
| docker run -p 8080:80 apache/polaris-console:latest |
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.
Does it makes sense to add a docker compose so people can easily try out? As the console and server are running in diff networks, we will need to either bridge it from local or put them in a virtual network with docker network.
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.
I'm trying to cover this by introducing Helm chart that will help to deploy Console into the same namespace and network as the main Apache Polaris instance.
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.
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| openapi: 3.0.3 |
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.
I think that a copy & paste is fine for now, but eventually, I'd expect that we would have some way of pulling in the Polaris APIs from the Polaris instance.
Same with the polaris-management-service spec as well.
Do you have an idea on how we want to do that? It's OK if not. We can just file a Git issue.
| "private": true, | ||
| "version": "1.3.0-incubating", | ||
| "type": "module", | ||
| "scripts": { |
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.
I'm not seeing any unit tests or any way to test this yet. Can we file a Git issue, so we can handle this after merge?
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.
yes definetly lets create a Git to track this, will work on them, starting next week
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.
I propose to wait the PR merged before creating the corresponding issues as it can look weird for contributors if they don't find the actual code 😄
I will create all issues after this PR.
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.
Sounds good! I just want to ensure they are tracked. Thanks, @sohanhonavar & @jbonofre, y'all can close this comment when you track it on your side. :)
| } | ||
|
|
||
| // Response interceptor for error handling | ||
| const responseErrorInterceptor = (error: unknown) => { |
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 have a way to do a retry for transient failures?
|
Great job, everything works fine for me as local DEV, except the known issues with Dockerfile. I was trying to fix Dockerfile issues and introduce a helm chart for unblocking a possibility to build and deploy Console UI into existing K8S namespace and connect to Polaris API. This PR can be merged and delivered with this PR together - jbonofre#2 |
binarycat0
left a comment
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.
I double-checked the current version, and it works for me well.
I followed the readme instruction that added in this PR - https://github.com/jbonofre/polaris-tools/tree/ui/console#k8s-deployment-with-helm
- dependencies installed from scratch
- docker images builds
- helm chart with console deploys well
- Console UI connects to existing polaris POD
- Authentication works and UI is functional
This is a great achievement and can unblock those who interested in this project and UI experience.
I believe that all the mentioned improvements like "External IDP AuthN" can be delivered in parallel in the follow-up PRs.
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.
I tested the UI locally and it worked fine and looked great :) I do not have much to say about the code itself as I'm not familiar with the framework.
|
Local testing works now with latest commit and I am able to integrated it with Polaris server in helm as well. There are couple files missed license headers. Other than this, same as @dimas-b , I am not very familiar with the framework as well. |
This PR is proposing the Apache Polaris Console, an UI to browse and manage Polaris server and entities.
A huge thanks to the Console contributors: @sohanhonavar and @binarycat0