Enable readOnlyRootFilesystem for all AAQ containers - #209
Conversation
Signed-off-by: davmarro <dmarro@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The AAQ security e2e test hardcodes deployment names, which may become brittle if names change; consider selecting the deployments via labels or other shared metadata instead.
- The AAQ security e2e test only inspects
spec.containersand ignoresinitContainers; if any AAQ init containers exist or are added later, they should also be checked forreadOnlyRootFilesystemto ensure consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The AAQ security e2e test hardcodes deployment names, which may become brittle if names change; consider selecting the deployments via labels or other shared metadata instead.
- The AAQ security e2e test only inspects `spec.containers` and ignores `initContainers`; if any AAQ init containers exist or are added later, they should also be checked for `readOnlyRootFilesystem` to ensure consistency.
## Individual Comments
### Comment 1
<location path="tests/aaq_operator_test.go" line_range="697-691" />
<code_context>
+ It("All AAQ deployments should have readOnlyRootFilesystem set to true", func() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also covering initContainers (if present) for the readOnlyRootFilesystem requirement
Since this test enforces `ReadOnlyRootFilesystem=true` on all main containers, it would be helpful to also iterate over `deployment.Spec.Template.Spec.InitContainers` and assert the same setting there, or assert that no initContainers exist. This would strengthen the test as a regression guard if initContainers are added later.
Suggested implementation:
```golang
deployment, err := f.K8sClient.AppsV1().Deployments(f.AAQInstallNs).Get(context.TODO(), deploymentName, metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred(), "failed to get deployment %s", deploymentName)
// ensure initContainers (if present) also comply with readOnlyRootFilesystem requirement
Expect(len(deployment.Spec.Template.Spec.InitContainers)).To(
Equal(0),
"deployment %s must not define initContainers unless they also enforce ReadOnlyRootFilesystem=true",
deploymentName,
)
```
If you later decide to allow initContainers, replace the `Equal(0)` assertion with a loop over `deployment.Spec.Template.Spec.InitContainers` mirroring the checks done for `Containers` (non-nil `SecurityContext`, non-nil `ReadOnlyRootFilesystem`, and `ReadOnlyRootFilesystem == true`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -690,6 +691,26 @@ var _ = Describe("ALL Operator tests", Serial, func() { | |||
| }) | |||
There was a problem hiding this comment.
suggestion (testing): Consider also covering initContainers (if present) for the readOnlyRootFilesystem requirement
Since this test enforces ReadOnlyRootFilesystem=true on all main containers, it would be helpful to also iterate over deployment.Spec.Template.Spec.InitContainers and assert the same setting there, or assert that no initContainers exist. This would strengthen the test as a regression guard if initContainers are added later.
Suggested implementation:
deployment, err := f.K8sClient.AppsV1().Deployments(f.AAQInstallNs).Get(context.TODO(), deploymentName, metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred(), "failed to get deployment %s", deploymentName)
// ensure initContainers (if present) also comply with readOnlyRootFilesystem requirement
Expect(len(deployment.Spec.Template.Spec.InitContainers)).To(
Equal(0),
"deployment %s must not define initContainers unless they also enforce ReadOnlyRootFilesystem=true",
deploymentName,
)If you later decide to allow initContainers, replace the Equal(0) assertion with a loop over deployment.Spec.Template.Spec.InitContainers mirroring the checks done for Containers (non-nil SecurityContext, non-nil ReadOnlyRootFilesystem, and ReadOnlyRootFilesystem == true).
|
@davmarro: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What this PR does / why we need it:
Enables
readOnlyRootFilesystem: truein the SecurityContext of all AAQ containers(aaq-operator, aaq-controller, aaq-server) to reduce attack surface.
All three containers are created via the shared
CreateContainer()helper inpkg/util/util.go,so a single-line addition covers them all.
No emptyDir
/tmpvolume is needed — none of the AAQ containers write to/tmpin production code.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Also fixed a pre-existing linter error in
pkg/util/util.go(lines 243-244):klog.Infof(fmt.Sprintf(...))→klog.Infof(...)to allow the package to compile.Release note: