Skip to content

fix(cli): classify failures and partial de-panic in the client library #489

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

Open
Caesarsage wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(cli): classify failures and partial de-panic in the client library #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccc1242
feat(errors): add Failure Kind classification
Caesarsage Jul 9, 2026
059891f
fix(connectors): return classified errors instead of panicking
Caesarsage Jul 9, 2026
732b4ae
added copyright note
Caesarsage Jul 23, 2026
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
58 changes: 41 additions & 17 deletions pkg/connectors/keycloak_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strings"

"github.com/microcks/microcks-cli/pkg/config"
"github.com/microcks/microcks-cli/pkg/errors"
"golang.org/x/oauth2"
)

Expand All @@ -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)
Comment on lines 52 to 56

@Vaishnav88sk Vaishnav88sk Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same concern here: this keeps a panic path in the client library.

For embeddability, constructors should ideally return errors instead of terminating the host process. If that signature migration has to happen separately, please call that out more explicitly and avoid over-claiming the scope of this PR.

@Caesarsage Caesarsage Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this has been fixed on the next PR

#490

}
kc.BaseURL = u
Expand Down Expand Up @@ -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()

Expand All @@ -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) {
Expand All @@ -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{
Expand All @@ -160,30 +177,37 @@ 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
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")

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
}
67 changes: 41 additions & 26 deletions pkg/connectors/microcks_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
}
Comment on lines 195 to 200

@Vaishnav88sk Vaishnav88sk Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This reintroduces a panic in library code, which conflicts with the stated goal of this PR.

Even if malformed URLs are a narrower case, a library constructor still should not panic here. If we cannot change the constructor signature in this PR, then I’d rather keep this work scoped differently than claim we have removed panics from the client layer.

@Caesarsage Caesarsage Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this has been fixed on the next PR

https://github.com/microcks/microcks-cli/pull/490/changes#diff-bac918769b81eb6fc95691c8e3f0c6131680f522f5b3e4dd1a53be291ffabbdaL52

mc.APIURL = u
Expand Down Expand Up @@ -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()

Expand All @@ -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
}
Comment on lines +257 to +259

@Vaishnav88sk Vaishnav88sk Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should not silently treat a missing / malformed enabled field as “Keycloak disabled”.

If the response shape is wrong, please return an API error instead. Otherwise a bad server/proxy response can be misclassified as a valid “no Keycloak” setup.

@Caesarsage Caesarsage Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Similarly, it was addressed on the #490 PR


// 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 {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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()

Expand All @@ -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{}
Expand All @@ -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()

Expand Down Expand Up @@ -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()

Expand All @@ -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
Expand Down Expand Up @@ -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()

Expand All @@ -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 {
Expand Down
Loading