Implement `kustomize localize` localization layer #4797

annasong20 · 2022-09-15T17:34:38Z

Builds off of #4779. Implements most of localization logic for kustomize localize, including

processing kustomization fields
changing file references in fields
writing files to localize destination

k8s-ci-robot · 2022-09-15T17:34:40Z

@annasong20: 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 · 2022-09-15T17:34:47Z

Hi @annasong20. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

annasong20 · 2022-09-15T17:40:35Z

EDIT: moving previous PR description into this comment

TL;DR The current implementation of kustomization field processing is more of a brute force approach. Feedback is welcome.

To process the kustomization fields, I tried

adding a MapFiles() method to the relevant kustomization fields under api/types
adding MapFiles() to each plugin under plugin/builtin
where

func (ga *GeneratorArgs) MapFiles(m func(string) (string, error)) error

is responsible for accepting a localizing function like localizer.localizePath and setting its path fields to the outputs the localizing function maps them to, respectively.

These designs were abandoned because

the 1st implementation requires logic that api/types doesn't have. Specifically, the implementation
- created an import cycle: types.GeneratorArgs (used by configMap and secretGenerator) would need to import api/kv (which indirectly imports types) logic to parse key, file pairs under the files field for configMapGenerator.
- for PatchStrategicMerge would need a resmap.PluginHelpers and repeat some of the logic under plugin/builtin/patchstrategicmergetransformer.
the 2nd implementation is a lot of work. Not only would we need to write
- MapFiles() for all the existing plugins, which includes re-factoring a lot of the internal code to keep consistent state,
- we'd also need to convert the plugins back to kustomization fields

============================================================================================

The purpose of this PR is for @natasha41575 to give high-level feedback. The PR is incomplete:

integration tests need to be completed
details need to be added to localizer, which also needs re-factoring
pre-submit check failures need to be fixed

I will continue to update this PR and mark it "Ready for review" when finished.

natasha41575

I think the logic mostly looks good. I have a few questions, but I think this is headed in a great direction!

api/internal/localizer/localizer.go

natasha41575 · 2022-09-16T20:50:21Z

/ok-to-test

annasong20 · 2022-09-21T01:28:06Z

I've completed the integration tests and refactored the code so that all checks should pass. I'll work on MapFiles() and update the PR when complete.

annasong20 · 2022-09-21T01:36:34Z

api/krusty/remotelocalizer_test.go

+	"sigs.k8s.io/kustomize/kyaml/filesys"
+)
+
+const simpleHTTPS = "https://github.com/kubernetes-sigs/kustomize//api/krusty/testdata/localize/simple"


Since #4783 has been merged, if you'd prefer, I believe I could replace these urls with local, file://-prefixed urls? I'd have to look into it, but lmk. If I made this change, I'd add tests for parseDomain.

annasong20 · 2022-09-23T19:49:19Z

api/internal/localizer/locloader.go

 	ldr, err := ll.Loader.New(path)
+	// timeout indicates path is a root, not a file
+	if utils.IsErrTimeout(err) {


Note to self: Add TODO statement to refactor once appropriate errors added to ifc.Loader

annasong20 · 2022-10-10T22:53:09Z

/label tide/merge-method-squash

annasong20 · 2022-10-11T14:51:13Z

Updated PR with most plugin changes. I have yet to fix some lint errors. There are more localize calling code changes to come.

plugin/builtin/configmapgenerator/ConfigMapGenerator.go

api/resmap/resmap.go

api/testutils/kusttest/harnessenhanced.go

annasong20 · 2022-10-14T21:15:22Z

TL;DR The functional logic in this PR is now complete, though test-writing, refactoring, etc. are still a WIP.

I've added localize logic in the form of MapFiles() and YAML() to the relevant plugins. The code localizes kustomization fields by converting them to plugins, calling said methods on the plugins, and writing the plugins back as kustomization fields. Ideally, in the future, these methods will be added to all built-in plugins.

I've also exposed an entry point Run() method for localize under api. The localize command will be able to call this method to initiate localization.

Next steps:

Continue writing tests for code that converts kustomization fields to and from plugins
Reorganize existing tests to handle exposed entry point Run() method
Refactor code for readability and inconsistencies
Fix lint errors

yuwenma · 2022-10-19T17:03:57Z

My major question about this PR is around the large amount of changes to builtin and plugin transformers. I think in general we need a progressive PR merge and release plan to make sure the minimum users are affected and relief the releasing burdens from the kustomize owners (e.g. if a single transformer has behavior shift, whether this is expected or not, it could break their productions).

From the PR description itself, it seems not necessary to touch these builtin and plugins since kustomize localize is an independent command and itself does not trigger kustomize build or any of those transformers. If these function changes are due to the change in kusttarget, I'd propose the following two options:

Option #1. Separating the localization code form the kustomize build main workflow, do not depend on the KustTarget in api/internal/target/kusttarget_configplugin.go. Have localizer specifically pull the needed code from kusttarget to its own directory e.g. loctarget/alpha/kusttarget.go. And develop the new functions there, do not worry about merging loctarget/alpha/kusttarget.go back to kusttarget_configplugin until it's released for over 6+ months and have users used it. e.g. ConfigSync

Option #2 Finding out another code structure that make the minimum change to kusttarget_configplugin.go, and do not change any of the builtin and plugin functions.

@annasong20 @natasha41575 Happy to further discuss the proposed two solutions.

yuwenma

Thanks for the work, Anna! I did some quick reviews mainly focusing on the localizer part. I have two ideas to help making this big feature move forward

yuwenma · 2022-09-15T17:47:32Z

api/internal/git/repospec.go

+	return []string{
+		"ssh://", "https://", "http://",
+	}
+}


Using a new method seems not necessary.
Can you use const var?

lint complains about global variables. Would you like me to override the linter?

qq: can we revert this change as discussed? I think RepoSpec code refactor and feature changes are better to be done in its own PRs.

api/internal/localizer/errors.go

api/internal/localizer/localizer.go

api/resmap/resmap.go

api/internal/loctarget/locloader.go

yuwenma · 2022-10-19T16:31:48Z

api/internal/loctarget/locloader.go

-			// should never happen
-			log.Fatalf(errors.WrapPrefixf(err, "cannot clean validated file path '%s'", abs).Error())
-		}
+	if !loader.HasRemoteFileScheme(path) && ll.local {


Shall we deal with the case loader.HasRemoteFileScheme(path)?

Passing silently (No-op) seems to be out of users' expectations and hard to triage.

localize doesn't currently put any constraints on remote files. Can you clarify what I should do here?

api/internal/loctarget/locloader.go

yuwenma · 2022-10-19T16:40:05Z

api/internal/loctarget/util.go

+// hasRef checks if path url has ref query string parameter
+func hasRef(path string) bool {
+	repoSpec, err := git.NewRepoSpecFromURL(path)
+	if err != nil {
+		log.Fatalf("%s: %s", "unable to parse validated root url", err.Error())
+	}
+	return repoSpec.Ref != ""
+}
+


super nit: seems to belong to git library. Shall we move this function there?

I'm down to move it, but can I clarify why it belongs with RepoSpec?

This is a helper function for localize and doesn't add any functionality to RepoSpec. I didn't want to clutter RepoSpec's method pool with a method that makes simple use of RepoSpec's existing functionality. Also, the log.Fatalf() is specific to localize; we only call it because we call hasRef after loading path, during which a RepoSpec must have been created.

I originally wanted to put the parseDomain function (in this file) in RepoSpec, but it seemed to require refactoring the existing code, which (I believe?) wasn't recommended. #4652 (comment) I'm also down to refactor if time allows.

yuwenma · 2022-10-19T16:43:42Z

api/internal/loctarget/util.go

+func toFSPath(urlPath string) string {
+	return filepath.Join(strings.Split(urlPath, "/")...)
+}


I have some further concerns about the risk conditions this function needs to handle. Would like to see some more test coverages to show what the filepaths look like if the url path contains special characters (or invalid file name characters like pipe)

I've added test coverage to the best of my knowledge in this commit: dd324d1 My current (uninformed) approach is to work around illegal file name characters for unix, linux, but leave windows.

I've also been concerned about the following edge cases, but like in this case, would like your feedback on whether they warrant attention:

same host, but different scheme, userinfo, or port

org, repo being the dot-segments '.' or '..'

yuwenma

Change approve to request changes.

k8s-ci-robot · 2022-10-19T17:07:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: annasong20
Once this PR has been reviewed and has the lgtm label, please assign natasha41575 for approval by writing /assign @natasha41575 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

natasha41575 · 2022-10-19T17:13:01Z

My major question about this PR is around the large amount of changes to builtin and plugin transformers. I think in general we need a progressive PR merge and release plan to make sure the minimum users are affected and relief the releasing burdens from the kustomize owners (e.g. if a single transformer has behavior shift, whether this is expected or not, it could break their productions).
From the PR description itself, it seems not necessary to touch these builtin and plugins since kustomize localize is an independent command and itself does not trigger kustomize build or any of those transformers.

I totally agree with this. I was not expecting such large changes to the builtin plugins or kusttarget. The only thing I expected from the plugins was the addition of MapFiles, and maybe moving some common code to some helper functions or shared libraries. It's okay to pull some common code out into their own functions and call those functions, but we shouldn't have too many changes to kustomize build code other than that.

Some of the changes to the plugins seem to be purely stylistic, for example the configureMap helper in the ConfigMapGenerator code seems like an unnecessary refactor; I don't see it used anywhere else besides the place where its code originally was. My suggestion would be to first undo all changes to the plugins that aren't strictly necessary for the functionality of kustomize localize. Then we can take another look to see which of the options Yuwen suggested make more sense.

api/internal/target/kusttarget_configplugin.go

Address feedback, expose errors, and improve error messages

Implement localizer with integration tests and fix lint.

yuwenma · 2022-11-05T15:11:06Z

api/kv/kv.go

-//  Acceptable formats include:
-//   1.  source-path: the basename will become the key name
-//   2.  source-name=source-path: the source-name will become the key name and
-//       source-path is the path to the key file.
+//	Acceptable formats include:
+//	 1.  source-path: the basename will become the key name
+//	 2.  source-name=source-path: the source-name will become the key name and
+//	     source-path is the path to the key file.


Can you revert this change or run gofmt ?

annasong20 · 2022-11-05T22:29:11Z

api/internal/localizer/locloader.go

@@ -116,24 +117,32 @@ func (ll *locLoader) Load(path string) ([]byte, error) {
 // New returns a Loader at path if path is a valid localize root.
 // Otherwise, New returns an error.
 func (ll *locLoader) New(path string) (ifc.Loader, error) {
-	repoSpec, err := git.NewRepoSpecFromURL(path)


Note to self: check invalid if remote file passed in

natasha41575 · 2022-11-08T17:17:01Z

If you are still working on this PR and it is not ready for review, could you mark it as draft or WIP?

If you do not plan to work on this PR, we should probably close it so that other reviewers don't spend a lot of time looking at it.

annasong20 · 2022-11-08T18:26:23Z

I'm closing this PR because its scope is too large. I'll be splitting this up into smaller, more focused PRs.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 15, 2022

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 15, 2022

k8s-ci-robot requested review from KnVerey and yuwenma September 15, 2022 17:34

annasong20 mentioned this pull request Sep 16, 2022

Fix flaky #4798

Merged

natasha41575 reviewed Sep 16, 2022

View reviewed changes

api/internal/localizer/localizer.go Outdated Show resolved Hide resolved

api/internal/localizer/localizer.go Show resolved Hide resolved

api/internal/localizer/localizer.go Show resolved Hide resolved

api/internal/localizer/localizer.go Show resolved Hide resolved

natasha41575 reviewed Sep 16, 2022

View reviewed changes

api/internal/localizer/localizer.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2022

annasong20 force-pushed the localizer branch from 41d0e23 to 7f1e996 Compare September 21, 2022 00:26

annasong20 commented Sep 21, 2022

View reviewed changes

annasong20 commented Sep 23, 2022

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 10, 2022

annasong20 force-pushed the localizer branch from 7f1e996 to 3eff780 Compare October 11, 2022 14:46

annasong20 commented Oct 11, 2022

View reviewed changes

plugin/builtin/configmapgenerator/ConfigMapGenerator.go Outdated Show resolved Hide resolved

annasong20 commented Oct 11, 2022

View reviewed changes

api/resmap/resmap.go Outdated Show resolved Hide resolved

annasong20 commented Oct 11, 2022

View reviewed changes

api/testutils/kusttest/harnessenhanced.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2022

annasong20 force-pushed the localizer branch from 61135c3 to 0297031 Compare October 14, 2022 20:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2022

yuwenma approved these changes Oct 19, 2022

View reviewed changes

yuwenma suggested changes Oct 19, 2022

View reviewed changes

natasha41575 reviewed Oct 19, 2022

View reviewed changes

api/internal/target/kusttarget_configplugin.go Outdated Show resolved Hide resolved

annasong20 mentioned this pull request Oct 20, 2022

Code struct for each kustomization field #4671

Closed

annasong20 force-pushed the localizer branch from 0297031 to 18a4771 Compare October 25, 2022 18:24

annasong20 marked this pull request as ready for review October 25, 2022 18:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2022

k8s-ci-robot requested a review from natasha41575 October 25, 2022 18:39

annasong20 added 3 commits November 1, 2022 12:51

Expose key,value parsing to handle generators

3328241

Patch locloader

6e72e75

Address feedback, expose errors, and improve error messages

Implement localizer

4376ee8

Implement localizer with integration tests and fix lint.

annasong20 force-pushed the localizer branch from 18a4771 to dd324d1 Compare November 1, 2022 19:53

This was referenced Nov 2, 2022

POC to fix RepoSpec bugs #4848

Closed

RepoSpec parses non-github ssh urls incorrectly #4847

Closed

RepoSpec parses ref with slash as path #4849

Closed

RepoSpec path can exit repo directory #4850

Closed

minimize scope of PR

0a2a321

annasong20 force-pushed the localizer branch from 6e60afe to 0a2a321 Compare November 4, 2022 22:30

yuwenma reviewed Nov 5, 2022

View reviewed changes

annasong20 commented Nov 5, 2022

View reviewed changes

This was referenced Nov 5, 2022

Improve readability of ifc.Loader's Repo() method #4857

Merged

Implement localizer skeleton for kustomize localize #4859

Merged

annasong20 closed this Nov 8, 2022

annasong20 mentioned this pull request Nov 9, 2022

Improve localizer readability #4860

Merged

annasong20 deleted the localizer branch February 9, 2023 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `kustomize localize` localization layer #4797

Implement `kustomize localize` localization layer #4797

annasong20 commented Sep 15, 2022 •

edited

Loading

k8s-ci-robot commented Sep 15, 2022

k8s-ci-robot commented Sep 15, 2022

annasong20 commented Sep 15, 2022 •

edited

Loading

natasha41575 left a comment

natasha41575 commented Sep 16, 2022

annasong20 commented Sep 21, 2022

annasong20 Sep 21, 2022 •

edited

Loading

annasong20 Sep 23, 2022

annasong20 commented Oct 10, 2022

annasong20 commented Oct 11, 2022

annasong20 commented Oct 14, 2022

yuwenma commented Oct 19, 2022

yuwenma left a comment

yuwenma Sep 15, 2022

annasong20 Oct 24, 2022

yuwenma Nov 5, 2022

yuwenma Oct 19, 2022

annasong20 Oct 25, 2022

yuwenma Oct 19, 2022

annasong20 Oct 25, 2022

yuwenma Oct 19, 2022

annasong20 Nov 1, 2022 •

edited

Loading

yuwenma left a comment

k8s-ci-robot commented Oct 19, 2022

natasha41575 commented Oct 19, 2022 •

edited

Loading

yuwenma Nov 5, 2022

annasong20 Nov 5, 2022

natasha41575 commented Nov 8, 2022

annasong20 commented Nov 8, 2022

Implement kustomize localize localization layer #4797

Implement kustomize localize localization layer #4797

Conversation

annasong20 commented Sep 15, 2022 • edited Loading

k8s-ci-robot commented Sep 15, 2022

k8s-ci-robot commented Sep 15, 2022

annasong20 commented Sep 15, 2022 • edited Loading

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 commented Sep 16, 2022

annasong20 commented Sep 21, 2022

annasong20 Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annasong20 commented Oct 10, 2022

annasong20 commented Oct 11, 2022

annasong20 commented Oct 14, 2022

yuwenma commented Oct 19, 2022

yuwenma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annasong20 Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

yuwenma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 19, 2022

natasha41575 commented Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented Nov 8, 2022

annasong20 commented Nov 8, 2022

Implement `kustomize localize` localization layer #4797

Implement `kustomize localize` localization layer #4797

annasong20 commented Sep 15, 2022 •

edited

Loading

annasong20 commented Sep 15, 2022 •

edited

Loading

annasong20 Sep 21, 2022 •

edited

Loading

annasong20 Nov 1, 2022 •

edited

Loading

natasha41575 commented Oct 19, 2022 •

edited

Loading