From 393fdb571cec0f24bc047d4b8157780253c7e9a8 Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Wed, 21 Jun 2023 13:44:14 +0530 Subject: [PATCH 1/8] HDDS-6814. Using service ID as default for HA if there is only 1 service ID configured. --- .../hadoop/ozone/shell/OzoneAddress.java | 20 ++++++++++++++----- .../hadoop/ozone/shell/s3/S3Handler.java | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index 4a9882404e7..a6392831728 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -200,12 +200,22 @@ public OzoneClient createClientForS3Commands( } else { // If om service id is not specified, consider it as a non-HA cluster. // But before that check if serviceId is defined. If it is defined - // throw an error om service ID needs to be specified. + // and has length = 1, then take that ID as default. + // otherwise throw an error om service ID needs to be specified. if (OmUtils.isServiceIdsDefined(conf)) { - throw new OzoneClientException("Service ID must not" - + " be omitted when " + OZONE_OM_SERVICE_IDS_KEY + " is defined. " + - "Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + - conf.getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY)); + Collection serviceIds=conf. + getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY); + if (serviceIds.size() == 1) { + return OzoneClientFactory.getRpcClient(serviceIds.iterator().next(), + conf); + } + else { + throw new OzoneClientException("Service ID must not" + + " be omitted when " + OZONE_OM_SERVICE_IDS_KEY + " is defined." + + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + + conf.getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY)); + } + } return OzoneClientFactory.getRpcClient(conf); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java index 34e3b9afa82..50ef9fced5c 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java @@ -34,8 +34,8 @@ public abstract class S3Handler extends Handler { @CommandLine.Option(names = {"--om-service-id"}, required = false, - description = "OM Service ID is required to be specified for OM HA" + - " cluster") + description = "OM Service ID is preferred to be specified for OM HA" + + " cluster when there are multiple service IDs") private String omServiceID; public String getOmServiceID() { From 6d3916efaa4bfcac1e377b88fcf99a0bea24c71f Mon Sep 17 00:00:00 2001 From: Tejaskriya <87555809+Tejaskriya@users.noreply.github.com> Date: Thu, 22 Jun 2023 12:44:32 +0530 Subject: [PATCH 2/8] Update hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java index 50ef9fced5c..2871dbd89ac 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java @@ -34,8 +34,8 @@ public abstract class S3Handler extends Handler { @CommandLine.Option(names = {"--om-service-id"}, required = false, - description = "OM Service ID is preferred to be specified for OM HA" + - " cluster when there are multiple service IDs") + description = "OM Service ID for OM HA, " + + "required when cluster has are multiple OM services") private String omServiceID; public String getOmServiceID() { From 48c846aa617ad839d873292245ae449eab0aeb37 Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Thu, 22 Jun 2023 12:54:19 +0530 Subject: [PATCH 3/8] Fixed exception message and description, added robot tests --- .../src/main/smoketest/s3/s3_serviceId.robot | 36 +++++++++++++++++++ .../hadoop/ozone/shell/OzoneAddress.java | 18 +++++----- .../hadoop/ozone/shell/s3/S3Handler.java | 2 +- 3 files changed, 45 insertions(+), 11 deletions(-) create mode 100644 hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot diff --git a/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot b/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot new file mode 100644 index 00000000000..c277d8daa43 --- /dev/null +++ b/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot @@ -0,0 +1,36 @@ +# 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. + +*** Settings *** +Documentation S3 gateway test with aws cli +Library OperatingSystem +Library String +Resource ../commonlib.robot +Resource commonawslib.robot +Test Timeout 5 minutes +Suite Setup Setup s3 tests + +*** Test Cases *** +Get-secret without om-service-id + Run Keyword Kinit test user testuser testuser.keytab + ${output}= Execute ozone s3 getsecret -u testuser + Should contain ${output} awsAccessKey + Should contain ${output} awsSecret + +Get-secret with om-service-id + Run Keyword Kinit test user testuser testuser.keytab + ${output}= Execute ozone s3 getsecret -u testuser ${OM_HA_PARAM} + Should contain ${output} awsAccessKey + Should contain ${output} awsSecret \ No newline at end of file diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index a6392831728..078e832f93e 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -187,6 +187,8 @@ public OzoneClient createClientForS3Commands( String omServiceID ) throws IOException, OzoneClientException { + Collection serviceIds = conf. + getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY); if (omServiceID != null) { // OM HA cluster if (OmUtils.isOmHAServiceId(conf, omServiceID)) { @@ -195,27 +197,23 @@ public OzoneClient createClientForS3Commands( throw new OzoneClientException("Service ID specified does not match" + " with " + OZONE_OM_SERVICE_IDS_KEY + " defined in the " + "configuration. Configured " + OZONE_OM_SERVICE_IDS_KEY + " are" + - conf.getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY)); + serviceIds); } } else { // If om service id is not specified, consider it as a non-HA cluster. // But before that check if serviceId is defined. If it is defined // and has length = 1, then take that ID as default. - // otherwise throw an error om service ID needs to be specified. + // otherwise throw an error "om service ID needs to be specified." if (OmUtils.isServiceIdsDefined(conf)) { - Collection serviceIds=conf. - getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY); if (serviceIds.size() == 1) { return OzoneClientFactory.getRpcClient(serviceIds.iterator().next(), conf); - } - else { + } else { throw new OzoneClientException("Service ID must not" - + " be omitted when " + OZONE_OM_SERVICE_IDS_KEY + " is defined." - + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + - conf.getTrimmedStringCollection(OZONE_OM_SERVICE_IDS_KEY)); + + " be omitted when cluster has multiple OM Services." + + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + + serviceIds); } - } return OzoneClientFactory.getRpcClient(conf); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java index 2871dbd89ac..91cde308cb2 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Handler.java @@ -35,7 +35,7 @@ public abstract class S3Handler extends Handler { @CommandLine.Option(names = {"--om-service-id"}, required = false, description = "OM Service ID for OM HA, " + - "required when cluster has are multiple OM services") + "required when cluster has multiple OM services") private String omServiceID; public String getOmServiceID() { From 3621eca29c7a7f2c93c104694bde2fc509312583 Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Thu, 22 Jun 2023 21:18:39 +0530 Subject: [PATCH 4/8] Test file changes --- .../src/main/smoketest/s3/s3_serviceId.robot | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot b/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot index c277d8daa43..146cceec5b7 100644 --- a/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot +++ b/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot @@ -17,20 +17,26 @@ Documentation S3 gateway test with aws cli Library OperatingSystem Library String +Library BuiltIn Resource ../commonlib.robot -Resource commonawslib.robot -Test Timeout 5 minutes -Suite Setup Setup s3 tests +Resource ./commonawslib.robot +Test Timeout 2 minutes +Suite Setup Setup v4 headers -*** Test Cases *** +*** Keywords *** Get-secret without om-service-id - Run Keyword Kinit test user testuser testuser.keytab - ${output}= Execute ozone s3 getsecret -u testuser + ${output}= Execute ozone s3 getsecret -u testuser2 Should contain ${output} awsAccessKey Should contain ${output} awsSecret Get-secret with om-service-id - Run Keyword Kinit test user testuser testuser.keytab - ${output}= Execute ozone s3 getsecret -u testuser ${OM_HA_PARAM} + ${output}= Execute ozone s3 getsecret -u testuser2 ${OM_HA_PARAM} Should contain ${output} awsAccessKey - Should contain ${output} awsSecret \ No newline at end of file + Should contain ${output} awsSecret + +*** Test Cases *** +S3 without om-service-id + Run Keyword if '${SECURITY_ENABLED}' == 'true' Get-secret without om-service-id + +S3 with om-service-id + Run Keyword if '${SECURITY_ENABLED}' == 'true' Get-secret with om-service-id \ No newline at end of file From 0132ab576c1a15b620375dbaa2469cf2f71bebef Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Thu, 22 Jun 2023 21:34:47 +0530 Subject: [PATCH 5/8] Removing unnecessary check for service IDs --- .../java/org/apache/hadoop/ozone/shell/OzoneAddress.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index 078e832f93e..fa76ac1af47 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -204,16 +204,11 @@ public OzoneClient createClientForS3Commands( // But before that check if serviceId is defined. If it is defined // and has length = 1, then take that ID as default. // otherwise throw an error "om service ID needs to be specified." - if (OmUtils.isServiceIdsDefined(conf)) { - if (serviceIds.size() == 1) { - return OzoneClientFactory.getRpcClient(serviceIds.iterator().next(), - conf); - } else { + if (serviceIds.size() == 1) { throw new OzoneClientException("Service ID must not" + " be omitted when cluster has multiple OM Services." + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + serviceIds); - } } return OzoneClientFactory.getRpcClient(conf); } From 6a81cb069e9ac1bae49898b6a410c95c6d9ddbae Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Fri, 23 Jun 2023 10:11:25 +0530 Subject: [PATCH 6/8] Improving check for service IDs --- .../org/apache/hadoop/ozone/shell/OzoneAddress.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index fa76ac1af47..964582f0036 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -204,11 +204,11 @@ public OzoneClient createClientForS3Commands( // But before that check if serviceId is defined. If it is defined // and has length = 1, then take that ID as default. // otherwise throw an error "om service ID needs to be specified." - if (serviceIds.size() == 1) { - throw new OzoneClientException("Service ID must not" - + " be omitted when cluster has multiple OM Services." + - " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " - + serviceIds); + if (serviceIds.size() > 1) { + throw new OzoneClientException("Service ID must not" + + " be omitted when cluster has multiple OM Services." + + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + + serviceIds); } return OzoneClientFactory.getRpcClient(conf); } From 9a87f46a037eec874ae0a752c8e64c5925e93046 Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Fri, 23 Jun 2023 10:17:51 +0530 Subject: [PATCH 7/8] Improving check for service IDs --- .../apache/hadoop/ozone/shell/OzoneAddress.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index 964582f0036..0306326cf78 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -199,19 +199,17 @@ public OzoneClient createClientForS3Commands( "configuration. Configured " + OZONE_OM_SERVICE_IDS_KEY + " are" + serviceIds); } - } else { + } else if (serviceIds.size() > 1) { // If om service id is not specified, consider it as a non-HA cluster. // But before that check if serviceId is defined. If it is defined // and has length = 1, then take that ID as default. // otherwise throw an error "om service ID needs to be specified." - if (serviceIds.size() > 1) { - throw new OzoneClientException("Service ID must not" - + " be omitted when cluster has multiple OM Services." + - " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " - + serviceIds); + throw new OzoneClientException("Service ID must not" + + " be omitted when cluster has multiple OM Services." + + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + + serviceIds); } - return OzoneClientFactory.getRpcClient(conf); - } + return OzoneClientFactory.getRpcClient(conf); } /** From 08335cd0f721ca405d687c76bcc84654dd0618bb Mon Sep 17 00:00:00 2001 From: Tejaskriya Madhan Date: Tue, 27 Jun 2023 10:29:19 +0530 Subject: [PATCH 8/8] Optimize test and remove unnecessary comments --- ...{s3_serviceId.robot => s3_getsecret.robot} | 29 ++++++++----------- .../hadoop/ozone/shell/OzoneAddress.java | 10 +++---- 2 files changed, 17 insertions(+), 22 deletions(-) rename hadoop-ozone/dist/src/main/smoketest/s3/{s3_serviceId.robot => s3_getsecret.robot} (58%) diff --git a/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot b/hadoop-ozone/dist/src/main/smoketest/s3/s3_getsecret.robot similarity index 58% rename from hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot rename to hadoop-ozone/dist/src/main/smoketest/s3/s3_getsecret.robot index 146cceec5b7..4218cc1934c 100644 --- a/hadoop-ozone/dist/src/main/smoketest/s3/s3_serviceId.robot +++ b/hadoop-ozone/dist/src/main/smoketest/s3/s3_getsecret.robot @@ -14,29 +14,24 @@ # limitations under the License. *** Settings *** -Documentation S3 gateway test with aws cli -Library OperatingSystem -Library String -Library BuiltIn +Documentation Test ozone s3 getsecret command Resource ../commonlib.robot -Resource ./commonawslib.robot Test Timeout 2 minutes -Suite Setup Setup v4 headers *** Keywords *** -Get-secret without om-service-id - ${output}= Execute ozone s3 getsecret -u testuser2 - Should contain ${output} awsAccessKey - Should contain ${output} awsSecret - -Get-secret with om-service-id - ${output}= Execute ozone s3 getsecret -u testuser2 ${OM_HA_PARAM} +Verify output + [arguments] ${output} Should contain ${output} awsAccessKey Should contain ${output} awsSecret *** Test Cases *** -S3 without om-service-id - Run Keyword if '${SECURITY_ENABLED}' == 'true' Get-secret without om-service-id +Without OM service ID + Pass Execution If '${SECURITY_ENABLED}' == 'false' N/A + ${output} = Execute ozone s3 getsecret -u testuser2 + Verify output ${output} -S3 with om-service-id - Run Keyword if '${SECURITY_ENABLED}' == 'true' Get-secret with om-service-id \ No newline at end of file +With OM service ID + Pass Execution If '${OM_HA_PARAM}' == '${EMPTY}' duplicate test in non-HA env. + Pass Execution If '${SECURITY_ENABLED}' == 'false' N/A + ${output} = Execute ozone s3 getsecret -u testuser2 ${OM_HA_PARAM} + Verify output ${output} \ No newline at end of file diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java index 0306326cf78..cef95dcf0f8 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java @@ -200,15 +200,15 @@ public OzoneClient createClientForS3Commands( serviceIds); } } else if (serviceIds.size() > 1) { - // If om service id is not specified, consider it as a non-HA cluster. - // But before that check if serviceId is defined. If it is defined - // and has length = 1, then take that ID as default. - // otherwise throw an error "om service ID needs to be specified." + // If multiple om service ids are there, + // throw an error "om service ID must not be omitted" throw new OzoneClientException("Service ID must not" + " be omitted when cluster has multiple OM Services." + " Configured " + OZONE_OM_SERVICE_IDS_KEY + " are " + serviceIds); - } + } + // for non-HA cluster and HA cluster with only 1 service ID + // get service ID from configurations return OzoneClientFactory.getRpcClient(conf); }