pvillard31 commented on code in PR #11314:
URL: https://github.com/apache/nifi/pull/11314#discussion_r3398749022
##########
nifi-extension-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -213,6 +228,46 @@ public void migrateProperties(PropertyConfiguration
config) {
config.renameProperty(SnowflakeProperties.OLD_SCHEMA_PROPERTY_NAME,
SnowflakeProperties.SCHEMA.getName());
}
+ private ProxySelector buildProxySelector(final ProxyConfigurationService
proxyConfigurationService) {
+ if (proxyConfigurationService == null) {
+ return null;
+ }
+ final Proxy proxy =
proxyConfigurationService.getConfiguration().createProxy();
+ // Use a custom ProxySelector so that the Proxy.Type (HTTP or SOCKS)
is preserved,
+ // and DIRECT explicitly disables proxying rather than falling back to
the system default.
+ return new ProxySelector() {
+ @Override
+ public List<Proxy> select(final URI uri) {
+ return List.of(proxy);
+ }
+
+ @Override
+ public void connectFailed(final URI uri, final SocketAddress sa,
final IOException ioe) {
+ }
+ };
+ }
+
+ private Authenticator buildProxyAuthenticator(final
ProxyConfigurationService proxyConfigurationService) {
Review Comment:
The Snowpipe endpoint is HTTPS, and the JDK disables Basic proxy
authentication over CONNECT tunneling by default via
jdk.http.auth.tunneling.disabledSchemes. Was the username and password path
tested against a real authenticated HTTPS proxy?
##########
nifi-extension-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/test/java/org/apache/nifi/snowflake/service/SnowpipeIngestClientTest.java:
##########
@@ -264,6 +266,31 @@ void testGetInsertReportErrorResponse() {
assertTrue(exception.getMessage().contains(String.valueOf(HttpURLConnection.HTTP_INTERNAL_ERROR)));
}
+ @Test
+ void testInsertFilesViaProxy() throws InterruptedException,
NoSuchAlgorithmException {
Review Comment:
This test uses an HTTP base URI and a null authenticator, so it does not
cover HTTPS tunneling or proxy credentials, which are the parts most likely to
break. Can we add coverage for the authenticated proxy and HTTPS cases?
##########
nifi-extension-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -136,7 +144,8 @@ public class StandardSnowflakeIngestManagerProviderService
extends AbstractContr
PRIVATE_KEY_SERVICE,
DATABASE,
SCHEMA,
- PIPE
+ PIPE,
+ ProxyConfigurationService.PROXY_CONFIGURATION_SERVICE
Review Comment:
Should we use ProxyConfiguration.createProxyConfigPropertyDescriptor(...)
here and add a customValidate that calls
ProxyConfiguration.validateProxySpec(...), like other proxy aware components,
so an unsupported proxy type is rejected at configuration time instead of
failing silently at runtime?
--
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]
