NIFI-9401: HashiCorpVaultParameterProvider by gresockj · Pull Request #6304 · apache/nifi

gresockj · 2022-08-16T17:42:32Z

Summary

NIFI-9401

This adds a new HashiCorpVaultParameterProvider to fetch parameters from HashiCorpVault.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 8
- JDK 11
- JDK 17

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

exceptionfactory

Thanks for putting forward this pull request for review @gresockj!

I reviewed the HashiCorp Vault Controller Service and noted several suggestions related to the property configuration, as well as a few other minor adjustments.

...t-service-api/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultClientService.java

nifi-nar-bundles/nifi-hashicorp-vault-bundle/nifi-hashicorp-vault-client-service/pom.xml

...-provider/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultParameterProvider.java

...vider/src/test/java/org/apache/nifi/vault/hashicorp/TestHashiCorpVaultParameterProvider.java

exceptionfactory

Thanks for the updates @gresockj, briefly noted where Resource References can be used to handle properties files.

exceptionfactory · 2022-09-09T18:34:42Z

...t-service-api/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultClientService.java

+                    "Environment Configuration documentation (https://docs.spring.io/spring-vault/docs/2.3.x/reference/html/#vault.core.environment-vault-configuration). " +
+                    "All of the Spring property keys and authentication-specific property keys are supported.")
+            .required(true)
+            .addValidator(MultiFileExistsValidator.INSTANCE)


This should be changed to use the identifiesExternalResource(ResourceCardinality.MULTIPLE, ResourceType.FILE) description to support framework-based resource handling.

...rvice/src/main/java/org/apache/nifi/vault/hashicorp/StandardHashiCorpVaultClientService.java

...-utils/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultCommunicationService.java

…ng framework Property Resources for Vault Properties Files

…h explicitly in test

exceptionfactory

Thanks for the updates @gresockj, this looks close to completion. Two additional minor recommendations related to property naming and the unnecessary custom validator.

exceptionfactory · 2022-09-12T15:44:06Z

...t-service-api/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultClientService.java

+ */
+public interface HashiCorpVaultClientService extends ControllerService, VerifiableControllerService {
+
+    AllowableValue DIRECT_PROPERTIES = new AllowableValue("direct-properties", "Direct Properties",


What do you think about naming this strategy Service Properties?

...t-service-api/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultClientService.java

exceptionfactory

Thanks for the updates @gresockj!

The Controller Service appears to function as designed at runtime, but in the process of building and verifying the NAR bundles, I noticed duplication of nifi-vault-utils and all transitive dependencies in nifi-hashicorp-vault-nar. This highlighted the need to mark nifi-vault-utils as provided in the Controller Service implementation JAR Maven configuration, as well as the in the nifi-hashicorp-vault-parameter-value-provider Maven configuration.

This raised another question, given the fact the nifi-vault-utils contains both the HashiCorpVaultCommunicationService interface and implementation. Providing that as part of a public Controller Service interface makes sense for this design, but it also highlights a need to change the module structure. Moving the Communication Service interface to a separate nifi-hashicorp-vault-api module would provide a clean separation for the purpose of making the interface public. I can help with the implementation if needed.

exceptionfactory · 2022-09-13T14:04:30Z

nifi-nar-bundles/nifi-hashicorp-vault-bundle/nifi-hashicorp-vault-client-service/pom.xml

+        <dependency>
+            <groupId>org.apache.nifi</groupId>
+            <artifactId>nifi-vault-utils</artifactId>
+            <version>1.18.0-SNAPSHOT</version>


This should be marked with the provided scope since it is included in the vault-client-service-api.

…p-vault-api

…aultClientService

exceptionfactory

Thanks for working through the feedback @gresockj, the latest version looks good! +1 merging

- Refactored nifi-vault-utils to nifi-hashicorp-vault-api and nifi-hashcorp-vault modules - Added HashiCorpVaultClientService and Standard implementation This closes apache#6304 Signed-off-by: David Handermann <exceptionfactory@apache.org>

Reamer · 2024-10-22T14:10:55Z

@gresockj How to configure the truststore and keystore if you have a valid public certificate. The certificate chain should already be known to the JVM via JAVA_HOME/lib/security/cacerts.

gresockj requested a review from mtien-apache August 16, 2022 17:42

gresockj marked this pull request as draft August 16, 2022 17:43

gresockj force-pushed the NIFI-9401 branch from 93140bd to 9fcad65 Compare August 31, 2022 01:05

exceptionfactory requested changes Aug 31, 2022

View reviewed changes

gresockj force-pushed the NIFI-9401 branch from 02b813f to 58741d6 Compare September 1, 2022 12:50

gresockj force-pushed the NIFI-9401 branch from 58741d6 to ccf0846 Compare September 9, 2022 15:51

NIFI-9401: Adding HashiCorpVaultParameterProvider

7078353

gresockj force-pushed the NIFI-9401 branch from ccf0846 to 7078353 Compare September 9, 2022 18:20

gresockj marked this pull request as ready for review September 9, 2022 18:28

exceptionfactory reviewed Sep 9, 2022

View reviewed changes

...-utils/src/main/java/org/apache/nifi/vault/hashicorp/HashiCorpVaultCommunicationService.java Outdated Show resolved Hide resolved

Changing testConnection to getServerVersion in Vault Service, and usi…

41cc20a

…ng framework Property Resources for Vault Properties Files

gresockj force-pushed the NIFI-9401 branch from a376b1b to 41cc20a Compare September 9, 2022 19:26

gresockj added 3 commits September 9, 2022 18:57

Attempting to resolve failing Windows test by removing Paths.get call

3f11ea5

Attempting to resolve failing Windows test by specifying keystore pat…

c90ca91

…h explicitly in test

Escaping windows paths in TestStandardHashiCorpVaultClientService

d5aaa3a

exceptionfactory requested changes Sep 12, 2022

View reviewed changes

Removing unnecessary custom validation in HashiCorpVaultClientService

018a762

exceptionfactory requested changes Sep 13, 2022

View reviewed changes

gresockj added 4 commits September 14, 2022 10:06

Refactoring modules to include nifi-hashicorp-vault and nifi-hashicor…

319f06f

…p-vault-api

Adding defaults for connection and read timeout on StandardHashiCorpV…

cf868d6

…aultClientService

Making Vault client timeouts required properties

13c4661

Setting nifi-hashicorp-vault-api as provided

453b79d

exceptionfactory approved these changes Sep 15, 2022

View reviewed changes

exceptionfactory closed this in 3987d39 Sep 15, 2022

Conversation

gresockj commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

exceptionfactory Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Reamer commented Oct 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gresockj commented Aug 16, 2022 •

edited

Loading