chungen0126 commented on code in PR #10652:
URL: https://github.com/apache/ozone/pull/10652#discussion_r3673799347
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequestWithFSO.java:
##########
@@ -202,11 +206,22 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
// Prepare response. Sets user given full key name in the 'keyName'
// attribute in response object.
int clientVersion = getOmRequest().getVersion();
- omResponse.setCreateKeyResponse(CreateKeyResponse.newBuilder()
+ CreateKeyResponse.Builder createKeyResponseBuilder =
CreateKeyResponse.newBuilder()
.setKeyInfo(omFileInfo.getNetworkProtobuf(keyName, clientVersion,
keyArgs.getLatestVersionLocation()))
.setID(clientID)
- .setOpenVersion(openVersion).build())
+ .setOpenVersion(openVersion);
+ if (getOmRequest().hasS3Authentication() &&
ozoneManager.isSecurityEnabled()
+ && createKeyRequest.hasDerivedKeyPiggyBacking()
+ && createKeyRequest.getDerivedKeyPiggyBacking()
+ ) {
+ OzoneTokenIdentifier s3Token =
S3SecurityUtil.constructS3Token(getOmRequest());
+ if (s3Token.getTokenType().equals(OMTokenProto.Type.S3AUTHINFO)) {
+ byte[] derivedKey =
ozoneManager.getS3DerivedKey(s3Token.getAwsAccessId(), s3Token.getStrToSign());
+
createKeyResponseBuilder.setDerivedKey(ByteString.copyFrom(derivedKey));
+ }
+ }
+ omResponse.setCreateKeyResponse(createKeyResponseBuilder.build())
Review Comment:
> 1. The same 7-line derived-key block is copy-pasted in
**OMKeyCreateRequest and OMKeyCreateRequestWithFSO.** Any fix in one path can
be missed in the other (FSO is the default layout for new buckets).
Extracted Shared Helper: I have extracted the shared logic into
OMKeyCreateRequest#getResponseBuilderWithDerivedKey to eliminate the code
duplication across both request classes.
> 2. Right now getS3DerivedKey() runs after addCacheEntry(). If secret
lookup fails, the open key is already in the cache but the client gets an error
and never commits — leaving an orphaned open key until cleanup.
> Also, this runs in validateAndUpdateCache, which executes on all OM nodes
during Raft apply, so every follower does a secret lookup even though only the
leader’s response is returned to the client.
>
> Suggested order: extract the shared helper first, then use it before
addCacheEntry in both classes. Optionally I think we can add preExecute
validation so invalid piggyback requests fail before Raft submission.
I moved getS3DerivedKey() to execute before addCacheEntry() as suggested.
However, I don't think we need to move it there. If security is enabled,
authentication and secret lookup validation are already performed before any
operations. If there are any security/secret issue, an exception will be thrown
early in that initial check anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
