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

Resource options as annotations #4061

Merged

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Jul 12, 2021

fixes #3975

An e2e test for generator options is at

func TestGeneratorOptionsWithBases(t *testing.T) {

ALLOW_MODULE_SPAN

@natasha41575 natasha41575 requested review from monopole and KnVerey July 12, 2021 19:07
@k8s-ci-robot
Copy link
Contributor

@natasha41575: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575

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 Jul 12, 2021
@natasha41575
Copy link
Contributor Author

/test kustomize-presubmit-master

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

I noticed that at least two of these options already have corresponding annotations, for transformers to use:

HashAnnotation = "kustomize.config.k8s.io/needs-hash"
BehaviorAnnotation = "kustomize.config.k8s.io/behavior"

Should we be migrating the options individually?

api/resource/resource.go Outdated Show resolved Hide resolved
api/resource/resource.go Show resolved Hide resolved
api/resource/resource_test.go Show resolved Hide resolved
api/types/genargs.go Outdated Show resolved Hide resolved
@natasha41575 natasha41575 force-pushed the resourceOptionsAsAnnotations branch 3 times, most recently from 00d2b86 to 0ad0d02 Compare July 15, 2021 22:22
@natasha41575
Copy link
Contributor Author

natasha41575 commented Jul 15, 2021

Regarding these annotations:

HashAnnotation = "kustomize.config.k8s.io/needs-hash"
BehaviorAnnotation = "kustomize.config.k8s.io/behavior"

I didn't realize those annotations were there, but they seem to be for users to be able to set the behavior/hash suffix option for their plugins.

The ones I have added are for kustomize to process and are replacing private variables, so arguably shouldn't be set or modified by users or anything else. In the spirit of aligning with #3995, WDYT about keeping the existing annotations, and having the new annotation be internal.config.kubernetes.io/generatorOptions ?

Arguably the other build annotations should also have the internal prefix, wdyt?

@KnVerey
Copy link
Contributor

KnVerey commented Jul 15, 2021

The ones I have added are for kustomize to process and are replacing private variables, so arguably shouldn't be set or modified by users or anything else. In the spirit of aligning with #3995, WDYT about keeping the existing annotations, and having the new annotation be internal.config.kubernetes.io/generatorOptions ?

sgtm

Arguably the other build annotations should also have the internal prefix, wdyt?

I agree

@natasha41575 natasha41575 force-pushed the resourceOptionsAsAnnotations branch 6 times, most recently from 3741652 to a7f1ad0 Compare July 20, 2021 00:24
@natasha41575
Copy link
Contributor Author

natasha41575 commented Jul 20, 2021

@KnVerey I've updated this PR to use the annotation key internal.config.kubernetes.io/generatorOptions and #4074 is a follow-up ready to go in after this one.

@natasha41575 natasha41575 requested a review from KnVerey July 20, 2021 16:59
@monopole
Copy link
Contributor

i'm reading through this, thanks for taking this on

api/konfig/general.go Outdated Show resolved Hide resolved
api/resource/resource.go Outdated Show resolved Hide resolved
// SetOptions updates the generator options for the resource.
func (r *Resource) SetOptions(o *types.GenArgs) {
r.options = o
if o.IsNilOrEmpty() {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this clear the annotation, e.g. is it possible it was present before and has been cleared during the lifetime of the Resource?

Copy link
Contributor Author

@natasha41575 natasha41575 Jul 22, 2021

Choose a reason for hiding this comment

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

I'm now clearing it when o == nil, but if I do so when o.IsNilOrEmpty, some generated resources don't get the hashes they need.

There's something very subtly different about o == nil vs o.args == nil, and the distinction is important so that most resources aren't hashed by default, but generated resources are hashed by default. But I don't fully understand the code and it confuses me.

In the case that o != nil and o.args == nil I tried

  1. clearing the annotation - which makes some generated resources not get the hashes they need
  2. setting the annotation to {} - which makes some nongenerated resources get hashes they shouldn't have. With some trial and error the only way that I've been able to make all the tests pass is to do nothing.

The only time options must be cleared is during merging I think, when a generated resource is merging with an existing resource. TestIssue3393 covers this case and the behavior is now corrected to what it was before.

@@ -101,7 +101,6 @@ func (r *Resource) CopyMergeMetaDataFieldsFrom(other *Resource) error {
}

func (r *Resource) copyKustomizeSpecificFields(other *Resource) {
r.options = other.options
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is subtly different than the previous behavior. Before, when performing a merge, the “other” object’s options completely override those of the receiver. But annotations are merged. So if the receiver has options and the “other” doesn’t, before that would presumably clear the options and after the options would be retained. That might be what's happening in our mysterious ConfigMap test case?

Copy link
Contributor Author

@natasha41575 natasha41575 Jul 22, 2021

Choose a reason for hiding this comment

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

Added a line - it's now r.SetOptions(other.getOptions) which seemed to fix the ConfigMap case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Should we move it up to CopyMergeMetaDataFieldsFrom field though? It's not a Kustomize-specific field anymore.

Looking at that code, I also wonder if we could avoid resetting the annotations repeatedly by making a version of mergeStringMaps that is build-annotation-aware.

Copy link
Contributor Author

@natasha41575 natasha41575 Jul 24, 2021

Choose a reason for hiding this comment

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

Looking at that code, I also wonder if we could avoid resetting the annotations repeatedly by making a version of mergeStringMaps that is build-annotation-aware

That's a good idea, I added one called mergeStringMapsWithBuildAnnotations, ptal!

api/krusty/configmaps_test.go Outdated Show resolved Hide resolved
@natasha41575 natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 22, 2021
@natasha41575 natasha41575 force-pushed the resourceOptionsAsAnnotations branch from aedc5c4 to c51710b Compare July 22, 2021 23:25
@natasha41575 natasha41575 requested a review from KnVerey July 22, 2021 23:31
delete(annotations, utils.BuildAnnotationsRefBy)
r.SetAnnotations(annotations)
}
for _, id := range ids {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we are repeatedly getting, mutating and setting the annotations here. Could we instead make a new helper like r.replaceCsvAnnotation(name, values...) to make this more straightforward/efficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mergeStringMapsWithBuildAnnotations makes r.setRefBy unnecessary so I removed it

@@ -101,7 +101,6 @@ func (r *Resource) CopyMergeMetaDataFieldsFrom(other *Resource) error {
}

func (r *Resource) copyKustomizeSpecificFields(other *Resource) {
r.options = other.options
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Should we move it up to CopyMergeMetaDataFieldsFrom field though? It's not a Kustomize-specific field anymore.

Looking at that code, I also wonder if we could avoid resetting the annotations repeatedly by making a version of mergeStringMaps that is build-annotation-aware.

api/testutils/kusttest/harnessenhanced.go Show resolved Hide resolved
BuildAnnotationsRefBy = konfig.ConfigAnnoDomain + "/refBy"
BuildAnnotationsGenOptions = konfig.InternalConfigAnnoDomain + "/generatorOptions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just change the definition of konfig.ConfigAnnoDomain to internal.blah.blah so they all have the same prefix instead of having two? All of these are internal in the sense that they only appear in the kustomize build pipeline and aren't emitted out the end or saved anywhere.

Copy link
Contributor Author

@natasha41575 natasha41575 Jul 24, 2021

Choose a reason for hiding this comment

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

done

The only exception to that is config.kubernetes.io/local-config which can be read and used by functions so I just updated it to not use ConfigAnnoDomain

api/resource/resource_test.go Outdated Show resolved Hide resolved
api/resource/resource_test.go Show resolved Hide resolved
api/resource/resource.go Outdated Show resolved Hide resolved
}

// GetArgs returns a copy of the underlying GeneratorArgs
func (g *GenArgs) GetArgs() GeneratorArgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

GenArgs exposes only

  • ShouldAddHashSuffixToName
  • Behavior (create, replace, merge)

These fields are special relative to other fields in GeneratorArgs because they must be consulted when overlays are processed and during the final name-fixing pass. The remaining GeneratorArgs fields are used only at resource creation time and are disposable after that.

To make the special fields available up the stack, it was clearest at the time to wrap GeneratorArgs in an immutable facade that exposed only those fields. Alternatively, the two fields could have been copied out into two new member fields of Resource. The thought was maybe more generator metadata might need to be exposed, so they were kept together.

Providing a GetArgs method which exposes the backing defeats this information hiding and opens the door to weirdness, so lets not. Since it's exposed only to marshal yaml, the thing to do here would be to offer a AsYaml method here - however....

However, the new pattern of using attributes to store resource metadata, and the fact that in several years of use we've not had to add more generator args, makes this GenArgs design moot.

Suggest :

  • No more GenArgs.
  • BuildAnnotationsGenOptions becomes two constants - for ShouldAddHashSuffixToName and Behavior for two new annotation fields
  • two new Resource setter/getter pairs that read/write these fields (in the spirit of AllowNameChange, NameChangeAllowed etc)
  • Where one would call SetOptions, one calls two setters, and likewise for getOptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, if you want to cut it off here, submit this, and continue in another PR (dedicated to deleting GenArgs) that's fine.

in that case just drop GetArgs and do AsYaml instead (don't break the hiding)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, if you want to cut it off here, submit this, and continue in another PR (dedicated to deleting GenArgs) that's fine.

I'd like to do that. This PR is getting pretty big, so I can start working on a followup to drop GenArgs right after this is merged. For this PR I changed GetArgs to AsYaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the context!

@natasha41575 natasha41575 force-pushed the resourceOptionsAsAnnotations branch from c51710b to ea918eb Compare July 24, 2021 00:16
@natasha41575 natasha41575 requested review from monopole and KnVerey July 24, 2021 00:19
@monopole
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2021
@k8s-ci-robot k8s-ci-robot merged commit 91f74e8 into kubernetes-sigs:master Jul 24, 2021
@natasha41575 natasha41575 deleted the resourceOptionsAsAnnotations branch July 26, 2021 17:21
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Resource.options with annotations
4 participants