Add audit logging support with a default logging policy #163
Conversation
| pathType: File | ||
| - name: audit-logs | ||
| hostPath: /var/log/kubernetes | ||
| mountPath: /var/log/kubernetes |
There was a problem hiding this comment.
Does this directory always exist? Should be sure that we create it/ensure it exists.
There was a problem hiding this comment.
Kubelet will create it if it does not exist (Due to DirectoryOrCreate pathType). Do you think it is better to create the dir in advance?
There was a problem hiding this comment.
Is there a reason not to have log destination set to - (stdout) so that standard docker log rotation and any log aggregation would be used? If staying with /var/log/kubernetes, what log rotation is in place, or what's available to prevent filling the partition?
Update - just looked at the full file, hadn't realized there were already rotation parameters in place, that addresses that concern. Only other question would be connecting to centralized logging - if sent to stdout then fluentd would add all pod labels for filtering (e.g. in elastic). If current destination is kept, I'm not quite sure how ansible hash merge strategy works for removing an element from a list, ie if user wants log destination to be '-', volumes list should be only the audit config, not the /var/log/kubernetes.
There was a problem hiding this comment.
Logs are sent to /var/log/kubernetes/ based on the default fluentd-kubernetes-daemonset configuration:
| template: | ||
| dest: /etc/kubernetes/apiserver-audit-policy.yaml | ||
| src: etc/kubernetes/apiserver-audit-policy.yaml | ||
| when: "'masters' in group_names" |
There was a problem hiding this comment.
Should this be in the kubernetes-master role instead of having this conditional?
There was a problem hiding this comment.
Hmmm yeah. Dropped it here because I added the kubeadm audit flags in this role. I agree it's better to move it to the kubernetes-master role.
| apiVersion: audit.k8s.io/v1beta1 | ||
| kind: Policy | ||
| rules: | ||
| {% if kubernetes_common_audit_policy_rules is defined and kubernetes_common_audit_policy_rules != "" %} |
There was a problem hiding this comment.
Do we define a default value for this? I think this conditional should be checking the length of the kubernetes_common_audit_policy_rules list.
This pattern is fine, but it is a bit different than how we have done other API templates. In those templates we have put the full spec into the defaults so that users can override values using ansible's merge hash functionality. As this is primarily a list of rules, this would be much harder to override specific parts.
There was a problem hiding this comment.
Ah yep! Will change to follow the existing pattern. Thanks 👍
|
@erictcgs can you review for whether this would address your needs? |
alexbrand
force-pushed
the
audit-logs
branch
2 times, most recently
from
2b303fe to
88e9d09
Compare
Enable Kubernetes API audit logging with a default logging policy. When desired, users can provide their custom audit policy rules using an ansible variable. Fixes vmware-archive#138 Signed-off-by: Alexander Brand <alexbrand09@gmail.com>
Enable Kubernetes API audit logging with a default logging policy. When
desired, users can provide their custom audit policy rules using an
ansible variable.
Fixes #138
Signed-off-by: Alexander Brand alexbrand09@gmail.com