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

CWS: fix runtime policies loading, when no config map is defined #522

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

paulcacheux
Copy link
Contributor

@paulcacheux paulcacheux commented Jun 21, 2022

What does this PR do?

When loading the agent through the operator with a default config like:

apiVersion: datadoghq.com/v1alpha1
kind: DatadogAgent
metadata:
  name: datadog
spec:
  credentials:
    apiKey: XX
    appKey: YY
  agent:
    security:
      runtime:
        enabled: true

The CWS policies directory is empty. The reason explaining this issue is that the policies dir is always mounted, even when no override is defined by the user. This means that in the default case we are overriding the default rules with a mount to a non-existant/empty folder

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Describe your test plan

Write there any instructions and details you may have to test your PR.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, refactoring, documentation, tooling

@paulcacheux paulcacheux added the bug Something isn't working label Jun 21, 2022
@paulcacheux paulcacheux marked this pull request as ready for review June 21, 2022 11:54
@paulcacheux paulcacheux requested review from a team as code owners June 21, 2022 11:54
@paulcacheux paulcacheux requested a review from a team June 21, 2022 11:54
@celenechang celenechang modified the milestones: v0.8.0, v0.8.1 Jun 21, 2022
@CharlyF
Copy link
Contributor

CharlyF commented Jun 21, 2022

Seems like we still create the empty volume at the pod spec level when no custom policies are used. Should we get rid of this too or the volume mount in the system probe is enough?

if isRuntimeSecurityEnabled(&dda.Spec) {
volumes = append(volumes,
corev1.Volume{
Name: datadoghqv1alpha1.SecurityAgentRuntimePoliciesDirVolumeName,
VolumeSource: corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{},
},
})
if dda.Spec.Agent.Security.Runtime.PoliciesDir != nil {
volumes = append(volumes,
getVolumeFromConfigDirSpec(datadoghqv1alpha1.SecurityAgentRuntimeCustomPoliciesVolumeName, dda.Spec.Agent.Security.Runtime.PoliciesDir),
)
}

@paulcacheux
Copy link
Contributor Author

What this PR does is enough to fix the issue, if you feel that the volume definition would be even better I can include it in the second if

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2022

Codecov Report

Merging #522 (5af58de) into main (34a1618) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #522   +/-   ##
=======================================
  Coverage   58.51%   58.51%           
=======================================
  Files           3        3           
  Lines         135      135           
=======================================
  Hits           79       79           
  Misses         43       43           
  Partials       13       13           
Flag Coverage Δ
unittests 58.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34a1618...5af58de. Read the comment docs.

@CharlyF
Copy link
Contributor

CharlyF commented Jun 22, 2022

What this PR does is enough to fix the issue, if you feel that the volume definition would be even better I can include it in the second if

Just want to make sure we are covering all grounds.
Thanks for the contrib! 🚀

@CharlyF CharlyF merged commit f45c5b8 into main Jun 22, 2022
@CharlyF CharlyF deleted the paulcacheux/fix-cws-agent branch June 22, 2022 14:17
celenechang pushed a commit that referenced this pull request Jun 23, 2022
* Only mount CWS policies volume if policies dir is defined

* Do not create system-probe/security-agent volumes is not needed
celenechang added a commit that referenced this pull request Jun 24, 2022
… (#529)

* Only mount CWS policies volume if policies dir is defined

* Do not create system-probe/security-agent volumes is not needed

Co-authored-by: Paul Cacheux <paul.cacheux@datadoghq.com>
@celenechang celenechang mentioned this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants