-
Notifications
You must be signed in to change notification settings - Fork 366
feat(trait): add jvm caCert support and propagate mounts to init containers #6416
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
Conversation
squakez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work. It may work under certain conditions. However, it is breaking a little the encapsulation. I wonder if you could instead moving all that code to the init-container trait instead. The JVM could still hold the variable of ca-secret, in a similar manner of what we did with #6256
|
✔️ Unit test coverage report - coverage increased from 51.6% to 51.7% (+0.1%) |
|
@squakez thanks for the feedback, I'll refer the PR and make the necessary changes. |
|
@squakez If you are satisfied with the current implementation, I am more than happy to work for the solution of #6397 here itself. I'll Update the PR title and Description as well. Just let me know what approach you will prefer ? and shall it be solved in a separate PR. |
|
✔️ Unit test coverage report - coverage increased from 51.6% to 51.7% (+0.1%) |
squakez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks!
Yeah, I think it makes sense to have a generic solution for the mounts to be shared among all containers. However, you can certainly work in a separate PR as it best suit to you. Let me know if you're continuing here, so I'll hold before merging this one. |
|
@squakez I'll be working here only. Will follow up shortly with a genric implementation. |
|
@squakez I have made the changes and also added another E2E test to verify working. |
squakez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool work indeed. Just a little but important security consideration we must address before merging.
|
✔️ Unit test coverage report - coverage increased from 51.6% to 51.8% (+0.2%) |
|
@squakez Addresses security concern where password was exposed in command-line arguments. The password is now injected as an environment variable from a user-provided Kubernetes Secret. |
|
✔️ Unit test coverage report - coverage increased from 50.6% to 50.8% (+0.2%) |
|
Hello.
|
|
@ephemeris-lappis Thanks for the feedback, I am sorry that a few things got overlooked in this implementation.
CC: @squakez |
This PR adds two related features for init container support:
ConfigMaps mounted via
mount.configsSecrets mounted via
mount.configsResources mounted via
mount.resourcesChanges:
Update
Closes
#2820
#6256