0AyanamiRei commented on PR #65379: URL: https://github.com/apache/doris/pull/65379#issuecomment-4914954592
> Thanks for the improvement. I think this still needs changes before merge. > > 1. The storage vault path still persists a bare S3 endpoint as HTTP, so the most important cloud vault/ObjectStoreInfoPB path does not actually get the new HTTPS default. `CREATE STORAGE VAULT` goes through `StorageVault.fromCommand` -> `S3StorageVault` -> `Resource.fromCommand` -> `S3Resource.setProperties`, and `S3Resource.setProperties` still rewrites a bare endpoint to `http://...`. That value is then copied into `ObjectStoreInfoPB` by `S3Properties.getObjStoreInfoPB`, persisted by meta-service as-is, and later consumed by BE/recycler through `S3Conf::get_s3_conf`. As a result, a new storage vault created with a bare `s3.endpoint` will still use/persist HTTP and will not follow the new default HTTPS behavior. > 2. `CREATE STAGE` still has an HTTP-only preflight check. `CreateStageCommand.tryConnect` unconditionally builds `http://` + endpoint before the real object-storage access. The real FE filesystem path already defaults a bare S3 endpoint to HTTPS in `S3ObjStorage`, so the preflight can fail for an HTTPS-only endpoint even though the actual object-storage client would use HTTPS and succeed. > 3. There are still FE S3-compatible client construction paths that do not use the same HTTPS-default normalization. `AbstractS3CompatibleConnectivityTester` passes `URI.create(endpoint)` directly, so an explicitly configured bare endpoint is not normalized before the AWS SDK client is built. `DLFCatalog.initializeFileIO` also converts the OSS endpoint to an S3-compatible endpoint and then defaults a bare endpoint to `http://`. These paths should share the same endpoint normalization rule: preserve explicit `http://` or `https://`, but default a bare endpoint to HTTPS. > > Testing suggestion: please add coverage for the storage vault/ObjectStoreInfoPB path, especially `CREATE STORAGE VAULT` with a bare `s3.endpoint`, and assert that it no longer persists `http://...` unintentionally. The existing `S3ResourceTest` still asserts that a bare endpoint becomes `http://aaa`, which is exactly the old behavior this PR should change for this path. It would also be useful to add coverage for `CREATE STAGE` with a bare endpoint and for the FE catalog/DLF connectivity paths so all object-storage client construction follows the same default HTTPS rule. draft first, we will test for this behavior change. -- 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]
