refactor: extract logic from cert manager to use cases #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nelsonochoam
commented
Sep 16, 2022
| } | ||
|
|
||
| @SuppressWarnings("PMD.AvoidCatchingGenericException") | ||
| private Pair<PrivateKey, X509Certificate[]> getCertificateChain(URI privateKeyUri, URI certChainUri) { |
There was a problem hiding this comment.
This is a temporary spot for this we ideally don't have this logic inside use cases
Nelsonochoam
requested review from
jbutler,
vaibhavmurkute,
jcosentino11 and
MikeDombo
and removed request for
jbutler
jbutler
reviewed
Sep 16, 2022
| certificateStore.setCaCertificateChain(result.getB()); | ||
| configuration.updateCACertificates( | ||
| Collections.singletonList(CertificateHelper.toPem(certificateStore.getCaCertificateChain()))); | ||
| } catch (CertificateEncodingException | KeyStoreException | IOException e) { |
There was a problem hiding this comment.
What about InvalidCertificateAuthorityException ?
.../greengrass/clientdevices/auth/certificate/usecases/ConfigureCustomCertificateAuthority.java
Outdated
Show resolved
Hide resolved
Nelsonochoam
force-pushed
the
refactor-configure-custom-ca
branch
from
3ad6f7b
to
1feb011
Compare
Nelsonochoam
force-pushed
the
refactor-configure-custom-ca
branch
from
1feb011
to
fa055d7
Compare
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against fa055d7 |
jbutler
approved these changes
Sep 20, 2022
| } | ||
|
|
||
| @SuppressWarnings("PMD.AvoidCatchingGenericException") | ||
| private Pair<PrivateKey, X509Certificate[]> getCertificateChain(URI privateKeyUri, URI certChainUri) { |
There was a problem hiding this comment.
getKeyAndCertificateChain?
There was a problem hiding this comment.
True, I will change in the next PR
MikeDombo
reviewed
Sep 20, 2022
|
|
||
| topics = Topics.of(new Context(), CLIENT_DEVICES_AUTH_SERVICE_NAME, null); |
There was a problem hiding this comment.
context must always be closed at the end of a test. It starts a thread and we don't want that to run forever. Make sure you always shutdown any context you create.
MikeDombo
reviewed
Sep 20, 2022
| @SuppressWarnings("PMD.AvoidCatchingGenericException") | ||
| private Pair<PrivateKey, X509Certificate[]> getCertificateChain(URI privateKeyUri, URI certChainUri) { | ||
| // TODO: Move retry logic out of useCases | ||
| RetryUtils.RetryConfig retryConfig = RetryUtils.RetryConfig.builder() |
There was a problem hiding this comment.
If this retry eventually gives up, then what happens?
200ms * 3 may not be enough time for some providers to initalize.
There was a problem hiding this comment.
+1 to this. I've commented on this several times as well. Since this is just a refactor, we can probably tackle that in a follow up. We'll want an event listener that listens to config update events and handles retries
MikeDombo
reviewed
Sep 20, 2022
| @@ -116,22 +120,27 @@ void GIVEN_customCAConfiguration_WHEN_configureCustomCA_THEN_returnsCustomCA() t | |||
| URI privateKeyUri = new URI("file:///private.key"); | |||
| URI certificateUri = new URI("file:///certificate.pem"); | |||
|
|
|||
| Topics configurationTopics = Topics.of(new Context(), CLIENT_DEVICES_AUTH_SERVICE_NAME, null); | |||
| configurationTopics.lookup(CONFIGURATION_CONFIG_KEY, CERTIFICATE_AUTHORITY_TOPIC, CA_PRIVATE_KEY_URI) | |||
| Topics topics = Topics.of(new Context(), CLIENT_DEVICES_AUTH_SERVICE_NAME, null); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Now we have use cases in place we can start extracting logic from the cert manager into use cases. Eventually certManager will not exist since it is just acting as a proxy for application logic and we already have a construct that better first that logic (use cases)
Why is this change necessary:
Continuation on the refactor work we have been doing
How was this change tested:
Ensure tests are passing