-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Kubernetes support of SecurityContext configuration #24089
Conversation
Allow to configure the security context section in the pods for Kubernetes, OpenShift and Knative. Fix quarkusio#23866
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
@@ -317,6 +317,10 @@ | |||
result.add(new DecoratorBuildItem(target, new ApplyRequestsMemoryDecorator(name, m))); | |||
}); | |||
|
|||
if (config.getSecurityContext().isAnyPropertySet()) { |
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.
This is a hack ... But I tried to make the security context optional (which it is, so it makes sense), but the configuration does not deal well with having a complex and nested configuration with optional. Concretely, if I use Optional instead of the hack isAnyPropertySet
, it fails with:
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.IllegalArgumentException: Can not set io.quarkus.kubernetes.deployment.SecurityContextConfig$WindowsOptions field io.quarkus.kubernetes.deployment.SecurityContextConfig.windowsOptions to java.util.Optional
This is related to #7862, which was recently closed with a workaround, but not to really give support of optional configuration groups.
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.
From my point of view this is clear as it communicates the intention and there is no room for interpretations.
Going the Optional way it would be unclear how default values are meant to be handled.
So, +1 for this.
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.
I would also like to have test coverage for the default options.
Generally speaking, I am wondering if we should have defaults
assertions in a reusable form as a way to capture cases were the default behavior is broken, which is something that occurs quite often lately.
By default, it won't add any security context-related configuration. This is already asserted in https://github.com/quarkusio/quarkus/pull/24089/files#diff-fcb7320608f71deafa5b1645553e8779d1ed22f1f3a667b025e6f003d9214142R78. |
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.
LGTM
Allow to configure the security context section in the pods for Kubernetes, OpenShift and Knative.
Fix #23866