Use go-getter only for archive fetches

[thatsmydoing]

This is another attempt to address #2538. This uses go-getter only to fetch archives via http. Otherwise, it uses the git cloner to clone repositories. Having 2 different ways of cloning git repositories was a big cause of confusion with url formats and we address this by just removing go-getter as an option entirely.

[k8s-ci-robot]

Hi @thatsmydoing. 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: thatsmydoing
To complete the pull request process, please assign monopole after the PR has been reviewed.
You can assign the PR to them by writing /assign @monopole in a comment when ready.

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

[thatsmydoing]

I'm reasonably confident this does the right thing and that go-getter will fallback to the git cloner when provided a git repo but succeed when provided with an archive url.

go-getter and git cloning has a number of issues regarding url formats #2538 (comment) and performance #3244 (comment)

[thatsmydoing]

This is copied from how the default getters are defined https://github.com/hashicorp/go-getter/blob/7e54fdc668a3c829d0349517e4b4b74cb04060c0/get.go#L63-L65

[thatsmydoing]

Remote bases must be directories containing a kustomization.yaml file so there's no sense in fetching a single file.

[thatsmydoing]

The detectors were only used to transform git style urls but we don't want to handle those here.

[monopole]

please rebase

[monopole]

The whole getter / cloner flow is getting hard to follow.
I just had to add timeout because it was hanging, and we needed better errors.
Can you clarify the change to add ClientModeDir? Does that mean it never worked?
The tests are hard to follow.

[thatsmydoing]

I think the flow is mostly described in

kustomize/api/loader/loader.go

Lines 15 to 34 in 0ecbd59

	// NewLoader returns a Loader pointed at the given target.
	// If the target is remote, the loader will be restricted
	// to the root and below only. If the target is local, the
	// loader will have the restrictions passed in. Regardless,
	// if a local target attempts to transitively load remote bases,
	// the remote bases will all be root-only restricted.
	func NewLoader(
	lr LoadRestrictorFunc,
	target string, fSys filesys.FileSystem) (ifc.Loader, error) {
	ldr, errGet := newLoaderAtGetter(
	target, fSys, nil, git.ClonerUsingGitExec, getRemoteTarget)
	if errGet == nil {
	return ldr, nil
	}
	repoSpec, errGit := git.NewRepoSpecFromUrl(target)
	if errGit == nil {
	// The target qualifies as a remote git target.
	return newLoaderAtGitClone(
	repoSpec, fSys, nil, git.ClonerUsingGitExec, getRemoteTarget)
	}

and

kustomize/api/loader/fileloader.go

Lines 175 to 195 in 0ecbd59

	// New returns a new Loader, rooted relative to current loader,
	// or rooted in a temp directory holding a git repo clone.
	func (fl *fileLoader) New(path string) (ifc.Loader, error) {
	if path == "" {
	return nil, fmt.Errorf("new root cannot be empty")
	}
	ldr, errGet := newLoaderAtGetter(path, fl.fSys, nil, fl.cloner, fl.getter)
	if errGet == nil {
	return ldr, nil
	}
	repoSpec, errGit := git.NewRepoSpecFromUrl(path)
	if errGit == nil {
	// Treat this as git repo clone request.
	if errGit := fl.errIfRepoCycle(repoSpec); errGit != nil {
	return nil, errGit
	}
	return newLoaderAtGitClone(
	repoSpec, fl.fSys, fl, fl.cloner, fl.getter)
	}

which is, given a url, try to download it with go-getter first and if it fails, try to use the git cloner.

This PR does not change the flow. All it does is make go-getter only fetch archives and fail on git repositories so that it falls back to the git cloner. While go-getter can handle git repositories, the way it handles urls and cloning behavior is different enough from the cloner that it causes confusion as to what the url format should be and also causes performance issues.

The flow could definitely be improved though. It might make more sense to use the cloner first and then fall back to go-getter for archive urls.

I just had to add timeout because it was hanging

Could you point me to a url where it hangs?

Can you clarify the change to add ClientModeDir? Does that mean it never worked?

The meanings are described here https://godoc.org/github.com/hashicorp/go-getter#ClientMode. It was previously set to ClientModeAny which allows it to download either a file or a directory/archive. Downloading a single file is rarely what we want as remote bases must be directories. ClientModeDir ensures that we only fetch archives (directories only make sense for things like S3 which we don't support). It wasn't broken before but this just tightens the spec.

[yujunz]

Downloading a single file is rarely what we want as remote bases must be directories.

I think bases are already merged to resources and it is common use case to refer a single file as resources, e.g. https://github.com/jetstack/cert-manager/releases/download/v1.1.0/cert-manager.yaml .

I don't remember exactly if resources fetching is based on getter or not. Please double check this does not break with the ClientMode change. @thatsmydoing

[thatsmydoing]

I think bases are already merged to resources and it is common use case to refer a single file as resources

While bases and resources are merged, I don't think the behavior changed. Only "bases" can be remote in much the same way that only "bases" can be referenced via parent directories.

Neither of

kustomize build https://github.com/jetstack/cert-manager/releases/download/v1.1.0/cert-manager.yaml

cat > kustomization.yaml <<EOF
resources:
 - https://github.com/jetstack/cert-manager/releases/download/v1.1.0/cert-manager.yaml
EOF
kustomize build .

work with kustomize 3.8.7

[yujunz]

It seems I gave a bad example because the getter doesn't follow redirection at the moment .

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
 - https://raw.githubusercontent.com/argoproj/argo-cd/master/manifests/install.yaml

The above should work though.

[thatsmydoing]

Ah, this feature is described in https://github.com/kubernetes-sigs/kustomize/blob/master/examples/loadHttp.md and the related issue #970

This will most likely break that. I'll look into it.

[thatsmydoing]

On further testing, 3.8.7 does support redirects. I don't know why it failed the first time I tried. I might have accidentally used a different version. I also realized that the http fetching is not done by go-getter but by kustomize itself so this PR should not affect fetching remote resources.

The reason that ClientModeDir is necessary is that with ClientModeAny, kustomize build "https://github.com/Liujingfang1/mysql" which go-getter will fetch as an HTML file and then not fallback to the cloner. Requiring a directory makes it properly fail here and fallback to the cloner.

[monopole]

/test kustomize-presubmit-master

[k8s-ci-robot]

@thatsmydoing: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kustomize-presubmit-master	2188fe9	link	/test kustomize-presubmit-master

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/test-infra repository. I understand the commands that are listed here.

[thatsmydoing]

/retest

[k8s-ci-robot]

@thatsmydoing: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

[thatsmydoing]

Ah. There were linting errors so I just fixed them and rebased.

[k8s-ci-robot]

@thatsmydoing: 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.

[thatsmydoing]

This is no longer necessary as of #3586