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

KEP-555: omit managed fields from audit log #2982

Merged
merged 1 commit into from Sep 28, 2021

Conversation

tkashem
Copy link
Contributor

@tkashem tkashem commented Sep 21, 2021

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Sep 21, 2021
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 21, 2021
@tkashem
Copy link
Contributor Author

tkashem commented Sep 21, 2021

/assign @lavalamp

OmitManagedFields *bool `json:"omitManagedFields,omitempty"`
}
```
The above API changes will be introduced in `v1`, `v1beta1` and `v1alpha1` of `audit.k8s.io`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we never roundtrip these, do we? So technically we could do this in v1 only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there's no harm in doing it in all, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only that we encourage use of v1 by not implementing new stuff in old versions.

@sttts
Copy link
Contributor

sttts commented Sep 22, 2021

Sgtm.

/assign @deads2k @liggitt

@lavalamp
Copy link
Member

/approve
/lgtm

/hold

(hold for someone from sig auth to ack)

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, tkashem

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021
@tkashem
Copy link
Contributor Author

tkashem commented Sep 22, 2021

/assign @ritazh @enj

@liggitt
Copy link
Member

liggitt commented Sep 22, 2021

looks reasonable to me


A new field `OmitManagedFields` is added to both `Policy` and `PolicyRule` making the following possible:
- `Policy.OmitManagedFields` sets the default policy for omitting managed fields globally.
- the default value is `false`, managed fields are not omitted, this retains the current behavior.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any plans for changing the default value in a future release since the fields may not be very useful? making it opt-out instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that i am aware of, primarily we didn't want to quietly drop managed fields from audit log. The level RequestResponse stipulates that the response object in its "entirety" is being audited, so if we are to drop any fields from the audit log then I think it should be an opt-in by the operator.

@ritazh
Copy link
Member

ritazh commented Sep 27, 2021

/lgtm

@sttts
Copy link
Contributor

sttts commented Sep 28, 2021

sig-auth has reviewed with @ritazh and @liggitt.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit c8441d9 into kubernetes:master Sep 28, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 28, 2021
@apelisse
Copy link
Member

Any chance we can back-port this into former releases since this is causing problems for people? This can be worked around by removing managed fields from big objects, but that's not super convenient?

@wojtek-t
Copy link
Member

Any chance we can back-port this into former releases since this is causing problems for people? This can be worked around by removing managed fields from big objects, but that's not super convenient?

Backporting API change doesn't sound like the best idea to me.
@liggitt ^^?

@tkashem
Copy link
Contributor Author

tkashem commented Oct 25, 2021

FYI - kubernetes/kubernetes#94986 implements the KEP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants