Copilot commented on code in PR #61904: URL: https://github.com/apache/doris/pull/61904#discussion_r3010407668
########## regression-test/suites/http_p0/test_large_http_header.groovy: ########## @@ -0,0 +1,44 @@ +// 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. + +suite("test_large_http_header", "p0") { + def feHost = context.config.feHttpAddress + def (host, port) = feHost.split(":") + + // Test with large HTTP header (100KB) + def largeHeaderValue = "x" * (100 * 1024) + + def url = "http://${host}:${port}/api/health"; + + try { + def connection = new URL(url).openConnection() + connection.setRequestMethod("GET") + connection.setRequestProperty("X-Large-Header", largeHeaderValue) + connection.setConnectTimeout(5000) + connection.setReadTimeout(5000) + + def responseCode = connection.getResponseCode() + + // Should not return 431 (Request Header Fields Too Large) + assertTrue(responseCode != 431, "Should not return 431 error with large header") Review Comment: The test can pass even if the request fails for reasons unrelated to header size (e.g., /api/health returns 503 when FE is not ready, or 401 when enable_all_http_auth is enabled), because it only asserts responseCode != 431. To avoid false positives, assert a success response (typically 200 for /api/health) and/or explicitly handle/avoid the not-ready/auth cases. ```suggestion // /api/health should return HTTP 200 even with a large header assertTrue(responseCode == 200, "Expected HTTP 200 from /api/health but got ${responseCode}") ``` ########## regression-test/suites/http_p0/test_large_http_header.groovy: ########## @@ -0,0 +1,44 @@ +// 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. + +suite("test_large_http_header", "p0") { + def feHost = context.config.feHttpAddress + def (host, port) = feHost.split(":") + + // Test with large HTTP header (100KB) + def largeHeaderValue = "x" * (100 * 1024) + + def url = "http://${host}:${port}/api/health"; + + try { + def connection = new URL(url).openConnection() + connection.setRequestMethod("GET") + connection.setRequestProperty("X-Large-Header", largeHeaderValue) + connection.setConnectTimeout(5000) + connection.setReadTimeout(5000) Review Comment: This test hardcodes the URL scheme to http and doesn't set Authorization. Many regression HTTP tests switch http/https based on context.config.otherConfigs.enableTLS and add Basic auth headers (e.g., existing http_rest_api suites) to work when TLS or enable_all_http_auth is enabled. Consider deriving the protocol from enableTLS and setting the Authorization header so the test is portable across TLS/auth-enabled runs. ########## fe/fe-core/src/main/java/org/apache/doris/httpv2/config/WebServerFactoryCustomizerConfig.java: ########## @@ -33,6 +33,21 @@ public class WebServerFactoryCustomizerConfig implements WebServerFactoryCustomizer<ConfigurableJettyWebServerFactory> { @Override public void customize(ConfigurableJettyWebServerFactory factory) { + + // Set HTTP header size for all connectors + factory.addServerCustomizers(server -> { + for (org.eclipse.jetty.server.Connector connector : server.getConnectors()) { + if (connector instanceof ServerConnector) { + ServerConnector serverConnector = (ServerConnector) connector; + HttpConnectionFactory httpFactory = + serverConnector.getConnectionFactory(HttpConnectionFactory.class); + if (httpFactory != null) { + HttpConfiguration httpConfig = httpFactory.getHttpConfiguration(); + httpConfig.setRequestHeaderSize(Config.jetty_server_max_http_header_size); + } + } + } + }); Review Comment: This server customizer only updates the connectors present at the time it runs. In the enable_https branch below, another ServerConnector is added with a new HttpConfiguration that doesn't have requestHeaderSize set, and (depending on Spring Boot customizer ordering) it may be added after this loop executes—leaving that connector with Jetty defaults and still susceptible to 431 for large headers. Consider either (1) setting requestHeaderSize on the HttpConfiguration used when creating the additional connector, or (2) registering the connector-iteration customizer after all connectors are added so it covers every connector. -- 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]
