ChenSammi commented on code in PR #11305:
URL: https://github.com/apache/ozone/pull/11305#discussion_r4089468637
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/STSSecurityUtil.java:
##########
@@ -213,23 +217,116 @@ public static void
ensureResolvedStsFieldsInvariants(OzoneManagerProtocolProtos.
if (!hasSessionToken) {
// If sessionToken is missing/empty, resolved fields must be empty.
- if (s3Auth.hasResolvedStsSessionPolicy() ||
s3Auth.hasResolvedStsRoleArn() ||
- s3Auth.hasResolvedStsOriginalAccessKeyId() ||
s3Auth.hasResolvedStsTempAccessKeyId() ||
- s3Auth.hasResolvedStsSecretKeyId()) {
+ if (hasAnyResolvedStsField(s3Auth)) {
throw new OMException("Resolved STS fields must be empty when
sessionToken is not present", INVALID_TOKEN);
}
return;
}
- ensureResolvedFieldsArePresent(s3Auth);
+ if (!hasAllResolvedStsFields(s3Auth)) {
+ throw new OMException("Resolved STS fields must be present when
sessionToken is present", INVALID_TOKEN);
+ }
+ }
+
+ private static boolean hasAnyResolvedStsField(S3Authentication s3Auth) {
+ return s3Auth.hasResolvedStsSessionPolicy() ||
s3Auth.hasResolvedStsRoleArn() ||
+ s3Auth.hasResolvedStsOriginalAccessKeyId() ||
s3Auth.hasResolvedStsTempAccessKeyId() ||
+ s3Auth.hasResolvedStsSecretKeyId() ||
s3Auth.hasResolvedStsAssumedRoleId() ||
+ s3Auth.hasResolvedStsAssumedRoleUserArn();
}
- private static void ensureResolvedFieldsArePresent(S3Authentication s3Auth)
throws OMException {
- if (!s3Auth.hasResolvedStsSessionPolicy() ||
!s3Auth.hasResolvedStsRoleArn() ||
- !s3Auth.hasResolvedStsOriginalAccessKeyId() ||
!s3Auth.hasResolvedStsTempAccessKeyId() ||
- !s3Auth.hasResolvedStsSecretKeyId()) {
- throw new OMException("Resolved STS fields must be present when
sessionToken is present", INVALID_TOKEN);
+ private static boolean hasAllResolvedStsFields(S3Authentication s3Auth) {
Review Comment:
Rolling upgrade compatibility concern from Cursor
hasAllResolvedStsFields() now requires all seven resolved fields to be
present, including the two new ones.
And ensureResolvedStsFieldsInvariants() is called on the Ratis apply thread
in OzoneManagerStateMachine.runCommand() (line 707). During a rolling upgrade
where the leader is still on the old version:
1 Old leader processes an STS write request → Ratis log entry is committed
without new fields
2. A follower upgraded to the new version applies the committed entry
3. hasAllResolvedStsFields() returns false →
ensureResolvedStsFieldsInvariants() throws OMException(INVALID_TOKEN)
4. Caught by catch (IOException e) in runCommand() → createErrorResponse()
is returned
5. handler.handleWriteRequest() is never called → metadata change is NOT
applied on this follower
The Ratis log says the entry was committed while the follower silently skips
applying it. If this follower later becomes leader (e.g., after the upgrade
completes), the cluster has diverged metadata for those STS-authenticated
writes.
This makes sense if this patch cannot be part of initial release, and later
cluster is upgrade with ZDU(if ZDU is not used, then no issue).
So can we make the s3Auth.hasResolvedStsAssumedRoleId() and
s3Auth.hasResolvedStsAssumedRoleUserArn() check optional? I believe these two
fields are for GetCallerIdentity only, right?
--
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]
