fix: enhance token refresh handling and improve error reporting - #138
Open
alexlovelltroy wants to merge 1 commit into
Open
alexlovelltroy wants to merge 1 commit into
alexlovelltroy wants to merge 1 commit into
Conversation
Signed-off-by: Alex Lovell-Troy <alovelltroy@lanl.gov>
synackd
requested changes
Sep 17, 2026
synackd
left a comment
Contributor
There was a problem hiding this comment.
Looks good, just one concern (with suggested patch) about separate HTTP client for token refresh.
| client := &SMDClient{ | ||
| clusterName: clusterName, | ||
| smdClient: c, | ||
| tokenClient: &http.Client{Timeout: 10 * time.Second}, |
Contributor
There was a problem hiding this comment.
We should probably reuse the SMDClient transport here so TLS can be used for token handling. We can also reuse the timeout value too.
diff --git a/internal/smdclient/SMDclient.go b/internal/smdclient/SMDclient.go
index 82ba48a..64319aa 100644
--- a/internal/smdclient/SMDclient.go
+++ b/internal/smdclient/SMDclient.go
@@ -94,7 +94,7 @@ func NewSMDClient(clusterName, baseurl, jwtURL, accessToken, certPath string, in
if err != nil {
return nil, fmt.Errorf("failed to read cert from path %s: %v", certPath, err)
}
- certPool := x509.NewCertPool()
+ certPool = x509.NewCertPool()
certPool.AppendCertsFromPEM(cacert)
}
@@ -114,9 +114,12 @@ func NewSMDClient(clusterName, baseurl, jwtURL, accessToken, certPath string, in
}
client := &SMDClient{
- clusterName: clusterName,
- smdClient: c,
- tokenClient: &http.Client{Timeout: 10 * time.Second},
+ clusterName: clusterName,
+ smdClient: c,
+ tokenClient: &http.Client{
+ Transport: c.Transport,
+ Timeout: c.Timeout,
+ },
smdBaseURL: baseurl,
tokenEndpoint: jwtURL,
accessToken: accessToken,
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request improves the reliability and test coverage of the SMD client’s access token refresh logic. The most important changes include stricter error handling and validation in token refresh, dedicated HTTP client usage for token requests, and new tests to ensure correct behavior when token refresh fails.
Error Handling and Validation Improvements:
refreshTokenWithContextmethod now returns detailed errors for various failure modes, such as missing HTTP client, non-2xx responses, malformed or empty tokens, and JSON decoding errors. It also trims whitespace from the received token and rejects empty tokens.getSMD, if the token refresh fails after a rejected access token, the error is now returned immediately instead of being silently ignored, providing better visibility and preventing retries with invalid tokens.HTTP Client Usage and Struct Changes:
tokenClientfield toSMDClient, ensuring token refresh requests use a separate HTTP client from regular SMD requests. Constructors and tests are updated to initialize this field appropriately. [1] [2] [3] [4]Testing Improvements:
getSMDreturns a refresh failure error without retrying, and that the access token remains unchanged.Code Consistency:
oidcTokenDatato use Go naming conventions (e.g.,AccessTokeninstead ofAccess_token).Checklist
make test(or equivalent) locally and all tests passgit commit -s) with my real name and email<filename>.licensesidecarLICENSES/directoryType of Change