-
Notifications
You must be signed in to change notification settings - Fork 16.7k
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions 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/test-infra repository. I understand the commands that are listed here. |
/check-cla |
/assign @lachie83 |
Please have a look at our RBAC best practices. https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/rbac.md |
/assign |
stable/falco/README.md
Outdated
@@ -50,7 +50,8 @@ The following table lists the configurable parameters of the Falco chart and the | |||
| `image.pullPolicy` | The image pull policy | `Always` | | |||
| `resources` | Specify container resources | `{}` | | |||
| `rbac.create` | If true, create & use RBAC resources | `true` | | |||
| `rbac.serviceAccountName` | If rbac.create is false, use this value as serviceAccountName | `default` | | |||
| `serviceAccount.create` | Create serviceAccount | `default` | |
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.
default -> true
subjects: | ||
- kind: ServiceAccount | ||
name: {{ template "falco.serviceAccountName" .}} | ||
namespace: default |
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.
default -> {{ .Release.Namespace }}
apiVersion: rbac.authorization.k8s.io/v1beta1 | ||
metadata: | ||
name: {{ template "falco.fullname" .}} | ||
namespace: default |
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.
Remove namespace
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" | ||
release: "{{ .Release.Name }}" | ||
heritage: "{{ .Release.Service }}" | ||
data: {} |
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.
Why do you create an empty configmap. The above rules file is not used by the chart. Resources created by Helm should not be edited manually as you suggest in the readme. This would get you in trouble when updating a release.
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.
Because we want to allow that chart users adds its own rules. Let me to explain a bit more the context.
Although Falco comes with a nice default rule set for detecting weird behavior in containers, our users are going to customize the run-time security rule sets or policies for the specific container images and applications they run. In Falco we provide an /etc/falco/rules.d directory for dropping new files with rules and we want to allow our users to upload its rules to that directory, so our idea was to mount the /etc/falco/rules.d directory as a configMap and allow users to add rules to that configMap.
For us, the ideal situation would be allowing to pass rules files when installing the helm but
I have been reading about this problem helm/helm#3276 or in Accessing files inside templates and looks like this is not supported. Perhaps you have some tips or guidance or a workaround for this issue?
Thanks!
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.
If you use Helm you should use it exclusively. Manipulating file installed by Helm will get you in trouble. Why don't you make the configmap configurable via values.yaml
?
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.
Done! We load custom rules via values.yaml file :)
Thanks!
@unguiculus can you have a look at the changes @nestorsalceda pushed? |
circle is failing on some changes that have been fixed in master. Can you rebase on master? |
We can correlate falco.* keys for falco related settings, and refer them in Falco Wiki
First one is too generic
@lachie83 I have rebased and right now CircleCI build is passing :) https://circleci.com/gh/kubernetes/charts/tree/pull%2F5853 Thanks! |
stable/falco/Chart.yaml
Outdated
@@ -0,0 +1,19 @@ | |||
apiVersion: v1 | |||
name: falco | |||
version: '0.1' |
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.
Remove single quotes
I'm not sure if this break lint stage in CircleCI
I did remove the quotes, but @lachie83 I think the build is failing because that quotes: |
Was not symverv2 compliant. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lachie83, nestorsalceda The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/ok-to-test |
* [stable/falco] Add Falco chart * Fix indentation and other stuff reported by CI * Add appVersion to Chart.yaml * Specify container resources * Allow to load external Falco rules * Move GCSCC integrations to a top level integrations section We can correlate falco.* keys for falco related settings, and refer them in Falco Wiki * Rename deployment to fakeEventGenerator First one is too generic * Add OWNERS file * Separate rbac and serviceAccount Follow RBAC best practices: https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/rbac.md * Use falco.serviceAccount name template for cluster role binding * Fixes required from reviewer * Allow passing rules in an external file instead of editing configMap by hand * Remove quotes from Chart version I'm not sure if this break lint stage in CircleCI * Update Chart.yaml
* [stable/falco] Add Falco chart * Fix indentation and other stuff reported by CI * Add appVersion to Chart.yaml * Specify container resources * Allow to load external Falco rules * Move GCSCC integrations to a top level integrations section We can correlate falco.* keys for falco related settings, and refer them in Falco Wiki * Rename deployment to fakeEventGenerator First one is too generic * Add OWNERS file * Separate rbac and serviceAccount Follow RBAC best practices: https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/rbac.md * Use falco.serviceAccount name template for cluster role binding * Fixes required from reviewer * Allow passing rules in an external file instead of editing configMap by hand * Remove quotes from Chart version I'm not sure if this break lint stage in CircleCI * Update Chart.yaml
What this PR does / why we need it:
This PR adds a chart for installing Sysdig Falco in Kubernetes.
Sysdig Falco is a behavioral activity monitor designed to detect anomalous activity in your applications.
You can use Falco to monitor run-time security of your Kubernetes applications and internal components.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #None, because this is a new chart.
Special notes for your reviewer: