diff --git a/pkg/connectors/keycloak_client.go b/pkg/connectors/keycloak_client.go index 5d6aeed..f44d567 100644 --- a/pkg/connectors/keycloak_client.go +++ b/pkg/connectors/keycloak_client.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/microcks/microcks-cli/pkg/config" + "github.com/microcks/microcks-cli/pkg/errors" "golang.org/x/oauth2" ) @@ -50,6 +51,8 @@ func NewKeycloakClient(realmURL string, username string, password string) Keyclo u, err := url.Parse(realmURL) if err != nil { + // url.Parse only fails on a malformed URL; returning it needs a + // signature change, done with the RunE command migration. panic(err) } kc.BaseURL = u @@ -88,7 +91,7 @@ func (c *keycloakClient) ConnectAndGetToken() (string, error) { resp, err := c.httpClient.Do(req) if err != nil { - return "", err + return "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() @@ -97,16 +100,23 @@ func (c *keycloakClient) ConnectAndGetToken() (string, error) { body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", errors.Wrap(errors.KindConnection, fmt.Errorf("reading Keycloak token response: %w", err)) + } + + if resp.StatusCode != http.StatusOK { + return "", errors.Wrapf(errors.KindAPI, "Keycloak returned HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return "", errors.Wrap(errors.KindAPI, fmt.Errorf("parsing Keycloak token response: %w", err)) } - accessToken := openIDResp["access_token"].(string) - return accessToken, err + accessToken, ok := openIDResp["access_token"].(string) + if !ok || accessToken == "" { + return "", errors.Wrapf(errors.KindAPI, "Keycloak token response missing access_token") + } + return accessToken, nil } func (c *keycloakClient) GetOIDCConfig() (*oauth2.Config, error) { @@ -116,27 +126,34 @@ func (c *keycloakClient) GetOIDCConfig() (*oauth2.Config, error) { // Create HTTP request req, err := http.NewRequest("GET", u.String(), nil) if err != nil { - fmt.Println("Error creating request:", err) + return nil, errors.Wrap(errors.KindGeneric, fmt.Errorf("creating Keycloak OIDC request: %w", err)) } resp, err := c.httpClient.Do(req) if err != nil { - return nil, err + return nil, errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return nil, errors.Wrap(errors.KindConnection, fmt.Errorf("reading Keycloak OIDC config: %w", err)) + } + + if resp.StatusCode != http.StatusOK { + return nil, errors.Wrapf(errors.KindAPI, "Keycloak returned HTTP %d for OIDC config: %s", resp.StatusCode, strings.TrimSpace(string(body))) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return nil, errors.Wrap(errors.KindAPI, fmt.Errorf("parsing Keycloak OIDC config: %w", err)) } - authURL := openIDResp["authorization_endpoint"].(string) - tokenURL := openIDResp["token_endpoint"].(string) + authURL, _ := openIDResp["authorization_endpoint"].(string) + tokenURL, _ := openIDResp["token_endpoint"].(string) + if authURL == "" || tokenURL == "" { + return nil, errors.Wrapf(errors.KindAPI, "Keycloak OIDC config missing authorization_endpoint or token_endpoint") + } return &oauth2.Config{ Endpoint: oauth2.Endpoint{ @@ -160,7 +177,7 @@ func (c *keycloakClient) ConnectAndGetTokenAndRefreshToken(username, password st // Create HTTP request req, err := http.NewRequest("POST", u.String(), bytes.NewBufferString(data.Encode())) if err != nil { - fmt.Println("Error creating request:", err) + return "", "", errors.Wrap(errors.KindGeneric, fmt.Errorf("creating Keycloak token request: %w", err)) } // Set headers @@ -168,22 +185,29 @@ func (c *keycloakClient) ConnectAndGetTokenAndRefreshToken(username, password st resp, err := c.httpClient.Do(req) if err != nil { - return "", "", err + return "", "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", "", errors.Wrap(errors.KindConnection, fmt.Errorf("reading Keycloak token response: %w", err)) + } + + if resp.StatusCode != http.StatusOK { + return "", "", errors.Wrapf(errors.KindAPI, "Keycloak returned HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return "", "", errors.Wrap(errors.KindAPI, fmt.Errorf("parsing Keycloak token response: %w", err)) } - authToken := openIDResp["access_token"].(string) - refreshToken := openIDResp["refresh_token"].(string) + authToken, _ := openIDResp["access_token"].(string) + refreshToken, _ := openIDResp["refresh_token"].(string) + if authToken == "" || refreshToken == "" { + return "", "", errors.Wrapf(errors.KindAPI, "Keycloak token response missing access_token or refresh_token") + } return authToken, refreshToken, nil } diff --git a/pkg/connectors/microcks_client.go b/pkg/connectors/microcks_client.go index 6021a3c..e4fb2c6 100644 --- a/pkg/connectors/microcks_client.go +++ b/pkg/connectors/microcks_client.go @@ -151,7 +151,7 @@ func NewClient(opts ClientOptions) (MicrocksClient, error) { u, err := url.Parse(apiURL) if err != nil { - panic(err) + return nil, errors.Wrap(errors.KindUsage, fmt.Errorf("invalid server URL %q: %w", apiURL, err)) } c.APIURL = u @@ -194,6 +194,8 @@ func NewMicrocksClient(apiURL string) MicrocksClient { u, err := url.Parse(apiURL) if err != nil { + // url.Parse only fails on a malformed URL; returning it needs a + // signature change, done with the RunE command migration. panic(err) } mc.APIURL = u @@ -230,7 +232,7 @@ func (c *microcksClient) GetKeycloakURL() (string, error) { resp, err := c.httpClient.Do(req) if err != nil { - return "", err + return "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() @@ -239,24 +241,29 @@ func (c *microcksClient) GetKeycloakURL() (string, error) { body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", errors.Wrap(errors.KindConnection, fmt.Errorf("reading Keycloak config response: %w", err)) + } + + if resp.StatusCode != http.StatusOK { + return "", errors.Wrapf(errors.KindAPI, "Microcks returned HTTP %d for Keycloak config: %s", resp.StatusCode, strings.TrimSpace(string(body))) } var configResp map[string]interface{} if err := json.Unmarshal(body, &configResp); err != nil { - panic(err) + return "", errors.Wrap(errors.KindAPI, fmt.Errorf("parsing Keycloak config response: %w", err)) } - // Retrieve auth server url and realm name. - enabled := configResp["enabled"].(bool) - authServerURL := configResp["auth-server-url"].(string) - realmName := configResp["realm"].(string) + // Return 'null' if Keycloak is disabled. + if enabled, _ := configResp["enabled"].(bool); !enabled { + return "null", nil + } - // Return a proper URL or 'null' if Keycloak is disables. - if enabled { - return authServerURL + "/realms/" + realmName + "/", nil + authServerURL, _ := configResp["auth-server-url"].(string) + realmName, _ := configResp["realm"].(string) + if authServerURL == "" || realmName == "" { + return "", errors.Wrapf(errors.KindAPI, "Keycloak config response missing auth-server-url or realm") } - return "null", nil + return authServerURL + "/realms/" + realmName + "/", nil } func (c *microcksClient) refreshAuthToken(localCfg *config.LocalConfig, ctxName, configPath string) error { @@ -304,10 +311,14 @@ func (c *microcksClient) refreshAuthToken(localCfg *config.LocalConfig, ctxName, func (c *microcksClient) redeemRefreshToken(auth config.Auth) (string, string, error) { keyCloakUrl, err := c.GetKeycloakURL() - errors.CheckError(err) + if err != nil { + return "", "", err + } kc := NewKeycloakClient(keyCloakUrl, "", "") oauth2Conf, err := kc.GetOIDCConfig() - errors.CheckError(err) + if err != nil { + return "", "", err + } oauth2Conf.ClientID = auth.ClientId oauth2Conf.ClientSecret = auth.ClientSecret @@ -372,18 +383,22 @@ func (c *microcksClient) CreateTestResult(serviceID string, testEndpoint string, resp, err := c.httpClient.Do(req) if err != nil { - return "", err + return "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { - return "", fmt.Errorf("failed to read response body: %w", err) + return "", errors.Wrap(errors.KindConnection, fmt.Errorf("reading test creation response: %w", err)) } // Check HTTP status before attempting to parse. if resp.StatusCode != 201 { - return "", fmt.Errorf("microcks returned HTTP %d: %s (is the service '%s' registered?)", resp.StatusCode, strings.TrimSpace(string(body)), serviceID) + kind := errors.KindAPI + if resp.StatusCode == http.StatusNotFound { + kind = errors.KindNotFound + } + return "", errors.Wrapf(kind, "Microcks returned HTTP %d: %s (is the service '%s' registered?)", resp.StatusCode, strings.TrimSpace(string(body)), serviceID) } var createTestResp map[string]interface{} @@ -416,7 +431,7 @@ func (c *microcksClient) GetTestResult(testResultID string) (*TestResultSummary, resp, err := c.httpClient.Do(req) if err != nil { - return nil, err + return nil, errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() @@ -425,7 +440,7 @@ func (c *microcksClient) GetTestResult(testResultID string) (*TestResultSummary, body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return nil, errors.Wrap(errors.KindConnection, fmt.Errorf("reading test result response: %w", err)) } result := TestResultSummary{} @@ -440,7 +455,7 @@ func (c *microcksClient) UploadArtifact(specificationFilePath string, mainArtifa // Ensure file exists on fs. file, err := os.Open(specificationFilePath) if err != nil { - return "", err + return "", errors.Wrap(errors.KindUsage, fmt.Errorf("cannot read artifact %q: %w", specificationFilePath, err)) } defer file.Close() @@ -490,7 +505,7 @@ func (c *microcksClient) UploadArtifact(specificationFilePath string, mainArtifa resp, err := c.httpClient.Do(req) if err != nil { - return "", err + return "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() @@ -509,7 +524,7 @@ func (c *microcksClient) UploadArtifact(specificationFilePath string, mainArtifa // Raise exception if not created. if resp.StatusCode != 201 { - return "", errs.New(string(respBody)) + return "", errors.Wrap(errors.KindAPI, errs.New(strings.TrimSpace(string(respBody)))) } return string(respBody), nil @@ -549,7 +564,7 @@ func (c *microcksClient) DownloadArtifact(artifactURL string, mainArtifact bool, resp, err := c.httpClient.Do(req) if err != nil { - return "", err + return "", errors.Wrap(errors.KindConnection, err) } defer resp.Body.Close() @@ -558,15 +573,15 @@ func (c *microcksClient) DownloadArtifact(artifactURL string, mainArtifact bool, respBody, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", errors.Wrap(errors.KindConnection, fmt.Errorf("reading download response: %w", err)) } // Raise exception if not created. if resp.StatusCode != 201 { - return "", errs.New(string(respBody)) + return "", errors.Wrap(errors.KindAPI, errs.New(strings.TrimSpace(string(respBody)))) } - return string(respBody), err + return string(respBody), nil } func ensureValidOperationsList(filteredOperations string) bool { diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 8438c93..6172ef2 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -1,10 +1,16 @@ package errors import ( + stderrors "errors" + "fmt" "log" "os" ) +// Deprecated: these numeric codes and the Check*/Fatal helpers below are the +// legacy exit mechanism. New code classifies failures with a Kind (see Wrap) and +// lets cmd.Handle map Kind -> exit code. Kept as a shim until every call site is +// migrated, then removed. const ( // ErrorCommandSpecific is reserved for command specific indications ErrorCommandSpecific = 1 @@ -18,23 +24,84 @@ const ( ErrorGeneric = 20 ) +// Deprecated: return errors.Wrap(kind, err) from a RunE command instead. func CheckError(err error) { if err != nil { Fatal(ErrorGeneric, err) } } +// Deprecated: return a KindNotFound-wrapped error instead. func CheckConfigNil(isNil bool, path string) { if isNil { Fatal(ErrorGeneric, "No contexts defined in "+path) } } - -// Fatal is a wrapper for log.Fatal() to exit with custom code +// Deprecated: only main/cmd.Handle should exit the process. Fatal is a wrapper +// for log.Fatal() to exit with a custom code. func Fatal(exitcode int, args ...interface{}) { log.Println(args...) os.Exit(exitcode) } +// Kind classifies why an operation failed. The library returns kinds; the cmd +// layer maps them to exit codes, so pkg/* never depends on exit codes and stays +// safe to embed. +type Kind int + +const ( + // KindGeneric is an unclassified failure. It is the zero value, so an + // unwrapped error is treated as generic. + KindGeneric Kind = iota + // KindUsage is a bad argument or flag supplied by the user. + KindUsage + // KindConnection is a failure to reach the Microcks or Keycloak endpoint. + KindConnection + // KindAPI is a server that rejected the request or returned an unusable response. + KindAPI + // KindNotFound is a requested remote resource that does not exist. + KindNotFound + // KindEnvironment is a local precondition not met — container runtime down, + // image unpullable, or ephemeral server not ready. Not KindConnection, which + // is about reaching the Microcks server. + KindEnvironment +) + +// KindError wraps an error with a Failure Kind. +type KindError struct { + Kind Kind + Err error +} + +func (e *KindError) Error() string { return e.Err.Error() } +func (e *KindError) Unwrap() error { return e.Err } + +// Wrap tags err with a Failure Kind. It returns nil when err is nil, so it is +// safe to write `return errors.Wrap(KindAPI, doThing())`. +func Wrap(kind Kind, err error) error { + if err == nil { + return nil + } + return &KindError{Kind: kind, Err: err} +} + +// Wrapf builds a kind-tagged error from a format string. +func Wrapf(kind Kind, format string, a ...any) error { + return &KindError{Kind: kind, Err: fmt.Errorf(format, a...)} +} + +// KindOf reports the Failure Kind carried by err's chain, defaulting to +// KindGeneric when none is present (including when err is nil). +func KindOf(err error) Kind { + var ke *KindError + if stderrors.As(err, &ke) { + return ke.Kind + } + return KindGeneric +} +// ErrTestFailed signals a completed test run whose result does not conform to the +// contract. It is not a failure to *run*: the command has already rendered the +// result, so the CLI exits non-zero without printing this sentinel. See cmd.Handle. +var ErrTestFailed = stderrors.New("contract test failed") diff --git a/pkg/errors/error_test.go b/pkg/errors/error_test.go new file mode 100644 index 0000000..68dd8bd --- /dev/null +++ b/pkg/errors/error_test.go @@ -0,0 +1,41 @@ +package errors + +import ( + stderrors "errors" + "fmt" + "testing" +) + +func TestWrapNilReturnsNil(t *testing.T) { + if Wrap(KindAPI, nil) != nil { + t.Fatal("Wrap of a nil error should return nil") + } +} + +func TestKindOf(t *testing.T) { + cases := []struct { + name string + err error + want Kind + }{ + {"nil", nil, KindGeneric}, + {"plain error", stderrors.New("boom"), KindGeneric}, + {"wrapped", Wrap(KindConnection, stderrors.New("refused")), KindConnection}, + {"wrapped again", fmt.Errorf("outer: %w", Wrap(KindNotFound, stderrors.New("404"))), KindNotFound}, + } + for _, c := range cases { + if got := KindOf(c.err); got != c.want { + t.Errorf("%s: KindOf = %v, want %v", c.name, got, c.want) + } + } +} + +func TestWrapfPreservesKindAndMessage(t *testing.T) { + err := Wrapf(KindEnvironment, "docker unreachable on attempt %d", 2) + if KindOf(err) != KindEnvironment { + t.Errorf("KindOf = %v, want KindEnvironment", KindOf(err)) + } + if got := err.Error(); got != "docker unreachable on attempt 2" { + t.Errorf("Error() = %q", got) + } +} diff --git a/pkg/watcher/watchManager.go b/pkg/watcher/watchManager.go index 5b0f717..6e0167c 100644 --- a/pkg/watcher/watchManager.go +++ b/pkg/watcher/watchManager.go @@ -7,7 +7,6 @@ import ( "github.com/fsnotify/fsnotify" "github.com/microcks/microcks-cli/pkg/config" - "github.com/microcks/microcks-cli/pkg/errors" ) type WatchManager struct { @@ -88,7 +87,9 @@ func (wm *WatchManager) Run() { err := wm.Reload() wm.lock.Unlock() if err != nil { - errors.CheckError(err) + // A bad config edit shouldn't kill the watcher; log and + // keep the previous config until the next valid save. + log.Printf("[ERROR] Config reload failed, keeping previous config: %v", err) } } else { wm.lock.Lock()