-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBase-22027: Split non-MR related parts of TokenUtil off into a Clien… #361
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
Changes from all commits
8747d9a
e76b623
fb1a743
d1979be
facfe8a
70dddb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.hbase.security.token; | ||
|
|
||
| import com.google.protobuf.ByteString; | ||
| import com.google.protobuf.ServiceException; | ||
| import java.io.IOException; | ||
| import java.lang.reflect.UndeclaredThrowableException; | ||
| import java.security.PrivilegedExceptionAction; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import org.apache.hadoop.hbase.HConstants; | ||
| import org.apache.hadoop.hbase.TableName; | ||
| import org.apache.hadoop.hbase.client.AsyncConnection; | ||
| import org.apache.hadoop.hbase.client.AsyncTable; | ||
| import org.apache.hadoop.hbase.client.Connection; | ||
| import org.apache.hadoop.hbase.client.Table; | ||
| import org.apache.hadoop.hbase.ipc.CoprocessorRpcChannel; | ||
| import org.apache.hadoop.hbase.protobuf.generated.AuthenticationProtos; | ||
| import org.apache.hadoop.hbase.security.User; | ||
| import org.apache.hadoop.io.Text; | ||
| import org.apache.hadoop.security.token.Token; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; | ||
|
|
||
| /** | ||
| * Utility methods for obtaining authentication tokens, that do not require hbase-server. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public final class ClientTokenUtil { | ||
| private static final Logger LOG = LoggerFactory.getLogger(ClientTokenUtil.class); | ||
|
|
||
| // Set in TestClientTokenUtil via reflection | ||
| private static ServiceException injectedException; | ||
|
|
||
| private ClientTokenUtil() {} | ||
|
|
||
| private static void injectFault() throws ServiceException { | ||
| if (injectedException != null) { | ||
| throw injectedException; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Obtain and return an authentication token for the current user. | ||
| * @param conn The async HBase cluster connection | ||
| * @return the authentication token instance, wrapped by a {@link CompletableFuture}. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public static CompletableFuture<Token<AuthenticationTokenIdentifier>> obtainToken( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is public so the TokenUtil on server can use this method? Can it be package private given both TokenUtil and this class are in the same java package? Then you could drop the IA Private.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... but seems like you want these to be public now? You are pointing users of TokenUtil here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is public so TokenUtil can delegate to this method, but also so any internal HBase code that needs this method can use it. The TokenUtil methods have been deprecated, so new internal HBase code would ideally use ClientTokenUtil instead. If there isn't a need for these methods elsewhere in HBase, I'm happy to try making them package private. The Javadoc on the deprecated methods in TokenUtil explicitly says not to use these methods when you are developing non-HBase code. The only users I want to point to ClientTokenUtil (except obtainAndCacheToken) are internal HBase users.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also happy to update any call sites in HBase so they use the ClientTokenUtil methods instead of the now deprecated TokenUtil methods.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The audience annotations are for downstreamers, not for hbase internally. Given ClientTokenUtil is a new class, we have opportunity for setting access as we see fit. The #toToken methods are used internal to the package only it seems (tests and the class-itself). Suggest these become package private or private especially the overrides that take protobufs. Otherwise, methods seem innocuous-looking. The less API we make public, the better otherwise, make a call. Regards changing internal usage, could do here or in a follow-on. Thanks for persisting on this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only obtainToken(AsyncConnection) seems to need to be public, as it is used by SecureBulkLoadManager. Everything else can be package private. Updated the PR to make the methods package private, as well as to get rid of uses of the deprecated TokenUtil methods.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced the use of reflection in TestClientTokenUtil with just calling the obtainToken methods directly. The existing code doesn't find package private methods, causing the test to fail. I grepped for the method names, and they don't seem to be used via reflection anywhere else in HBase. |
||
| AsyncConnection conn) { | ||
| CompletableFuture<Token<AuthenticationTokenIdentifier>> future = new CompletableFuture<>(); | ||
| if (injectedException != null) { | ||
| future.completeExceptionally(injectedException); | ||
| return future; | ||
| } | ||
| AsyncTable<?> table = conn.getTable(TableName.META_TABLE_NAME); | ||
| table.<AuthenticationProtos.AuthenticationService.Interface, | ||
| AuthenticationProtos.GetAuthenticationTokenResponse> coprocessorService( | ||
| AuthenticationProtos.AuthenticationService::newStub, | ||
| (s, c, r) -> s.getAuthenticationToken(c, | ||
| AuthenticationProtos.GetAuthenticationTokenRequest.getDefaultInstance(), r), | ||
| HConstants.EMPTY_START_ROW).whenComplete((resp, error) -> { | ||
| if (error != null) { | ||
| future.completeExceptionally(ProtobufUtil.handleRemoteException(error)); | ||
| } else { | ||
| future.complete(toToken(resp.getToken())); | ||
| } | ||
| }); | ||
| return future; | ||
| } | ||
|
|
||
| /** | ||
| * Obtain and return an authentication token for the current user. | ||
| * @param conn The HBase cluster connection | ||
| * @throws IOException if a remote error or serialization problem occurs. | ||
| * @return the authentication token instance | ||
| */ | ||
| @InterfaceAudience.Private | ||
| static Token<AuthenticationTokenIdentifier> obtainToken( | ||
| Connection conn) throws IOException { | ||
| Table meta = null; | ||
| try { | ||
| injectFault(); | ||
|
|
||
| meta = conn.getTable(TableName.META_TABLE_NAME); | ||
| CoprocessorRpcChannel rpcChannel = meta.coprocessorService( | ||
| HConstants.EMPTY_START_ROW); | ||
| AuthenticationProtos.AuthenticationService.BlockingInterface service = | ||
| AuthenticationProtos.AuthenticationService.newBlockingStub(rpcChannel); | ||
| AuthenticationProtos.GetAuthenticationTokenResponse response = | ||
| service.getAuthenticationToken(null, | ||
| AuthenticationProtos.GetAuthenticationTokenRequest.getDefaultInstance()); | ||
|
|
||
| return toToken(response.getToken()); | ||
| } catch (ServiceException se) { | ||
| throw ProtobufUtil.handleRemoteException(se); | ||
| } finally { | ||
| if (meta != null) { | ||
| meta.close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Converts a Token instance (with embedded identifier) to the protobuf representation. | ||
| * | ||
| * @param token the Token instance to copy | ||
| * @return the protobuf Token message | ||
| */ | ||
| @InterfaceAudience.Private | ||
| static AuthenticationProtos.Token toToken(Token<AuthenticationTokenIdentifier> token) { | ||
| AuthenticationProtos.Token.Builder builder = AuthenticationProtos.Token.newBuilder(); | ||
| builder.setIdentifier(ByteString.copyFrom(token.getIdentifier())); | ||
| builder.setPassword(ByteString.copyFrom(token.getPassword())); | ||
| if (token.getService() != null) { | ||
| builder.setService(ByteString.copyFromUtf8(token.getService().toString())); | ||
| } | ||
| return builder.build(); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a protobuf Token message back into a Token instance. | ||
| * | ||
| * @param proto the protobuf Token message | ||
| * @return the Token instance | ||
| */ | ||
| @InterfaceAudience.Private | ||
| static Token<AuthenticationTokenIdentifier> toToken(AuthenticationProtos.Token proto) { | ||
| return new Token<>( | ||
| proto.hasIdentifier() ? proto.getIdentifier().toByteArray() : null, | ||
| proto.hasPassword() ? proto.getPassword().toByteArray() : null, | ||
| AuthenticationTokenIdentifier.AUTH_TOKEN_TYPE, | ||
| proto.hasService() ? new Text(proto.getService().toStringUtf8()) : null); | ||
| } | ||
|
|
||
| /** | ||
| * Obtain and return an authentication token for the given user. | ||
| * @param conn The HBase cluster connection | ||
| * @param user The user to obtain a token for | ||
| * @return the authentication token instance | ||
| */ | ||
| @InterfaceAudience.Private | ||
| static Token<AuthenticationTokenIdentifier> obtainToken( | ||
| final Connection conn, User user) throws IOException, InterruptedException { | ||
| return user.runAs(new PrivilegedExceptionAction<Token<AuthenticationTokenIdentifier>>() { | ||
| @Override | ||
| public Token<AuthenticationTokenIdentifier> run() throws Exception { | ||
| return obtainToken(conn); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Obtain an authentication token for the given user and add it to the | ||
| * user's credentials. | ||
| * @param conn The HBase cluster connection | ||
| * @param user The user for whom to obtain the token | ||
| * @throws IOException If making a remote call to the authentication service fails | ||
| * @throws InterruptedException If executing as the given user is interrupted | ||
| */ | ||
| public static void obtainAndCacheToken(final Connection conn, | ||
| User user) | ||
| throws IOException, InterruptedException { | ||
| try { | ||
| Token<AuthenticationTokenIdentifier> token = obtainToken(conn, user); | ||
|
|
||
| if (token == null) { | ||
| throw new IOException("No token returned for user " + user.getName()); | ||
| } | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("Obtained token " + token.getKind().toString() + " for user " + | ||
| user.getName()); | ||
| } | ||
| user.addToken(token); | ||
| } catch (IOException | InterruptedException | RuntimeException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| throw new UndeclaredThrowableException(e, | ||
| "Unexpected exception obtaining token for user " + user.getName()); | ||
| } | ||
| } | ||
| } | ||
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.
Why are we using the non-relocated classes here?
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.
This is a mistake, nice catch. Will fix.
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.
The deleted code in TokenUtil was using non relocated classes, as AuthenticationProtos uses the unshaded classes. Should I update AuthenticationProtos to reference shaded classes, or what do you think I should do here?
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.
Which I guess would require figuring out how to modify the generated code.
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've left the imports as-is for now.
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 thats right for this client-facing class. Client stuff is all unshaded 2.5 pb hbase-protocol (as opposed to hbase-protocol-shaded).
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.
Having second thoughts now.
Why can't this new class be an Interface?
If an Interface, can hide stuff like this.
I can help refactor if you think Interface will work. Thanks
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 sure I follow. How does making ClientTokenUtil an interface affect class shading? The implementation of the ClientTokenUtil interface would still have to reference the unshaded classes, right?
If you have an idea for refactoring this, I'd be happy to merge it into this PR. If you put up a PR against the HBASE-22027 branch at https://github.com/srdo/hbase, I could update this PR with your changes.
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 tried my 'idea' and realize my suggestion of no help -- pardon me. Trying my idea did help me w/ this review though.
Yes, it is appropriate here to use the non-relocated protobuf stuff --i.e. as you have it -- while auth goes via Coprocessor API.