[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7172: NIFI-11439 - GCS processors; expose config of custom client endpoint

["exceptionfactory (via GitHub)" <gi...@apache.org>]

exceptionfactory commented on code in PR #7172:
URL: https://github.com/apache/nifi/pull/7172#discussion_r1165758118


##########
nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/storage/DeleteGCSObject.java:
##########
@@ -81,6 +81,7 @@ public class DeleteGCSObject extends AbstractGCSProcessor {
     public List<PropertyDescriptor> getSupportedPropertyDescriptors() {
         final List<PropertyDescriptor> descriptors = new ArrayList<>();
         descriptors.addAll(super.getSupportedPropertyDescriptors());
+        descriptors.add(STORAGE_API_HOST);

Review Comment:
   Instead of adding the Property Descriptor in individual components, it looks like it could be added after the Proxy Configuration Service in the AbstractGCSProcessor.



##########
nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/storage/AbstractGCSProcessor.java:
##########
@@ -141,6 +142,10 @@ protected StorageOptions getServiceOptions(ProcessContext context, GoogleCredent
             storageOptionsBuilder.setProjectId(projectId);
         }
 
+        if (storageApiHost != null && !storageApiHost.isEmpty()) {
+            storageOptionsBuilder.setHost(storageApiHost);

Review Comment:
   According to the [Private Service Connect for Google APIs](https://codelabs.developers.google.com/cloudnet-psc#12) documentation, the HTTP `Host` header should also be overridden to send `www.googleapis.com`.



##########
nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/AbstractGCPProcessor.java:
##########
@@ -106,6 +106,16 @@
             .sensitive(true)
             .build();
 
+    // https://cloud.google.com/storage/docs/request-endpoints#storage-set-client-endpoint-java
+    public static final PropertyDescriptor STORAGE_API_HOST = new PropertyDescriptor
+            .Builder().name("storage-api-host")
+            .displayName("Storage API Host")
+            .description("Cloud Storage client libraries manage request endpoints automatically. Optionally, you can set the request endpoint manually.")

Review Comment:
   Recommend adjusting the description to include some additional details on the default setting and expected custom behavior.
   ```suggestion
               .description("Overrides the default storage URL. Configuring an alternative Storage API Host URL also overrides the HTTP Host header on requests as described in the Google documentation for Private Service Connections.")
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org