[GitHub] [nifi] markap14 commented on a diff in pull request #6057: NIFI-9958 Add Framework Support for Sensitive Dynamic Properties

[GitBox <gi...@apache.org>]

markap14 commented on code in PR #6057:
URL: https://github.com/apache/nifi/pull/6057#discussion_r877399549


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java:
##########
@@ -616,9 +616,10 @@ Set<DocumentedTypeDTO> getControllerServiceTypes(final String serviceType, final
      *
      * @param id id
      * @param property property
+     * @param sensitive Sensitive Status
      * @return descriptor
      */
-    PropertyDescriptorDTO getProcessorPropertyDescriptor(String id, String property);
+    PropertyDescriptorDTO getProcessorPropertyDescriptor(String id, String property, boolean sensitive);

Review Comment:
   Rather than passing the `sensitive` flag all the way down in these methods, what do you think of leaving the methods as they are, and then in the Resource class that calls this method, creating a PropertyDescriptor, if necessary, that sets the sensitive flag to `true`? Going with that approach, it keeps the `get<type>PropertyDescriptor` method a little cleaner, as it could be potentially used elsewhere if need be. Adding the `sensitive` flag seems to sort of dictate the use case where it would be applicable.



##########
nifi-api/src/main/java/org/apache/nifi/documentation/AbstractDocumentationWriter.java:
##########
@@ -141,6 +142,7 @@ protected void writeBody(final ConfigurableComponent component, Map<String,Servi
             writeTriggerWhenEmpty(processor.getClass().getAnnotation(TriggerWhenEmpty.class));
             writeTriggerWhenAnyDestinationAvailable(processor.getClass().getAnnotation(TriggerWhenAnyDestinationAvailable.class));
             writeSupportsBatching(processor.getClass().getAnnotation(SupportsBatching.class));
+            writeSupportsSensitiveDynamicProperties(processor.getClass().getAnnotation(SupportsSensitiveDynamicProperties.class));

Review Comment:
   This should be done for all `ConfigurableComponent`s, not just `Processor`s.



##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/NiFiClientUtil.java:
##########
@@ -768,6 +752,48 @@ public void waitForControllerServiceState(final String groupId, final String des
         }
     }
 
+    public void waitForControllerServiceValidationStatus(final String controllerServiceId, final String validationStatus) throws NiFiClientException, IOException {
+        while (true) {
+            final ControllerServiceEntity controllerServiceEntity = nifiClient.getControllerServicesClient().getControllerService(controllerServiceId);
+            final String currentValidationStatus = controllerServiceEntity.getStatus().getValidationStatus();
+            if (validationStatus.contentEquals(currentValidationStatus)) {

Review Comment:
   Any reason for using `contentEquals` here instead of `equals`? Both are `String`s so equals() should work.



##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/controllerservice/ControllerServiceApiValidationIT.java:
##########
@@ -150,4 +167,59 @@ public void testMatchingGenericDynamicPropertyControllerService() throws NiFiCli
         assertEquals("ENABLED", controllerStatus);
         assertEquals("Stopped", processorStatus);
     }
+
+    @Test

Review Comment:
   This is very minor but i wonder if we should put these tests into a different class? This one is really intended to verify that Controller Service API's work properly. I.e., processors can reference service by its interface, etc., whereas the new ones are around sensitive property validation



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/ComponentNode.java:
##########
@@ -190,6 +191,15 @@ public default void setProperties(Map<String, String> properties) {
      */
     boolean isValidationNecessary();
 
+    /**
+     * Indicates whether the Component supports sensitive dynamic properties
+     *
+     * @return Support status for Sensitive Dynamic Properties
+     */
+    default boolean isSupportsSensitiveDynamicProperties() {
+        return false;
+    }
+

Review Comment:
   I don't think I'd put a default implementation here that returns `false`. I'd recommend either defining the method in the interface with no implementation, or maybe implementing with:
   ```
   return getComponent().getClass().isAnnotationPresent(SupportsSensitiveDynamicProperties.class);
   ```



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/audit/ReportingTaskAuditor.java:
##########
@@ -138,13 +142,13 @@ public Object updateReportingTaskAdvice(ProceedingJoinPoint proceedingJoinPoint,
                 // create a configuration action accordingly
                 if (operation != null) {
                     // clear the value if this property is sensitive
-                    final PropertyDescriptor propertyDescriptor = reportingTask.getReportingTask().getPropertyDescriptor(property);
-                    if (propertyDescriptor != null && propertyDescriptor.isSensitive()) {
+                    final PropertyDescriptor propertyDescriptor = reportingTask.getPropertyDescriptor(property);
+                    if (propertyDescriptor != null && (propertyDescriptor.isSensitive() || sensitiveDynamicPropertyNames.contains(property))) {

Review Comment:
   Would recommend same thing here - adding a comment as to why we're not just trusting the PropertyDescriptor's isSenstiive() method



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/audit/ControllerServiceAuditor.java:
##########
@@ -144,13 +148,13 @@ public Object updateControllerServiceAdvice(ProceedingJoinPoint proceedingJoinPo
                 // create a configuration action accordingly
                 if (operation != null) {
                     // clear the value if this property is sensitive
-                    final PropertyDescriptor propertyDescriptor = controllerService.getControllerServiceImplementation().getPropertyDescriptor(property);
-                    if (propertyDescriptor != null && propertyDescriptor.isSensitive()) {
+                    final PropertyDescriptor propertyDescriptor = controllerService.getPropertyDescriptor(property);
+                    if (propertyDescriptor != null && (propertyDescriptor.isSensitive() || sensitiveDynamicPropertyNames.contains(property))) {

Review Comment:
   Might be worth adding a comment here that we have to check if isSensitive() or sensitiveDynamicPropertyNames.contains() because at this point in the stack, the changes will not have been applied yet. Otherwise, I can see someone changing this in the future, thinking oh this would be simpler....



-- 
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