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.

Sign up for GitHub

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

Handle HTTP error codes in file loader #4334

Merged
merged 4 commits into from
Dec 31, 2021
Merged

Handle HTTP error codes in file loader #4334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49b464f
Handle HTTP error codes in file loader
sylr Dec 8, 2021
e65e571
Do not try to load HTTP resources from FS when error occurs
sylr Dec 8, 2021
738573b
Error on HTTP resources are not nescessarly protocol related
sylr Dec 10, 2021
16495c6
Test HTTP Error thrown by the file loader
sylr Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/internal/target/kusttarget.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"sigs.k8s.io/kustomize/api/internal/plugins/loader"
"sigs.k8s.io/kustomize/api/internal/utils"
"sigs.k8s.io/kustomize/api/konfig"
load "sigs.k8s.io/kustomize/api/loader"
"sigs.k8s.io/kustomize/api/resmap"
"sigs.k8s.io/kustomize/api/resource"
"sigs.k8s.io/kustomize/api/types"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sylr sylr Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@natasha41575 natasha41575 Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Try to load the resource as a file.
  2. If that fails, then regardless of the reason for the error, try to load it as a directory.
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sylr sylr Feb 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
ldr, err := kt.ldr.New(path)
if err != nil {
return nil, errors.Wrapf(
Expand Down
28 changes: 28 additions & 0 deletions api/krusty/remoteload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@
package krusty_test

import (
"fmt"
"net/http"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"sigs.k8s.io/kustomize/api/internal/utils"
"sigs.k8s.io/kustomize/api/krusty"
"sigs.k8s.io/kustomize/api/loader"
"sigs.k8s.io/kustomize/kyaml/filesys"
)

Expand Down Expand Up @@ -74,6 +77,31 @@ spec:
assert.NoError(t, fSys.RemoveAll(tmpDir.String()))
}

func TestRemoteResourceWithHTTPError(t *testing.T) {
fSys := filesys.MakeFsOnDisk()
b := krusty.MakeKustomizer(krusty.MakeDefaultOptions())
tmpDir, err := filesys.NewTmpConfirmedDir()
assert.NoError(t, err)

url404 := "https://github.com/thisisa404.yaml"
kusto := filepath.Join(tmpDir.String(), "kustomization.yaml")
assert.NoError(t, fSys.WriteFile(kusto, []byte(fmt.Sprintf(`
resources:
- %s
`, url404))))

_, err = b.Run(fSys, tmpDir.String())
if utils.IsErrTimeout(err) {
// Don't fail on timeouts.
t.SkipNow()
}

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)
Copy link
Contributor

@natasha41575 natasha41575 Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert.EqualErrorf(t, err, expectedErr.Error(), url404)
}

func TestRemoteResourceAnnoOrigin(t *testing.T) {
fSys := filesys.MakeFsOnDisk()
b := krusty.MakeKustomizer(krusty.MakeDefaultOptions())
Expand Down
5 changes: 5 additions & 0 deletions api/loader/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package loader

import "fmt"

var ErrorHTTP = fmt.Errorf("HTTP Error")
3 changes: 3 additions & 0 deletions api/loader/fileloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,9 @@ func (fl *fileLoader) Load(path string) ([]byte, error) {
return nil, err
}
defer resp.Body.Close()
if resp.StatusCode < 200 || resp.StatusCode > 299 {
return nil, fmt.Errorf("%w: status code %d (%s)", ErrorHTTP, resp.StatusCode, http.StatusText(resp.StatusCode))
}
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return nil, err
Expand Down