Handle HTTP error codes in file loader #4334

sylr · 2021-12-08T13:32:36Z

GitHub release files like https://github.com/fluxcd/helm-controller/releases/download/v0.14.0/helm-controller.crds.yaml
seems to be hosted on Azure and it seems that there are egress limits that can be reached, e.g.:

<?xml version="1.0" encoding="utf-8"?><Error><Code>ServerBusy</Code><Message>Egress is over the account limit.
RequestId:f4a46b38-001e-0046-2437-ec16e2000000
Time:2021-12-08T13:28:03.8542138Z</Message></Error>

This patch allows to have a clear Error message instead of just missing Resource metadata:

Error: accumulating resources: accumulation err='accumulating resources from 'https://github.com/fluxcd/source-controller/releases/download/v0.19.0/source-controller.crds.yaml': URL returned error 503 (Service Unavailable)': evalsymlink failure on '/private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases/download/v0.19.0/source-controller.crds.yaml' : lstat /private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases: no such file or directory

Signed-off-by: Sylvain Rabot [email protected]

GitHub release files like https://github.com/fluxcd/helm-controller/releases/download/v0.14.0/helm-controller.crds.yaml seems to be hosted on Azure and it seems that there are egress limits that can be reached, e.g.: ```xml <?xml version="1.0" encoding="utf-8"?><Error><Code>ServerBusy</Code><Message>Egress is over the account limit. RequestId:f4a46b38-001e-0046-2437-ec16e2000000 Time:2021-12-08T13:28:03.8542138Z</Message></Error> ``` This patch allows to have a clear Error message instead of just `missing Resource metadata`: ``` Error: accumulating resources: accumulation err='accumulating resources from 'https://github.com/fluxcd/source-controller/releases/download/v0.19.0/source-controller.crds.yaml': URL returned error 503 (Service Unavailable)': evalsymlink failure on '/private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases/download/v0.19.0/source-controller.crds.yaml' : lstat /private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases: no such file or directory ``` Signed-off-by: Sylvain Rabot <[email protected]>

k8s-ci-robot · 2021-12-08T13:32:45Z

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

KnVerey · 2021-12-08T16:14:18Z

/ok-to-test

It is useless and it clogs the error message. Signed-off-by: Sylvain Rabot <[email protected]>

k8s-ci-robot · 2021-12-08T17:25:33Z

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

api/internal/target/kusttarget.go

api/loader/fileloader.go

Signed-off-by: Sylvain Rabot <[email protected]>

natasha41575 · 2021-12-10T20:08:31Z

api/internal/target/kusttarget.go

@@ -366,6 +367,10 @@ func (kt *KustTarget) accumulateResources(
 	for _, path := range paths {
 		// try loading resource as file then as base (directory or git repository)
 		if errF := kt.accumulateFile(ra, path, origin); errF != nil {
+			// not much we can do if the error is an HTTP error so we bail out
+			if errors.Is(errF, load.ErrorHTTP) {
+				return nil, errF


If we get an HTTP error, wouldn't the line 374 ldr, err := kt.ldr.New(path) take care of this case?

The problem with the error handling of line 374 is that it wraps the initial error with the error of the ldr.New() call and we end up with a very long error which half does not make any sens, i.e., no such file or directory for an URL:

Error: accumulating resources: accumulation err='accumulating resources from 'https://github.com/fluxcd/source-controller/releases/download/v0.19.0/source-controller.crds.yaml': URL returned error 503 (Service Unavailable)': evalsymlink failure on '/private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases/download/v0.19.0/source-controller.crds.yaml' : lstat /private/var/folders/hq/ttl6jyh539q55fz6282w0jyc0000gn/T/kustomize-3508224975/releases: no such file or directory

Understood, thanks! In that case, can we move these two lines to within the error handling of line 374? E.g.

ldr, err := kt.ldr.New(path) if err != nil { if errors.Is(errF, load.ErrorHTTP) { return nil, errF } return nil, errors.Wrapf( err, "accumulation err='%s'", errF.Error()) }

Perhaps this is messier, but the intent of this code is:

Try to load the resource as a file.

If that fails, then regardless of the reason for the error, try to load it as a directory.

If that also fails, return the error from step 1.

I would like to keep the code as close to its original thought process as possible, while still supporting the improved error message you've added.

I don't like it to be honest, this feels really weird.

Can you elaborate? My intent is so that the code is changed as little as possible while still handling your error case. If we are improving the error message, IMO the check should go in the error handling. It will be helpful to have more justification than "this feels really weird" if you want to put it elsewhere.

It would be very strange to expect that to load from the filesystem, and if it is somehow happening, I don't think it is behaviour we should preserve.

Unfortunately, that code path also handles remote bases which can have https schemes and it does cause breakage.

Can you share a kustomization example that reproduces the breakage you are talking about ? Thank you.

It appears the test changed by #4030 used to pass and may have been broken by this PR. @sylr Would you be able to investigate and confirm?

resources: - https://github.com/kubernetes-sigs/kustomize.git//examples/multibases/dev/?ref=v1.0.6

works fine in kustomize 4.4 but gives a 404 not found error in 4.5

@natasha41575 absolutely, I'll try to fix that today or tomorrow.

natasha41575 · 2021-12-10T20:10:18Z

api/krusty/remoteload_test.go

+
+	httpErr := fmt.Errorf("%w: status code %d (%s)", loader.ErrorHTTP, 404, http.StatusText(404))
+	accuFromErr := fmt.Errorf("accumulating resources from '%s': %w", url404, httpErr)
+	expectedErr := fmt.Errorf("accumulating resources: %w", accuFromErr)


So the error we will get is accumulating resources: accumulating resources from 'https://github.com/thisisa404.yaml': HTTP error: status code 404 (not found). That's a pretty good description, probably much more helpful than whatever we were outputting before.

natasha41575 · 2021-12-31T00:08:20Z

/lgtm
/approve

k8s-ci-robot · 2021-12-31T00:08:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575, sylr

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:

~~OWNERS~~ [natasha41575]

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 added 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 Dec 8, 2021

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 8, 2021

k8s-ci-robot requested review from KnVerey and natasha41575 December 8, 2021 13:33

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 Dec 8, 2021

Do not try to load HTTP resources from FS when error occurs

e65e571

It is useless and it clogs the error message. Signed-off-by: Sylvain Rabot <[email protected]>

natasha41575 reviewed Dec 9, 2021

View reviewed changes

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

natasha41575 reviewed Dec 9, 2021

View reviewed changes

api/loader/fileloader.go Outdated Show resolved Hide resolved

sylr added 2 commits December 10, 2021 09:27

Error on HTTP resources are not nescessarly protocol related

738573b

Signed-off-by: Sylvain Rabot <[email protected]>

Test HTTP Error thrown by the file loader

16495c6

Signed-off-by: Sylvain Rabot <[email protected]>

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 10, 2021

natasha41575 reviewed Dec 10, 2021

View reviewed changes

k8s-ci-robot assigned natasha41575 Dec 31, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 31, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 31, 2021

k8s-ci-robot merged commit 59c8268 into kubernetes-sigs:master Dec 31, 2021

thatsmydoing mentioned this pull request Feb 5, 2022

Update supported URLs in remoteBuild.md #4030

Merged

natasha41575 mentioned this pull request Feb 7, 2022

Kustomize build fails pulling remote resource on GitHub with 404 #4455

Closed

natasha41575 mentioned this pull request Apr 18, 2022

Kustomize 4.5.3+ breaks remote resources #4559

Closed

robbiemcmichael mentioned this pull request Nov 23, 2023

Consider restoring the git:: prefix for remote resources #5462

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle HTTP error codes in file loader #4334

Handle HTTP error codes in file loader #4334

sylr commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

KnVerey commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

natasha41575 Dec 10, 2021

sylr Dec 10, 2021 •

edited

Loading

natasha41575 Dec 10, 2021 •

edited

Loading

sylr Dec 10, 2021

natasha41575 Dec 10, 2021

thatsmydoing Feb 5, 2022

sylr Feb 5, 2022 •

edited

Loading

natasha41575 Feb 5, 2022

thatsmydoing Feb 5, 2022

sylr Feb 5, 2022

natasha41575 Dec 10, 2021 •

edited

Loading

natasha41575 commented Dec 31, 2021

k8s-ci-robot commented Dec 31, 2021

Handle HTTP error codes in file loader #4334

Handle HTTP error codes in file loader #4334

Conversation

sylr commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

KnVerey commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

natasha41575 Dec 10, 2021

Choose a reason for hiding this comment

sylr Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

natasha41575 Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

sylr Dec 10, 2021

Choose a reason for hiding this comment

natasha41575 Dec 10, 2021

Choose a reason for hiding this comment

thatsmydoing Feb 5, 2022

Choose a reason for hiding this comment

sylr Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

natasha41575 Feb 5, 2022

Choose a reason for hiding this comment

thatsmydoing Feb 5, 2022

Choose a reason for hiding this comment

sylr Feb 5, 2022

Choose a reason for hiding this comment

natasha41575 Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

natasha41575 commented Dec 31, 2021

k8s-ci-robot commented Dec 31, 2021

sylr Dec 10, 2021 •

edited

Loading

natasha41575 Dec 10, 2021 •

edited

Loading

sylr Feb 5, 2022 •

edited

Loading

natasha41575 Dec 10, 2021 •

edited

Loading