From 16d6e5fa0d3c1ad12327dd7e36c1aa3dda7787f0 Mon Sep 17 00:00:00 2001 From: Pavlos Tzianos Date: Fri, 12 Apr 2024 23:24:11 +0100 Subject: [PATCH] Make HTTPFileLoader more intelligent (Fixes #128) Splits away the Loader and its implementations into the pkg/files module and adds more logic to the HTTPFileLoader so that it can append the target file to the URL if it's missing. Tests are also added to make sure that the HTTPFileLoader is 100% covered now. A new interface and a mock implementation has been added for HTTP requests. --- cmd/draconctl/pipelines/build.go | 10 +- pkg/files/httpfileloader.go | 84 ++++++++++++++++ pkg/files/httpfileloader_test.go | 105 ++++++++++++++++++++ pkg/files/loader.go | 32 ++++++ pkg/{manifests => files}/localfileloader.go | 19 ++-- pkg/files/mock/mock.go | 29 ++++++ pkg/{manifests => files}/utils.go | 2 +- pkg/http/mock/http.go | 17 ++++ pkg/manifests/decoder.go | 16 +-- pkg/manifests/httpfileloader.go | 47 --------- pkg/manifests/loader.go | 21 ---- pkg/manifests/mock.go | 24 ----- pkg/manifests/tekton.go | 14 +-- pkg/utils/http.go | 9 ++ 14 files changed, 308 insertions(+), 121 deletions(-) create mode 100644 pkg/files/httpfileloader.go create mode 100644 pkg/files/httpfileloader_test.go create mode 100644 pkg/files/loader.go rename pkg/{manifests => files}/localfileloader.go (51%) create mode 100644 pkg/files/mock/mock.go rename pkg/{manifests => files}/utils.go (89%) create mode 100644 pkg/http/mock/http.go delete mode 100644 pkg/manifests/httpfileloader.go delete mode 100644 pkg/manifests/loader.go delete mode 100644 pkg/manifests/mock.go create mode 100644 pkg/utils/http.go diff --git a/cmd/draconctl/pipelines/build.go b/cmd/draconctl/pipelines/build.go index 9280a6f35..85de08d76 100644 --- a/cmd/draconctl/pipelines/build.go +++ b/cmd/draconctl/pipelines/build.go @@ -6,10 +6,12 @@ import ( "path" "strings" + "github.com/spf13/cobra" + kustomizetypes "sigs.k8s.io/kustomize/api/types" + + "github.com/ocurity/dracon/pkg/files" "github.com/ocurity/dracon/pkg/manifests" "github.com/ocurity/dracon/pkg/pipelines" - "github.com/spf13/cobra" - kustomizeType "sigs.k8s.io/kustomize/api/types" ) var buildSubCmd = &cobra.Command{ @@ -32,7 +34,7 @@ func buildPipeline(cmd *cobra.Command, args []string) error { } kustomizationPath := args[0] - kustomizationLoader, err := manifests.NewLoader(".", kustomizationPath, "kustomization.yaml") + kustomizationLoader, err := files.NewLoader(".", kustomizationPath, "kustomization.yaml") if err != nil { return fmt.Errorf("%s: could not read contents of file: %w", kustomizationPath, err) } @@ -44,7 +46,7 @@ func buildPipeline(cmd *cobra.Command, args []string) error { } // Parse Pipeline kustomization - kustomization := &kustomizeType.Kustomization{} + kustomization := &kustomizetypes.Kustomization{} if err = kustomization.Unmarshal(fileContents); err != nil { return fmt.Errorf("%s: could not unmarshal YAML file: %w", kustomizationPath, err) } diff --git a/pkg/files/httpfileloader.go b/pkg/files/httpfileloader.go new file mode 100644 index 000000000..5325cf070 --- /dev/null +++ b/pkg/files/httpfileloader.go @@ -0,0 +1,84 @@ +package files + +import ( + "context" + "io" + "net/http" + "net/url" + "path/filepath" + "strings" + + "github.com/go-errors/errors" + + "github.com/ocurity/dracon/pkg/utils" +) + +var ( + _ Loader = (*httpFileLoader)(nil) + // ErrInvalidURL is returns when the URL of the remote file is not a URL + // with an HTTPS scheme. + ErrInvalidURL = errors.New("invalid url") + // ErrUnsuccessfulRequest is returned when the + ErrUnsuccessfulRequest = errors.New("unsuccessful request") +) + +type httpFileLoader struct { + uri string + client utils.MockableRequestDoer +} + +func newHTTPFileLoader(uri, targetFile string) (*httpFileLoader, error) { + fileURL, err := url.Parse(uri) + if err != nil { + return nil, errors.Errorf("%s: invalid URL: %w", uri, err) + } else if fileURL.Scheme != "https" { + return nil, errors.Errorf("%s: %w", uri, ErrInvalidURL) + } + + if !strings.HasSuffix(fileURL.Path, targetFile) { + fileURL.Path = filepath.Join(fileURL.Path, targetFile) + } + return &httpFileLoader{uri: fileURL.String()}, nil +} + +// Load will read a file using HTTP or HTTPS +func (hfl *httpFileLoader) Load(ctx context.Context) (content []byte, err error) { + if hfl.client == nil { + hfl.client = &http.Client{} + } + + req, err := http.NewRequestWithContext(ctx, "GET", hfl.uri, nil) + if err != nil { + return nil, errors.Errorf("could not create a request: %w", err) + } + + resp, err := hfl.client.Do(req) + if err != nil { + return nil, errors.Errorf("%s: could not create request: %w", hfl.uri, err) + } + + defer func() { + closeErr := resp.Body.Close() + if closeErr != nil && err != nil { + err = errors.Errorf("%w: %w", closeErr, err) + } else if closeErr != nil && err == nil { + err = closeErr + } + }() + + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return nil, errors.Errorf("%s: %d: %w", hfl.uri, resp.StatusCode, ErrUnsuccessfulRequest) + } + + content, err = io.ReadAll(resp.Body) + if err != nil { + return nil, errors.Errorf("%s: could not read body of response: %w", hfl.uri, err) + } + + return content, nil +} + +// Path retuns the URL of the file +func (hfl *httpFileLoader) Path() string { + return hfl.uri +} diff --git a/pkg/files/httpfileloader_test.go b/pkg/files/httpfileloader_test.go new file mode 100644 index 000000000..4724f2108 --- /dev/null +++ b/pkg/files/httpfileloader_test.go @@ -0,0 +1,105 @@ +package files + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ocurity/dracon/pkg/http/mock" + "github.com/ocurity/dracon/pkg/utils" +) + +func TestHTTPFileLoaderInit(t *testing.T) { + _, err := newHTTPFileLoader("-", "kustomization.yaml") + require.ErrorIs(t, err, ErrInvalidURL) + + fl, err := newHTTPFileLoader("https://github.com/ocurity/dracon/pkg", "kustomization.yaml") + require.NoError(t, err) + require.Equal(t, "https://github.com/ocurity/dracon/pkg/kustomization.yaml", fl.Path()) + + fl, err = newHTTPFileLoader("https://github.com/ocurity/dracon/pkg/kustomization.yaml", "kustomization.yaml") + require.NoError(t, err) + require.Equal(t, "https://github.com/ocurity/dracon/pkg/kustomization.yaml", fl.Path()) +} + +func TestHTTPFileLoaderLoad(t *testing.T) { + testCases := []struct { + name string + url string + targetFile string + mockRequestDoer utils.MockableRequestDoer + expectedURL string + expectedErr error + }{ + { + name: "success", + url: "https://github.com/ocurity/dracon/pkg", + targetFile: "kustomization.yaml", + mockRequestDoer: &mock.HTTPReqDoer{ + Hook: func(req *http.Request) (*http.Response, error) { + require.Equal(t, "https://github.com/ocurity/dracon/pkg/kustomization.yaml", req.URL.String()) + + recorder := httptest.NewRecorder() + recorder.Code = http.StatusOK + _, err := recorder.WriteString(`--- +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +nameSuffix: -golang-project +resources: + - ../../../components/base/pipeline.yaml + - ../../../components/base/task.yaml +components: + - ../../../components/sources/git`) + require.NoError(t, err) + return recorder.Result(), nil + }, + }, + }, + { + name: "404", + url: "https://github.com/ocurity/dracon/pkg", + targetFile: "kustomization.yaml", + mockRequestDoer: &mock.HTTPReqDoer{ + Hook: func(req *http.Request) (*http.Response, error) { + require.Equal(t, "https://github.com/ocurity/dracon/pkg/kustomization.yaml", req.URL.String()) + + recorder := httptest.NewRecorder() + recorder.Code = http.StatusNotFound + return recorder.Result(), nil + }, + }, + expectedURL: "https://github.com/ocurity/dracon/pkg/kustomization.yaml", + expectedErr: ErrUnsuccessfulRequest, + }, + } + + testCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + fl, err := newHTTPFileLoader("https://github.com/ocurity/dracon/pkg", "kustomization.yaml") + require.NoError(t, err) + + runCtx, cancel := context.WithCancel(testCtx) + defer cancel() + + fl.client = testCase.mockRequestDoer + _, err = fl.Load(runCtx) + require.ErrorIs(t, err, testCase.expectedErr) + }) + } +} + +func TestCancelledContext(t *testing.T) { + fl, err := newHTTPFileLoader("https://github.com/ocurity/dracon/pkg", "kustomization.yaml") + require.NoError(t, err) + + runCtx, cancel := context.WithCancel(context.Background()) + cancel() + _, err = fl.Load(runCtx) + require.ErrorIs(t, err, context.Canceled) +} diff --git a/pkg/files/loader.go b/pkg/files/loader.go new file mode 100644 index 000000000..0286b6724 --- /dev/null +++ b/pkg/files/loader.go @@ -0,0 +1,32 @@ +package files + +import ( + "context" + "strings" + + "github.com/go-errors/errors" +) + +type Loader interface { + // Root returns the root location for this Loader. + Path() string + + // Load returns the bytes read from the location or an error. + Load(ctx context.Context) ([]byte, error) +} + +var ErrBadTargetFile = errors.New("targetFile should a filename not a directory") + +// NewLoader takes as an argumet a directory containing some configuration, a +// path or a URI and the expected file name that we want to fetch. If the +func NewLoader(configurationDir, pathOrURI, targetFile string) (Loader, error) { + if strings.Contains(targetFile, "/") { + return nil, errors.Errorf("%s: %w", targetFile, ErrBadTargetFile) + } + + if IsRemoteFile(pathOrURI) { + return newHTTPFileLoader(pathOrURI, targetFile) + } + + return newLocalFileLoader(configurationDir, pathOrURI, targetFile) +} diff --git a/pkg/manifests/localfileloader.go b/pkg/files/localfileloader.go similarity index 51% rename from pkg/manifests/localfileloader.go rename to pkg/files/localfileloader.go index 7acbbd217..94f55a323 100644 --- a/pkg/manifests/localfileloader.go +++ b/pkg/files/localfileloader.go @@ -1,10 +1,11 @@ -package manifests +package files import ( "context" "fmt" "os" "path" + "path/filepath" ) var _ Loader = (*localFileLoader)(nil) @@ -13,9 +14,9 @@ type localFileLoader struct { path string } -func newLocalFileLoader(root, fileOrDir, targetFile string) (*localFileLoader, error) { - if !path.IsAbs(fileOrDir) { - fileOrDir = path.Join(root, fileOrDir) +func newLocalFileLoader(configurationDir, fileOrDir, targetFile string) (*localFileLoader, error) { + if !filepath.IsAbs(fileOrDir) { + fileOrDir = path.Clean(filepath.Join(configurationDir, fileOrDir)) } info, err := os.Stat(fileOrDir) @@ -25,21 +26,19 @@ func newLocalFileLoader(root, fileOrDir, targetFile string) (*localFileLoader, e if info.IsDir() { fileOrDir = path.Join(fileOrDir, targetFile) - } else { - if path.Base(fileOrDir) != targetFile { - return nil, fmt.Errorf("path %s should be pointing to a %s", fileOrDir, targetFile) - } + } else if path.Base(fileOrDir) != targetFile { + return nil, fmt.Errorf("path %s should be pointing to a %s", fileOrDir, targetFile) } - fileOrDir = path.Clean(fileOrDir) - return &localFileLoader{path: fileOrDir}, nil } +// Path returns the path in the local file system func (lfl *localFileLoader) Path() string { return lfl.path } +// Load reads a file from the local file system func (lfl *localFileLoader) Load(_ context.Context) ([]byte, error) { return os.ReadFile(lfl.path) } diff --git a/pkg/files/mock/mock.go b/pkg/files/mock/mock.go new file mode 100644 index 000000000..d4ec736a5 --- /dev/null +++ b/pkg/files/mock/mock.go @@ -0,0 +1,29 @@ +package manifests + +import ( + "context" + + "github.com/ocurity/dracon/pkg/files" +) + +var _ files.Loader = FakeLoader{} + +type FakeLoader struct { + path string + mockLoad func() ([]byte, error) +} + +func (l FakeLoader) NewFakeLoader(mockPath string, mockLoad func() ([]byte, error)) FakeLoader { + return FakeLoader{ + path: mockPath, + mockLoad: mockLoad, + } +} + +func (f FakeLoader) Load(ctx context.Context) ([]byte, error) { + return f.mockLoad() +} + +func (f FakeLoader) Path() string { + return f.path +} diff --git a/pkg/manifests/utils.go b/pkg/files/utils.go similarity index 89% rename from pkg/manifests/utils.go rename to pkg/files/utils.go index 48ee8a160..181eeecc8 100644 --- a/pkg/manifests/utils.go +++ b/pkg/files/utils.go @@ -1,4 +1,4 @@ -package manifests +package files import ( "net/url" diff --git a/pkg/http/mock/http.go b/pkg/http/mock/http.go new file mode 100644 index 000000000..36794888f --- /dev/null +++ b/pkg/http/mock/http.go @@ -0,0 +1,17 @@ +package mock + +import ( + "net/http" + + "github.com/ocurity/dracon/pkg/utils" +) + +var _ utils.MockableRequestDoer = (*HTTPReqDoer)(nil) + +type HTTPReqDoer struct { + Hook func(*http.Request) (*http.Response, error) +} + +func (m *HTTPReqDoer) Do(r *http.Request) (*http.Response, error) { + return m.Hook(r) +} diff --git a/pkg/manifests/decoder.go b/pkg/manifests/decoder.go index c44bbcfac..8e709868a 100644 --- a/pkg/manifests/decoder.go +++ b/pkg/manifests/decoder.go @@ -4,13 +4,15 @@ import ( "context" "fmt" - tektonV1Beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" batchv1 "k8s.io/api/batch/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" - jsonSerializer "k8s.io/apimachinery/pkg/runtime/serializer/json" + jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/client-go/kubernetes/scheme" + + "github.com/ocurity/dracon/pkg/files" ) var K8sObjDecoder runtime.Decoder @@ -23,18 +25,18 @@ func init() { if err := scheme.AddToScheme(sch); err != nil { panic(err) } - if err := tektonV1Beta1.AddToScheme(sch); err != nil { + if err := tektonv1beta1.AddToScheme(sch); err != nil { panic(err) } CodecFactory = serializer.NewCodecFactory(sch) K8sObjDecoder = CodecFactory.UniversalDeserializer() - serializer := jsonSerializer.NewYAMLSerializer(jsonSerializer.DefaultMetaFactory, scheme.Scheme, scheme.Scheme) - TektonV1Beta1ObjEncoder = CodecFactory.EncoderForVersion(serializer, tektonV1Beta1.SchemeGroupVersion) + serializer := jsonserializer.NewYAMLSerializer(jsonserializer.DefaultMetaFactory, scheme.Scheme, scheme.Scheme) + TektonV1Beta1ObjEncoder = CodecFactory.EncoderForVersion(serializer, tektonv1beta1.SchemeGroupVersion) BatchV1ObjEncoder = CodecFactory.EncoderForVersion(serializer, batchv1.SchemeGroupVersion) } -func LoadK8sManifest(ctx context.Context, root, pathOrURI, targetFile string) (runtime.Object, *schema.GroupVersionKind, error) { - loader, err := NewLoader(root, pathOrURI, targetFile) +func LoadK8sManifest(ctx context.Context, configurationDir, pathOrURI, targetFile string) (runtime.Object, *schema.GroupVersionKind, error) { + loader, err := files.NewLoader(configurationDir, pathOrURI, targetFile) if err != nil { return nil, nil, fmt.Errorf("%s: could not resolve path or URI: %w", pathOrURI, err) } diff --git a/pkg/manifests/httpfileloader.go b/pkg/manifests/httpfileloader.go deleted file mode 100644 index 40a62a5db..000000000 --- a/pkg/manifests/httpfileloader.go +++ /dev/null @@ -1,47 +0,0 @@ -package manifests - -import ( - "context" - "errors" - "fmt" - "io" - "net/http" -) - -var _ Loader = (*httpFileLoader)(nil) - -type httpFileLoader struct { - uri string - client *http.Client -} - -func newHTTPFileLoader(uri string) (*httpFileLoader, error) { - return &httpFileLoader{uri: uri}, nil -} - -func (hfl *httpFileLoader) Load(ctx context.Context) (content []byte, err error) { - if hfl.client == nil { - hfl.client = &http.Client{} - } - resp, err := hfl.client.Get(hfl.uri) - if err != nil { - return nil, fmt.Errorf("%s: could not create request: %w", hfl.uri, err) - } - defer func() { - err = errors.Join(err, resp.Body.Close()) - }() - - if resp.StatusCode < 200 || resp.StatusCode > 299 { - return nil, fmt.Errorf("%s: could not fetch file: %d", hfl.uri, resp.StatusCode) - } - - content, err = io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("%s: could not read body of response: %w", hfl.uri, err) - } - return content, nil -} - -func (hfl *httpFileLoader) Path() string { - return hfl.uri -} diff --git a/pkg/manifests/loader.go b/pkg/manifests/loader.go deleted file mode 100644 index ba5894f38..000000000 --- a/pkg/manifests/loader.go +++ /dev/null @@ -1,21 +0,0 @@ -package manifests - -import ( - "context" -) - -type Loader interface { - // Root returns the root location for this Loader. - Path() string - - // Load returns the bytes read from the location or an error. - Load(ctx context.Context) ([]byte, error) -} - -func NewLoader(root, pathOrURI, targetFile string) (Loader, error) { - if IsRemoteFile(pathOrURI) { - return newHTTPFileLoader(pathOrURI) - } - - return newLocalFileLoader(root, pathOrURI, targetFile) -} diff --git a/pkg/manifests/mock.go b/pkg/manifests/mock.go deleted file mode 100644 index 16c7a6dbb..000000000 --- a/pkg/manifests/mock.go +++ /dev/null @@ -1,24 +0,0 @@ -package manifests - -import "fmt" - -type FakeLoader struct { - path string -} - -func (l FakeLoader) Root() string { - return "" -} - -func (l FakeLoader) New(newRoot string) (Loader, error) { - if newRoot == l.path { - return nil, nil - } - return nil, fmt.Errorf("%s not exist", newRoot) -} -func (l FakeLoader) Load(location string) ([]byte, error) { - return nil, nil -} -func (l FakeLoader) Cleanup() error { - return nil -} diff --git a/pkg/manifests/tekton.go b/pkg/manifests/tekton.go index 7ac53a7aa..de5fd17a3 100644 --- a/pkg/manifests/tekton.go +++ b/pkg/manifests/tekton.go @@ -4,29 +4,29 @@ import ( "context" "fmt" - tektonV1Beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" ) -func LoadTektonV1Beta1Task(ctx context.Context, root, pathOrURI string) (*tektonV1Beta1.Task, error) { - obj, gKV, err := LoadK8sManifest(ctx, root, pathOrURI, "task.yaml") +func LoadTektonV1Beta1Task(ctx context.Context, configurationDir, pathOrURI string) (*tektonv1beta1.Task, error) { + obj, gKV, err := LoadK8sManifest(ctx, configurationDir, pathOrURI, "task.yaml") if err != nil { return nil, err } - task, isATask := obj.(*tektonV1Beta1.Task) + task, isATask := obj.(*tektonv1beta1.Task) if !isATask { return nil, fmt.Errorf("object loaded is not a task: %v", gKV) } return task, nil } -func LoadTektonV1Beta1Pipeline(ctx context.Context, root, pathOrURI string) (*tektonV1Beta1.Pipeline, error) { - obj, gKV, err := LoadK8sManifest(ctx, root, pathOrURI, "pipeline.yaml") +func LoadTektonV1Beta1Pipeline(ctx context.Context, configurationDir, pathOrURI string) (*tektonv1beta1.Pipeline, error) { + obj, gKV, err := LoadK8sManifest(ctx, configurationDir, pathOrURI, "pipeline.yaml") if err != nil { return nil, err } - pipeline, isAPipeline := obj.(*tektonV1Beta1.Pipeline) + pipeline, isAPipeline := obj.(*tektonv1beta1.Pipeline) if !isAPipeline { return nil, fmt.Errorf("object loaded is not a pipeline: %v", gKV) } diff --git a/pkg/utils/http.go b/pkg/utils/http.go new file mode 100644 index 000000000..7484ce92a --- /dev/null +++ b/pkg/utils/http.go @@ -0,0 +1,9 @@ +package utils + +import "net/http" + +// MockableRequestDoer is an interface that can be used wherever we have an +// HTTP client that needs to be mocked to inject custom responses and errors +type MockableRequestDoer interface { + Do(*http.Request) (*http.Response, error) +}