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 docker build for java-spiffe-helper container #187

Merged
merged 18 commits into from
Feb 5, 2024

Conversation

moritzschmitz-oviva
Copy link
Contributor

No description provided.

@moritzschmitz-oviva moritzschmitz-oviva marked this pull request as draft January 21, 2024 19:56
@moritzschmitz-oviva moritzschmitz-oviva marked this pull request as ready for review January 22, 2024 07:02
@moritzschmitz-oviva moritzschmitz-oviva marked this pull request as draft January 22, 2024 07:03
@@ -0,0 +1,10 @@
FROM eclipse-temurin:17-jdk AS builder
Copy link
Member

Choose a reason for hiding this comment

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

I suggest moving this Dockerfile for java-spiffe-helper to the project's root. This simplifies build context management and path referencing, making the build process more straightforward.

Copy link
Member

Choose a reason for hiding this comment

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

Suggestions for Dockerfile:

  • Cache Gradle Dependencies: Separate Gradle config files from the rest of the code to improve caching, and run two separate gradle commands (dependencies and build).
  • Non-Root User: Consider creating a non-root user in the runner stage.
  • Explicit File Copying: Instead of copying the entire project context (COPY . /builder), explicitly copy only what's necessary.
  • Versioning Strategy: Rethink including JAVA_SPIFFE_HELPER_VERSION in the Dockerfile. Tagging the Docker image with the project version might be more efficient.

Copy link
Contributor Author

@moritzschmitz-oviva moritzschmitz-oviva Jan 23, 2024

Choose a reason for hiding this comment

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

  • Explicit File Copying: I don't see any benefit here, except build time probably. 1. The context file won't end up in the final image. 2. Every change to the dependencies of java-spiffe-helper would require an adjustment of the Dockerfile as well.

  • Versioning Strategy: Yeah, my issue here was that the name of the built Jar is dependent on the version. Solved this now by setting some special version during docker build. Easier to find and copy then.

@moritzschmitz-oviva moritzschmitz-oviva marked this pull request as ready for review January 23, 2024 09:33
--chown=nobody:nobody \
/builder/java-spiffe-helper/build/libs/java-spiffe-helper-docker-docker.jar /app/java-spiffe-helper.jar
USER nobody
ENTRYPOINT ["java", "-jar", "/app/java-spiffe-helper.jar"]
Copy link
Member

Choose a reason for hiding this comment

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

The ENTRYPOINT requires a --config configfile parameter. I'm submitting a PR to add a default config file and set a default for this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, wasn't sure about this one. Effectively the app doesn't start without it, but I didn't want to force users to a location for the file.

Choose a reason for hiding this comment

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

ENTRYPOINT ["java", "-jar", "/app/java-spiffe-helper.jar"]
COMMAND ["--config", "/etc/java-spiffe-helper.conf"]

So by default it works if they mount the config file in the right place, or they can easily override it otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably the way to go, yes! Will add it now so no PR necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it would be beneficial to implement a default value for the --config parameter. This change would enable the application to run out of the box. I'll be submitting the PR shortly.

Copy link
Member

Choose a reason for hiding this comment

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

See #199

@moritzschmitz-oviva moritzschmitz-oviva force-pushed the add-docker-build branch 3 times, most recently from 6823bfd to 03b04a7 Compare February 1, 2024 09:11
Comment on lines 1 to 12
keyStorePath = /tmp/keystore.p12
keyStorePass = example123
keyPass = pass123

trustStorePath = /tmp/truststore.p12
trustStorePass = otherpass123

keyStoreType = pkcs12

keyAlias = spiffe

spiffeSocketPath = unix:/tmp/agent.sock
Copy link
Member

Choose a reason for hiding this comment

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

This file is redundant now as #199 is merged, which adds a java-spiffe-helper.properties, an example configuration in the default config path. That file could be copied in the Dockerfile. WDYT?

moritzschmitz-oviva and others added 12 commits February 5, 2024 16:45
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
…sion to gradle properties

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Co-authored-by: Ryan Turner <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
Copy link
Member

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

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

Left two minor comments. It's looking good.

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>
@moritzschmitz-oviva
Copy link
Contributor Author

Left two minor comments. It's looking good.

Thank you. Made the adjustments.

Copy link
Member

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

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

@maxlambrecht maxlambrecht merged commit 27b2a01 into spiffe:main Feb 5, 2024
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.

4 participants