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

dynamic audit configuration api #67547

Merged
merged 2 commits into from Oct 17, 2018

Conversation

@pbarker
Contributor

pbarker commented Aug 17, 2018

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

@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao and jbeda Aug 17, 2018

@pbarker pbarker changed the title from dynamic audit api to dynamic audit configuration api Aug 17, 2018

@tallclair tallclair self-requested a review Aug 17, 2018

@tallclair

This comment has been minimized.

Member

tallclair commented Aug 17, 2018

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

/assign @kubernetes/api-reviewers

@tallclair

This comment has been minimized.

Member

tallclair commented Aug 17, 2018

@pbarker

This comment has been minimized.

Contributor

pbarker commented Aug 17, 2018

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

@tallclair

This comment has been minimized.

Member

tallclair commented Aug 17, 2018

I think they belong here.

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

This comment has been minimized.

@lavalamp

lavalamp Aug 17, 2018

Member

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

This comment has been minimized.

@pbarker

pbarker Aug 17, 2018

Contributor

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

This comment has been minimized.

@tallclair

tallclair Aug 17, 2018

Member

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.

This comment has been minimized.

@pbarker

pbarker Aug 17, 2018

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

@lavalamp

lavalamp Aug 17, 2018

Member

Copied from the admission webhook configuration?

This comment has been minimized.

@pbarker

pbarker Aug 17, 2018

Contributor

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.

// +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 {

This comment has been minimized.

@lavalamp

lavalamp Aug 17, 2018

Member

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

This comment has been minimized.

@pbarker

pbarker Aug 17, 2018

Contributor

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

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

This comment has been minimized.

@lavalamp

lavalamp Aug 17, 2018

Member

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 comment has been minimized.

@pbarker

pbarker Aug 17, 2018

Contributor

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

This comment has been minimized.

Member

lavalamp commented Aug 17, 2018

/assign

@lavalamp

This comment has been minimized.

Member

lavalamp commented Aug 17, 2018

@@ -2,7 +2,7 @@
"swagger": "2.0",
"info": {
"title": "Kubernetes",
"version": "v1.13.0"
"version": "v1.12.0"

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Aug 20, 2018

Member

I think v1.13 here should not be decreased to 1.12.

Maybe this mumber is related to git tag.

git describe 
v1.13.0-alpha.0-299-gf8f220b49b
if c.URL == nil && c.Service == nil {
allErrs = append(allErrs, field.Invalid(fldPath, c, "either url or service definition must be set for webhook"))
}

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Aug 20, 2018

Member

URL and Service can not be set at the same time?

@pbarker

This comment has been minimized.

Contributor

pbarker commented Oct 12, 2018

@thockin please advise if this can be merged, there is a second piece to it here #67257. There are a lot of moving parts between the two and it may be a bit tricky in this case to keep things synced while the reviews continue. WDYT?

@jbeda

This comment has been minimized.

Contributor

jbeda commented Oct 12, 2018

@thockin Please advise - we are in a situation where we either have to (a) move to multiple feature branches or (b) merge partial changes as long as they are feature flagged or (c) create mono PRs that are impossible to review well.

I wasn't at SIG-arch so I missed the discussion and some of the nuance here.

I'm perfectly happy to give this an "approve" now but I don't want to abuse my top level OWNERS powers if there are active concerns about this particular PR. No one has put a hold on it so perhaps we should start a lazy consensus clock?

@lavalamp

This comment has been minimized.

Member

lavalamp commented Oct 12, 2018

// DefaultBurst is the default burst value
DefaultBurst = func(i int64) *int64 { return &i }(int64(15))
// DefaultThrottle is a default throttle config
DefaultThrottle = func(qps, burst *int64) *auditregistrationv1alpha1.WebhookThrottleConfig {

This comment has been minimized.

@tallclair

tallclair Oct 12, 2018

Member

Make this a function declaration, not a function assignment.

@thockin

This comment has been minimized.

Member

thockin commented Oct 15, 2018

The discussion was a little superficial. My concerns are around PR sequences that could plausibly make a release in half-complete form. Like, the API goes in and the impl does not. I think we need to do better than that.

This is big but it's not horrible, in that it is only 2 commits and they make sense. Stacking more commits into a PR like this seems workable, except that rebases make it hard to confirm that a previous review still applies (e.g. nobody can sneak something in). So I 100% ACK the desire to get this code in.

Even with a feature branch, you eventually have to rebase or merge, so reviews are complicated.

I want us to do better than this, but I know this work has been in progress and I don't feel it is fair to move the goalposts. I do think we (@kubernetes/sig-architecture-api-reviews) need to formalize how we want to handle this and soon. I'll hold my objections for now.

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 15, 2018

[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:

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

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

This comment has been minimized.

@lavalamp

lavalamp Oct 15, 2018

Member

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.

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

This comment has been minimized.

@tallclair

tallclair Oct 16, 2018

Member

nit: use

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

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

@tallclair

This comment has been minimized.

Member

tallclair commented Oct 16, 2018

@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

This comment has been minimized.

Contributor

pbarker commented Oct 16, 2018

@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

This comment has been minimized.

Member

tallclair commented Oct 16, 2018

@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

This comment has been minimized.

Contributor

pbarker commented Oct 16, 2018

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

@pbarker

This comment has been minimized.

Contributor

pbarker commented Oct 16, 2018

/retest

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 16, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized be4c4a8 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.

pbarker added some commits Aug 17, 2018

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

This comment has been minimized.

@lavalamp

lavalamp Oct 16, 2018

Member

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

This comment has been minimized.

@tallclair

tallclair Oct 16, 2018

Member

Yeah, sorry. To quote my original comment:

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

@lavalamp

This comment has been minimized.

Member

lavalamp commented Oct 16, 2018

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

@spiffxp

This comment has been minimized.

Member

spiffxp commented Oct 17, 2018

/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

This comment has been minimized.

Member

lavalamp commented Oct 17, 2018

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

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 17, 2018

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

18 checks passed

cla/linuxfoundation pbarker authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@WanLinghao

This comment has been minimized.

Contributor

WanLinghao commented Nov 2, 2018

@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

This comment has been minimized.

Contributor

pbarker commented Nov 2, 2018

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

This comment has been minimized.

Contributor

WanLinghao commented Nov 5, 2018

@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

This comment has been minimized.

Contributor

pbarker commented Nov 5, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment