github-actions[bot] commented on code in PR #68054:
URL: https://github.com/apache/doris/pull/68054#discussion_r4345552028
##########
be/src/runtime/small_file_mgr.cpp:
##########
@@ -191,13 +190,16 @@ Status SmallFileMgr::_download_file(int64_t file_id,
const std::string& md5,
return true;
};
- std::string url = "http://"; + host_port + query;
+ std::string url = get_internal_http_scheme() + host_port + query;
LOG(INFO) << "download file from: " << url;
HttpClient client;
- RETURN_IF_ERROR(client.init(url));
+ RETURN_IF_ERROR(client.init_internal(url));
Status execute_status = client.execute(download_cb);
- if (!execute_status.ok()) {
+ // Preserve the legacy FE-only HTTPS fallback when unified TLS is
disabled. With unified TLS,
+ // the configured protocol is authoritative: do not downgrade, retry
another protocol, or
+ // bypass certificate validation after a failed request.
+ if (!execute_status.ok() && !config::enable_tls) {
Review Comment:
[P2] Preserve legacy FE HTTPS when unified HTTP is excluded
With `enable_tls=true`, `tls_excluded_protocols=http`, and legacy FE
`enable_https=true`, `HeartbeatMgr.setMaster()` still advertises `https_port`
via `HttpURLUtil.getHttpPort()`. `OssHttpServerTlsProvider` explicitly permits
this combination. Here `get_internal_http_scheme()` chooses HTTP, so the
initial request goes to that HTTPS port, but this new guard suppresses the
HTTPS retry that previously made the download work. Uncached small-file
downloads now fail in this supported configuration. Base the fallback decision
on effective HTTP TLS inclusion, or carry the FE endpoint scheme, while
preserving the configured verification policy.
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/ScriptContext.groovy:
##########
@@ -62,6 +63,16 @@ class ScriptContext implements Closeable {
def outputRelativePath = path.substring(0, path.lastIndexOf(".")) +
".out"
this.outputFile = new File(new File(config.dataPath),
outputRelativePath)
this.dataPath = this.outputFile.getParentFile().getCanonicalFile()
+ Http.configure(
+
(config.otherConfigs.get("enableTLS")?.toString()?.equalsIgnoreCase("true")) ?:
false,
Review Comment:
[P2] Keep temporary plaintext clusters out of global HTTP TLS mode
When `enableTLS=true` and Docker suites are enabled, this configures the
process-global `Http.enableTls` for those suites too. `Suite.dockerImpl()`
creates independent clusters from `ClusterOptions`, whose defaults do not
enable TLS or install certificates. Their `ServerNode.enableDebugPoint()` calls
now reach `DebugPoint`/`Http.POST` with HTTPS forced onto a plaintext HTTP
port. For example, `cloud_p0/query_retry/test_retry_e-230.groovy` creates a
cloud-mode Docker cluster without TLS, then fails at
`cluster.injectDebugPoints()` before exercising its retry scenario. Keep HTTP
policy specific to the target cluster/request; toggling the global flag around
`docker()` would also race concurrently running normal TLS suites.
##########
be/src/runtime/snapshot_loader.cpp:
##########
@@ -89,8 +90,9 @@ class SnapshotHttpDownloader {
auto& remote_be_addr = remote_tablet_snapshot.remote_be_addr;
// HEAD
http://172.16.0.14:6781/api/_tablet/_download?token=e804dd27-86da-4072-af58-70724075d2a4&file=/home/ubuntu/doris_master/output/be/storage/snapshot/20230410102306.9.180/
- _base_url =
fmt::format("http://{}:{}/api/_tablet/_download?token={}&channel=ingest_binlog";,
- remote_be_addr.hostname, remote_be_addr.port,
token);
+ _base_url =
fmt::format("{}{}:{}/api/_tablet/_download?token={}&channel=ingest_binlog",
+ get_internal_http_scheme(),
remote_be_addr.hostname,
Review Comment:
[P2] Select the remote source protocol independently of this BE
`remote_be_addr` belongs to the source snapshot cluster:
`RestoreJob.downloadLocalSnapshots()` obtains it and the remote token from
`jobInfo`. Its protocol need not match the receiving BE's listener. With
unified HTTP TLS enabled on the destination and an existing plaintext source,
this change sends HTTPS to the source HTTP port and remote snapshot copying
fails; the previous HTTP request worked. `TRemoteTabletSnapshot` carries no
source protocol to override this choice. The same assumption affects
incremental copying in `backend_service.cpp`'s `_ingest_binlog`. Preserve an
explicit source-side scheme/configuration for both paths instead of deriving it
from the destination's `enable_tls`.
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/ScriptContext.groovy:
##########
@@ -62,6 +63,16 @@ class ScriptContext implements Closeable {
def outputRelativePath = path.substring(0, path.lastIndexOf(".")) +
".out"
this.outputFile = new File(new File(config.dataPath),
outputRelativePath)
this.dataPath = this.outputFile.getParentFile().getCanonicalFile()
+ Http.configure(
Review Comment:
[P2] Close TLS store streams before initializing them per script
This new constructor call makes every script run `Http.configure()` ->
`initSSLContext()`. In strict mode, that helper opens fresh truststore and
keystore `FileInputStream`s and never closes them (`Http.groovy:86,93`);
`KeyStore.load()` does not close its input. `RegressionTest.runScripts()`
constructs contexts for every source in both NORMAL and DOCKER passes, plus
SINGLE when enabled, before suite filtering, so a TLS run now repeatedly leaves
descriptors for GC/cleaner reclamation and can hit the runner's file limit.
Close both streams deterministically and reuse run-level TLS initialization
instead of rebuilding it for each script.
--
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]
