diff --git a/internal/worker/clienterrors/errors.go b/internal/worker/clienterrors/errors.go deleted file mode 100644 index 1e5cd4a372..0000000000 --- a/internal/worker/clienterrors/errors.go +++ /dev/null @@ -1,136 +0,0 @@ -package clienterrors - -import ( - "fmt" -) - -const ( - ErrorNoDynamicArgs ClientErrorCode = 1 - ErrorInvalidTargetConfig ClientErrorCode = 2 - ErrorSharingTarget ClientErrorCode = 3 - ErrorInvalidTarget ClientErrorCode = 4 - ErrorDepsolveDependency ClientErrorCode = 5 - ErrorReadingJobStatus ClientErrorCode = 6 - ErrorParsingDynamicArgs ClientErrorCode = 7 - ErrorManifestGeneration ClientErrorCode = 8 - ErrorManifestDependency ClientErrorCode = 9 - ErrorBuildJob ClientErrorCode = 10 - ErrorUploadingImage ClientErrorCode = 11 - ErrorImportingImage ClientErrorCode = 12 - ErrorKojiFailedDependency ClientErrorCode = 13 - ErrorKojiBuild ClientErrorCode = 14 - ErrorKojiInit ClientErrorCode = 15 - ErrorKojiFinalize ClientErrorCode = 16 - ErrorInvalidConfig ClientErrorCode = 17 - ErrorOldResultCompatible ClientErrorCode = 18 - ErrorEmptyManifest ClientErrorCode = 19 - ErrorDNFDepsolveError ClientErrorCode = 20 - ErrorDNFMarkingErrors ClientErrorCode = 21 - ErrorDNFOtherError ClientErrorCode = 22 - ErrorRPMMDError ClientErrorCode = 23 - ErrorEmptyPackageSpecs ClientErrorCode = 24 - ErrorDNFRepoError ClientErrorCode = 25 - ErrorJobDependency ClientErrorCode = 26 - ErrorJobMissingHeartbeat ClientErrorCode = 27 - ErrorTargetError ClientErrorCode = 28 - ErrorParsingJobArgs ClientErrorCode = 29 - ErrorContainerResolution ClientErrorCode = 30 - ErrorContainerDependency ClientErrorCode = 31 - ErrorOSTreeRefInvalid ClientErrorCode = 32 - ErrorOSTreeRefResolution ClientErrorCode = 33 - ErrorOSTreeParamsInvalid ClientErrorCode = 34 - ErrorOSTreeDependency ClientErrorCode = 35 - ErrorRemoteFileResolution ClientErrorCode = 36 -) - -type ClientErrorCode int - -type Error struct { - ID ClientErrorCode `json:"id"` - Reason string `json:"reason"` - Details interface{} `json:"details,omitempty"` -} - -func (e *Error) String() string { - return fmt.Sprintf("Code: %d, Reason: %s, Details: %v", e.ID, e.Reason, e.Details) -} - -const ( - JobStatusSuccess = "2xx" - JobStatusUserInputError = "4xx" - JobStatusInternalError = "5xx" -) - -type StatusCode string - -func (s *StatusCode) ToString() string { - return string(*s) -} - -func GetStatusCode(err *Error) StatusCode { - if err == nil { - return JobStatusSuccess - } - switch err.ID { - case ErrorDNFDepsolveError: - return JobStatusUserInputError - case ErrorDNFMarkingErrors: - return JobStatusUserInputError - case ErrorDNFRepoError: - return JobStatusInternalError - case ErrorNoDynamicArgs: - return JobStatusUserInputError - case ErrorInvalidTargetConfig: - return JobStatusUserInputError - case ErrorSharingTarget: - return JobStatusUserInputError - case ErrorInvalidTarget: - return JobStatusUserInputError - case ErrorTargetError: - return JobStatusUserInputError - case ErrorDepsolveDependency: - return JobStatusUserInputError - case ErrorManifestDependency: - return JobStatusUserInputError - case ErrorJobDependency: - return JobStatusUserInputError - case ErrorEmptyPackageSpecs: - return JobStatusUserInputError - case ErrorEmptyManifest: - return JobStatusUserInputError - case ErrorContainerResolution: - return JobStatusUserInputError - case ErrorOSTreeDependency: - return JobStatusUserInputError - default: - return JobStatusInternalError - } -} - -// IsDependencyError returns true if the error means that a dependency of a job failed -func (e *Error) IsDependencyError() bool { - switch e.ID { - case ErrorContainerDependency: - return true - case ErrorOSTreeDependency: - return true - case ErrorDepsolveDependency: - return true - case ErrorManifestDependency: - return true - case ErrorKojiFailedDependency: - return true - case ErrorJobDependency: - return true - default: - return false - } -} - -func WorkerClientError(code ClientErrorCode, reason string, details interface{}) *Error { - return &Error{ - ID: code, - Reason: reason, - Details: details, - } -} diff --git a/pkg/customizations/remotefile/resolver.go b/pkg/customizations/remotefile/resolver.go deleted file mode 100644 index 2c73390f19..0000000000 --- a/pkg/customizations/remotefile/resolver.go +++ /dev/null @@ -1,66 +0,0 @@ -package remotefile - -import ( - "context" - - "github.com/osbuild/images/internal/worker/clienterrors" -) - -type resolveResult struct { - url string - content []byte - err error -} - -// TODO: could make this more generic -// since this is shared with the container -// resolver -type Resolver struct { - jobs int - queue chan resolveResult - - ctx context.Context -} - -func NewResolver() *Resolver { - return &Resolver{ - ctx: context.Background(), - queue: make(chan resolveResult, 2), - } -} - -func (r *Resolver) Add(url string) { - client := NewClient() - r.jobs += 1 - - go func() { - content, err := client.Resolve(url) - r.queue <- resolveResult{url: url, content: content, err: err} - }() -} - -func (r *Resolver) Finish() []Spec { - - resultItems := make([]Spec, 0, r.jobs) - for r.jobs > 0 { - result := <-r.queue - r.jobs -= 1 - - var resultError *clienterrors.Error - if result.err != nil { - resultError = clienterrors.WorkerClientError( - clienterrors.ErrorRemoteFileResolution, - result.err.Error(), - result.url, - ) - } - - resultItems = append(resultItems, Spec{ - URL: result.url, - Content: result.content, - ResolutionError: resultError, - }) - } - - return resultItems -} diff --git a/pkg/customizations/remotefile/spec.go b/pkg/customizations/remotefile/spec.go deleted file mode 100644 index 5e1cecbfa3..0000000000 --- a/pkg/customizations/remotefile/spec.go +++ /dev/null @@ -1,9 +0,0 @@ -package remotefile - -import "github.com/osbuild/images/internal/worker/clienterrors" - -type Spec struct { - URL string - Content []byte - ResolutionError *clienterrors.Error -} diff --git a/pkg/customizations/remotefile/client.go b/pkg/remotefile/client.go similarity index 68% rename from pkg/customizations/remotefile/client.go rename to pkg/remotefile/client.go index 0378822c77..62100ac7aa 100644 --- a/pkg/customizations/remotefile/client.go +++ b/pkg/remotefile/client.go @@ -1,6 +1,7 @@ package remotefile import ( + "context" "fmt" "io" "net/http" @@ -17,8 +18,8 @@ func NewClient() *Client { } } -func (c *Client) makeRequest(u *url.URL) ([]byte, error) { - req, err := http.NewRequest("GET", u.String(), nil) +func (c *Client) makeRequest(ctx context.Context, u *url.URL) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) if err != nil { return nil, err } @@ -27,8 +28,12 @@ func (c *Client) makeRequest(u *url.URL) ([]byte, error) { if err != nil { return nil, err } - defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("unexpected status %d: %s", resp.StatusCode, u.String()) + } + output, err := io.ReadAll(resp.Body) if err != nil { return nil, err @@ -50,11 +55,11 @@ func (c *Client) validateURL(u string) (*url.URL, error) { // resolve and return the contents of a remote file // which can be used later, in the pipeline -func (c *Client) Resolve(u string) ([]byte, error) { +func (c *Client) Resolve(ctx context.Context, u string) ([]byte, error) { parsedURL, err := c.validateURL(u) if err != nil { return nil, err } - return c.makeRequest(parsedURL) + return c.makeRequest(ctx, parsedURL) } diff --git a/pkg/customizations/remotefile/client_test.go b/pkg/remotefile/client_test.go similarity index 76% rename from pkg/customizations/remotefile/client_test.go rename to pkg/remotefile/client_test.go index 6d40e97ae7..a09470e83a 100644 --- a/pkg/customizations/remotefile/client_test.go +++ b/pkg/remotefile/client_test.go @@ -1,6 +1,7 @@ package remotefile import ( + "context" "fmt" "net/http" "net/http/httptest" @@ -19,6 +20,9 @@ func makeTestServer() *httptest.Server { if r.URL.Path == "/key2" { fmt.Fprintln(w, "key2") } + if r.URL.Path == "/notfound" { + w.WriteHeader(http.StatusNotFound) + } })) } @@ -29,7 +33,7 @@ func TestClientResolve(t *testing.T) { client := NewClient() - output, err := client.Resolve(url) + output, err := client.Resolve(context.Background(), url) assert.NoError(t, err) expectedOutput := "key1\n" @@ -37,6 +41,15 @@ func TestClientResolve(t *testing.T) { assert.Equal(t, expectedOutput, string(output)) } +func TestClientResolveNonOKStatus(t *testing.T) { + server := makeTestServer() + + client := NewClient() + _, err := client.Resolve(context.Background(), server.URL+"/notfound") + assert.Error(t, err) + assert.Contains(t, err.Error(), "unexpected status 404") +} + func TestInputSpecValidation(t *testing.T) { server := makeTestServer() diff --git a/pkg/remotefile/resolver.go b/pkg/remotefile/resolver.go new file mode 100644 index 0000000000..782128beb6 --- /dev/null +++ b/pkg/remotefile/resolver.go @@ -0,0 +1,72 @@ +package remotefile + +import ( + "context" + "fmt" + "slices" + "sort" + "strings" + "sync" +) + +type resolveResult struct { + url string + content []byte + err error +} + +// TODO: could make this more generic since this is shared with the container +// resolver +type Resolver struct { + queue chan resolveResult + wg sync.WaitGroup + ctx context.Context +} + +func NewResolver(ctx context.Context) *Resolver { + return &Resolver{ + queue: make(chan resolveResult), + wg: sync.WaitGroup{}, + ctx: ctx, + } +} + +// Add a URL to the resolver queue. When called after Finish was called, +// it may panic. +func (r *Resolver) Add(url string) { + r.wg.Add(1) + client := NewClient() + + go func() { + defer r.wg.Done() + + content, err := client.Resolve(r.ctx, url) + r.queue <- resolveResult{url: url, content: content, err: err} + }() +} + +// Finish starts collecting of results and returns them. No further calls to Add +// are allowed after this call. It blocks until all results are collected. +func (r *Resolver) Finish() ([]Spec, error) { + go func() { + r.wg.Wait() + close(r.queue) + }() + + var resultItems []Spec + var errs []string + for result := range r.queue { + if result.err == nil { + resultItems = append(resultItems, Spec{URL: result.url, Content: result.content}) + } else { + errs = append(errs, result.err.Error()) + } + } + + if len(errs) > 0 { + sort.Strings(errs) + return resultItems, fmt.Errorf("failed to resolve remote files: %s", strings.Join(slices.Compact(errs), "; ")) + } + + return resultItems, nil +} diff --git a/pkg/customizations/remotefile/resolver_test.go b/pkg/remotefile/resolver_test.go similarity index 52% rename from pkg/customizations/remotefile/resolver_test.go rename to pkg/remotefile/resolver_test.go index 4c8371ef23..32e261b4e2 100644 --- a/pkg/customizations/remotefile/resolver_test.go +++ b/pkg/remotefile/resolver_test.go @@ -1,6 +1,7 @@ package remotefile import ( + "context" "fmt" "testing" @@ -11,22 +12,18 @@ func TestSingleInputResolver(t *testing.T) { server := makeTestServer() url := server.URL + "/key1" - resolver := NewResolver() + resolver := NewResolver(context.Background()) expectedOutput := Spec{ - URL: url, - Content: []byte("key1\n"), - ResolutionError: nil, + URL: url, + Content: []byte("key1\n"), } resolver.Add(url) - resultItems := resolver.Finish() + resultItems, err := resolver.Finish() + assert.NoError(t, err) assert.Contains(t, resultItems, expectedOutput) - - for _, item := range resultItems { - assert.Nil(t, item.ResolutionError) - } } func TestMultiInputResolver(t *testing.T) { @@ -36,53 +33,47 @@ func TestMultiInputResolver(t *testing.T) { urlTwo := server.URL + "/key2" expectedOutputOne := Spec{ - URL: urlOne, - Content: []byte("key1\n"), - ResolutionError: nil, + URL: urlOne, + Content: []byte("key1\n"), } expectedOutputTwo := Spec{ - URL: urlTwo, - Content: []byte("key2\n"), - ResolutionError: nil, + URL: urlTwo, + Content: []byte("key2\n"), } - resolver := NewResolver() + resolver := NewResolver(context.Background()) resolver.Add(urlOne) resolver.Add(urlTwo) - resultItems := resolver.Finish() + resultItems, err := resolver.Finish() + assert.NoError(t, err) assert.Contains(t, resultItems, expectedOutputOne) assert.Contains(t, resultItems, expectedOutputTwo) - - for _, item := range resultItems { - assert.Nil(t, item.ResolutionError) - } } func TestInvalidInputResolver(t *testing.T) { url := "" - resolver := NewResolver() + resolver := NewResolver(context.Background()) resolver.Add(url) expectedErr := fmt.Errorf("File resolver: url is required") - resultItems := resolver.Finish() - - for _, item := range resultItems { - assert.Equal(t, item.ResolutionError.Reason, expectedErr.Error()) - } + resultItems, err := resolver.Finish() + assert.Error(t, err) + assert.Equal(t, fmt.Errorf("failed to resolve remote files: %s", expectedErr.Error()), err) + assert.Len(t, resultItems, 0) } func TestMultiInvalidInputResolver(t *testing.T) { urlOne := "" urlTwo := "hello" - resolver := NewResolver() + resolver := NewResolver(context.Background()) resolver.Add(urlOne) resolver.Add(urlTwo) @@ -90,13 +81,11 @@ func TestMultiInvalidInputResolver(t *testing.T) { expectedErrMessageOne := "File resolver: url is required" expectedErrMessageTwo := fmt.Sprintf("File resolver: invalid url %s", urlTwo) - resultItems := resolver.Finish() - - errs := []string{} - for _, item := range resultItems { - errs = append(errs, item.ResolutionError.Reason) - } + resultItems, err := resolver.Finish() + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve remote files:") + assert.Contains(t, err.Error(), expectedErrMessageOne) + assert.Contains(t, err.Error(), expectedErrMessageTwo) - assert.Contains(t, errs, expectedErrMessageOne) - assert.Contains(t, errs, expectedErrMessageTwo) + assert.Len(t, resultItems, 0) } diff --git a/pkg/remotefile/spec.go b/pkg/remotefile/spec.go new file mode 100644 index 0000000000..7aa5f39bdf --- /dev/null +++ b/pkg/remotefile/spec.go @@ -0,0 +1,6 @@ +package remotefile + +type Spec struct { + URL string + Content []byte +}