Conversation
📝 WalkthroughWalkthroughConnector and identity-protection clients now accept and propagate a version string; the CrowdStrike client/User-Agent is set from that version (conductorone-crowdstrike/%s). Call sites and builders were updated to pass the version through the initialization chain. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Claude finished @mstanbCO's task in 30s —— View job PR Review: Set User Agent String
Review SummaryLGTM — This is a clean, straightforward change that standardizes the user agent string to Changes
Notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/connector.go (1)
95-100: Optional: centralize the user-agent string in one package constantLine 99 is correct for this PR. To avoid drift with Line 205 in
pkg/connector/identity_protection.go, consider a shared constant.Proposed refactor
package connector +const crowdStrikeUserAgent = "conductorone-crowdstrike" + import ( "context" "fmt" @@ client, err := falcon.NewClient(&falcon.ApiConfig{ ClientId: clientId, ClientSecret: clientSecret, Cloud: cloudRegion, Context: ctx, - UserAgentOverride: "conductorone-crowdstrike", + UserAgentOverride: crowdStrikeUserAgent, })And in
pkg/connector/identity_protection.go:- req.Header.Set("User-Agent", "conductorone-crowdstrike") + req.Header.Set("User-Agent", crowdStrikeUserAgent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/connector.go` around lines 95 - 100, Create a package-level constant for the shared user-agent string and replace the hardcoded "conductorone-crowdstrike" occurrences with that constant; specifically add something like const DefaultUserAgent = "conductorone-crowdstrike" in package connector and update the Client construction's UserAgentOverride field (where UserAgentOverride is set) and the usage in identity_protection.go (the other occurrence at Line ~205) to reference DefaultUserAgent so both locations use the single centralized symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/connector/connector.go`:
- Around line 95-100: Create a package-level constant for the shared user-agent
string and replace the hardcoded "conductorone-crowdstrike" occurrences with
that constant; specifically add something like const DefaultUserAgent =
"conductorone-crowdstrike" in package connector and update the Client
construction's UserAgentOverride field (where UserAgentOverride is set) and the
usage in identity_protection.go (the other occurrence at Line ~205) to reference
DefaultUserAgent so both locations use the single centralized symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51fc068e-7470-42a2-bc80-8fea560e072e
📒 Files selected for processing (2)
pkg/connector/connector.gopkg/connector/identity_protection.go
|
Claude finished @mstanbCO's task in 49s —— View job PR Review: Set User Agent String
Review SummaryLGTM — Clean change that sets a versioned user agent string ( Changes
Feedback
No issues found. Approving. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/connector.go (2)
102-104:⚠️ Potential issue | 🟡 MinorAdd connector prefix to SDK init error message.
Line 103 wraps correctly with
%w, but it should include the connector prefix for consistency with project rules.Suggested patch
- return nil, fmt.Errorf("failed to initialize SDK client: %w", err) + return nil, fmt.Errorf("baton-crowdstrike: failed to initialize SDK client: %w", err)As per coding guidelines: "
**/*.go: Always wrap errors with %w and include connector prefix (e.g., 'baton-service:') in error messages".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/connector.go` around lines 102 - 104, The error return that currently reads fmt.Errorf("failed to initialize SDK client: %w", err) must include the connector prefix per project rules; update the error wrapping to prepend the connector prefix (e.g., "connector:") so the returned error becomes something like "connector: failed to initialize SDK client: %w" while still using %w to wrap err—locate the fmt.Errorf(...) call in pkg/connector/connector.go (the SDK init error return) and modify its message accordingly.
17-23:⚠️ Potential issue | 🔴 CriticalImplement
Close()method onConnectorto clean up HTTP client connections.The
Connectortype inpkg/connector/connector.gois missing aClose()method. The connector initializes an HTTP client viafalcon.NewClient()(line 95-104) which maintains persistent connection pools viaCrowdStrikeAPISpecification.Transport. Without proper cleanup, these connections leak resources.Add a
Close()method that invokesCloseIdleConnections()on the underlying HTTP transport to release pooled connections. Return an error only if cleanup fails; do not return nil when clients exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/connector.go` around lines 17 - 23, Add a Close method on the Connector type that cleans up the HTTP client's persistent connections: implement Connector.Close() to locate the underlying HTTP transport from the Connector.client (the *fClient.CrowdStrikeAPISpecification returned by falcon.NewClient()) and call CloseIdleConnections() on it; ensure the method returns an error only if cleanup fails and otherwise returns nil, and safely handle the case where Connector.client or its Transport is nil to avoid panics.cmd/baton-crowdstrike/main.go (1)
20-66:⚠️ Potential issue | 🔴 CriticalAdd missing
--base-urland--insecureconfiguration flags.Both flags are required per coding guidelines but are not implemented. The configuration schema in
pkg/config/config.gois missingBaseURLFieldandInsecureFielddefinitions, and theconnector.New()function inpkg/connector/connector.godoes not accept these parameters. These flags are essential for mock server testing and supporting self-signed certificates in test scenarios.
- Add
BaseURLFieldandInsecureFieldtopkg/config/config.go- Add these fields to the
Crowdstrikestruct generated inpkg/config/conf.gen.go- Update
connector.New()signature to acceptbaseURLandinsecureparameters- Pass these values through to the underlying HTTP client configuration
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/baton-crowdstrike/main.go` around lines 20 - 66, Add missing BaseURLField and InsecureField to the configuration schema (define BaseURLField and InsecureField in the config schema), add matching fields to the Crowdstrike struct in the generated conf (ensure Crowdstrike has BaseURL string and Insecure bool), update connector.New(ctx, ...) signature to accept baseURL string and insecure bool and propagate those args through to the HTTP client creation inside connector.New so the underlying HTTP client uses the provided base URL and toggles TLS verification for self-signed certs; update all call sites (e.g., getConnector) to pass cfg.BaseURL and cfg.Insecure into connector.New.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/identity_protection.go`:
- Around line 202-207: The RoundTrip method on identityProtectionTransport
unconditionally sets the User-Agent header from t.userAgent, but RefreshContext
rebuilds the transport without initializing userAgent causing it to erase the
header; fix by preserving/initializing the value when the transport is rebuilt
(in RefreshContext) or change RoundTrip to only set the header when t.userAgent
is non-empty (e.g., check t.userAgent != "" before calling req.Header.Set).
Update the identityProtectionTransport construction logic to copy or pass
through the existing userAgent and ensure RefreshContext initializes
identityProtectionTransport.userAgent.
---
Outside diff comments:
In `@cmd/baton-crowdstrike/main.go`:
- Around line 20-66: Add missing BaseURLField and InsecureField to the
configuration schema (define BaseURLField and InsecureField in the config
schema), add matching fields to the Crowdstrike struct in the generated conf
(ensure Crowdstrike has BaseURL string and Insecure bool), update
connector.New(ctx, ...) signature to accept baseURL string and insecure bool and
propagate those args through to the HTTP client creation inside connector.New so
the underlying HTTP client uses the provided base URL and toggles TLS
verification for self-signed certs; update all call sites (e.g., getConnector)
to pass cfg.BaseURL and cfg.Insecure into connector.New.
In `@pkg/connector/connector.go`:
- Around line 102-104: The error return that currently reads fmt.Errorf("failed
to initialize SDK client: %w", err) must include the connector prefix per
project rules; update the error wrapping to prepend the connector prefix (e.g.,
"connector:") so the returned error becomes something like "connector: failed to
initialize SDK client: %w" while still using %w to wrap err—locate the
fmt.Errorf(...) call in pkg/connector/connector.go (the SDK init error return)
and modify its message accordingly.
- Around line 17-23: Add a Close method on the Connector type that cleans up the
HTTP client's persistent connections: implement Connector.Close() to locate the
underlying HTTP transport from the Connector.client (the
*fClient.CrowdStrikeAPISpecification returned by falcon.NewClient()) and call
CloseIdleConnections() on it; ensure the method returns an error only if cleanup
fails and otherwise returns nil, and safely handle the case where
Connector.client or its Transport is nil to avoid panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eb2e4c0-510f-4aca-95ee-2bfcc80c4e02
📒 Files selected for processing (4)
cmd/baton-crowdstrike/main.gopkg/connector/connector.gopkg/connector/identity_protection.gopkg/connector/security_insight.go
| base http.RoundTripper | ||
| userAgent string | ||
| } | ||
|
|
||
| func (t *identityProtectionTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| req.Header.Set("User-Agent", "baton-crowdstrike") | ||
| req.Header.Set("User-Agent", t.userAgent) |
There was a problem hiding this comment.
User-Agent can be unintentionally cleared after RefreshContext().
Line 207 always sets User-Agent from t.userAgent, but RefreshContext() rebuilds transport without initializing userAgent, so it becomes empty and wipes the header.
Suggested patch
type IdentityProtectionClient struct {
httpClient *http.Client
endpoint string
+ userAgent string
}
func NewIdentityProtectionClient(ctx context.Context, clientID, clientSecret, host, version string) *IdentityProtectionClient {
+ ua := fmt.Sprintf("conductorone-crowdstrike/%s", version)
// Create OAuth2 client credentials config
config := clientcredentials.Config{
ClientID: clientID,
ClientSecret: clientSecret,
TokenURL: "https://" + host + "/oauth2/token",
@@
// Wrap the transport to add user-agent header
httpClient.Transport = &identityProtectionTransport{
base: httpClient.Transport,
- userAgent: fmt.Sprintf("conductorone-crowdstrike/%s", version),
+ userAgent: ua,
}
return &IdentityProtectionClient{
httpClient: httpClient,
endpoint: "https://" + host + "/identity-protection/combined/graphql/v1",
+ userAgent: ua,
}
}
@@
func (c *IdentityProtectionClient) RefreshContext(ctx context.Context, clientID, clientSecret, host string) {
@@
httpClient.Transport = &identityProtectionTransport{
- base: httpClient.Transport,
+ base: httpClient.Transport,
+ userAgent: c.userAgent,
}
c.httpClient = httpClient
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/identity_protection.go` around lines 202 - 207, The RoundTrip
method on identityProtectionTransport unconditionally sets the User-Agent header
from t.userAgent, but RefreshContext rebuilds the transport without initializing
userAgent causing it to erase the header; fix by preserving/initializing the
value when the transport is rebuilt (in RefreshContext) or change RoundTrip to
only set the header when t.userAgent is non-empty (e.g., check t.userAgent != ""
before calling req.Header.Set). Update the identityProtectionTransport
construction logic to copy or pass through the existing userAgent and ensure
RefreshContext initializes identityProtectionTransport.userAgent.
1 participant
Description
Set's the user agent string to conductorone-crowdstrike
Summary by CodeRabbit