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

apiserver: add a webhook implementation of the audit backend #45919

Merged

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented May 16, 2017

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

Webhook added to the API server which omits structured audit log events.

/cc @soltysh @timstclair @soltysh @deads2k

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 16, 2017
@timstclair
Copy link

/cc @ihmccreery

@k8s-ci-robot k8s-ci-robot requested a review from ikehz May 17, 2017 00:03
@timothysc
Copy link
Member

/cc @jbeda @kensimon

@k8s-ci-robot k8s-ci-robot requested a review from jbeda May 17, 2017 00:06
@k8s-ci-robot
Copy link
Contributor

@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.
.

In response to this:

/cc @jbeda @kensimon

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.

Copy link

@timstclair timstclair left a 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?

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()

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ericchiang ericchiang force-pushed the audit-webhook-backend branch 2 times, most recently from e397f24 to ca6f107 Compare May 17, 2017 04:42
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2017
@ericchiang ericchiang force-pushed the audit-webhook-backend branch from ca6f107 to 1d7f225 Compare May 17, 2017 04:45
@ericchiang
Copy link
Contributor Author

ericchiang commented May 17, 2017

Awesome! Were you able to test it in an actual cluster?

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.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 17, 2017
@timothysc
Copy link
Member

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.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 17, 2017
@ericchiang
Copy link
Contributor Author

@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}}

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

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...)

Copy link
Contributor

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)
}

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.

Copy link
Contributor Author

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]

// 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]
func init() {
Install(api.GroupFactoryRegistry, api.Registry, api.Scheme)
}

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@ericchiang ericchiang force-pushed the audit-webhook-backend branch from 11fad6e to e7235bf Compare May 31, 2017 15:48

c := conversion.NewCloner()
for _, f := range metav1.GetGeneratedDeepCopyFuncs() {
if err := c.RegisterGeneratedDeepCopyFunc(f); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

@sttts
Copy link
Contributor

sttts commented May 31, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017
@sttts
Copy link
Contributor

sttts commented May 31, 2017

@deads2k can you approve?

@ericchiang ericchiang force-pushed the audit-webhook-backend branch from e7235bf to a88e018 Compare May 31, 2017 16:45
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

// Copied from generated code in k8s.io/apiserver/pkg/apis/audit.
//
// TODO(ericchiang): Have the generated code expose these methods like metav1.GetGeneratedDeepCopyFuncs().
Copy link
Contributor Author

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:

https://github.com/kubernetes/apiserver/blob/c809cf8581e1e44c6174bf5ab4415e6ee39965ca/pkg/apis/audit/zz_generated.deepcopy.go#L37-L45

@sttts

@ericchiang ericchiang added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017
@ericchiang
Copy link
Contributor Author

Re-applying lgtm per #45919 (comment)

@deads2k
Copy link
Contributor

deads2k commented May 31, 2017

/approve

@k8s-github-robot
Copy link

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2017
@ericchiang
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@timstclair
Copy link

Test failing due to #46713

@timstclair
Copy link

@k8s-bot gce etcd3 e2e test this

@timstclair timstclair mentioned this pull request May 31, 2017
1 task
@timstclair
Copy link

@k8s-bot gce etcd3 e2e test this

@ericchiang
Copy link
Contributor Author

:(

would rebasing be a good idea?

@timstclair
Copy link

I think we just need to wait for #46713 to be resolved :(

@timstclair
Copy link

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@sttts
Copy link
Contributor

sttts commented Jun 1, 2017

@k8s-bot pull-kubernetes-node-e2e test this
@k8s-bot gce etcd3 e2e test this

@sttts
Copy link
Contributor

sttts commented Jun 1, 2017

@k8s-bot kops aws e2e test this
@k8s-bot gce etcd3 e2e test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 1, 2017

@ericchiang: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins kops AWS e2e 08e8d564fd455206567abe0b1e209d9a13ad4991 link @k8s-bot kops aws e2e test this
Jenkins GCE etcd3 e2e 08e8d564fd455206567abe0b1e209d9a13ad4991 link @k8s-bot gce etcd3 e2e test this

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.

@timstclair
Copy link

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f7a1f10 into kubernetes:master Jun 1, 2017
@ericchiang ericchiang deleted the audit-webhook-backend branch June 1, 2017 21:23
k8s-github-robot pushed a commit that referenced this pull request Jun 3, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants