diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index 9ec587bd100ec5..78dcff2a795e7a 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1594,7 +1594,7 @@ DEFINE_Validator(paimon_file_system_scheme_mappings, DEFINE_mInt32(thrift_client_open_num_tries, "1"); // http scheme in S3Client to use. E.g. http or https -DEFINE_String(s3_client_http_scheme, "http"); +DEFINE_String(s3_client_http_scheme, "https"); DEFINE_Validator(s3_client_http_scheme, [](const std::string& config) -> bool { return config == "http" || config == "https"; }); diff --git a/cloud/src/common/config.h b/cloud/src/common/config.h index 29e795ca078423..35c54e432e55b7 100644 --- a/cloud/src/common/config.h +++ b/cloud/src/common/config.h @@ -325,7 +325,7 @@ CONF_String(priority_networks, ""); CONF_Bool(enable_cluster_name_check, "false"); // http scheme in S3Client to use. E.g. http or https -CONF_String(s3_client_http_scheme, "http"); +CONF_String(s3_client_http_scheme, "https"); CONF_Validator(s3_client_http_scheme, [](const std::string& config) -> bool { return config == "http" || config == "https"; }); diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java index 31de8d1313e404..7f8859cfc04a61 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java @@ -18,9 +18,9 @@ package org.apache.doris.catalog; import org.apache.doris.common.DdlException; -import org.apache.doris.common.credentials.CloudCredentialWithEndpoint; import org.apache.doris.common.proc.BaseProcResult; import org.apache.doris.common.util.DatasourcePrintableMap; +import org.apache.doris.common.util.S3Util; import org.apache.doris.datasource.property.storage.S3Properties; import org.apache.doris.filesystem.UploadPartResult; import org.apache.doris.filesystem.spi.ObjFileSystem; @@ -103,12 +103,9 @@ protected void setProperties(ImmutableMap newProperties) throws } // the endpoint for ping need add uri scheme. - String pingEndpoint = properties.get(S3Properties.ENDPOINT); - if (!pingEndpoint.startsWith("http://") && !pingEndpoint.startsWith("https://")) { - pingEndpoint = "http://" + properties.get(S3Properties.ENDPOINT); - properties.put(S3Properties.ENDPOINT, pingEndpoint); - properties.put(S3Properties.Env.ENDPOINT, pingEndpoint); - } + String pingEndpoint = S3Util.buildEndpointUrl(properties.get(S3Properties.ENDPOINT)); + properties.put(S3Properties.ENDPOINT, pingEndpoint); + properties.put(S3Properties.Env.ENDPOINT, pingEndpoint); String region = S3Properties.getRegionOfEndpoint(pingEndpoint); properties.putIfAbsent(S3Properties.REGION, region); @@ -232,6 +229,11 @@ public void modifyProperties(Map properties) throws DdlException } // compatible with old version, Need convert if modified properties map uses old properties. S3Properties.convertToStdProperties(properties); + if (properties.containsKey(S3Properties.ENDPOINT)) { + String endpoint = S3Util.buildEndpointUrl(properties.get(S3Properties.ENDPOINT)); + properties.put(S3Properties.ENDPOINT, endpoint); + properties.put(S3Properties.Env.ENDPOINT, endpoint); + } boolean needCheck = isNeedCheck(properties); if (LOG.isDebugEnabled()) { LOG.debug("s3 info need check validity : {}", needCheck); @@ -278,18 +280,6 @@ public void modifyProperties(Map properties) throws DdlException super.modifyProperties(properties); } - private CloudCredentialWithEndpoint getS3PingCredentials(Map properties) { - String ak = properties.getOrDefault(S3Properties.ACCESS_KEY, this.properties.get(S3Properties.ACCESS_KEY)); - String sk = properties.getOrDefault(S3Properties.SECRET_KEY, this.properties.get(S3Properties.SECRET_KEY)); - String token = properties.getOrDefault(S3Properties.SESSION_TOKEN, - this.properties.get(S3Properties.SESSION_TOKEN)); - String endpoint = properties.getOrDefault(S3Properties.ENDPOINT, this.properties.get(S3Properties.ENDPOINT)); - String pingEndpoint = "http://" + endpoint; - String region = S3Properties.getRegionOfEndpoint(pingEndpoint); - properties.putIfAbsent(S3Properties.REGION, region); - return new CloudCredentialWithEndpoint(pingEndpoint, region, ak, sk, token); - } - private boolean isNeedCheck(Map newProperties) { boolean needCheck = !this.properties.containsKey(S3Properties.VALIDITY_CHECK) || Boolean.parseBoolean(this.properties.get(S3Properties.VALIDITY_CHECK)); @@ -328,4 +318,3 @@ protected void getProcNodeData(BaseProcResult result) { readUnlock(); } } - diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/util/S3Util.java b/fe/fe-core/src/main/java/org/apache/doris/common/util/S3Util.java index 3e4f4e7a62f9f6..8f62b08950575c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/util/S3Util.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/util/S3Util.java @@ -394,11 +394,7 @@ public static String extendGlobNumberRange(String pathPattern) { public static void validateAndTestEndpoint(String endpoint) throws UserException { HttpURLConnection connection = null; try { - String urlStr = endpoint; - // Add default protocol if not specified - if (!endpoint.startsWith("http://") && !endpoint.startsWith("https://")) { - urlStr = "http://" + endpoint; - } + String urlStr = buildEndpointUrl(endpoint); endpoint = endpoint.replaceFirst("^http://", ""); endpoint = endpoint.replaceFirst("^https://", ""); List whiteList = new ArrayList<>(Arrays.asList(Config.s3_load_endpoint_white_list)); @@ -434,6 +430,13 @@ public static void validateAndTestEndpoint(String endpoint) throws UserException } } + public static String buildEndpointUrl(String endpoint) { + if (endpoint.contains("://")) { + return endpoint; + } + return "https://" + endpoint; + } + /** * Check if a path pattern is deterministic, meaning all file paths can be determined * without listing. A pattern is deterministic if it contains no true wildcard characters diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java index 551448dc767a72..23380c1e363629 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java @@ -62,7 +62,7 @@ public void testFeConnection() throws Exception { String endpoint = properties.getEndpoint(); try (S3Client client = S3Util.buildS3Client( - URI.create(endpoint), + URI.create(S3Util.buildEndpointUrl(endpoint)), properties.getRegion(), Boolean.parseBoolean(properties.getUsePathStyle()), properties.getAwsCredentialsProvider())) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/dlf/DLFCatalog.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/dlf/DLFCatalog.java index fb90260f09aeae..0357dc41ec61ec 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/dlf/DLFCatalog.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/dlf/DLFCatalog.java @@ -61,10 +61,7 @@ protected FileIO initializeFileIO(Map properties, Configuration boolean isUsePathStyle = Boolean.parseBoolean(ossProperties.getUsePathStyle()); // s3 file io just supports s3-like endpoint String s3Endpoint = endpoint.replace("oss-" + region, "s3.oss-" + region); - if (!s3Endpoint.contains("://")) { - s3Endpoint = "http://" + s3Endpoint; - } - URI endpointUri = URI.create(s3Endpoint); + URI endpointUri = URI.create(S3Util.buildEndpointUrl(s3Endpoint)); FileIO io = new S3FileIO(() -> S3Util.buildS3Client(endpointUri, region, credential, isUsePathStyle)); io.initialize(properties); return io; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommand.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommand.java index b54e0d92ec1ef5..5f12f4c3172f1c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommand.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommand.java @@ -38,6 +38,7 @@ import org.apache.doris.common.FeNameFormat; import org.apache.doris.common.InternalErrorCode; import org.apache.doris.common.UserException; +import org.apache.doris.common.util.S3Util; import org.apache.doris.datasource.property.storage.StorageProperties; import org.apache.doris.filesystem.spi.ObjFileSystem; import org.apache.doris.fs.FileSystemFactory; @@ -145,7 +146,7 @@ private void checkObjectStorageInfo() throws UserException { private void tryConnect(String endpoint) throws Exception { HttpURLConnection connection = null; try { - String urlStr = "http://" + endpoint; + String urlStr = S3Util.buildEndpointUrl(endpoint); // TODO: Server-Side Request Forgery Check is still need? URL url = new URL(urlStr); SecurityChecker.getInstance().startSSRFChecking(urlStr); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java index 7fcfe99393288a..4a4264ae374ab6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java @@ -21,6 +21,7 @@ import org.apache.doris.cloud.proto.Cloud.ObjectStoreInfoPB.Provider; import org.apache.doris.cloud.proto.Cloud.StagePB.StageAccessType; import org.apache.doris.common.util.FileFormatConstants; +import org.apache.doris.common.util.S3Util; import org.apache.doris.nereids.exceptions.AnalysisException; import com.google.common.collect.ImmutableSet; @@ -155,7 +156,8 @@ public boolean isDryRun() { * getObjectStoreInfoPB */ public ObjectStoreInfoPB getObjectStoreInfoPB() { - ObjectStoreInfoPB.Builder builder = ObjectStoreInfoPB.newBuilder().setEndpoint(properties.get(ENDPOINT)) + ObjectStoreInfoPB.Builder builder = ObjectStoreInfoPB.newBuilder() + .setEndpoint(S3Util.buildEndpointUrl(properties.get(ENDPOINT))) .setRegion(properties.get(REGION)).setBucket(properties.get(BUCKET)).setPrefix(properties.get(PREFIX)) .setProvider(Provider.valueOf(properties.get(PROVIDER).toUpperCase())); if (getAccessType() == StageAccessType.AKSK) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java index 54f2a58af8ac6c..766dc03daa079c 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java @@ -194,7 +194,9 @@ public void testSerialization() throws Exception { Assert.assertEquals("s3_1", rS3Resource1.getName()); Assert.assertEquals("s3_2", rS3Resource2.getName()); - Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ENDPOINT), "http://aaa"); + Assert.assertEquals("https://aaa", rS3Resource2.getProperty(S3Properties.ENDPOINT)); + Assert.assertEquals("https://aaa", + S3Properties.getObjStoreInfoPB(rS3Resource2.getCopiedProperties()).getEndpoint()); Assert.assertEquals(rS3Resource2.getProperty(S3Properties.REGION), "bbb"); Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ROOT_PATH), "/path/to/root"); Assert.assertEquals(rS3Resource2.getProperty(S3Properties.ACCESS_KEY), "xxx"); @@ -226,11 +228,24 @@ public void testModifyProperties() throws Exception { Map modify = new HashMap<>(); modify.put("s3.access_key", "aaa"); s3Resource.modifyProperties(modify); + + modify.clear(); + modify.put(S3Properties.ENDPOINT, "new-endpoint"); + s3Resource.modifyProperties(modify); + Assert.assertEquals("https://new-endpoint", s3Resource.getProperty(S3Properties.ENDPOINT)); + Assert.assertEquals("https://new-endpoint", s3Resource.getProperty(S3Properties.Env.ENDPOINT)); + Assert.assertEquals("https://new-endpoint", + S3Properties.getObjStoreInfoPB(s3Resource.getCopiedProperties()).getEndpoint()); + + modify.clear(); + modify.put(S3Properties.Env.ENDPOINT, "http://other-endpoint"); + s3Resource.modifyProperties(modify); + Assert.assertEquals("http://other-endpoint", s3Resource.getProperty(S3Properties.ENDPOINT)); + Assert.assertEquals("http://other-endpoint", s3Resource.getProperty(S3Properties.Env.ENDPOINT)); } @Test public void testHttpScheme() throws DdlException { - // if https:// is set, it should be replaced with http:// ImmutableMap properties = ImmutableMap.of( "AWS_ENDPOINT", "https://aaa", "AWS_REGION", "bbb", @@ -242,7 +257,23 @@ public void testHttpScheme() throws DdlException { ); S3Resource s3Resource = new S3Resource("s3_2"); s3Resource.setProperties(properties); - Assert.assertEquals(s3Resource.getProperty(S3Properties.ENDPOINT), "https://aaa"); + Assert.assertEquals("https://aaa", s3Resource.getProperty(S3Properties.ENDPOINT)); + } + + @Test + public void testExplicitHttpScheme() throws DdlException { + ImmutableMap properties = ImmutableMap.of( + "AWS_ENDPOINT", "http://aaa", + "AWS_REGION", "bbb", + "AWS_ROOT_PATH", "/path/to/root", + "AWS_ACCESS_KEY", "xxx", + "AWS_SECRET_KEY", "yyy", + "AWS_BUCKET", "test-bucket", + "s3_validity_check", "false" + ); + S3Resource s3Resource = new S3Resource("s3_2"); + s3Resource.setProperties(properties); + Assert.assertEquals("http://aaa", s3Resource.getProperty(S3Properties.ENDPOINT)); } @Test diff --git a/fe/fe-core/src/test/java/org/apache/doris/common/util/S3UtilTest.java b/fe/fe-core/src/test/java/org/apache/doris/common/util/S3UtilTest.java index 4b976ed86cd3c5..9cef6a74143b4b 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/common/util/S3UtilTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/common/util/S3UtilTest.java @@ -25,6 +25,36 @@ public class S3UtilTest { + @Test + public void testBuildEndpointUrlDefaultsToHttps() { + Assert.assertEquals("https://s3.us-east-1.amazonaws.com", + S3Util.buildEndpointUrl("s3.us-east-1.amazonaws.com")); + } + + @Test + public void testBuildEndpointUrlKeepsExplicitHttp() { + Assert.assertEquals("http://127.0.0.1:9000", + S3Util.buildEndpointUrl("http://127.0.0.1:9000")); + } + + @Test + public void testBuildEndpointUrlKeepsExplicitHttps() { + Assert.assertEquals("https://s3.us-east-1.amazonaws.com", + S3Util.buildEndpointUrl("https://s3.us-east-1.amazonaws.com")); + } + + @Test + public void testBuildEndpointUrlKeepsUppercaseScheme() { + Assert.assertEquals("HTTP://127.0.0.1:9000", + S3Util.buildEndpointUrl("HTTP://127.0.0.1:9000")); + } + + @Test + public void testBuildEndpointUrlKeepsExistingScheme() { + Assert.assertEquals("ftp://127.0.0.1:21", + S3Util.buildEndpointUrl("ftp://127.0.0.1:21")); + } + @Test public void testExtendGlobNumberRange_simpleRange() { // Test simple range expansion {1..3} @@ -457,4 +487,3 @@ public void testExpandBracketPatterns_malformedBracket() { Assert.assertEquals("file[abc.csv", S3Util.expandBracketPatterns("file[abc.csv")); } } - diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommandTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommandTest.java index 606139ae58194e..cad84587630247 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommandTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/CreateStageCommandTest.java @@ -101,13 +101,20 @@ public void testAnalyzeStorageProperties() { CreateStageCommand command3 = getCreateStageCommand(sql); ObjectStoreInfoPB objectStoreInfoPB = command3.getStageProperties().getObjectStoreInfoPB(); - Assertions.assertEquals("cos.ap-beijing.myqcloud.com", objectStoreInfoPB.getEndpoint()); + Assertions.assertEquals("https://cos.ap-beijing.myqcloud.com", objectStoreInfoPB.getEndpoint()); Assertions.assertEquals("ap-beijing", objectStoreInfoPB.getRegion()); Assertions.assertEquals("tmp-bucket", objectStoreInfoPB.getBucket()); Assertions.assertEquals("tmp_prefix", objectStoreInfoPB.getPrefix()); Assertions.assertEquals("tmp_ak", objectStoreInfoPB.getAk()); Assertions.assertEquals("tmp_sk", objectStoreInfoPB.getSk()); Assertions.assertEquals(Provider.COS, objectStoreInfoPB.getProvider()); + + sql = "create stage if not exists ex_stage_1 " + + OBJ_INFO.replace("cos.ap-beijing.myqcloud.com", "http://cos.ap-beijing.myqcloud.com") + ")"; + ObjectStoreInfoPB explicitHttpObjectStoreInfoPB = + getCreateStageCommand(sql).getStageProperties().getObjectStoreInfoPB(); + Assertions.assertEquals("http://cos.ap-beijing.myqcloud.com", + explicitHttpObjectStoreInfoPB.getEndpoint()); } @Test @@ -292,7 +299,7 @@ public void testStagePB() throws Exception { Assertions.assertEquals("ex_stage_1", stagePB.getName()); Assertions.assertTrue(StringUtils.isNotBlank(stagePB.getStageId())); ObjectStoreInfoPB objInfo = stagePB.getObjInfo(); - Assertions.assertEquals("cos.ap-beijing.myqcloud.com", objInfo.getEndpoint()); + Assertions.assertEquals("https://cos.ap-beijing.myqcloud.com", objInfo.getEndpoint()); Map propertiesMap = stagePB.getPropertiesMap(); Assertions.assertEquals(4, propertiesMap.size()); Assertions.assertEquals("csv", propertiesMap.get("default.file.type"));