search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
11.03k stars 2.25k forks source link

Resources accumulation fails with remote resources using git and short git commit id as ref #3761

Closed xakraz closed 2 years ago

xakraz commented 3 years ago

Describe the bug

Context

We are using a kind off "off-the-shelf" pattern for our deployments environments:

On the environment overlays, we usually have 

resources:
- https://github.com/MY_ORG/MY_SERVICE/deployment?ref=A_GIT_REF

📝 where the GIT_REF is also part of the docker image tag name, which in turn refers to the commit on the master branch of the code included in the image.

Issue

Kustomize fails to build the accumulated manifest when using a short git id, but works using the full length format.

Short ID

$ kustomize build > url1.yaml

Error: accumulating resources: 
  accumulation err='accumulating resources from 'https://github.com/MY_ORG/MY_SERBICE/deployment?ref=SHORT_GIT_REF': yaml: line 155: mapping values are not allowed in this context': git cmd = '/usr/bin/git fetch --depth=1 origin SHORT_GIT_REF': exit status 128

Notes

📝 1: This url works fine With kustomize 3.6.1.

We faced an other issue referenced here(https://github.com/kubernetes-sigs/kustomize/issues/3716) that lead us to try the latest version of kustomize (v4.0.5) since go-getter has been removed in 4.0.0.

📝 2: I have tried different length of the SHORT_GIT_REF Defaults is 7, on our CI system, git cli returns a 8 char length REF. I have tested up to 15 but it does not work.

Kustomize version

$ kustomize version

{Version:kustomize/v4.0.5 GitCommit:9e8e7a7fe99ec9fbf801463e8607928322fc5245 BuildDate:2021-03-08T20:53:03Z GoOs:linux GoArch:amd64}

Platform

Linux

thatsmydoing commented 3 years ago

I believe that feature only worked because of go-getter. I think it actually fetches the archive from github instead of doing an actual git clone. Since we only have the cloner now, you can't git clone a short hash (unless github supports it, which it unfortunately doesn't).

Technically, we could clone the whole repository and checkout the short hash that way, but that comes with a massive performance penalty for large repositories as we currently do a shallow clone of just the ref being requested.

I also wouldn't recommend using the short hash to identify refs for this anyway because there is a potential for collision in the future when your repository gets bigger. Practically, it probably doesn't matter that much but the possibility is there.

vit-zikmund commented 3 years ago

Hi there, I hit the same problem and frankly, I also miss the go-getter functionality. I've searched the internets and found a way how to clone the repo without blobs:

git clone --filter=blob:none --no-checkout 'https://github.com/kubernetes-sigs/kustomize.git' 
Cloning into 'kustomize'...
remote: Enumerating objects: 34092, done.
remote: Counting objects: 100% (652/652), done.
remote: Compressing objects: 100% (371/371), done.
remote: Total 34092 (delta 320), reused 536 (delta 259), pack-reused 33440
Receiving objects: 100% (34092/34092), 5.21 MiB | 2.67 MiB/s, done.
Resolving deltas: 100% (17155/17155), done.

This trims the repo that's currently 89MB in size down to 6.3MB, which is pretty good. After that one can obtain the full commit hash from a short one with e.g.:

git rev-parse 9f5697b
9f5697b15451ed87a1fc327afd1ba15b9a89692a

Exploring more on the git clone --filter topic, I've also found the tree:0 filter, which gets even less - 2.4MB:

git clone  --filter=tree:0 --no-checkout 'https://github.com/kubernetes-sigs/kustomize.git'

git rev-parse still works on such clone.

One can then fall back to the usual cloner procedure and either fetch & checkout with the full commit hash:

git fetch --depth=1 origin 9f5697b15451ed87a1fc327afd1ba15b9a89692a
git checkout FETCH_HEAD

or checkout the short hash directly, which apparently does the shallow fetch on its own:

git checkout 9f5697b

I'm using git version 2.31.1 @ Fedora 34.

Would this procedure be possible to use at least as a fallback? I suppose downloading few megs from a big repo might be an acceptable penalty, perhaps complemented with a warning.

thatsmydoing commented 3 years ago

Exploring more on the git clone --filter topic, I've also found the tree:0 filter, which gets even less - 2.4MB:

It looks like that functionality was added in git version 2.27.0 (2020/6/1) which I presume is a bit too new to assume by default. This also isn't implemented in go-git either so if we do ever integrate that in, we still wouldn't be able to resolve it without fetching everything.

Would this procedure be possible to use at least as a fallback? I suppose downloading few megs from a big repo might be an acceptable penalty, perhaps complemented with a warning.

It would still negatively affect people who typo the commit as any failure would cause a fallback. That said, you could try making a PR for it and see how it goes.

I'm a bit curious why the need for short hashes though? Is it purely for convenience?

vit-zikmund commented 3 years ago

Thanks for your interest @thatsmydoing.

This also isn't implemented in go-git either so if we do ever integrate that in, we still wouldn't be able to resolve it without fetching everything.

Does go-git really matter here, when cloner uses the system's git binary? Could we add a version check and bail out for the older versions? It wouldn't be worse than what it is now :)

It would still negatively affect people who typo the commit as any failure would cause a fallback.

True, but that's why I was suggesting the warning. People would then get notified and could choose what's best for them. After all, the docs on resources for the URLs still refer to HashiCorp's go-getter, vaguely suggesting that it's being used by kustomize:

Directory specification can be relative, absolute, or part of a URL. URL specifications should follow the hashicorp URL format.

There's definitely some space for improvement :smile: as it tends to bring false hopes.

A little rant you don't have to read. As a curious `kustomize` and Go newbie, I've spent 2 days so far just figuring out that it's not the case anymore (after I learned to build `go-getter` from source and realized it works for my case, then I searched for "go-getter" in the repo, to trip another mine with the attempt to switch to a lighter `yujunz/go-getter`, which I tweaked & built too, to realize it also works, only then I finally searched for the `git fetch` args from the error and found the real logic) Alas, searching for keywords in the github repo drags you to the past... I learned something on the way, so it's also good :smile:

I'm a bit curious why the need for short hashes though? Is it purely for convenience?

Yes... as far as I know. I was hoping to use the short commit hashes in my gitops CI/CD, where the docker image tags currently contain a couple human-readable parts along with the short hash. Having the long hash there is really hard on the eyes (and line wrapping :face_with_head_bandage:). I guess I'd still be safe not to cross the 128 characters limit for a tag, but it's simply annoying. It's especially surprising I can't use the short hash with git fetch when pretty much all the usual commands support it. That in fact led me to falsely believe it's supported everywhere. I guess than hacking this into kustomize I'll just get along with it, but I strongly believe this limitation is more than worth documenting.

thatsmydoing commented 3 years ago

Does go-git really matter here, when cloner uses the system's git binary?

The maintainers have been saying they'd like to move to go-git for some time to drop the dependency on an external binary (and maybe improve performance). I only mention it here because one option against having "too old git" is just use a library for it instead. But unfortunately, that path isn't available for this.

There's definitely some space for improvement smile as it tends to bring false hopes.

Oh I've definitely complained about this many times in https://github.com/kubernetes-sigs/kustomize/issues/2538#issuecomment-730114411 maybe I should just go and make a PR to update the docs now that they've removed go-getter.

Yes... as far as I know. I was hoping to use the short commit hashes in my gitops CI/CD, where the docker image tags currently contain a couple human-readable parts along with the short hash. Having the long hash there is really hard on the eyes (and line wrapping face_with_head_bandage). I guess I'd still be safe not to cross the 128 characters limit for a tag, but it's simply annoying.

We also use this for gitops and we just put in the full hash as that avoids any potential collisions in the future. Since it's automated anyway you don't even really look at the hash so it doesn't really matter how long it is.

I can't use the short hash with git fetch when pretty much all the usual commands support it. That in fact led me to falsely believe it's supported everywhere. I guess than hacking this into kustomize I'll just get along with it, but I strongly believe this limitation is more than worth documenting.

If you really want this, a horrible thing you can do is tag each commit with its short hash. You can't fetch short hashes, but you can fetch any kind of ref including tags. But I'd still suggest just using the full hash since that's overall more correct.

vit-zikmund commented 3 years ago

@thatsmydoing If you could update the docs, that would be just awesome. I tried to do a PR myself, but hit some legal barriers :/ Also with my so far shallow understanding of all this, I'm not convinced it would be good enough. (I also considered creating a documentation issue, but judging from the issues there, it seems very little go noticed let alone acted upon. A PR seems like the only sensible way to go.)

In the meantime, I'll just go forward with the long hashes, no real problem with that. The "horrible tag way" made me :joy: Thanks for your help!

thatsmydoing commented 3 years ago

I've made a PR here https://github.com/kubernetes-sigs/kustomize/pull/4030 still waiting for approval of the message before trying to update the main kustomize docs.

Also, I might sound like a broken record but here's an article (about GitHub of all things) showing why short hashes are a bad idea https://blog.teddykatz.com/2019/11/12/github-actions-dos.html

k8s-triage-robot commented 3 years ago

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-triage-robot commented 2 years ago

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-triage-robot commented 2 years ago

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-triage-robot commented 2 years ago

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.

/close

k8s-ci-robot commented 2 years ago

@k8s-triage-robot: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/3761#issuecomment-1063881084): >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: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue or PR with `/reopen` >- Mark this issue or PR as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.