-
Notifications
You must be signed in to change notification settings - Fork 20
[MCP] feat: Add realm support #83
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
Conversation
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 a lot for working on it, @MonkeyCanCode ! LGTM overall. Left one minor comment.
|
|
||
| realm = arguments.get("realm") | ||
| if isinstance(realm, str) and realm.strip(): | ||
| delegate_args["realm"] = realm |
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 raise an error here if the realm isn't configured in the current MCP server? Either because that realm is invalid or MCP server just didn't configure it. We could have common method for that, as each tool needs it. Not a blocker though. I'm fine with a followup.
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.
But realm is optional as if user didn't provide the realm, it will not be set via header. In this case, it will continue to work with Polairs server as it will just default to the first set realm in polaris.realm-context.realms.
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.
Sorry, I may not have expressed myself clearly earlier. Let me restate it: if the realm config is missing, it should absolutely fall back to the non-realm setting. However, if a user explicitly configures a realm but that realm is invalid or not configured, we should fail fast rather than silently falling back.
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.
Got it. Yes, the latest PR ensure of realm specific auth is provided but failed, it will not fail back to the default one.
|
|
||
| # Try realm specific first then global | ||
| for _ in (realm, None): | ||
| creds = load_creds(_) |
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 fail fast when realm specific credentials are there? Falling back to the global one may not always be the expected behavior. Along with my comment here, https://github.com/apache/polaris-tools/pull/83/files#r2582866657, I think it may be a good idea to build a list of realms and their credentials first based on the configurations, so that we can alway fail fast for invalid/un-configured realms.
The only downside is that the MCP server cannot support dynamic configurations, like adding a new env variable when it's running, which seems fine to me. We will clarify that isn't supported.
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. Yeah, I was thinking maybe users will use same credential for diff realm and not wanting to set credential for each of them (which is currently a support deployed model with polaris server). But I think the fail-fast may be better route in this case and this also matches to what we are asking users to do via current polaris CLI:
Loading profiles from /Users/yong/.polaris/.polaris.json
Polaris Client ID: xxxxx
Polaris Client Secret: xxxxx
Polaris Host [localhost]:
Polaris Port [8181]:
Polaris Context Realm:
Polaris Context Header Name [Polaris-Realm]:
Polaris profile new created successfully.
Regarding the comment mentioned above, do we want to realm an implicit config as it can be optional and server can still support.
Let me update this to reflect this change.
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 above change is completed. Do you mind take another look when you have a chance?
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 was thinking maybe users will use same credential for diff realm and not wanting to set credential for each of them (which is currently a support deployed model with polaris server).
That's a valid point. Thanks for pointing it out. Do we want the config without realm to handle both: 1. global setting to all realms, 2. REST calls without realm header? My minor concern is that it may confuse users. For use case 1, we mighty introduce a new config like, POLARIS_GLOBAL_CLIENT_ID. I don't have a strong option on this though. Happy to discuss further.
Thanks for the change. LGTM!
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.
Yeah, maybe let's proceed with this for now (not fall back and quick fail if missing). So the behavior is matching to the CLI.
Current mcp implementation lacks of support for realm context as everything is default to the default realm. To supports environments with multiple realms, this PR adds the following:
CLIENT_ID,CLIENT_SECRET,TOKEN_SCOPE, andTOKEN_URLPOLARIS_REALM_CONTEXT_HEADER_NAMEto overwrite default realm name in HTTP headerSample output:
Call without realm:
Call with realm: