adoroszlai commented on code in PR #4524:
URL: https://github.com/apache/ozone/pull/4524#discussion_r1156729741
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java:
##########
@@ -127,17 +127,25 @@ private static void doNameChecks(String resName) {
}
}
- private static boolean isSupportedCharacter(char c) {
- return (c == '.' || c == '-' ||
- Character.isLowerCase(c) || Character.isDigit(c));
+ private static boolean isSupportedCharacter(char c, boolean
isNamespaceS3RuleCompliant) {
+ if (isNamespaceS3RuleCompliant) {
+ return (c == '.' || c == '-' ||
+ Character.isLowerCase(c) || Character.isDigit(c));
+ } else {
+ // allow ozone namespace to follow other volume/bucket naming convention,
+ // for example, here adds 'underscore',
+ // which is a valid character in POSIX-compliant system, like HDFS.
+ return (c == '.' || c == '-' || c == '_' ||
+ Character.isLowerCase(c) || Character.isDigit(c));
+ }
Review Comment:
This can be simplified:
```suggestion
return (c == '.' || c == '-' ||
Character.isLowerCase(c) || Character.isDigit(c)) ||
(c == '_' && !isNamespaceS3RuleCompliant);
}
```
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java:
##########
@@ -498,10 +498,10 @@ public static void validateVolumeName(String volumeName)
throws OMException {
/**
* Verify bucket name is a valid DNS name.
*/
- public static void validateBucketName(String bucketName)
+ public static void validateBucketName(String bucketName, boolean
isNamespaceS3RuleCompliant)
Review Comment:
I think the name `isNamespaceS3RuleCompliant` could be improved.
(`strictS3` may be one choice, but feel free to find another one.)
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java:
##########
@@ -184,6 +192,35 @@ public static void verifyResourceName(String resName) {
}
}
+ /**
+ * verifies that bucket name / volume name is a valid DNS name.
+ *
+ * @param resName Bucket or volume Name to be validated
+ *
+ * @throws IllegalArgumentException
+ */
+ public static void verifyResourceName(String resName, boolean
isNamespaceS3RuleCompliant) {
+
+ doNameChecks(resName);
+
+ boolean isIPv4 = true;
+ char prev = (char) 0;
+
+ for (int index = 0; index < resName.length(); index++) {
+ char currChar = resName.charAt(index);
+ if (currChar != '.') {
+ isIPv4 = ((currChar >= '0') && (currChar <= '9')) && isIPv4;
+ }
+ doCharacterChecks(currChar, prev, isNamespaceS3RuleCompliant);
+ prev = currChar;
+ }
+
+ if (isIPv4) {
+ throw new IllegalArgumentException(
+ "Bucket or Volume name cannot be an IPv4 address or all
numeric");
+ }
+ }
Review Comment:
The only difference is in the call to `doCharacterChecks`, where one
function passes `true`, the other passes the argument
`isNamespaceS3RuleCompliant`. Instead of duplicating the entire function,
`verifyResourceName(String)` can simply call `verifyResourceName(String,
boolean)` with `true`.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java:
##########
@@ -357,6 +357,12 @@ private OMConfigKeys() {
"ozone.om.s3.grpc.server_enabled";
public static final boolean OZONE_OM_S3_GRPC_SERVER_ENABLED_DEFAULT =
false;
+
+ public static final String OZONE_OM_NAMESPACE_S3_RULE_COMPLIANT =
+ "ozone.om.s3.naming.rule_compliant";
Review Comment:
I think this should mention "buckets" somewhere and "rule_compliant" could
be improved. Please avoid `_` if possible.
It also needs to be added to `ozone-default.xml`.
--
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]
