github-actions[bot] commented on code in PR #68540:
URL: https://github.com/apache/doris/pull/68540#discussion_r4335134084
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/S3ResourceTest.java:
##########
@@ -323,4 +402,713 @@ public void testPingS3WithRoleArn() {
Assertions.assertTrue(false, e.getMessage());
}
}
+
+ @Test
+ public void testGsEndpointAndAlterAliases() throws Exception {
+ Map<String, String> properties = new HashMap<>();
+ properties.put("provider", "gcp");
+ properties.put("s3.endpoint", "https://old.example.com";);
+ properties.put("gs.endpoint", "https://storage.googleapis.com";);
+ properties.put("s3.region", "us-central1");
+ properties.put("s3.bucket", "bucket");
+ properties.put("s3.root.path", "prefix");
+ properties.put("s3.access_key", "access");
+ properties.put("s3.secret_key", "secret");
+ properties.put("s3_validity_check", "false");
+ properties.put("type", "s3");
+ S3Resource resource = new S3Resource("gcp_resource");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Assertions.assertEquals("us-central1",
resource.getProperty(S3ResourceCompat.REGION));
+ Assertions.assertEquals("bucket",
resource.getProperty(S3ResourceCompat.BUCKET));
+ Assertions.assertEquals("prefix",
resource.getProperty(S3ResourceCompat.ROOT_PATH));
+ Assertions.assertEquals("https://storage.googleapis.com";,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ Assertions.assertEquals("https://storage.googleapis.com";,
+
S3ThriftAdapter.getS3TStorageParam(resource.getCopiedProperties()).getEndpoint());
+
Assertions.assertFalse(resource.getCopiedProperties().containsKey("gs.endpoint"));
+ resource.modifyProperties(ImmutableMap.of("gs.endpoint",
"https://custom.example.com";,
+ "s3_validity_check", "false"));
+ Assertions.assertEquals("https://custom.example.com";,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ resource.modifyProperties(ImmutableMap.of("s3.endpoint",
"https://storage.googleapis.com";,
+ "s3_validity_check", "false"));
+ Assertions.assertEquals("https://storage.googleapis.com";,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ Assertions.assertEquals("https://storage.googleapis.com";,
properties.get("gs.endpoint"));
+ Assertions.assertEquals("https://old.example.com";,
properties.get("s3.endpoint"));
+ }
+
+ @Test
+ public void testExplicitS3DefaultChainSurvivesGcsPolicyAndVaultCreation()
throws Exception {
+ Map<String, String> properties = new HashMap<>();
+ properties.put("provider", "S3");
+ properties.put("type", "s3");
+ properties.put("s3.endpoint", "https://storage.googleapis.com";);
+ properties.put("s3.region", "us-central1");
+ properties.put("s3.bucket", "bucket");
+ properties.put("s3.root.path", "prefix");
+ AtomicBoolean pinged = new AtomicBoolean();
+ try (MockedStatic<S3Resource> resourceMock =
Mockito.mockStatic(S3Resource.class, Mockito.CALLS_REAL_METHODS);
+ MockedStatic<Env> envMock = Mockito.mockStatic(Env.class)) {
+
envMock.when(Env::getCurrentEnv).thenReturn(Mockito.mock(Env.class));
+ resourceMock.when(() -> S3Resource.pingS3(Mockito.anyString(),
Mockito.anyString(), Mockito.anyMap()))
+ .thenAnswer(invocation -> {
+ Map<String, String> pingProperties =
invocation.getArgument(2);
+ StorageAdapter adapter =
StorageAdapter.of(pingProperties);
+ Assertions.assertEquals("S3",
adapter.getSpiProperties().providerName());
+ Assertions.assertEquals("DEFAULT",
adapter.getBackendConfigProperties()
+ .get("AWS_CREDENTIALS_PROVIDER_TYPE"));
+ pinged.set(true);
+ return null;
+ });
+ CreateResourceCommand command = new CreateResourceCommand(
+ new CreateResourceInfo(false, false, "s3_vault",
ImmutableMap.copyOf(properties)));
+ command.getInfo().analyzeResourceType();
+ S3StorageVault vault = new S3StorageVault("s3_vault", false,
false, command);
+ Assertions.assertTrue(pinged.get());
+ Map<String, String> stored = vault.getCopiedProperties();
+
Assertions.assertFalse(S3ThriftAdapter.getS3TStorageParam(stored).isSetCredProviderType());
+
Assertions.assertFalse(CloudObjectStoreAdapter.getObjStoreInfoPB(stored).hasCredProviderType());
+ // Anonymous remains an explicit choice and remains forbidden for
GCS vaults.
+ stored.put("s3.credentials_provider_type", "ANONYMOUS");
+
Assertions.assertEquals(org.apache.doris.thrift.TCredProviderType.ANONYMOUS,
+
S3ThriftAdapter.getS3TStorageParam(stored).getCredProviderType());
+ Assertions.assertThrows(IllegalArgumentException.class,
+ () -> CloudObjectStoreAdapter.getObjStoreInfoPB(stored));
+ }
+ }
+
+ @Test
+ public void testInferredGcpAliasesForResourceAndVault() throws Exception {
+ Map<String, String> properties = new HashMap<>();
+ properties.put("type", "s3");
+ properties.put("gs.endpoint", "https://storage.googleapis.com";);
+ properties.put("gs.credential_provider_type", "DEFAULT");
+ properties.put("s3.bucket", "bucket");
+ properties.put("s3.root.path", "prefix");
+ properties.put("s3_validity_check", "false");
+ S3Resource resource = new S3Resource("inferred_gcp");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Assertions.assertEquals("GCP", resource.getProperty("provider"));
+ Assertions.assertEquals(properties.get("gs.endpoint"),
resource.getProperty("s3.endpoint"));
+ resource.modifyProperties(ImmutableMap.of("gs.connection.timeout",
"789"));
+ Assertions.assertEquals("789",
resource.getProperty("s3.connection.timeout"));
+ try (MockedStatic<Env> envMock = Mockito.mockStatic(Env.class)) {
+
envMock.when(Env::getCurrentEnv).thenReturn(Mockito.mock(Env.class));
+ CreateResourceCommand command = new CreateResourceCommand(
+ new CreateResourceInfo(false, false, "gcp_vault",
ImmutableMap.copyOf(properties)));
+ command.getInfo().analyzeResourceType();
+ S3StorageVault vault = new S3StorageVault("gcp_vault", false,
false, command);
+ Assertions.assertEquals("https://storage.googleapis.com";,
+
CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties()).getEndpoint());
+
Assertions.assertTrue(CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties())
+ .getCredential().hasGcpCredential());
+ }
+ // Legacy persisted resources can have an inferred identity without a
provider field.
+ Field field = S3Resource.class.getDeclaredField("properties");
+ field.setAccessible(true);
+ Map<String, String> legacy = resource.getCopiedProperties();
+ legacy.remove("provider");
+ field.set(resource, legacy);
+ resource.modifyProperties(ImmutableMap.of("gs.connection.timeout",
"987"));
+ Assertions.assertEquals("987",
resource.getProperty("s3.connection.timeout"));
+ Assertions.assertEquals("GCP", resource.getProperty("provider"));
+ }
+
+ @Test
+ public void testGsEndpointForStorageVault() throws Exception {
+ Env env = Mockito.mock(Env.class);
+ try (MockedStatic<Env> mockedEnv = Mockito.mockStatic(Env.class)) {
+ mockedEnv.when(Env::getCurrentEnv).thenReturn(env);
+ for (String provider : new String[] {"GCP", "gcp", "S3", "OSS"}) {
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", provider);
+ properties.put("gs.endpoint",
"https://storage.googleapis.com";);
+ properties.put("gs.access_key", "gcp-access");
+ properties.put("gs.secret_key", "gcp-secret");
+ properties.put("gs.use_path_style", "true");
+ properties.put("s3.root.path", "prefix");
+ CreateResourceCommand command = new CreateResourceCommand(
+ new CreateResourceInfo(false, false, "gcp_vault",
ImmutableMap.copyOf(properties)));
+ command.getInfo().analyzeResourceType();
+ S3StorageVault vault = new S3StorageVault("gcp_vault", false,
false, command);
+ vault.checkCreationProperties(ImmutableMap.copyOf(properties));
+ Assertions.assertEquals("prefix",
vault.getCopiedProperties().get(S3ResourceCompat.ROOT_PATH));
+ String expectedEndpoint = "GCP".equalsIgnoreCase(provider)
+ ? "https://storage.googleapis.com"; : s3Endpoint;
+ Assertions.assertEquals(expectedEndpoint,
+
CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties()).getEndpoint());
+ Assertions.assertEquals("GCP".equalsIgnoreCase(provider) ?
"gcp-access" : s3AccessKey,
+
CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties()).getAk());
+ Assertions.assertEquals("GCP".equalsIgnoreCase(provider) ?
"gcp-secret" : s3SecretKey,
+
CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties()).getSk());
+ Assertions.assertEquals("GCP".equalsIgnoreCase(provider),
+
CloudObjectStoreAdapter.getObjStoreInfoPB(vault.getCopiedProperties()).getUsePathStyle());
+ }
+ }
+ }
+
+ @Test
+ public void testStorageVaultRejectsNonAllowlistedAliases() throws
Exception {
+ StorageVaultMgr manager = new StorageVaultMgr(null);
+ for (String key : new String[] {"gs.endpoint",
S3ResourceCompat.ENDPOINT, S3ResourceCompat.Env.ENDPOINT,
+ "gs.access_key", "gs.secret_key", "gs.session_token",
"gs.connection.maximum",
+ "gs.connection.request.timeout", "gs.connection.timeout",
"gs.use_path_style",
+ "gs.force_parsing_by_standard_uri"}) {
+ try {
+ manager.alterStorageVault(StorageVault.StorageVaultType.S3,
+ ImmutableMap.of(key, "https://custom.example.com";),
"gcp_vault");
+ Assertions.fail("Storage Vault must reject properties outside
its ALTER whitelist: " + key);
+ } catch (IllegalArgumentException exception) {
+ Assertions.assertEquals("Alter property " + key + " is not
allowed.", exception.getMessage());
+ }
+ }
+ }
+
+ @Test
+ public void testBlankGsEndpointPreservesResourceAliases() throws Exception
{
+ for (String blank : new String[] {"", " \t"}) {
+ for (String endpointKey : new String[] {S3ResourceCompat.ENDPOINT,
S3ResourceCompat.Env.ENDPOINT}) {
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", "GCP");
+ properties.remove(S3ResourceCompat.Env.ENDPOINT);
+ properties.put(endpointKey, s3Endpoint);
+ properties.put("gs.endpoint", blank);
+ properties.put("gs.connection.timeout", blank);
+ S3Resource resource = new S3Resource("gcp_resource");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+
Assertions.assertFalse(resource.getCopiedProperties().containsKey("gs.endpoint"));
+ Assertions.assertEquals(Integer.parseInt(s3ConnTimeoutMs),
+
S3ThriftAdapter.getS3TStorageParam(resource.getCopiedProperties()).getConnTimeoutMs());
+ Map<String, String> updated = new HashMap<>();
+ updated.put("gs.endpoint", blank);
+ updated.put("s3_validity_check", "false");
+ resource.modifyProperties(updated);
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ Assertions.assertEquals(blank, updated.get("gs.endpoint"));
+
Assertions.assertFalse(resource.getCopiedProperties().containsKey("gs.connection.timeout"));
+ }
+ }
+ }
+
+ @Test
+ public void testGsEndpointCannotAlterPolicyResource() throws Exception {
+ S3Resource resource = new S3Resource("gcp_resource");
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", "GCP");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ resource.references.put("policy", Resource.ReferenceType.POLICY);
+ try {
+ resource.modifyProperties(ImmutableMap.of("gs.endpoint",
"https://custom.example.com";));
+ Assertions.fail("A gs.endpoint alias must not bypass the policy
endpoint restriction");
+ } catch (DdlException exception) {
+
Assertions.assertTrue(exception.getMessage().contains(S3ResourceCompat.ENDPOINT));
+ }
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ }
+
+ @Test
+ public void testGsEndpointDoesNotOverrideOtherProviders() throws Exception
{
+ for (String provider : new String[] {"S3", "OSS", "AZURE"}) {
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ if (!provider.isEmpty()) {
+ properties.put("provider", provider);
+ }
+ properties.put("gs.endpoint", "https://storage.googleapis.com";);
+ S3Resource resource = new S3Resource("other_resource");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ resource.modifyProperties(ImmutableMap.of("gs.endpoint",
"https://custom.example.com";));
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.Env.ENDPOINT));
+ }
+ }
+
+ @Test
+ public void testGsEndpointUsesEffectiveProviderOnAlter() throws Exception {
+ S3Resource resource = new S3Resource("gcp_resource");
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", "GCP");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ resource.modifyProperties(ImmutableMap.of("provider", "S3",
"gs.endpoint", "https://ignored.example.com";));
+ Assertions.assertEquals("S3", resource.getProperty("provider"));
+ Assertions.assertEquals(s3Endpoint,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ resource.modifyProperties(ImmutableMap.of("provider", "gcp",
"gs.endpoint", "https://storage.googleapis.com";));
+ Assertions.assertEquals("gcp", resource.getProperty("provider"));
+ Assertions.assertEquals("https://storage.googleapis.com";,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ resource.modifyProperties(ImmutableMap.of("provider", "",
"gs.endpoint", "https://custom.example.com";));
+ Assertions.assertEquals("gcp", resource.getProperty("provider"));
+ Assertions.assertEquals("https://custom.example.com";,
resource.getProperty(S3ResourceCompat.ENDPOINT));
+ }
+
+ @Test
+ public void testInactiveGsAliasesCannotReappearAfterProviderAlter() throws
Exception {
+ for (boolean replayLegacyAliases : new boolean[] {false, true}) {
+ S3Resource resource = new S3Resource("s3_resource");
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", "S3");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Map<String, String> aliases = ImmutableMap.of("gs.endpoint",
"https://ignored.example.com";,
+ "gs.access_key", "ignored-access", "gs.secret_key",
"ignored-secret");
+ if (replayLegacyAliases) {
+ // Simulate the dormant aliases persisted by an older FE.
+ Map<String, String> stored = resource.getCopiedProperties();
+ stored.putAll(aliases);
+ Field field = S3Resource.class.getDeclaredField("properties");
+ field.setAccessible(true);
+ field.set(resource, stored);
+ ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ try (DataOutputStream output = new DataOutputStream(bytes)) {
+ resource.write(output);
+ }
+ try (DataInputStream input = new DataInputStream(new
ByteArrayInputStream(bytes.toByteArray()))) {
+ resource = (S3Resource) Resource.read(input);
+ }
+ } else {
+ resource.modifyProperties(aliases);
+
Assertions.assertFalse(resource.getCopiedProperties().keySet().stream()
+ .anyMatch(key -> key.startsWith("gs.")));
+ }
+ resource.modifyProperties(ImmutableMap.of("provider", "GCP"));
+ TS3StorageParam thrift =
S3ThriftAdapter.getS3TStorageParam(resource.getCopiedProperties());
+ Assertions.assertEquals(s3Endpoint, thrift.getEndpoint());
+ Assertions.assertEquals(s3AccessKey, thrift.getAk());
+ Assertions.assertEquals(s3SecretKey, thrift.getSk());
+
Assertions.assertFalse(resource.getCopiedProperties().keySet().stream()
+ .anyMatch(key -> key.startsWith("gs.")));
+ }
+ }
+
+ @Test
+ public void testLegacyGcpDefaultChainSurvivesReplayAndPolicyPush() throws
Exception {
+ for (String key : new String[] {"s3.credentials_provider_type",
"AWS_CREDENTIALS_PROVIDER_TYPE"}) {
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.remove("AWS_ACCESS_KEY");
+ properties.remove("AWS_SECRET_KEY");
+ properties.put("provider", "GCP");
+ properties.put(key, "DEFAULT");
+ S3Resource resource = new S3Resource("gcp_default_chain");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+
resource.modifyProperties(ImmutableMap.of(S3ResourceCompat.CONNECTION_TIMEOUT_MS,
"2000"));
+ ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ try (DataOutputStream output = new DataOutputStream(bytes)) {
+ resource.write(output);
+ }
+ S3Resource restored;
+ try (DataInputStream input = new DataInputStream(new
ByteArrayInputStream(bytes.toByteArray()))) {
+ restored = (S3Resource) Resource.read(input);
+ }
+ Assertions.assertEquals(resource.getCopiedProperties(),
restored.getCopiedProperties());
+ Assertions.assertEquals("DEFAULT",
StorageAdapter.of(restored.getCopiedProperties())
+
.getBackendConfigProperties().get("AWS_CREDENTIALS_PROVIDER_TYPE"));
+ PushStoragePolicyTask task = new PushStoragePolicyTask(1L,
List.of(), List.of(restored), List.of());
+ TS3StorageParam thrift =
task.toThrift().getResource().get(0).getS3StorageParam();
+ Assertions.assertFalse(thrift.isSetCredential());
+ Assertions.assertFalse(thrift.isSetCredProviderType());
+ Assertions.assertEquals(2000, thrift.getConnTimeoutMs());
+ }
+ }
+
+ @Test
+ public void testMinioAlterSurvivesReplayAndBatchedPolicyPush() throws
Exception {
+ S3Resource minio = new S3Resource("minio_resource");
+ minio.id = 1L;
+ minio.version = 0L;
+ minio.setProperties(ImmutableMap.copyOf(s3Properties));
+ minio.modifyProperties(ImmutableMap.of("provider", "MINIO",
"s3_validity_check", "false"));
+ ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ try (DataOutputStream output = new DataOutputStream(bytes)) {
+ minio.write(output);
+ }
+ S3Resource restored;
+ try (DataInputStream input = new DataInputStream(new
ByteArrayInputStream(bytes.toByteArray()))) {
+ restored = (S3Resource) Resource.read(input);
+ }
+ Assertions.assertEquals("MINIO", restored.getProperty("provider"));
+ S3Resource unrelated = new S3Resource("unrelated_resource");
+ unrelated.id = 2L;
+ unrelated.version = 0L;
+ unrelated.setProperties(ImmutableMap.copyOf(s3Properties));
+
unrelated.modifyProperties(ImmutableMap.of(S3ResourceCompat.CONNECTION_TIMEOUT_MS,
"2000"));
+ // The MinIO resource is unreferenced but participates in the same
update as other resources/drops.
+ PushStoragePolicyTask task = new PushStoragePolicyTask(1L, List.of(),
+ List.of(restored, unrelated), List.of(9L));
+ TPushStoragePolicyReq request = task.toThrift();
+ Assertions.assertEquals(2, request.getResourceSize());
+ Assertions.assertEquals(1L, request.getResource().get(0).getId());
+ Assertions.assertEquals(2L, request.getResource().get(1).getId());
+ Assertions.assertEquals(List.of(9L),
request.getDroppedStoragePolicy());
+ TS3StorageParam minioThrift =
request.getResource().get(0).getS3StorageParam();
+ Assertions.assertEquals(TObjStorageType.AWS,
minioThrift.getProvider());
+ Assertions.assertEquals(s3AccessKey, minioThrift.getAk());
+ Assertions.assertEquals(s3SecretKey, minioThrift.getSk());
+ Assertions.assertEquals(2000,
request.getResource().get(1).getS3StorageParam().getConnTimeoutMs());
+ }
+
+ @Test
+ public void testPolicyPushReleasesResourceLockOnInvalidAuthentication()
throws Exception {
+ S3Resource resource = new S3Resource("invalid_gcp_resource");
+ Map<String, String> properties = new HashMap<>(s3Properties);
+ properties.put("provider", "GCP");
+ resource.setProperties(ImmutableMap.copyOf(properties));
+ Map<String, String> invalid = resource.getCopiedProperties();
+ invalid.put("gs.credential_provider_type", "DEFAULT");
+ Field field = S3Resource.class.getDeclaredField("properties");
+ field.setAccessible(true);
+ field.set(resource, invalid);
+ S3Resource tracked = Mockito.spy(resource);
+ PushStoragePolicyTask task = new PushStoragePolicyTask(1L, List.of(),
List.of(tracked), List.of());
+ Assertions.assertThrows(IllegalArgumentException.class,
task::toThrift);
+ Mockito.verify(tracked).readLock();
+ Mockito.verify(tracked).readUnlock();
+ }
+
+ @Test
+ public void testGsAliasesMatchConnectorAndResourceProtocols() throws
Exception {
+ for (String provider : new String[] {"GCP", "gcp", "GCS", "gcs"}) {
Review Comment:
[P2] Make the GCS CREATE cases pass the resource validation gates
The new `GCS`/`gcs` iterations fail before reaching the parity assertions:
`GcsFileSystemProvider.normalizeProperties` retains that provider spelling,
then `resource.setProperties` calls `S3ResourceCompat.requiredS3Properties`,
whose provider whitelist contains `GCP` but no `GCS`. `s3_validity_check=false`
only skips the remote ping, so it cannot prevent this `DdlException`. Extending
the whitelist alone still leaves `CloudObjectStoreAdapter.getObjStoreInfoPB`
calling `Provider.valueOf("GCS")`, which also throws. Canonicalize/map the
alias through both CREATE validation and Cloud encoding so the newly added test
can complete. This is separate from the earlier successful-ALTER key-rotation
issue.
--
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]
