[BB-1613] - Add Wrap Errors - Crowdstrike Connector #38
Conversation
WalkthroughRename primary type from CrowdStrike to Connector, add centralized CrowdStrike SDK/HTTP→gRPC error mapping, introduce package-level resource type definitions, promote grpc to a direct dependency, and standardize error wrapping and pagination error returns across connector files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (17)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/connector/errors.go (1)
33-51: String-based error detection may produce false positives.The substring matching on error messages could incorrectly classify errors. For example, an error containing
"user ID 401234"would match"401"and be treated as an authentication error. Consider using word boundaries or more precise patterns.- if strings.Contains(errMsg, "401") || strings.Contains(errMsg, "unauthorized") { + if strings.Contains(errMsg, " 401 ") || strings.Contains(errMsg, "401:") || + strings.Contains(errMsg, "unauthorized") { return uhttp.WrapErrors(codes.Unauthenticated, fmt.Sprintf("%s: authentication failed", operation), err) }Alternatively, consider using a regex pattern like
\b401\bfor stricter matching, or rely more heavily on the structured error extraction path.pkg/connector/connector.go (1)
97-97: Consider wrapping SDK initialization error for consistency.The SDK initialization error uses
fmt.Errorfwhile other errors in this file use wrapper functions (wrapCrowdStrikeErrorfor API errors,uhttp.WrapErrorsfor validation). For consistency with the error handling pattern established in this PR, consider wrapping this error as well.If this is intentional (treating SDK initialization errors differently from CrowdStrike API errors), this can be ignored. Otherwise, consider applying a consistent error wrapping approach:
- return nil, fmt.Errorf("failed to initialize SDK client: %w", err) + return nil, wrapCrowdStrikeError(err, "failed to initialize SDK client")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/connector/connector.go(6 hunks)pkg/connector/errors.go(1 hunks)pkg/connector/helpers.go(4 hunks)pkg/connector/resource_types.go(1 hunks)pkg/connector/role.go(12 hunks)pkg/connector/user.go(3 hunks)
🔇 Additional comments (20)
go.mod (1)
12-12: LGTM - grpc promoted to direct dependency.The promotion of
google.golang.org/grpcfrom indirect to direct is appropriate since the newerrors.godirectly importsgoogle.golang.org/grpc/codes.pkg/connector/helpers.go (3)
27-35: LGTM - Improved error wrapping.The error wrapping in
handleNextPagenow provides context withfmt.Errorfand the%wverb for proper error chaining.
37-57: LGTM - Error handling in parsePageToken.The function properly returns errors with contextual messages for both unmarshal and conversion failures. Using
fmt.Errorfhere is appropriate since these are pagination errors, not CrowdStrike API errors.
60-71: LGTM - Error context in convertPageToken.Error wrapping with descriptive message aids debugging when page token parsing fails.
pkg/connector/resource_types.go (1)
7-23: LGTM - Clean extraction of resource type definitions.The resource type variables are well-defined with appropriate traits. Extracting these to a dedicated file improves code organization and makes the resource types easily discoverable.
pkg/connector/errors.go (1)
100-131: LGTM - Comprehensive HTTP to gRPC code mapping.The mapping covers common HTTP status codes appropriately, with sensible defaults for 4xx and 5xx ranges.
pkg/connector/user.go (3)
29-35: LGTM - Updated tomap[string]any.Using
anyinstead ofinterface{}is the idiomatic choice for Go 1.18+.
88-91: LGTM - Proper error wrapping for API call.The
wrapCrowdStrikeErrorwrapper provides consistent error handling with appropriate gRPC codes for the user query operation.
124-127: LGTM - Consistent error wrapping pattern.The error wrapping for
RetrieveUsersGETV1follows the same pattern, maintaining consistency across API calls.pkg/connector/role.go (8)
39-43: LGTM - Updated tomap[string]any.Consistent with the same change in
user.go.
69-71: LGTM - Error wrapping for role ID query.Properly wraps the CrowdStrike API error with context.
80-82: LGTM - Error wrapping for role details retrieval.Consistent error handling pattern for the API call.
88-90: Appropriate use offmt.Errorffor internal error.This error originates from SDK resource creation, not from CrowdStrike API, so using
fmt.Errorfinstead ofwrapCrowdStrikeErroris correct.
272-283: LGTM - Grant validation with proper error code.The validation rejects non-user principals with
InvalidArgument, which is the correct gRPC code for invalid input. The warning log provides useful debugging context.
312-326: LGTM - Revoke validation mirrors Grant.Consistent validation pattern with Grant method, using the same error code and logging approach.
297-299: LGTM - Grant API error wrapped properly.
338-340: LGTM - Revoke API error wrapped properly.pkg/connector/connector.go (3)
17-17: LGTM! Consistent type rename refactoring.The rename from
CrowdStriketoConnectoris consistently applied across the type declaration, method receivers, constructor signature, and return statement. This improves code clarity and follows Go naming conventions for connector implementations.Also applies to: 21-21, 28-28, 36-36, 75-75, 100-102
47-47: Error wrapping inValidatemethod is properly implemented.The three error wrapping calls on lines 47, 57, and 68 correctly use
wrapCrowdStrikeErrorwith descriptive operation messages. The function extracts CrowdStrike-specific error details and converts HTTP status codes to gRPC error codes with clear context for each failure point, which improves debuggability.
87-87: No issues found. Theuhttp.WrapErrorsusage at line 87 is correct and follows the established pattern in the codebase for input validation errors using the 2-argument form(code, message).
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors
branch
from
b27e052 to
0a38f04
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/role.go (1)
128-160: Slice initialized with length instead of capacity.Line 129 creates
rateLimitInfowithmake([]RateLimitInfo, len(userIDs)), which initializeslen(userIDs)zero-value elements. Then more elements are appended in the loop, resulting in the slice containing unwanted zero values at the beginning.If the intent is to pre-allocate capacity, use:
- rateLimitInfo := make([]RateLimitInfo, len(userIDs)) + rateLimitInfo := make([]RateLimitInfo, 0, len(userIDs))
🧹 Nitpick comments (1)
pkg/connector/connector.go (1)
74-75: Update stale comment.The comment references "CrowdStrike connector" but the type has been renamed to
Connector. Consider updating for consistency.-// New returns the CrowdStrike connector. +// New returns the Connector. func New(ctx context.Context, clientId, clientSecret string, region string) (*Connector, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/connector/connector.go(6 hunks)pkg/connector/errors.go(1 hunks)pkg/connector/helpers.go(4 hunks)pkg/connector/resource_types.go(1 hunks)pkg/connector/role.go(12 hunks)pkg/connector/user.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/connector/user.go
- pkg/connector/resource_types.go
- pkg/connector/errors.go
🔇 Additional comments (14)
go.mod (1)
12-12: LGTM!Promoting
google.golang.org/grpcto a direct dependency is appropriate given the new imports ofgoogle.golang.org/grpc/codesinconnector.goandrole.gofor standardized error handling.pkg/connector/helpers.go (3)
27-35: LGTM!The error wrapping in
handleNextPageprovides useful context for debugging pagination issues.
37-57: LGTM!The updated
parsePageTokenfunction now properly returns errors with context. The signature change to include an error return is a good improvement for error propagation.
59-71: LGTM!Consistent error wrapping in
convertPageTokenaligns with the pattern used throughout this file.pkg/connector/role.go (6)
13-19: LGTM!The new imports for
uhttpandgrpc/codesare required for the standardized error handling pattern.
36-61: LGTM!Using
map[string]anyis the idiomatic Go 1.18+ style, preferred overmap[string]interface{}.
63-108: LGTM!The
Listmethod correctly differentiates between CrowdStrike API errors (usingwrapCrowdStrikeError) and internal errors (usingfmt.Errorf). Error messages include operation context ("role list:").
162-270: LGTM!The
Grantsmethod properly handles the new error return fromparsePageTokenand consistently useswrapCrowdStrikeErrorfor API errors while usingfmt.Errorffor internal resource creation errors.
272-310: LGTM!The
Grantmethod properly usesuhttp.WrapErrorswithcodes.InvalidArgumentfor validation errors andwrapCrowdStrikeErrorfor API errors. The warning log provides useful context for debugging.
312-351: LGTM!The
Revokemethod follows the same error handling pattern asGrant, maintaining consistency across the codebase.pkg/connector/connector.go (4)
10-14: LGTM!The new imports support the standardized error handling pattern introduced in this PR.
17-33: LGTM!The type rename from
CrowdStriketoConnectoris a cleaner naming convention. This is a breaking change if external code references the type directly, though typical usage through theNewconstructor andconnectorbuilderinterfaces should be unaffected.
35-72: LGTM!The
Validatemethod consistently useswrapCrowdStrikeErrorfor API errors with descriptive prefixes ("validate:") that provide clear operational context.
86-102: LGTM!The error handling in
Newappropriately usescodes.InvalidArgumentfor invalid region input andfmt.Errorffor SDK client initialization failures.
ggreer
left a comment
ggreer
left a comment
There was a problem hiding this comment.
I noticed a few unrelated improvements that could be done, but that shouldn't block merging this.
| } | ||
| return codes.Unknown | ||
| } | ||
| } |
There was a problem hiding this comment.
uhttp does this, but unfortunately it's not in its own utility function. You have to use uhttp to do the request to get http status -> grpc status conversion. We should probably fix that at some point.
| } | ||
|
|
||
| // Fallback: Inspect error message for common HTTP status code patterns | ||
| // This is necessary because gofalcon SDK doesn't always expose structured errors |
There was a problem hiding this comment.
Why would they write an SDK and then mess up the errors this bad? 😭
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors
branch
from
0a38f04 to
21b0695
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/errors.go (1)
30-72: Add test coverage for error message pattern matching to catch SDK changes early.String-based error detection in the fallback path is necessary due to inconsistent SDK error structures but remains fragile. Currently there is no test coverage for
wrapCrowdStrikeErroror its error message patterns. If the gofalcon SDK changes error message formats, these patterns will fail silently. The same brittle pattern also appears inisConflictError()(lines 113-115).Add unit tests that verify each error pattern (401, "unauthorized", 403, "forbidden", etc.) maps to the correct gRPC code, ensuring SDK changes are caught early.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go.mod(1 hunks)pkg/connector/connector.go(6 hunks)pkg/connector/errors.go(1 hunks)pkg/connector/helpers.go(4 hunks)pkg/connector/resource_types.go(1 hunks)pkg/connector/role.go(12 hunks)pkg/connector/user.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/user.go
🔇 Additional comments (11)
go.mod (1)
12-12: LGTM! Dependency promotion is appropriate.Promoting
google.golang.org/grpcto a direct dependency is correct, as the connector now directly importsgoogle.golang.org/grpc/codesin the new error handling utilities.pkg/connector/helpers.go (1)
21-65: LGTM! Error wrapping is clear and follows best practices.The added error wrapping provides clear context for pagination failures while maintaining proper error chains with
%w. The removal ofannotationsForUserResourceType()addresses the previous review feedback about unnecessary indirection.pkg/connector/role.go (3)
298-305: Excellent implementation of conflict handling!The grant flow now properly detects 409 Conflict responses and returns the
GrantAlreadyExistsannotation instead of an error, which aligns with the previous review feedback and the expected idempotent behavior.
345-352: Proper revoke conflict handling.The revoke flow correctly handles already-revoked grants by returning the
GrantAlreadyRevokedannotation for 409 Conflict responses, maintaining consistency with the grant implementation.
39-43: Good modernization tomap[string]any.The change from
map[string]interface{}tomap[string]anyis a nice idiomatic improvement that leverages Go 1.18+ syntax.pkg/connector/resource_types.go (1)
1-24: LGTM! Clean resource type definitions.Moving resource type definitions to a dedicated file improves code organization. The inline annotations address the previous review feedback about removing unnecessary indirection from the
annotationsForUserResourceType()helper.pkg/connector/errors.go (2)
85-96: LGTM! Proper error unwrapping witherrors.As.The implementation correctly uses
errors.Asto detect thecsErrorinterface in the error chain, addressing the previous review feedback about wrapped error handling.
118-150: Comprehensive HTTP-to-gRPC status mapping.The mapping covers common HTTP status codes and includes sensible fallback logic for status code ranges. This provides consistent error handling across the connector.
pkg/connector/connector.go (3)
17-26: Type rename improves clarity.Renaming from
CrowdStriketoConnectorprovides better semantic clarity and aligns with common naming conventions for connector implementations.
36-72: Consistent validation error handling.The validation logic now properly wraps CrowdStrike SDK errors with contextual operation names, making it easier to diagnose authentication and permission issues during connector setup.
75-103: Region handling and SDK initialization look good.The expanded region support and proper error wrapping for invalid regions and SDK initialization failures provide clear feedback to users.
pkg/connector/errors.go
Outdated
| } | ||
|
|
||
| // Default: treat as unavailable (generic API error) | ||
| return uhttp.WrapErrors(codes.Unavailable, fmt.Sprintf("%s: API error", operation), err) |
There was a problem hiding this comment.
should we default to Unknown just as uhttp does on baton-sdk?
https://github.com/ConductorOne/baton-sdk/blob/main/pkg/uhttp/wrapper.go#L466
There was a problem hiding this comment.
remember Unavailable retries the operation and we don't want that sometimes if we are not sure about the incoming error
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors
branch
from
21b0695 to
be3c35e
Compare
Description
Golang-based SDK to CrowdStrike's Falcon APIs:
Useful links:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.