KEP-5051: Add opt-in support for unset markers #280
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz 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
approved
|
|
||
| const ( | ||
| // markerKey is the key used to store marker values in the object. | ||
| markerKey = "k8s_io__value" |
There was a problem hiding this comment.
@deads2k @sttts Suggestions welcome on how to name the key/value of the markers used in the proposal. BTH, I'm actively looking for something more pleasing to the eye. Having a key that is also a valid CEL identifier is important for MutatingAdmissionPolicy, so [a-z][A-Z][0-9]_ (non-number first char) is strongly preferred. And picking something that "no one could reasonably already be using for something else" is also desirable.
jpbetz
force-pushed
the
unsetting-fields
branch
2 times, most recently
from
224104f to
e6a5a35
Compare
jpbetz
force-pushed
the
unsetting-fields
branch
from
e6a5a35 to
38f711b
Compare
jpbetz
changed the title
|
/retest |
|
@jpbetz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -128,6 +137,42 @@ func (tv TypedValue) ToFieldSet() (*fieldpath.Set, error) { | |||
| return w.set, nil | |||
| } | |||
|
|
|||
| // ExtractMarkers finds and all marker values in TypedValue. | |||
There was a problem hiding this comment.
| // ExtractMarkers finds and all marker values in TypedValue. | |
| // ExtractMarkers finds all marker values in TypedValue. |
| } | ||
|
|
||
| // validateMarkerLocation returns true if the current value is a marker and false otherwise. | ||
| // It the value is a marker, it returns a list of errors if the marker is not in a valid location. |
There was a problem hiding this comment.
| // It the value is a marker, it returns a list of errors if the marker is not in a valid location. | |
| // If the value is a marker, it returns a list of errors if the marker is not in a valid location. |
| return false, nil | ||
| } | ||
| if marker != unsetMarkerValue { | ||
| // Should never happen since validation already checks for allowed marker values. |
There was a problem hiding this comment.
So how about just getting rid of this function and just use isUnsetMarker(v)?
func (v *markerExtractorWalker) validateMarkerLocation(t interface{}) (bool, ValidationErrors) {
if !isUnsetMarker(v.value) {
return false, nil
}
...
...| } | ||
| return true | ||
| }) | ||
| return errs |
There was a problem hiding this comment.
Do we descend further if this map is atomic?
| for i := len(v.parentElementRelationship) - 1; i >= 0; i-- { | ||
| if v.parentElementRelationship[i] != nil { | ||
| return v.parentElementRelationship[i] | ||
| } | ||
| } |
There was a problem hiding this comment.
We're only looking for the nearestElementRelationship, what if it is nested? e.g. an atomic List which contains maps(not atomic),
types:
- name: TestType
map:
fields:
- name: items
type:
list:
elementType:
map:
fields:
- name: key
type:
scalar: string
- name: value
type:
scalar: string
elementRelationship: atomic
and applyconfig is {"items": [{"key": "k1", "value": {"k8s_io__value": "unset"}}]}, should we throw an error for it? The parentElementRelationship would be ["", "atomic", ""] when validating the marker location.
Or, it should be if v.parentElementRelationship[i] != nil && *v.parentElementRelationship[i] != "" && *v.parentElementRelationship[i] != "separable { to looking for the nearest "atomic" or 'associative"?
|
/close |
|
@jpbetz: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This implements the SMD bits of kubernetes/enhancements#5052.
All the new functionality is opt-in. No default behavior is changed. This is extensively tested.
Benchmark data suggests negligible performance impact: https://gist.github.com/jpbetz/df3806a5605a174597a3d0d6459125c5