add keystore reference back #2876
Conversation
JaCoCo Test Coverage
|
There was a problem hiding this comment.
Great work Cheng! It looks like you were able to regenerate the keystore during container startup, which helps facilitate a more maintainable image going forward. Please see the one other comment/question i have added to the review. If there is something to be done about that, then great, but this enhancement is approved either way.
| export KEYSTORE_FILE="$PWD/keystore.p12" | ||
| echo "$BIE_KAFKA_KEYSTORE_INBASE64" | base64 -d > "$KEYSTORE_FILE" | ||
| echo -e "\nVerifying keystore ($KEYSTORE_FILE) and its password..." | ||
| if ! keytool -list -v -keystore "$KEYSTORE_FILE" -storepass "$BIE_KAFKA_KEYSTORE_PASSWORD" | grep "Alias name:"; then |
There was a problem hiding this comment.
I recognize that this if statement on line 18 is trying to test whether the password successfully unlocks the keystore, and I am wondering if we are able to use the command's error code to check for the pass/fail state in favor of the "Alias name" text. If there is some reason that doesn't work in this case, it is fine the way it is, but I wanted to ask the question since it stood out as a potential improvement.
There was a problem hiding this comment.
The improvement could be to either implement RBAC changes to the mock Kafka cluster or find a way to ensure that the keystore attribute in the application.yaml file applies only to local and integration tests, without affecting other environments.
What was the problem?
The
devlog reports a missing keystore file, which was due to a recent script in kafka docker entrypoint file responsible for generating the file being removed.Associated tickets or Slack threads:
svc-bie-kafkain ourdevenvironment. #2530How does this fix it?1
This PR is to revert the code back and get dev back to a working state
How to test this PR
Footnotes
Pull-Requests guidelines. If PR is significant, update Current Software State wiki page. ↩
To check if a PR will succeed in the SecRel workflow, test PRs in the SecRel pipeline. ↩