Add Identity API to QueryCtx and ConnectorQueryCtx #9982
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
velox/common/security/Identity.h
Outdated
|
|
||
| namespace facebook::velox::common { | ||
|
|
||
| class Identity { |
There was a problem hiding this comment.
Prefer plain struct if all the fields are real data. Class is only used to encapsulate temporary state, not real data.
|
@mbasmanova please share your feedback on this API. The CI failures are due to a missing argument from this change. I will fix them. Thanks! |
There was a problem hiding this comment.
@majetideepak Deepak, would you provide some more context about this change and how do you plan to develop it further. It might be nice to create a GitHub issue to explain overall thinking and describe development plan.
In Presto, Identity class has a lot more fields. Do you expect to add them all?
public class Identity
{
private final String user;
private final Optional<Principal> principal;
private final Map<String, SelectedRole> roles;
private final Map<String, String> extraCredentials;
private final Optional<String> selectedUser;
private final Optional<String> reasonForSelect;
/**
* extraAuthenticators is used when short-lived access token has to be refreshed periodically.
* Otherwise, extraCredentials should be used to store pre-fetched long-lived access token.
*
* extraAuthenticators will not be serialized. It has to be injected on Presto worker directly.
*/
private final Map<String, TokenAuthenticator> extraAuthenticators;
|
@mbasmanova I will open an issue with more design details. Thanks! |
velox/common/security/Identity.h
Outdated
| struct Identity { | ||
| Identity() = default; | ||
|
|
||
| Identity( |
There was a problem hiding this comment.
No need for any constructors or getters
|
@mbasmanova I opened an issue here #10107 with some details on this. |
majetideepak
changed the title
velox/connectors/Connector.h
Outdated
| @@ -331,6 +338,7 @@ class ConnectorQueryCtx { | |||
| memory::MemoryPool* const operatorPool_; | |||
| memory::MemoryPool* const connectorPool_; | |||
| const Config* const sessionProperties_; | |||
| const common::Identity* identity_; | |||
There was a problem hiding this comment.
Storing raw pointers is not safe. Can we store this by value or use a smart pointer?
There was a problem hiding this comment.
I think it's not a big problem (rather a win in readability) since ConnectorQueryCtx is always outlived by QueryCtx. This way it's more readable that QueryCtx owns the object and ConnectorQueryCtx is just borrowing it.
| namespace facebook::velox::common { | ||
|
|
||
| /// Used to store user name and credentials. | ||
| struct Identity { |
There was a problem hiding this comment.
Would it make sense to put this into 'security' namespace?
There was a problem hiding this comment.
I think so too! Fixed.
Presto clients can provide extra credentials to a connector.
The Presto protocol sends this information as part of the TaskUpdateRequest.
Add an Identity API in Velox to support this.
Resolves: #10107