Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#364 Update universal configuration store to use Java 17 #385

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

carter-cundiff
Copy link
Contributor

No description provided.

RUN pip install hvac==0.11.0
# Create a virtual environment to install dependencies - PEP 668
RUN python3 -m venv venv \
&& ./venv/bin/pip install hvac==0.11.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I: Newer image required python modules be installed within a venv

@@ -285,10 +285,12 @@ public boolean isFullyLoaded() {
try {
Property statusProperty = propertyDao.read(new PropertyKey("load-status", "fully-loaded"));
return statusProperty != null && BooleanUtils.toBoolean(statusProperty.getValue());
} catch (NullPointerException e) {
logger.warn("Properties are not loaded previously, continue");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I: Cleaned up to avoid printing stack trace when the load status fails due to null pointer - this happens everything you deploy the config store for the first time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A: Would prefer just doing a null check on the nullable value instead of using exceptions for flow control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated VaultPropertDao to return null when the property doesn't exist. This mirrors the current logic in KrauseningPropertyDao

name: boozallen/aissemble-vault
imagePullPolicy: IfNotPresent
dockerRepo: ghcr.io/
tag: "${versionTag}"
Copy link
Contributor Author

@carter-cundiff carter-cundiff Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I: Update vault deployment to use the aiSSEMBLE image by default rather than a docker image within the project. User would have to create a custom execution for adding the vault docker image build to their project. The dockerfile that is then added, doesn't enable any additional functionality (see the dockerfile template).

By using the aiSSEMBLE image instead, we can simplify the vault deployment to work out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably go ahead and deprecate the profile that uses that Dockerfile template. I think the only reason we expected projects to extend the base image is because we expected that to be the way we would provide the krausening properties files. But with universal config that is no longer true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated


COPY ./src/main/resources/vault/config.hcl /vault/setup/config.hcl
COPY ./src/main/resources/scripts/init_encrypt.py /vault/setup/init_encrypt.py

RUN export VAULT_ADDR='https://0.0.0.0:8200' \
&& python3 /vault/setup/init_encrypt.py
&& ./venv/bin/python3 /vault/setup/init_encrypt.py

COPY ./src/main/resources/vault/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
RUN chmod +x /usr/local/bin/docker-entrypoint.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A: While we're in the neighborhood we could combine this RUN chmod into the COPY command with --chmod=755

@@ -27,8 +27,8 @@ aissemble-quarkus-chart:
readOnly: true
configMap:
supplementalQuarkusConfig:
- quarkus.http.ssl.certificate.file=/etc/webhook/cert/tls.crt
- quarkus.http.ssl.certificate.key-file=/etc/webhook/cert/tls.key
- quarkus.http.ssl.certificate.files=/etc/webhook/cert/tls.crt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: This config name changed in newer version of Quarkus?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

@carter-cundiff carter-cundiff force-pushed the 364-config-store-java-17 branch from 8f6eb4a to 9007c40 Compare October 2, 2024 17:52
@carter-cundiff carter-cundiff force-pushed the 364-config-store-java-17 branch from 9007c40 to 43daef2 Compare October 2, 2024 17:54
@carter-cundiff carter-cundiff merged commit 3bf3e3d into dev Oct 2, 2024
@carter-cundiff carter-cundiff deleted the 364-config-store-java-17 branch October 2, 2024 17:59
@carter-cundiff carter-cundiff linked an issue Oct 3, 2024 that may be closed by this pull request
8 tasks
@carter-cundiff carter-cundiff linked an issue Oct 3, 2024 that may be closed by this pull request
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Upgrade Universal Configuration Store to JDK 17
3 participants