Fix issue that Audit Server could not correctly encode metav1.DeleteOption #110110

sxllwx · 2022-05-18T12:30:37Z

What type of PR is this?

What this PR does / why we need it:

When using kube-apiserver audit webhook, the following logs were found:

W0518 20:34:43.069379   20982 request.go:169] Auditing failed of  request: encoding failed: v1.DeleteOptions is not suitable for converting to "meta.k8s.io/v1" in scheme "pkg/api/legacyscheme/scheme.go:30"
W0518 20:34:49.597520   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.600364   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.614399   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.625765   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.628741   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.648162   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.661601   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.669906   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.686909   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1
W0518 20:34:49.689894   20982 request.go:169] Auditing failed of  request: encoding failed: request to convert CR to an invalid group/version: v1

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The first log is triggered by Delete Namespace, and the rest can be triggered by installing any CRD and Delete Namespace.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

sxllwx · 2022-05-19T09:02:56Z

@wojtek-t @sttts PTAL, thx~

leilajal · 2022-05-19T16:38:17Z

/triage accepted

wojtek-t · 2022-05-19T19:40:19Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

@@ -103,7 +103,7 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc
 				trace.Step("Decoded delete options")

 				objGV := gvk.GroupVersion()
-				audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer)
+				audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, metainternalversionscheme.Codecs)


Can you please add some unit test for these?

Thanks for your suggestion, I have added Unit test to this modification. PTAL, thx~

/ping @wojtek-t

The test you added doesn't verify the fix you're making in this PR.

I would like to see some test that fails before this change and is passing with this change...

sxllwx · 2022-05-20T08:32:15Z

/retest-required

staging/src/k8s.io/apiserver/pkg/audit/request_test.go

wojtek-t · 2022-05-24T13:46:09Z

staging/src/k8s.io/apiserver/pkg/audit/request_test.go

+			serializer: metainternalversionscheme.Codecs,
+		},
+		{
+			name: "encode v1 DeleteOptions",


I don't understand the difference between those two cases (TypeMeta shouldn't really matter here...)

ok..., I have a wrong understanding of Codec here, and the redundant tests have been deleted.

wojtek-t · 2022-05-24T13:46:44Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

@@ -103,7 +103,7 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc
 				trace.Step("Decoded delete options")

 				objGV := gvk.GroupVersion()
-				audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer)
+				audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, metainternalversionscheme.Codecs)


The test you added doesn't verify the fix you're making in this PR.

I would like to see some test that fails before this change and is passing with this change...

sxllwx · 2022-05-25T09:26:13Z

re: #110110 (comment)

I've added test cases for encoding v1.DeleteOptions without using metainternalversionscheme.Codecs. This use case will only output a line of log output after a failure.
like...

$ go test . -v --run=TestEncodeDeleteOptions
=== RUN   TestEncodeDeleteOptions
=== RUN   TestEncodeDeleteOptions/meta_built-in_Codec_encode_v1.DeleteOptions
=== RUN   TestEncodeDeleteOptions/fake_corev1_registerd_codec_encode_v1_DeleteOptions
    request_test.go:278: encode object: encoding failed: v1.DeleteOptions is not suitable for converting to "meta.k8s.io/v1" in scheme "k8s.io/apiserver/pkg/audit/request_test.go:240"
--- PASS: TestEncodeDeleteOptions (0.00s)
    --- PASS: TestEncodeDeleteOptions/meta_built-in_Codec_encode_v1.DeleteOptions (0.00s)
    --- PASS: TestEncodeDeleteOptions/fake_corev1_registerd_codec_encode_v1_DeleteOptions (0.00s)
PASS
ok      k8s.io/apiserver/pkg/audit      0.822s

Please take a look. thank you for your time. /ping @wojtek-t

sxllwx · 2022-05-25T13:11:02Z

/retest-required

sxllwx · 2022-05-31T04:03:55Z

/ping @wojtek-t PTAL thx~

sxllwx · 2022-06-06T09:19:32Z

#110110 (comment) wasn't addressed

The test was passing also before this PR, right?

Please correct me if I understand wrong.

audit.LogRequestObject

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

Lines 104 to 107 in 2d7dcf9

    
           objGV := gvk.GroupVersion() 
        
           audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) 
        
           trace.Step("Recorded the audit event")

The function itself will not generate any errors, so whether the PR is merged or not will not affect it. But inside the function will call encodeObject.

encodeObject

kubernetes/staging/src/k8s.io/apiserver/pkg/audit/request.go

Lines 166 to 171 in 2d7dcf9

    
           ae.RequestObject, err = encodeObject(obj, objGV, s) 
        
           if err != nil { 
        
           	// TODO(audit): add error slice to audit event struct 
        
           	klog.Warningf("Auditing failed of %v request: %v", reflect.TypeOf(obj).Name(), err) 
        
           	return 
        
           }

A Warning log appears when encoding fails.

Based on the above situation, I added a unit test for encodeObject, in the unit test, describe two situations

Without this PR, encodeObject will fail https://github.com/sxllwx/kubernetes/blob/fix/audit-webhook-delete-codec/staging/src/k8s.io/apiserver/pkg/audit/request_test.go#L262-L270
Using this PR, encodeObject will succeed https://github.com/sxllwx/kubernetes/blob/fix/audit-webhook-delete-codec/staging/src/k8s.io/apiserver/pkg/audit/request_test.go#L251-L260

wojtek-t · 2022-06-06T11:43:19Z

Based on the above situation, I added a unit test for encodeObject, in the unit test, describe two situations

But the same test is passing without the big being done in this PR.
Yes - encocdeObject was failing, but the problem isn't in the encodeObject, but in the parameters that are passed to it.

I would like to test the fix that you're doing [so whether the right parameters are passed].

sxllwx · 2022-06-07T03:56:17Z

Based on the above situation, I added a unit test for encodeObject, in the unit test, describe two situations

But the same test is passing without the big being done in this PR. Yes - encocdeObject was failing, but the problem isn't in the encodeObject, but in the parameters that are passed to it.

I would like to test the fix that you're doing [so whether the right parameters are passed].

I see what you mean. In response to your suggestion, I have revised the unit test again, please take a look at it, thank you for your time. @wojtek-t

sxllwx · 2022-06-08T11:58:21Z

/ping @wojtek-t PTAL thx.

sxllwx · 2022-06-13T04:04:25Z

What do you need my help to do to move this thing forward? @wojtek-t

wojtek-t · 2022-06-13T13:37:46Z

OK - this looks fine - thanks!

/release-note none
/lgtm
/approve

k8s-ci-robot · 2022-06-13T13:38:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sxllwx, wojtek-t

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [wojtek-t]

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

sxllwx · 2022-06-13T13:55:27Z

OK - this looks fine - thanks!

/release-note none /lgtm /approve

thx~

fusida · 2022-11-15T08:38:46Z

@sxllwx @wojtek-t did this commit will cherry pick 1.24? i meet in 1.24

sxllwx · 2022-11-15T11:56:26Z

@sxllwx @wojtek-t did this commit will cherry pick 1.24? i meet in 1.24

I have submitted a PR #113918 to try to fix this issue for release-1.24.

…10-upstream-release-1.24 Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode

…10-upstream-release-1.23 Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode

k8s-ci-robot requested review from sttts and wojtek-t May 18, 2022 12:31

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 19, 2022

wojtek-t reviewed May 19, 2022

View reviewed changes

sxllwx force-pushed the fix/audit-webhook-delete-codec branch from 943ec09 to 16a9dbc Compare May 20, 2022 07:54

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 20, 2022

sxllwx force-pushed the fix/audit-webhook-delete-codec branch from 16a9dbc to 18aed16 Compare May 20, 2022 08:43

wojtek-t reviewed May 24, 2022

View reviewed changes

sxllwx force-pushed the fix/audit-webhook-delete-codec branch 2 times, most recently from 12287ad to 4b1445f Compare May 25, 2022 09:18

sxllwx force-pushed the fix/audit-webhook-delete-codec branch from 4b1445f to a40110a Compare May 25, 2022 12:08

enj added this to Needs Triage in SIG Auth Old Jun 6, 2022

sxllwx force-pushed the fix/audit-webhook-delete-codec branch from a40110a to a47c94d Compare June 7, 2022 03:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2022

Fix issue that Audit Server could not correctly encode DeleteOption

286a67d

sxllwx force-pushed the fix/audit-webhook-delete-codec branch from a47c94d to 286a67d Compare June 7, 2022 03:57

k8s-ci-robot assigned wojtek-t Jun 13, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2022

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 13, 2022

k8s-ci-robot merged commit 90834e0 into kubernetes:master Jun 13, 2022

SIG Auth Old automation moved this from Needs Triage to Closed / Done Jun 13, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 13, 2022

tallclair mentioned this pull request Aug 4, 2022

More useful audit error logs #111706

Merged

Jefftree mentioned this pull request Sep 19, 2022

DeleteCollection API fails if request body is non-empty #111985

Closed

sxllwx mentioned this pull request Nov 15, 2022

Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode #113918

Merged

sxllwx mentioned this pull request Nov 17, 2022

Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode #113965

Merged

k8s-ci-robot added a commit that referenced this pull request Jan 17, 2023

Merge pull request #113918 from sxllwx/automated-cherry-pick-of-#1101…

badcd03

…10-upstream-release-1.24 Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode

k8s-ci-robot added a commit that referenced this pull request Jan 17, 2023

Merge pull request #113965 from sxllwx/automated-cherry-pick-of-#1101…

c8e9642

…10-upstream-release-1.23 Automated cherry pick of #110110: Fix issue that Audit Server could not correctly encode

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue that Audit Server could not correctly encode metav1.DeleteOption #110110

Fix issue that Audit Server could not correctly encode metav1.DeleteOption #110110

sxllwx commented May 18, 2022 •

edited by wojtek-t

sxllwx commented May 19, 2022

leilajal commented May 19, 2022

wojtek-t May 19, 2022

sxllwx May 20, 2022

sxllwx May 24, 2022

wojtek-t May 24, 2022

sxllwx commented May 20, 2022

wojtek-t May 24, 2022

sxllwx May 25, 2022 •

edited

wojtek-t May 24, 2022

sxllwx commented May 25, 2022 •

edited

sxllwx commented May 25, 2022

sxllwx commented May 31, 2022

sxllwx commented Jun 6, 2022 •

edited

wojtek-t commented Jun 6, 2022

sxllwx commented Jun 7, 2022

sxllwx commented Jun 8, 2022 •

edited

sxllwx commented Jun 13, 2022

wojtek-t commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

sxllwx commented Jun 13, 2022

fusida commented Nov 15, 2022

sxllwx commented Nov 15, 2022

Fix issue that Audit Server could not correctly encode metav1.DeleteOption #110110

Fix issue that Audit Server could not correctly encode metav1.DeleteOption #110110

Conversation

sxllwx commented May 18, 2022 • edited by wojtek-t

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

sxllwx commented May 19, 2022

leilajal commented May 19, 2022

wojtek-t May 19, 2022

Choose a reason for hiding this comment

sxllwx May 20, 2022

Choose a reason for hiding this comment

sxllwx May 24, 2022

Choose a reason for hiding this comment

wojtek-t May 24, 2022

Choose a reason for hiding this comment

sxllwx commented May 20, 2022

wojtek-t May 24, 2022

Choose a reason for hiding this comment

sxllwx May 25, 2022 • edited

Choose a reason for hiding this comment

wojtek-t May 24, 2022

Choose a reason for hiding this comment

sxllwx commented May 25, 2022 • edited

sxllwx commented May 25, 2022

sxllwx commented May 31, 2022

sxllwx commented Jun 6, 2022 • edited

wojtek-t commented Jun 6, 2022

sxllwx commented Jun 7, 2022

sxllwx commented Jun 8, 2022 • edited

sxllwx commented Jun 13, 2022

wojtek-t commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

sxllwx commented Jun 13, 2022

fusida commented Nov 15, 2022

sxllwx commented Nov 15, 2022

sxllwx commented May 18, 2022 •

edited by wojtek-t

sxllwx May 25, 2022 •

edited

sxllwx commented May 25, 2022 •

edited

sxllwx commented Jun 6, 2022 •

edited

sxllwx commented Jun 8, 2022 •

edited