NIFI-16333 - Bypass Secrets Manager cache when a connector starts - #11663
NIFI-16333 - Bypass Secrets Manager cache when a connector starts#11663pvillard31 wants to merge 3 commits into
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for adjusting this behavior @pvillard31, this sounds reasonable for Connector starting. The general approach looks good, I noted a couple minor recommendations
|
|
||
| final StartRecordingSecretConnector connector = new StartRecordingSecretConnector(); | ||
| final StandardConnectorNode connectorNode = createConnectorNode(connector, secretsManager); | ||
| seedActiveConfiguration(connectorNode, "requiredStep", Map.of("RequiredSecret", secretReference)); |
There was a problem hiding this comment.
The requiredStep and RequiredSecret string literals are repeated here and in the Connector class, they should be moved to static final variables.
| connectorNode.start(scheduler).get(5, TimeUnit.SECONDS); | ||
| connectorNode.start(scheduler).get(5, TimeUnit.SECONDS); | ||
|
|
||
| assertEquals(List.of("first", "second"), connector.getStartedSecrets()); |
There was a problem hiding this comment.
The first and second strings are repeated and should be declared once and reused.
| * @implSpec Implementations that cache Secret values must override this method to honor {@code useCache}. | ||
| * @param useCache whether cached Secret values may be used | ||
| */ | ||
| default void resolvePropertyValues(final boolean useCache) { |
There was a problem hiding this comment.
Although it is more verbose, I recommend creating an enum named something like CacheResolution with values of ENABLED and DISABLED to start with. That should make the behavior more readable, and provide the option for future extension.
| } | ||
|
|
||
| try (final NarCloseable ignored = NarCloseable.withComponentNarLoader(extensionManager, connectorDetails.getConnector().getClass(), getIdentifier())) { | ||
| activeFlowContext.getConfigurationContext().resolvePropertyValues(false); |
There was a problem hiding this comment.
Does this introduce a potential issue where we would call resolvePropertyValues(false); and then later call resolvePropertyValues(true) such that the latter call would resolve values using a stale cache, whereas this call used newer versions?
If so, would it make sense to just simply call invalidateCache() instead? Then there'd be no need to introduce a new override to resolvePropertyValues...
|
Thanks @markap14 - this is a good idea and I followed your recommendation, this makes it cleaner. Thanks for the review @exceptionfactory. |
markap14
left a comment
markap14
left a comment
There was a problem hiding this comment.
Thanks @pvillard31 this looks simpler and more straight-forward. +1 from me. @exceptionfactory any other concerns or shall we merge (pending successful CI checks)?
b241ff4 to
f24cb79
Compare
|
rebased and fixed merge conflicts |
3 participants
Summary
NIFI-16333 - Bypass Secrets Manager cache when a connector starts
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation