-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
apiserver: add a webhook implementation of the audit backend #45919
apiserver: add a webhook implementation of the audit backend #45919
Conversation
/cc @ihmccreery |
@timothysc: GitHub didn't allow me to request PR reviews from the following users: kensimon. Note that only people with write access to kubernetes/kubernetes can review this PR. |
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.
Awesome! Were you able to test it in an actual cluster?
// New returns an audit backend at sends events over HTTP to the. | ||
func New(kubeConfigFile string) (*AuditBackend, error) { | ||
// TODO(ericchiang): Figure out some way to allow protobuf encoding? Is there | ||
// a kubeconfig extension to request this? |
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.
FYI - we were having trouble with the protobuf code generation, so that needs to happen first.
// LogAuditEvent attempts to POST a serialized audit event to an external service. | ||
func (a *AuditBackend) LogAuditEvent(ev *audit.Event) error { | ||
// TODO(ericchiang): With retry and backoff? | ||
return a.w.RestClient.Post().Body(ev).Do().Error() |
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.
Can we make this send an EventList instead? Even if we're sending single events now, it means we can add batching later without changing the endpoint api at all.
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
e397f24
to
ca6f107
Compare
ca6f107
to
1d7f225
Compare
Nope. Haven't pulled in sttts' PR yet. Unit tests produce a nice, empty audit object though. EDIT: everything's broken. going to wait for the other PRs to merge before moving forward. |
This needs both a release note and full doc behind it. Is there an entry in the features repo? Almost every operator is going to need to hook into this. |
@timstclair added release notes and linked the feature and design proposal. |
// payload contains an audit.EventList with a single element. The audit webhook may batch | ||
// multiple events in the future. | ||
func (a *AuditBackend) LogAuditEvent(ev *audit.Event) error { | ||
list := &audit.EventList{Items: []audit.Event{*ev}} |
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.
Does this need to be converted to the versioned API, or is that done automatically by the RestClient?
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 by the rest client, compare the internal client, e.g. pkg/client/clientset_generated/internalclientset/typed/batch/internalversion/job.go
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.
That's correct. Let me update the test to show that.
// Since the audit API isn't registered with the client or apimachinery API, | ||
// create a unique scheme to register this API group with here. | ||
// | ||
// TODO(ericchiang): Refactor this once the audit API group is moved to k8s.io/api. |
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.
I think the current plan is that it will move into a new repository, e.g. k8s.io/apiserverapi
(needs a better name...)
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.
Repository inflation :-/
} | ||
|
||
install.Install(groupFactoryRegistry, registry, scheme) | ||
} |
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.
Can we move this to a common location? I'm not sure what the "right" way to do it is, but I need something similar to parse the Policy out of a file.
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.
This first bit, at least, is just copied from other webhooks.[1] The install call was because "k8s.io/apiserver/pkg/apis/audit/install" doesn't call this like other api groups.[2] Maybe we could have the audit/install package actually register the API group?
[1]
kubernetes/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go
Lines 210 to 221 in e9b02c2
// NOTE: client-go doesn't provide a registry. client-go does registers the | |
// authorization/v1beta1. We construct a registry that acknowledges | |
// authorization/v1beta1 as an enabled version to pass a check enforced in | |
// NewGenericWebhook. | |
var registry = registered.NewOrDie("") | |
func init() { | |
registry.RegisterVersions(groupVersions) | |
if err := registry.EnableVersions(groupVersions...); err != nil { | |
panic(fmt.Sprintf("failed to enable version %v", groupVersions)) | |
} | |
} |
[2]
kubernetes/pkg/apis/imagepolicy/install/install.go
Lines 31 to 33 in e9b02c2
func init() { | |
Install(api.GroupFactoryRegistry, api.Registry, api.Scheme) | |
} |
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.
It looks like most API groups are installed on k8s.io/kubernetes/pkg/api (see the second link above). If I'm not mistaken, we can't import that package here. The other k8s.io/apiserver/pkg/apis group doesn't do this import.
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.
We don't have a global scheme and registry in k8s.io/apiserver. All the new api servers (aggregation, apiextensions, ...) have their private one for the crud served resources. Having a private scheme is the way to go.
11fad6e
to
e7235bf
Compare
|
||
c := conversion.NewCloner() | ||
for _, f := range metav1.GetGeneratedDeepCopyFuncs() { | ||
if err := c.RegisterGeneratedDeepCopyFunc(f); err != nil { |
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.
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.
also the audit deepcopy funcs. Feel free to re-apply lgtm after that.
/lgtm |
@deads2k can you approve? |
e7235bf
to
a88e018
Compare
|
||
// Copied from generated code in k8s.io/apiserver/pkg/apis/audit. | ||
// | ||
// TODO(ericchiang): Have the generated code expose these methods like metav1.GetGeneratedDeepCopyFuncs(). |
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.
Going to follow up with a change to not have this copied from the audit package. It's currently in a generated file that I don't want to mess with:
Re-applying lgtm per #45919 (comment) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ericchiang, sttts, timstclair
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
Test failing due to #46713 |
@k8s-bot gce etcd3 e2e test this |
@k8s-bot gce etcd3 e2e test this |
:( would rebasing be a good idea? |
I think we just need to wait for #46713 to be resolved :( |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
@ericchiang: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
Automatic merge from submit-queue |
Automatic merge from submit-queue (batch tested with PRs 46620, 46732, 46773, 46772, 46725) Instrument advanced auditing Add prometheus metrics for audit logging, including: - A total count of audit events generated and sent to the output backend - A count of audit events that failed to be audited due to an error (per backend) - A count of request audit levels (1 per request) For kubernetes/enhancements#22 - [x] TODO: Call `HandlePluginError` from the webhook backend, once #45919 merges (in this or a separate PR, depending on timing of the merge) /cc @ihmccreery @sttts @soltysh @ericchiang
This builds off of #45315 and is intended to implement an interfaced defined in #45766.
TODO:
Features issue kubernetes/enhancements#22
Design proposal https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auditing.md
/cc @soltysh @timstclair @soltysh @deads2k