Introduce a simple datapolicy library #96097
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
sig/api-machinery
k8s-ci-robot
added
the
area/dependency
|
/assign serathius |
|
Please review @PurelyApplied |
k8s-ci-robot
added
area/kubectl
sig/cli
| "net/http" | ||
| "reflect" | ||
|
|
||
| "golang.org/x/oauth2" |
There was a problem hiding this comment.
Those two imports introduce additional dependecies to all components using component-base. To make review faster I would propose move handling of those types to separate PR.
| return []string{"password", "token"} | ||
| case reflect.TypeOf(http.Cookie{}): | ||
| return []string{"token"} | ||
| case reflect.TypeOf(oauth2.Token{}): |
There was a problem hiding this comment.
Checking this way for type means that component-base will take on lot's of dependencies that are not related to it's functionallity. Is there a way that we could avoid this?
There was a problem hiding this comment.
I changed the type checking to be purely on the strings, the dependencies are test only now, which would hopefully alleviate some of the concerns.
There was a problem hiding this comment.
Hmm, working on strings feels fragile, since when building in bazel the resulting types are different than when i just did a go test on my local machine
|
|
||
| // Verify returns a list of the datatypes embeded in the argument that can be | ||
| // considered sensitive w.r.t. to logging | ||
| func Verify(value interface{}) []string { |
There was a problem hiding this comment.
The reflect package can be very panicky. Consider adding a recover to this entry-point as a defensive measure against developer error.
Ideally, that would be linked to some metric that would provide enough information for us to correct any possible oversight, but metric / api changes are probably beyond the scope for this PR. Maybe just logging a warning for now?
There was a problem hiding this comment.
Ack, will do that and leave a todo.
rf232
force-pushed
the
datapol
branch
2 times, most recently
from
e38dcf6
to
feed65c
Compare
k8s-ci-robot
removed
the
sig/api-machinery
k8s-ci-robot
added
the
sig/api-machinery
|
PTAL @PurelyApplied |
rf232
force-pushed
the
datapol
branch
2 times, most recently
from
099b2af
to
073456a
Compare
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
staging/src/k8s.io/component-base/logs/datapol/externaltypes.go
Outdated
Show resolved
Hide resolved
|
/retest |
|
/assign @pwittrock @sttts |
The datapolicy fullfills the contracts needed to support the log sanitization kep (https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1753-logs-sanitization) The external types list is mostly a proof of concept now, and is open to extension.
|
/assign @serathius On @serathius advice split out third party types from the CL to make merging easier, will do a follow up with just the additional types from thirdparty libraries |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PurelyApplied, rf232, serathius 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 |
k8s-ci-robot
added
the
approved
|
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
The datapolicy fullfills the contracts needed to support the log
sanitization kep
(https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1753-logs-sanitization)
The external types list is mostly a proof of concept now, and is open to
extension.
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.: