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

Add CORE_STUB_KEYSTORE_FILE property #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

merlinc
Copy link
Contributor

@merlinc merlinc commented Mar 28, 2022

Proposed changes

What changed

  • Add CORE_STUB_KEYSTORE_FILE property
  • Add conditional logic to handle missing both of keystore_file and keystore_base64 properties

Why did it change

This Docker image needs to be built without config in it to be a publicly available image

Issue tracking

Checklists

Environment variables or secrets

  • Documented in the README
  • Added to deployment repository
  • Added to local startup repository

Other considerations

  • Update README with any new instructions or tasks

@merlinc merlinc requested a review from a team as a code owner March 28, 2022 14:51
@merlinc merlinc force-pushed the KBV-254/build-container-without-config branch 2 times, most recently from c13c2ad to 59f2e06 Compare March 29, 2022 11:36
@merlinc merlinc changed the title Build container without config Add CORE_STUB_KEYSTORE_FILE property Apr 1, 2022
@@ -21,7 +21,6 @@ COPY --from=build \
/app/

WORKDIR /app
ADD config config
Copy link
Contributor

@MattBidewell MattBidewell Apr 4, 2022

Choose a reason for hiding this comment

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

Why are you removing the adding of the config directory?

Edit: I reread the PR. Its so it can be public facing?

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'm not that fussed about whether the image is public or private - I just need it somewhere that it can be easily accessed. The config on the other hand is definitely private, and I don't want an entire config repo exposed for a few settings.

So the approach is build the image so it works without config. Locally it can be run with a sibling path to the config. In a PR we get the necessary config from a S3 bucket, which reduces the blast radius if something went wrong.

MattBidewell
MattBidewell previously approved these changes Apr 4, 2022
@merlinc merlinc force-pushed the KBV-254/build-container-without-config branch from 59f2e06 to c6aedc4 Compare April 5, 2022 16:00
@@ -0,0 +1,17 @@
version: "3.9"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script & .env file have been swapped for a docker-compose file with environment variables and volumes. The secret stuff is still elsewhere

@@ -17,29 +17,30 @@ This stub allows us to:
## Config

Core Stub config from the `di-ipv-config` repository directory `/stubs/di-ipv-core-stub` will be available at:
* Docker image: `/app/config/`
* Docker image: `/app/`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than create an empty config directory, it seemed easier to mount the required files into the existing app directory.

@MattBidewell MattBidewell self-requested a review April 6, 2022 08:50
MattBidewell
MattBidewell previously approved these changes Apr 6, 2022
Copy link
Contributor

@MattBidewell MattBidewell left a comment

Choose a reason for hiding this comment

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

Looks good to me

My understanding

  • We're providing the config variables as a file now, file loc to be set in the .env.

My only suggestion would be provide an example env file, if needed.

@merlinc
Copy link
Contributor Author

merlinc commented Apr 7, 2022

Looks good to me

My understanding

  • We're providing the config variables as a file now, file loc to be set in the .env.

Yeah, there's an additional property now for the "keystore as a keystore file" to match the "keystore as a base64 string".

My only suggestion would be provide an example env file, if needed.

Local usage of the stub doesn't change. The config directory is referenced by the Dockerfile, and that's mentioned in the .env.sample file as well.

@merlinc merlinc force-pushed the KBV-254/build-container-without-config branch from c6aedc4 to 2ef7284 Compare April 14, 2022 08:32
keystore.load(
inputStream, CoreStubConfig.CORE_STUB_KEYSTORE_PASSWORD.toCharArray());
}
} else if (CoreStubConfig.CORE_STUB_KEYSTORE_BASE64 != null) {
Copy link
Contributor

@MattBidewell MattBidewell Apr 14, 2022

Choose a reason for hiding this comment

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

Heads up but looking at the current deployment in PaaS, this has already been set. So you might need to update it

MattBidewell
MattBidewell previously approved these changes Apr 14, 2022
Copy link
Contributor

@MattBidewell MattBidewell left a comment

Choose a reason for hiding this comment

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

lgtm - just to note a the base64 keystore env has been added in paas already, you might need to update it or something

- Add CORE_STUB_KEYSTORE_FILE property
- Add property for keystore_file
- Add conditional logic to handle missing both of keystore_file and keystore_base64 properties
- Add compose file for docker-compose startup
- Delete bash script for startup
@merlinc merlinc force-pushed the KBV-254/build-container-without-config branch from 2ef7284 to bb91fb4 Compare May 5, 2022 15:38
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.

2 participants