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

Fix truncating and batch backends integration. #65823

Merged
merged 1 commit into from Jul 9, 2018

Conversation

@loburm
Copy link
Contributor

loburm commented Jul 4, 2018

Truncating backend was not starting batch thread that is responsible for reading events from the channel.

Fixes #65819

Fix an issue with dropped audit logs, when truncating and batch backends enabled at the same time.
@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 4, 2018

/assign @sttts

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 4, 2018

/assign @CaoShuFeng

@@ -129,7 +129,7 @@ func truncate(e *auditinternal.Event) *auditinternal.Event {

func (b *backend) Run(stopCh <-chan struct{}) error {
// Nothing to do here

This comment has been minimized.

@idealhack

idealhack Jul 4, 2018

Member

Do we need to change the comment?

This comment has been minimized.

@loburm

loburm Jul 5, 2018

Author Contributor

Removed, thanks for noticing.

@@ -129,7 +129,7 @@ func truncate(e *auditinternal.Event) *auditinternal.Event {

func (b *backend) Run(stopCh <-chan struct{}) error {
// Nothing to do here
return nil
return b.delegateBackend.Run(stopCh)
}

func (b *backend) Shutdown() {

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jul 5, 2018

Member

Should also call b.delegateBackend.Shutdown() here.

This comment has been minimized.

@loburm

loburm Jul 5, 2018

Author Contributor

Done.

@CaoShuFeng

This comment has been minimized.

Copy link
Member

CaoShuFeng commented Jul 5, 2018

Please add fix https://github.com/kubernetes/kubernetes/pull/65823 in the comment

@@ -129,7 +129,7 @@ func truncate(e *auditinternal.Event) *auditinternal.Event {

func (b *backend) Run(stopCh <-chan struct{}) error {
// Nothing to do here
return nil

This comment has been minimized.

@sttts

sttts Jul 5, 2018

Contributor

is this a general pattern that a backend starts it's delegates? Do we do that elsewhere as well in auditing? Feels a bit odd.

This comment has been minimized.

@loburm

loburm Jul 5, 2018

Author Contributor

For example, that's how we are doing in union (it's not an ordinary backend): https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/audit/union.go#L46

Also the same we have in buffered backend:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go#L121

About other places in the apiserver I'm not sure.

If we imagine that structure of backends is a tree, then it makes complete sense to start sub-backends from each node of this tree.

This comment has been minimized.

@CaoShuFeng

This comment has been minimized.

@sttts

sttts Jul 6, 2018

Contributor

If it's uniform in the audit backends, it's fine.

@loburm loburm force-pushed the loburm:fix_truncate branch from ee03c12 to 66eb8bc Jul 5, 2018

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 5, 2018

/test pull-kubernetes-verify
/test pull-kubernetes-bazel-test

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 5, 2018

/test pull-kubernetes-verify

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 6, 2018

@@ -128,12 +128,11 @@ func truncate(e *auditinternal.Event) *auditinternal.Event {
}

func (b *backend) Run(stopCh <-chan struct{}) error {
// Nothing to do here
return nil
return b.delegateBackend.Run(stopCh)

This comment has been minimized.

@sttts

sttts Jul 6, 2018

Contributor

can we add that to the (not existing) godoc? Either here or above for the constructor at least: It runs and shuts down the delegate.

This comment has been minimized.

@loburm

loburm Jul 6, 2018

Author Contributor

I have added comment both to the truncate and buffered backends.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jul 6, 2018

Some godoc is missing. Otherwise lgtm.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 6, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jul 6, 2018

/approve

Please squash. @loburm can give the final lgtm.

@loburm loburm force-pushed the loburm:fix_truncate branch from 4255048 to 20fb0b5 Jul 9, 2018

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 9, 2018

@sttts probably you mean, that someone else could give the lgtm. I have squashed commits.

@CaoShuFeng could you give lgtm, and merge this PR?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jul 9, 2018

@loburm indeed :D was of the impression @CaoShuFeng was the author.

@CaoShuFeng

This comment has been minimized.

Copy link
Member

CaoShuFeng commented Jul 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 9, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, loburm, sttts

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

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 9, 2018

/test pull-kubernetes-integration

@loburm

This comment has been minimized.

Copy link
Contributor Author

loburm commented Jul 9, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 40806a2 into kubernetes:master Jul 9, 2018

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation loburm authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
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-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

k8s-github-robot pushed a commit that referenced this pull request Jul 13, 2018

Kubernetes Submit Queue
Merge pull request #65971 from loburm/automated-cherry-pick-of-#65823…
…-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #65823: Fix truncating and buffering backends integration.

Cherry pick of #65823 on release-1.11.

#65823: Fix truncating and buffering backends integration.

```release-note
Fix an issue with dropped audit logs, when truncating and batch backends enabled at the same time.
```

k8s-github-robot pushed a commit that referenced this pull request Jul 18, 2018

Kubernetes Submit Queue
Merge pull request #65970 from loburm/automated-cherry-pick-of-#65823…
…-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #65823: Fix truncating and buffering backends integration.

Cherry pick of #65823 on release-1.10.

#65823: Fix truncating and buffering backends integration.
@marpaia

This comment has been minimized.

Copy link
Member

marpaia commented Jul 31, 2018

/sig auth

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.