Add sentinel errors for HTTP status code handling

[welteki]

Description

Add ErrNotFound, ErrUnauthorized, ErrForbidden, and ErrUnexpectedStatus sentinel errors so callers can use errors.Is to distinguish error cases returned by Client methods.

• Add status code switches to GetNamespaces, GetFunctions, GetFunction, GetInfo, and GetSecrets which previously had no or only partial status code checks
• Update all existing methods to wrap sentinel errors consistently
• Handle 403 Forbidden responses with ErrForbidden across all methods

We chose sentinel errors specifically to avoid diverging from the current method call signatures. Callers can use errors.Is without any changes to return types:

errors.Is(err, sdk.ErrNotFound)
errors.Is(err, sdk.ErrUnauthorized)
errors.Is(err, sdk.ErrForbidden)
errors.Is(err, sdk.ErrUnexpectedStatus)

Breaking change: Error messages returned by Client methods have changed. Code that matches on exact error strings will need to be updated to use errors.Is with the new sentinel errors instead.

Motivation and context

The OpenFaaS dashboard uses this SDK and needed to distinguish between different API error conditions—particularly authorization errors—to handle them differently for display in the UI. Without sentinel errors, callers had to resort to fragile string matching on error messages which made it difficult to reliably detect specific error conditions.

How has this been tested

• Unit tests have been updated to use errors.Is with the new sentinel errors and new test cases for 403 Forbidden have been added across all methods
• The updated SDK was vendored into the OpenFaaS dashboard and used while making the UI changes to verify the new error handling works end to end

[reviewfn]

AI Pull Request Overview

Summary

• Adds sentinel errors (ErrNotFound, ErrUnauthorized, ErrForbidden, ErrUnexpectedStatus) for consistent error handling
• Updates all Client methods to use status code switches instead of partial checks
• Adds 403 Forbidden handling across all methods
• Changes error messages, making this a breaking change for code that matches error strings
• Tests updated to use errors.Is and include 403 Forbidden cases
• Methods like GetNamespaces, GetFunctions, GetInfo, GetSecrets now properly validate HTTP status codes
• GetFunction and GetNamespace methods added 404 and 403 handling where missing

Approval rating (1-10)

8 - Solid implementation with proper error handling improvements, but introduces breaking changes to error messages.

Summary per file

Summary per file

File path	Summary
client.go	Added sentinel errors and updated all methods to use status code switches with consistent error wrapping
client_test.go	Updated tests to expect sentinel errors using errors.Is and added 403 Forbidden test cases

Overall Assessment

The changes significantly improve error handling consistency and reliability by introducing sentinel errors that enable proper error type checking with errors.Is. Methods that previously had incomplete status code validation now correctly handle 401, 403, 404, and unexpected status codes. The addition of 403 Forbidden handling is comprehensive. However, this is a breaking change as error messages have changed, requiring callers to update from string matching to errors.Is checks. The implementation appears correct and well-tested.

Detailed Review

Detailed Review

client.go

• Sentinel Error Definitions: Four new exported sentinel errors are added at package level. This is appropriate for a library where callers need to distinguish error types. The errors are simple errors.New() instances, which is correct for sentinel usage.
• Import Addition: Added "errors" import, which is necessary for the sentinel definitions.
• Error Wrapping: In all updated methods, when response body is present, the sentinel error is wrapped using fmt.Errorf("%w: %s", sentinel, body). This preserves the original error message from the server while allowing errors.Is() to work. When body is empty, returns the sentinel directly.
• Status Code Handling Improvements:
  • Methods like GetNamespaces, GetFunctions, GetInfo, GetSecrets previously had no or minimal status validation (only checking 401 in some cases). Now all implement full switch statements covering 200/OK, 401/Unauthorized, 403/Forbidden, and default cases.
  • This prevents silent failures where non-200 responses were treated as success.
  • GetFunction and GetNamespace now properly handle 404/NotFound, which they previously did not check.
• Breaking Change Risk: All error messages have changed format. Code relying on exact string matching (e.g., strings.Contains(err.Error(), "unauthorized")) will break. The PR description correctly calls this out as a breaking change.
• Consistency: The error handling pattern is now consistent across all methods: read body once, switch on status code, return appropriate sentinel-wrapped error.
• Potential Regression: In GetLogs method, the body reading pattern differs slightly - it reads body inline in each case rather than once at the top. This is inconsistent with other methods but doesn't affect functionality.
• Security/Performance: No issues. The changes don't introduce new dependencies or performance concerns.

client_test.go

• Test Updates: All test cases updated to expect the new sentinel errors directly instead of formatted error strings. The assertion logic simplified from checking both errors.Is() and string equality to just errors.Is(), which is correct.
• 403 Forbidden Coverage: Added test cases for 403 Forbidden status in all relevant test functions (DeployFunction, DeleteFunction, GetNamespace, CreateNamespace, UpdateNamespace, DeleteNamespace, CreateSecret, UpdateSecret, DeleteSecret). This ensures the new 403 handling is tested.
• Test Quality: The tests now properly validate that errors.Is() works as intended, which is the primary benefit of sentinel errors.
• Coverage Gap: No tests added for the new 403 handling in GetFunctions, GetInfo, GetSecrets, or GetFunction methods. While the test structure suggests these methods have basic tests, adding 403 cases would be prudent.

General Concerns

• Migration Impact: Callers must update error handling code. The PR mentions the OpenFaaS dashboard as a consumer that was updated, but other users will need to migrate. Consider providing a migration guide or deprecation period in a future version.
• Error Message Loss: For methods where body is empty, only the sentinel message is returned. Previously, some methods included status code in the message. This might reduce debugging information in edge cases.
• No New Dependencies: Good, no external dependencies added.
• Testing: The changes are well-tested with updated assertions and new test cases. However, integration testing with real servers would be ideal to verify the error wrapping behavior.

Recommendations

• Add 403 Forbidden test cases for GetFunctions, GetInfo, GetSecrets, and GetFunction methods to ensure complete coverage.
• Consider documenting the migration path from string matching to errors.Is() in the changelog or README.
• The GetLogs method should be updated to match the body-reading pattern of other methods for consistency.

AI agent details.

Agent processing time: 29.391s
Environment preparation time: 3.846s
Total time from webhook: 35.74s

[alexellis]

but introduces breaking changes to error messages.

I'm trusting you've tested these with all callers and have found no regressions.