avoid duplicate status in audit events

[CaoShuFeng]

Fixes: #60108

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
/assign @sttts @tallclair

Release note:

Action required: When Response is a metav1.Status, it is no longer copied into the audit.Event status. Only the "status", "reason" and "code" fields are set.

[sttts]

what do we lose in practice?

[CaoShuFeng]

Before this change:

    "responseStatus": {
        "apiVersion": "v1",
        "code": 403,
        "details": {
            "kind": "pods"
        },
        "kind": "Status",
        "message": "pods is forbidden: User \"tom\" cannot list pods in the namespace \"default\"",
        "metadata": {},
        "reason": "Forbidden",
        "status": "Failure"
    },

After this change:

    "responseStatus": {
        "code": 403,
        "metadata": {},
        "reason": "Forbidden",
        "status": "Failure"
    },

[sttts]

Why do you want to lose the message?

[CaoShuFeng]

As @tallclair said:
#60108 (comment)

[sttts]

And is this message duplicated as well? If yes, I can agree with the change.

[CaoShuFeng]

I also think Message is necessay.
Add it back.

[tallclair]

When is a status returned? I think maybe for delete requests, and errors?

There are 2 concerns with copying the status:

1. Duplicate data returned
2. Potentially large data returned at the Metadata level

The first could be dealt with simply by omitting the response object when it's a status, but this won't handle the case where the message can be really large. We could truncate the message when level == Metadata

WDYT? What is the desired behavior here?

[CaoShuFeng]

Will fix the unit test tomorrow.

[CaoShuFeng]

The first could be dealt with simply by omitting the response object when it's a status

Done.

We could truncate the message when level == Metadata

Done.

[tallclair]

I meant for that to be a discussion. I'm not sure the approach I suggested is the right one. I'm interested in hearing other suggestions.

@sttts @loburm

[CaoShuFeng]

/area audit

[tallclair]

Looking at how we use the status, I'm thinking something closer to your original PR might be the way to go. Don't copy the status, Just set the code, reason, and status. Long term, I think we should consider changing ResponseStatus to just ResponseCode.

EDIT: The info from the fake status we generate can be added as annotations if it's really needed. For cases where you want the full response status from the response, just log at response level.

[CaoShuFeng]

EDIT: The info from the fake status we generate can be added as annotations if it's really needed.

Can't follow this. Which part to add to annotations?

Don't copy the status, Just set the code, reason, and status.

Message is deleted, and it should be only saved at response level.

[CaoShuFeng]

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-integration

[tallclair]

EDIT: The info from the fake status we generate can be added as annotations if it's really needed.

Can't follow this. Which part to add to annotations?

I was just reviewing the status objects we generate for auditing (as opposed to copy from the response):

1. on panic
2. connection closed
3. auth failed

What I meant was that the messages we set in those 3 cases could be included elsewhere, if we got rid of the ResponseStatus field (replaced with ResponseCode).

[tallclair]

I think we should get rid of the else case. This just duplicates the status when it's already being returned in the Response field.

[CaoShuFeng]

This just duplicates the status when it's already being returned in the Response field.

We have a return in line 184, so it's not duplicated.

If we delete it, it will be diffacult for users to query the http response code from response level audit events. WDYT @tallclair

[CaoShuFeng]

Hi, @tallclair @sttts
I changed this pull request to the original version.

With this version, users can still get detailed status info in response audit level.
And users can get status code at all audit levels.

[CaoShuFeng]

Long term, I think we should consider changing ResponseStatus to just ResponseCode.

What about we leave other parts of ResponseStatus empty, and only set the Code?

[tallclair]

/lgtm

I updated your release note to highlight that this is a potentially breaking change.

[tallclair]

Ping @sttts - would be great to get this in 1.11

[tallclair]

/sig auth

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@CaoShuFeng @sttts @tallclair @kubernetes/sig-auth-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 7 days, the pull request will be moved out of the v1.11 milestone.

Pull Request Labels

• sig/auth: Pull Request will be escalated to these SIGs if needed.
• priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
• kind/cleanup: Adding tests, refactoring, fixing old bugs.

Help

• Additional instructions
• Commands for setting labels

[CaoShuFeng]

/test pull-kubernetes-e2e-gce

[sttts]

/approve
/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [sttts]

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

[k8s-github-robot]

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

[k8s-github-robot]

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