@ilayaperumalg
~The motivating issue also calls out the need for container level securityContext.(allowPrivilegeEscalation|readOnlyRootFilesystem) support . However, I would prefer if we split that into another ticket (or at least an additional pull request) to keep the changes in this code proposal concise~
ℹ️ Initially I was going to break this into 2 separate pull requests but decided to combine them once I saw them both implemented.
Adds support for configuring the seccompProfile on the pod security context
Adds support for configuring the security context on the main container
Question 1: I only apply the context to the main container. Should we add this to the initContainer as well? There is no need to do anything special for additional containers as the only mechanism to configure those is via full yaml - consumers can simply add the security context in that yaml if need be.
Question 2: Should we support more than these 2 requested properties? I know we do not support all available properties on the pod security context either. On one hand, adding more properties increases code to maintain. On the other hand, adding more properties gives users more options.
Refactors KubernetesAppDeployerTests as follows:
Moved the pod and container security related tests into their own @Nested tests.
Simplified the assertions to use actual/expected object equality rather than individual property asserts
I implemented the new container security context tests using the @Nested approach and it looked like a great fit for the existing pod security context tests as well. Because re-ordering and renaming code during a review makes for a not so fun review, I intentionally made small commits (2-6) to show step by step what I did w/ the pod security context tests to get to the final result. I apologize in advance for any grief this causes the reviewer. These of course should all be squashed into a single commit into main.
Motivation:
The KubernetesAppDeployerTests has grown quite large and it is a challenge for a newbie (me) to grok all of whats going on in there. I leveraged @Nested to group the pod and container security context tests. I really love the output that @Nested produces in IntelliJ as well (see below)
Question 3: I am curious to what the team thinks on this test as a whole. Should it be:
left as-is (don't use @Nested)?
split into separate KubernetesAppDeployer<tested-area-goes-here>Tests?
apply @Nessted to other areas of the test as well?
@ilayaperumalg ~The motivating issue also calls out the need for container level
securityContext.(allowPrivilegeEscalation|readOnlyRootFilesystem)
support . However, I would prefer if we split that into another ticket (or at least an additional pull request) to keep the changes in this code proposal concise~Adds support for configuring the seccompProfile on the pod security context
Adds support for configuring the security context on the main container
Refactors KubernetesAppDeployerTests as follows:
@Nested
tests.@Nested
approach and it looked like a great fit for the existing pod security context tests as well. Because re-ordering and renaming code during a review makes for a not so fun review, I intentionally made small commits (2-6) to show step by step what I did w/ the pod security context tests to get to the final result. I apologize in advance for any grief this causes the reviewer. These of course should all be squashed into a single commit into main.Motivation: The
KubernetesAppDeployerTests
has grown quite large and it is a challenge for a newbie (me) to grok all of whats going on in there. I leveraged@Nested
to group the pod and container security context tests. I really love the output that@Nested
produces in IntelliJ as well (see below)@Nested
)?KubernetesAppDeployer<tested-area-goes-here>Tests
?Remaining items