dynamic audit configuration api #67547

pbarker · 2018-08-17T16:44:06Z

What this PR does / why we need it:
Implements dynamic audit configuration api

Special notes for your reviewer:
This is just the api changes for the dynamic audit configuration feature. Part 2 of the implementation is here #67257

Release note:

Add dynamic audit configuration api

tallclair · 2018-08-17T16:55:12Z

/ok-to-test
/kind api-change
/priority important-soon
/sig auth
/milestone v1.12

/assign @kubernetes/api-reviewers

tallclair · 2018-08-17T16:56:20Z

@kubernetes/sig-api-machinery-api-reviews

pbarker · 2018-08-17T18:39:17Z

@tallclair would you like to see the storage and registration pieces for the api here or in the other PR?

tallclair · 2018-08-17T18:54:32Z

I think they belong here.

lavalamp · 2018-08-17T19:59:20Z

staging/src/k8s.io/apiserver/hack/update-codegen.sh

@@ -0,0 +1,81 @@
+
+#!/usr/bin/env bash


Why are we copying hack scripts into staging repos? That doesn't seem like a good precedent.

I copied the pattern from the sample api server, should this be placed elsewhere?

It looks like we already do this: https://github.com/kubernetes/kubernetes/blob/master/hack/update-codegen.sh#L133

If you follow this pattern, this script should be called from there too.

yeah thats the pattern I went with, if we theres a new pattern were shooting for I'm happy to change

lavalamp · 2018-08-17T20:01:16Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go

+
+// +k8s:openapi-gen=true
+// WebhookClientConfig contains the information to make a connection with the webhook
+type WebhookClientConfig struct {


Copied from the admission webhook configuration?

yeah, it essentially needs the same methods but there were incompatibilities with the admission implementation. Talking internally we decided to factor it into a common package. This PR doesn't look to refactor the admission pieces but does allow the owners of that package the ability to go that route in the future.

lavalamp · 2018-08-17T20:03:33Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// Webhook describes a webhook and the resources and operations it applies to.
+type Webhook struct {


I am confused about why this is a separate type from the backend configuration.

yeah this may not be necessary, just wrapping the client configuration similar to the current admission package. We can scrap it

lavalamp · 2018-08-17T20:04:33Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

+
+	// ClientConfig holds the connection parameters for the webhook
+	// required
+	ClientConfig apiv1alpha1.WebhookClientConfig


If this is supposed to be a reference to a webhook, shouldn't it be a name? OTOH if this is where the webhook configuration is stored, why is there a top level Webhook type?

This is a nested webhook configuration, the top level webhook type was just to keep parity with the original design, it can be scrapped though

lavalamp · 2018-08-17T20:06:14Z

/assign

lavalamp · 2018-08-17T20:33:45Z

Sounds good, please scrap the top-level webhook type :) Unlike normal code, it's not problematic to copy the webhook configuration API definition types instead of trying to reuse: you may want the documentation to differ, for example.

…

On Fri, Aug 17, 2018 at 1:13 PM Patrick Barker ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/apiserver/hack/update-codegen.sh <#67547 (comment)> : > @@ -0,0 +1,81 @@ + +#!/usr/bin/env bash I copied the pattern from the sample api server, should this be placed elsewhere? ------------------------------ In staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go <#67547 (comment)> : > + // The name of the webhook. + // Required. + Name string `json:"name" protobuf:"bytes,2,opt,name=name"` + + // ClientConfig defines how to communicate with the hook. + // Required + ClientConfig WebhookClientConfig `json:"clientConfig" protobuf:"bytes,3,opt,name=clientConfig"` +} + +// +genclient +// +genclient:nonNamespaced +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object + +// +k8s:openapi-gen=true +// WebhookClientConfig contains the information to make a connection with the webhook +type WebhookClientConfig struct { yeah, it essentially needs the same methods but there were incompatibilities with the admission implementation. Talking internally we decided to factor it into a common package. This PR doesn't look to refactor the admission pieces but does allow the owners of that package the ability to go that route in the future. ------------------------------ In staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go <#67547 (comment)> : > @@ -48,3 +48,89 @@ type AdmissionPluginConfiguration struct { // +optional Configuration *runtime.Unknown } + +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object + +// Webhook describes a webhook and the resources and operations it applies to. +type Webhook struct { yeah this may not be necessary, just wrapping the client configuration similar to the current admission package. We can scrap it ------------------------------ In staging/src/k8s.io/apiserver/pkg/apis/audit/types.go <#67547 (comment)> : > + + // ThrottleBurst is the maximum number of events sent at the same moment + // +optional + ThrottleBurst *int64 + + // ThrottleEnabled determines whether throttling is enabled + // +optional + ThrottleEnabled *bool + + // ThrottleQPS maximum number of batches per second + // +optional + ThrottleQPS *float32 + + // ClientConfig holds the connection parameters for the webhook + // required + ClientConfig apiv1alpha1.WebhookClientConfig This is a nested webhook configuration, the top level webhook type was just to keep parity with the original design, it can be scrapped though — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#67547 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAngll8-hOOr8Ig9CiJKI3ZLmAXQcpk8ks5uRyPtgaJpZM4WB3h0> .

k8s-ci-robot · 2018-10-15T22:22:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pbarker, thockin

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:

~~api/OWNERS~~ [thockin]
~~docs/api-reference/OWNERS~~ [thockin]
~~hack/OWNERS~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~staging/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~test/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lavalamp

Sorry, still one more thing about the defaulting...

lavalamp · 2018-10-15T22:32:54Z

pkg/apis/auditregistration/v1alpha1/defaults.go

+
+var (
+	// DefaultQPS is the default QPS value
+	DefaultQPS = func(i int64) *int64 { return &i }(int64(10))


Something got scrambled, no doubt in a comment I gave :)

I suggest making these consts, declaring func intPtr(i int64) *int64 { return &i } somewhere, and calling intPtr(DefaultQPS) in the DefaultThrottle function.

Making these constant will prevent people from taking their address, which is the error I think it's important to avoid.

tallclair · 2018-10-16T00:56:48Z

pkg/apis/auditregistration/v1alpha1/defaults.go

+	DefaultBurst = int64(15)
+)
+
+func intPtr(i int64) *int64 { return &i }


nit: use

utilpointer "k8s.io/utils/pointer" utilpointer.Int64Ptr

(of dubious value, but we favour consistency...)

tallclair · 2018-10-16T01:00:26Z

@lavalamp - I just noticed these APIs are being added to Kubernetes (k8s.io/api/auditregistration/v1alpha1/types.go) but auditing is really a generic apiserver feature. Should these APIs be moved to apiserver apis (k8s.io/apiserver/pkg/apis/auditregistration/v1alpha1/types.go)?

pbarker · 2018-10-16T01:08:42Z

@tallclair that was initially explored but there were issues with the scheme #67547 (comment) In order to do this properly there wound need to be some fundamental changes to the apiserver

tallclair · 2018-10-16T01:20:27Z

@pbarker Thanks, but I interpreted that comment as separating audit & auditregistration into 2 different API groups. IIUC, the auditregistration API group could still be part of the apiserver apis. I don't feel particularly strongly on the distinction, but I'd like an ACK from someone with more of a stake in the difference.

pbarker · 2018-10-16T01:33:09Z

Sorry @tallclair, couldn't remember where all the conversations happened, there is more context here #67547 (comment)

pbarker · 2018-10-16T03:15:57Z

/retest

k8s-ci-robot · 2018-10-16T03:59:01Z

@pbarker: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	be4c4a8f5c6f56222ce9818e34f64b85ecd8b8e9	link	`/test pull-kubernetes-local-e2e-containerized`

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.

lavalamp · 2018-10-16T19:29:10Z

pkg/apis/auditregistration/v1alpha1/defaults.go

+func DefaultThrottle() *auditregistrationv1alpha1.WebhookThrottleConfig {
+	return &auditregistrationv1alpha1.WebhookThrottleConfig{
+		QPS:   utilpointer.Int64Ptr(DefaultQPS),
+		Burst: utilpointer.Int64Ptr(DefaultBurst),


I argued so hard against making these library functions :( I just don't think it's worth the import dependency to avoid a 3 line function definition.

But this is tilting at windmills so I'm not asking for a change :(

Yeah, sorry. To quote my original comment:

(of dubious value, but we favour consistency...)

lavalamp · 2018-10-16T19:30:46Z

Give me a minute to think about whether it now makes sense to put this in with the meta api.

spiffxp · 2018-10-17T00:47:21Z

/milestone v1.13
I'm adding this to the v1.13 milestone since it relates to kubernetes/enhancements#600 which is currently planned for v1.13

lavalamp · 2018-10-17T22:06:48Z

/lgtm

I don't want to block this any more. Likely it should go in a more general place. We can always move it later.

WanLinghao · 2018-11-02T07:56:05Z

@pbarker In the design doc(https://github.com/kubernetes/community/blob/master/keps/sig-auth/0014-dynamic-audit-configuration.md), it is mentioned that the Policy in dynamic audit feature is v1beta1 of legacy audit. Why it is changed in your implementation. Did I miss anything?

 // Policy is the current audit v1beta1 Policy object
    // if undefined it will default to the statically configured cluster policy if available
    // if neither exist the backend will fail
    Policy *Policy

pbarker · 2018-11-02T13:40:03Z

Hey @WanLinghao, this was a piece we iterated on in this PR. There was a desire to have the audit mechanism in the API be more composable and additive, so that as an app developer I could write audit rules and have them contained with the rest of my deployment configuration. So we temporarily slimmed down the audit policy so that we could do this rework as we head to beta. We'll be having some talks on this in the future.

WanLinghao · 2018-11-05T10:18:45Z

@pbarker So do we have any plan to push the Policy in dynamic audit feature into something similar with the one in legacy audit feature?

pbarker · 2018-11-05T15:21:36Z

@WanLinghao its not possible to delete fields from an API once you put them there. So we scoped down the policy object to the bare minimum until we have a clear direction, then it will be added to.

k8s-ci-robot requested review from caesarxuchao and jbeda August 17, 2018 16:44

pbarker mentioned this pull request Aug 17, 2018

dynamic audit configuration #67257

Merged

pbarker changed the title ~~dynamic audit api~~ dynamic audit configuration api Aug 17, 2018

tallclair self-requested a review August 17, 2018 16:53

k8s-ci-robot added this to the v1.12 milestone Aug 17, 2018

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 17, 2018

lavalamp reviewed Aug 17, 2018

View reviewed changes

k8s-ci-robot assigned lavalamp Aug 17, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2018

pbarker force-pushed the audit-api branch 2 times, most recently from d52579d to a0b5315 Compare August 18, 2018 19:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2018

lavalamp reviewed Oct 15, 2018

View reviewed changes

pbarker force-pushed the audit-api branch 2 times, most recently from 0a03327 to 1c16ef6 Compare October 16, 2018 00:36

tallclair reviewed Oct 16, 2018

View reviewed changes

pbarker force-pushed the audit-api branch from 1c16ef6 to 9995c58 Compare October 16, 2018 02:14

pbarker added 2 commits October 16, 2018 06:46

adds dynamic audit api

381d0a5

adds dynamic audit api generated

b8e1250

pbarker force-pushed the audit-api branch from 9995c58 to b8e1250 Compare October 16, 2018 13:08

lavalamp reviewed Oct 16, 2018

View reviewed changes

pbarker mentioned this pull request Oct 16, 2018

integration test for dynamic audit #69902

Merged

pbarker mentioned this pull request Oct 17, 2018

Dynamic Audit Configuration kubernetes/enhancements#600

Closed

2 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018

k8s-ci-robot merged commit 0652e09 into kubernetes:master Oct 17, 2018

pbarker deleted the audit-api branch December 18, 2018 22:50

tallclair mentioned this pull request Jan 10, 2019

Dynamic Audit Policy #71230

Closed

dynamic audit configuration api #67547

dynamic audit configuration api #67547

Conversation

pbarker commented Aug 17, 2018 • edited Loading

tallclair commented Aug 17, 2018

tallclair commented Aug 17, 2018

pbarker commented Aug 17, 2018

tallclair commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Aug 17, 2018

lavalamp commented Aug 17, 2018 via email

k8s-ci-robot commented Oct 15, 2018

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Oct 16, 2018

pbarker commented Oct 16, 2018

tallclair commented Oct 16, 2018

pbarker commented Oct 16, 2018 • edited Loading

pbarker commented Oct 16, 2018

k8s-ci-robot commented Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Oct 16, 2018

spiffxp commented Oct 17, 2018

lavalamp commented Oct 17, 2018

WanLinghao commented Nov 2, 2018 • edited Loading

pbarker commented Nov 2, 2018

WanLinghao commented Nov 5, 2018

pbarker commented Nov 5, 2018

pbarker commented Aug 17, 2018 •

edited

Loading

pbarker commented Oct 16, 2018 •

edited

Loading

k8s-ci-robot commented Oct 16, 2018 •

edited

Loading

WanLinghao commented Nov 2, 2018 •

edited

Loading