merge/replace fieldSpecs in transformer configs per strategy (#4404) #4461
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @khrisrichardson. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: khrisrichardson The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/assign @yuwenma |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -164,3 +164,157 @@ metadata: | |||
| name: cm | |||
| `) | |||
| } | |||
|
|
|||
| func TestSuffixTransformerWithDefaultFieldSpecs(t *testing.T) { | |||
There was a problem hiding this comment.
Do you mind explaining how this test covers the code changes in plugin/builtin/suffixtransformer/SuffixTransformer.go?
IIUC, without the code in SuffixTransformer.go this test still passes, is that right?
There was a problem hiding this comment.
It differs from the previous test by the absence of the fieldSpecs attribute. As per the parent commit, the absence of fieldSpecs would otherwise result in an error
if p.FieldSpecs == nil {
return errors.New("fieldSpecs is not expected to be nil")
}
| } | ||
| if p.FieldSpecs == nil { | ||
| p.FieldSpecs = annotations.DefaultFieldSpecs() | ||
| } |
There was a problem hiding this comment.
I agree with @KnVerey suggestion in #4404 but I'm wondering if we should change the Config() behavior this way.
c is a passed-in parameter which expects users to provide custom fieldspec string value. The useful return value would be either telling the unmarshalling error if the c is bad or found/not-found the fieldspec. Returning DefaultFieldSpecs() hides the message whether the json attribute "annotation" ever shows up or not in the passed-in string.
Considering the situations where
c = "annotation-typo:" and c="irrelevant: ", both will returns a smooth default fieldspec with "annotations: " meaning no annotation fieldspec specified. I don't think it's good for Config to swallow this info. @KnVerey @natasha41575 WDYT?
There was a problem hiding this comment.
I agree that we shouldn't swallow this error here; there are many reasons besides an empty c that p.FieldSpecs may be nil here.
Maybe an equivalent alternative that doesn't swallow errors for malformed values of c would be to check at the start of the function if c does not contain the string fieldSpecs, and in that case use the default fieldspecs?
There was a problem hiding this comment.
Maybe I'm being daft, but doesn't returning the error on unmarshal failure address that concern (i.e. the error is not swallowed, rendering the p.FieldSpecs clause unreachable in the event of failure). It's entirely likely that I'm misunderstanding, so please bear with me.
if err = yaml.Unmarshal(c, p); err != nil {
return err
}There was a problem hiding this comment.
Ah, I think you're right, sorry I missed that. @yuwenma I don't think we are swallowing anything that wasn't previously swallowed.
If there is a typo in c, e.g. annotations-typo: the transformer currently does nothing, silently.
This PR will change it so that if there is a typo in c, the transformer will have the default fieldspecs.
Neither is great, but I don't think this PR introduces a problem that wasn't already there, and I'm not sure how we would change it so I think this is fine.
There was a problem hiding this comment.
I believe yaml.UnmarshalStrict disallows unknown fields, but perhaps that decision might be outside the scope of this particular behavioral change.
There was a problem hiding this comment.
The two examples are not for unmarshal error. Sorry I was distracted today. My main concern is the shifting behavior of transformers. Calling Config with passed-in c reads as "parsing the content of c and finding out the annotations pieces". So the valid return is either found c's content or nothing found (nil or error). If the p.FieldSpecs is reset to something not from c like
[fieldspec{path: "metadata/annotations"}, fieldspec{path: "spec/template/metadata/annotations"}, fieldspec{path: "spec/template/metadata/annotations", kind: "Deployment"}], it would be unexpected for users.
This is the swallowing info (or shifting behavior) I meant.
There was a problem hiding this comment.
I see. So your concern cuts to the core of the UX change and whether it's acceptable for the behavior to morph without some signal from the end-user about whether or not they accept the change in behavior. I share those concerns and wavered over whether the right answer was to introduce an attribute (e.g. - defaults: true), to version the builtin apiVersion (e.g. - apiVersion: builtin/v1alpha2), or to postpone until kustomize v5.
While you'd likely agree that it's not unheard of for defaults to be set in the absence of a value, something that may be lacking in this case is a manner for the end-user to probe what those defaults are via the CLI or glancing at API docs (another signal that suggests that perhaps builtin should be versioned, since those API docs would unlikely remain static). Right now it seems that the docs point the end-user to the source code to understand the shape of fieldSpecs but there doesn't appear to be any mention of what the defaults are when working with the kustomization field as opposed to a transformer.
Which brings up another point: the current behavior of transformers diverges from that of the kustomization field counterparts. The kustomization field uses the defaults, yet I believe it's possible to change the behavior either via a grandfathered configurations clause or by adding fieldSpecs inline. Neither of which get a mention in the kustomize docs but if you squint you can see the usage of configurations described here and the usage of inline fieldSpecs described here. So in a way, the change would be bringing alignment to divergent behavior between kustomize fields and transformers. I know that doesn't make any of this any easier to swallow.
I am fully appreciative of the fact that you are much better versed than I am when it comes to navigating best practices here, so am open to any input you have to improve the overall end-user experience.
There was a problem hiding this comment.
@natasha41575 and I chatted about this today, and we're leaning towards wanting the new behaviour to be opt-in after all. Although we still agree that it seems very unlikely that using any builtin transformer with a nil fieldspec would make sense / have a useful result, it's also impossible to verify that. Knowing how important result stability is to our end users, that leads us to want to create a user-facing option for this. The use of fieldspecs in the configuration of our built in transformers is a convention, and we could enhance that convention by augmenting all the transformers that currently support it with an additional related field.
// Add the given annotations to the given field specifications.
type AnnotationsTransformerPlugin struct {
Annotations map[string]string `json:"annotations,omitempty" yaml:"annotations,omitempty"`
FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"`
FieldSpecStrategy FieldSpecStrategy `json:"fieldSpecStrategy" yaml:"fieldSpecStrategy"`
}apiVersion: builtin
kind: AnnotationsTransformer
metadata:
name: not-important-to-example
annotations:
app: myApp
greeting/morning: a string with blanks
fieldSpecStrategy: merge | replace # defaults to replace for backwards compatibility
fieldSpecs:
- path: metadata/annotations
create: trueWith this solution:
- We immediately have a way to use the defaults as-is like you wanted, albeit requiring an extra LOC.
- We also immediately have a way to augment the defaults too.
- In Kustomization v1, we could start versioning the built-ins as you suggest. In the new version of these transformers, we could flip the default for this new field to "merge" (although who knows... by then, maybe the openapi-based approach will be ready instead).
WDYT?
There was a problem hiding this comment.
I appreciate the additional feedback and like the opt-in nature of your and @natasha41575's suggestion plus the stability it provides. And thank you @yuwenma for initiating this discussion. All the permutations make sense to me and I should be able to find some time to work on this tomorrow.
The one gap that I can conceive of with the merge behavior is perhaps the desire not to have it be exclusively additive, but instead out of the 10s of commonLabels field specs, perhaps one wishes to delete one or two via $patch: delete strategic merge semantics. Let me see what it will take to support that. I'm pretty sure examples abound within other parts of kustomize.
There was a problem hiding this comment.
The idea that the merge could work as a real SMP and support deletion is interesting for sure! Feel free to investigate it in a follow up though.
api/filters/suffix/suffix.go
Outdated
| @@ -48,3 +49,7 @@ func (f Filter) evaluateField(node *yaml.RNode) error { | |||
| return f.trackableSetter.SetScalar(fmt.Sprintf( | |||
| "%s%s", node.YNode().Value, f.Suffix))(node) | |||
| } | |||
|
|
|||
| func DefaultFieldSpecs() types.FsSlice { | |||
There was a problem hiding this comment.
builtinconfig.MakeDefaultConfig().NameSuffix seems to be handy enough to be called directly. Maybe not need a new function.
There was a problem hiding this comment.
I tried that first, but given that builtinconfig is internal to api, it was not accessible from plugin/buitin
There was a problem hiding this comment.
@natasha41575 Shall we surface the builtin const fieldspec var so users can import directly?
There was a problem hiding this comment.
The default fieldspecs are already surfaced here:
Rather than surfacing the default fieldspecs in each filter individually as done in this PR, I would suggest adding a new method to this file, maybe something like func GetDefaultFieldSpecFor(filter string) string, or just use the existing method GetDefaultFieldSpecsAsMap() to get the default fieldspecs. A call for these methods might look like GetDefaultFieldSpecFor("nameprefix") or GetDefaultFieldSpecsAsMap()["nameprefix"].
Since it's already exposed, I don't think we need to create a new DefaultFieldSpecs() in each filter.
There was a problem hiding this comment.
+1 to call GetDefaultFieldSpecsAsMap()
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
size/XXL
khrisrichardson
changed the title
|
@khrisrichardson: PR needs rebase. 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/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: 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/test-infra repository. |
This PR includes a
DefaultFieldSpecs()function for each of the filters utilizingfsslice, sincebuiltinconfigis internal toapiand thus not accessible fromplugin/builtin. Likewise, for each of the builtin plugins using said filters, if the end-user omits afieldSpecsattribute in the respective builtin transformer config function specification (i.e. -p.FieldSpecs == nil), the defaults will be applied by way of the aforementioned filter function related to that plugin.Regarding backwards compatibility, as @KnVerey noted in #4404, if
fieldSpecsare not specified then the builtin transformer config functions were of limited use, so the end-user was required to set thefieldSpecsattribute to achieve the desired results. Given that most of the builtin plugins did not croak on the absence of thefieldSpecsattribute, it is conceivable that there are builtin transformer config function specifications in the field that will suddenly start mutating resources with this change. Builtin transformer config functions with afieldSpecsattribute set will continue functioning as previously.On a forwards compatibility note, it would now be possible for two versions of kustomize to produce unequal results, which I think poses the question whether the
builtinapiVersionought not be versioned.