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

Add audit feature #84

Merged
merged 10 commits into from Apr 17, 2019

Conversation

Projects
None yet
3 participants
@ritazh
Copy link
Contributor

commented Apr 8, 2019

This adds the audit feature for gatekeeper.
Note:

  • message is currently truncated at 256.
  • flag --auditInterval is the interval to run audit in seconds. defaulted to 60 secs if unspecified.
  • flag --constraintViolationsLimit is the limit of number of violations per constraint. defaulted to 20 violations if unspecified.
  • audit violations are added to each failed constraint's status field:
status:
  auditTimestamp: 2019-04-11T07:24:44Z
  enforced: true
  violations:
  - kind: Namespace
    message: 'you must provide labels: {"gatekeeper"}'
    name: kube-public
  • will add test as a followup PR. wanted to get feedback on this first.

Sample output:

apiVersion: constraints.gatekeeper.sh/v1alpha1
kind: K8sRequiredLabel
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"constraints.gatekeeper.sh/v1alpha1","kind":"K8sRequiredLabel","metadata":{"annotations":{},"name":"ns-must-have-gk","namespace":""},"spec":{"match":{"kinds":[{"apiGroups":[""],"kinds":["Namespace"]}]},"parameters":{"labels":["gatekeeper"]}}}
  creationTimestamp: 2019-04-08T22:37:46Z
  finalizers:
  - finalizers.gatekeeper.sh/constraint
  generation: 47
  name: ns-must-have-gk
  resourceVersion: "12030711"
  selfLink: /apis/constraints.gatekeeper.sh/v1alpha1/k8srequiredlabels/ns-must-have-gk
  uid: ee6d646b-5a4e-11e9-9d70-000d3a94fc72
spec:
  match:
    kinds:
    - apiGroups:
      - ""
      kinds:
      - Namespace
  parameters:
    labels:
    - gatekeeper
status:
  auditTimestamp: 2019-04-11T07:24:44Z
  enforced: true
  violations:
  - kind: Namespace
    message: 'you must provide labels: {"gatekeeper"}'
    name: bad-ns

Signed-off-by: Rita Zhang rita.z.zhang@gmail.com

@ritazh ritazh force-pushed the ritazh:audit branch from ead5cfa to c07d377 Apr 8, 2019

@ritazh ritazh requested review from maxsmythe and tsandall Apr 8, 2019

@maxsmythe
Copy link
Contributor

left a comment

Looking good so far!

Some thoughts:

  • We probably want to post a timestamp of the last audit to the status as well so people can tell if they're looking at an old audit

  • It would be cool if we could implement the Runnable interface and the injection interface to avoid needing to create a new manager. This will allow us to re-use the client cache and operate in a manner identical to the controllers, webhooks, etc.

Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved cmd/manager/main.go
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
@tsandall
Copy link
Member

left a comment

This looks like a great start. I notice we're not truncating the audit result set (we're truncating the individual message strings, but not the number of violations). I could imagine passing a query limit down from the audit controller to the framework. WDYT?

Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
@maxsmythe

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@tsandall

Re: Passing a query limit to the framework.

I was thinking of adding varargs terms to the and of the Audit(), Review() etc. endpoints. I think that might be a good place to put something like this. It'd also be useful for things like "run a trace for this specific query".

WRT a query limit for audit... what's the granularity? Is it a limit per violating resource? Total number of resources returned? Is there pagination? If so, are there transaction semantics to make sure we're paginating over a stable set of audit data?

To be clear, I'm in support of pagination and limits to support scalability generally. Depending on where and how these limits are used, some of the above features may be essential to guarantee a minimum set of behavioral characteristics for audit, review, etc.

@tsandall

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@maxsmythe @ritazh I was mostly thinking of putting a limit on the number of violations per constraint since that seems like the most obvious bottleneck. Do we know the upper limit on # of violations we can expect to reasonably store on the constraint status? EDIT: Perhaps just documenting that at the outset would be a good idea.

@ritazh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@tsandall AFAIK, there is the 1MB limit from the etcd. It makes sense to me to set a limit for the number of violations. But make it configurable.

@maxsmythe
Copy link
Contributor

left a comment

Looking good!

I think the biggest piece missing is a retry framework to catch failed updates (e.g. resource version gets out of sync due to parallel write).

I'd suggest building a struct that gets created/destroyed every loop cycle to handle the retries to avoid clobbering more recent updates.

Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated

@ritazh ritazh force-pushed the ritazh:audit branch from 4c97051 to 8d26de9 Apr 16, 2019

@maxsmythe
Copy link
Contributor

left a comment

Almost there! Some nits, then mergeable.

Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go
Show resolved Hide resolved pkg/audit/manager.go Outdated
Show resolved Hide resolved pkg/audit/manager.go Outdated
@maxsmythe
Copy link
Contributor

left a comment

1 nit about deepcopy, then good to merge

Namespace: namespace,
}
// get the latest constraint
err := ucloop.client.Get(ctx, namespacedName, &item)

This comment has been minimized.

Copy link
@maxsmythe

maxsmythe Apr 17, 2019

Contributor

We should deepcopy item to avoid unexpected side effects in the future. Maybe the DeepCopyInto() method?

This comment has been minimized.

Copy link
@ritazh

ritazh Apr 17, 2019

Author Contributor

done

ritazh added some commits Apr 8, 2019

Add audit feature
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
use same top level manager; rm dup clients; separate getUpdateLists; …
…pass gvk;

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
create new client to update restmapper; separate gateAllConstraintsKi…
…nds; separate updateConstraintsForKinds; combine clear and update voilation into single loop

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
StatusViolation omit Namespace; rm dup context; add auditTimestamp to…
… status

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
Add constraintViolationsLimit
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
incorporate feedback
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
add retry framework; move crd global var; listkind
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
address comments
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>
get and update deepcopy instead
Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

@ritazh ritazh force-pushed the ritazh:audit branch from 3133a9f to b3296bd Apr 17, 2019

@maxsmythe maxsmythe merged commit 8eadf3e into open-policy-agent:master Apr 17, 2019

3 checks passed

DCO DCO
Details
cla/linuxfoundation maxsmythe authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.