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

git retrieval failures (such as a timeout) are not reported in certain scenarios #5540

Closed
ephesused opened this issue Feb 14, 2024 · 2 comments · Fixed by #5542
Closed

git retrieval failures (such as a timeout) are not reported in certain scenarios #5540

ephesused opened this issue Feb 14, 2024 · 2 comments · Fixed by #5542
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ephesused
Copy link
Contributor

What happened?

Under these conditions ...

  1. A resource is defined with an https git reference
  2. The web server uses a catch-all authentication challenge web page
  3. The git retrieval has a failure of some sort (such as a timeout)

... the git retrieval error is not reported.

Since a catch-all authentication challenge web page is in place, the attempt to accumulate the resource as a file receives a 200 response. As a result, fileloader.httpClientGetContent() does not do a check of the path to see if it's a git repo. That means FromFile processes the HTML of the web page, and properly returns a YAML parse error.

accumulateResources then attempts to process the resource as a git reference. If that git retrieval fails (such as in a timeout), a decision is made how to report the failure. When errF is a malformed YAML error - as it is in a timeout scenario, the error from kt.ldr.New(path) is dropped:

			ldr, err := kt.ldr.New(path)
			if err != nil {
				if kusterr.IsMalformedYAMLError(errF) { // Some error occurred while tyring to decode YAML file
					return nil, errF
				}

The result is that the user does not receive any information about the git retrieval failure.

Generally, in cases where the reference is a URL that has a response in the 200s, the user's goal is to load that URL as YAML. I understand why the malformed YAML error would suppress the kt.ldr.New(path) error. The challenge here is to try to identify the uncommon scenarios when the kt.ldr.New(path) error is useful information.

What did you expect to happen?

In this scenario, it would be good if the git retrieval error were reported.

For a partial solution, perhaps accumulateResources could check for utils.IsErrTimeout(), and then fall into something similar to this case:

				return nil, errors.WrapPrefixf(
					err, "accumulation err='%s'", errF.Error())

The idea of checking for the time out is simple - if there isn't a git repo at the address, then the failure will come back from the git calls, not the timeout. If the failure is a timeout, chances are good that there is a git repo at the address. In that case, the error message from the git attempt is meaningful.

How can we reproduce it (as minimally and precisely as possible)?

I do not have a test case that I can share - the server where we're seeing this problem is internal. Offhand, I don't know of an example public host with this behavior.

Internally, I can reproduce this easily with something like so, but I use the actual values for the resource address. The short timeout forces the git retrieval failure.

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - https://fake.host.example/path/to/repo.git//path/to/directory?timeout=1

Expected output

It'd be nice to see both the YAML error and the git error - something like so:

Error: accumulating resources: accumulation err='accumulating resources from 'https://fake.host.example/path/to/repo.git//path/to/directory?timeout=1': MalformedYAMLError: yaml: line 3: mapping values are not allowed in this context in File: https://fake.host.example/path/to/repo.git//path/to/directory?timeout=1 URL is a git repository': hit 1s timeout running '/path/to/git fetch --depth=1 https://fake.host.example/path/to/repo.git HEAD'

Actual output

Error: accumulating resources: accumulating resources from 'https://fake.host.example/path/to/repo.git//path/to/directory?timeout=1': MalformedYAMLError: yaml: line 3: mapping values are not allowed in this context in File: https://fake.host.example/path/to/repo.git//path/to/directory?timeout=1

Kustomize version

v5.3.0

Operating system

None

@ephesused ephesused added the kind/bug Categorizes issue or PR as related to a bug. label Feb 14, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 14, 2024
@ephesused
Copy link
Contributor Author

/assign

@koba1t
Copy link
Member

koba1t commented Feb 14, 2024

Thanks @ephesused
I believe this feature request is to improve the error output.

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants