[DRAFT] Lambda #301
[DRAFT] Lambda #301
Conversation
WalkthroughThis pull request updates dependency versions in the Go module, refactors the internal connector registration and client creation, and introduces comprehensive validation for protobuf messages. It also enhances configuration management and CLI command definitions with stricter type constraints. A complete gRPC communication layer is added to support AWS Lambda, including client setup, STS request signing, a transport server, and utility functions. In addition, several vendor packages are updated to support new AWS Lambda runtime API features, error/panic handling, and SIGTERM signal management while new protobuf definitions and lambda server commands are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI Command"
participant GRPC as "gRPC Server"
participant Connector as "Connector Service"
participant Lambda as "AWS Lambda Runtime API"
CLI->>GRPC: Start Lambda Server Command
GRPC->>Connector: Register connector using RegisterOps
Lambda->>GRPC: Invoke request
GRPC->>Connector: Process gRPC method call
Connector-->>GRPC: Return response/error
GRPC-->>Lambda: Return response/error
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pkg/cli/lambda_commands.go
Outdated
| return connector.NewConnectorClient(ctx, cc), nil | ||
| } | ||
|
|
||
| func MakeLambdaServerCommand[T any]( |
There was a problem hiding this comment.
TODO: compile time flag or something.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
pkg/lambda/grpc/transport/transport.go (1)
1-57:⚠️ Potential issueConsolidate duplicate Lambda transport implementations.
This file appears to be a duplicate of
pkg/lambda/lambda.gowith only minor differences in import paths. Consider consolidating these implementations into a single package to avoid code duplication.Options to resolve this:
- Move the implementation to a common package and import it in both locations.
- Create an interface in a common package and implement it in one location.
- Remove one of the duplicate implementations and update all references.
🧹 Nitpick comments (47)
vendor/github.com/aws/aws-lambda-go/lambda/invoke_loop.go (3)
25-25: Remember to address the TODO.
The TODO references dropping support for Go versions <1.17 so we can directly usetime.UnixMilli. Let me know if you'd like me to open a new issue or prepare an additional PR once <1.17 is no longer supported in CI.
69-72: Potential concurrency issue with environment variable.
os.Setenv("_X_AMZN_TRACE_ID")modifies the process-wide environment, which can cause conflicts if multiple goroutines handle concurrent invocations. Consider storing the trace ID exclusively in the context, or ensuring single-threaded dispatch.Additionally, a typed key for context is generally safer than using raw string keys to avoid collisions.
98-98: Minor grammatical concern in the error message.
The message “unexpected error occurred when sending the function functionResponse…” seems to repeat “function” twice. Consider updating the text.- return fmt.Errorf("unexpected error occurred when sending the function functionResponse to the API: %v", err) + return fmt.Errorf("unexpected error occurred when sending the function response to the API: %v", err)pkg/lambda/grpc/transport.go (3)
38-48: Minor enhancement for error clarity could be beneficial.
Currently, you return"error unmarshalling request: %v"if an error occurs. While not strictly incorrect, you may want to add more context (e.g., which proto type failed or additional fallback strategies) for easier debugging in production.
50-53: Memoization TODO suggestion.
There is aTODO(morgabra): Memoize this.comment in the Headers method. Memoizing could improve performance if callingHeaders()multiple times.Would you like help implementing a simple caching mechanism or opening a tracking issue?
86-98: Fix mislabeled error message.
When an error occurs inUnmarshalResponse, line 95 says"error unmarshalling request:"but should reference "response."Proposed fix:
- return status.Errorf(codes.Internal, "error unmarshalling request: %v", err) + return status.Errorf(codes.Internal, "error unmarshalling response: %v", err)pkg/lambda/grpc/config/config.go (2)
30-87: Consider adding connection timeouts when creating the gRPC client.
grpc.NewClient(tokenHost, dialOpts...)can potentially hang if the endpoint is unreachable. AddingWithBlock()plus a timeout or some context-based dial logic would improve resilience.
106-137: Ed25519 private key parsing is valid.
The code rigorously checks format assumptions (e.g., 4 parts,v1token identifier, base64 decoding, etc.). Consider doc comments clarifying upgrade paths if future secret formats are needed.vendor/github.com/aws/aws-lambda-go/lambda/rpc_function.go (1)
35-46: Consider a graceful shutdown strategy instead oflog.Fatal.
Whennet.Listenfails orrpc.Registerfails, the code forcibly terminates the process. In some scenarios, returning the error or allowing a controlled shutdown could be cleaner.vendor/github.com/aws/aws-lambda-go/lambda/entry.go (2)
48-70: Deprecated functions observed (StartWithContext, StartHandler).
These methods are marked as deprecated. Consider removing or refactoring any internal usage to avoid confusion and ensure future maintainability.
88-114: Consider a more graceful error-handling strategy.
UsinglogFatalfwill terminate the process unconditionally. If needed, you may opt for a controlled shutdown or more nuanced error handling.pkg/lambda/grpc/config/sts.go (4)
28-31: Prefer consistent naming for helper methods.
Thesha256AndHexEncodefunction name is clear, but consider aligning it with other hashing utility names in the codebase for consistency.
37-44: Verify whether external AWS configuration is needed.
Currently,NewAdjusteralways loads the default AWS config. If you plan to allow passing a custom config or region, consider adding a constructor option for flexibility.
54-69: Good approach for embedding identity attestation.
Storing the base64-encoded STS request inid_attis clean. However, confirm that theid_attfield name is stable and recognized by upstream consumers.Would you like help drafting unit tests to ensure this field is properly handled?
91-146: Consider robust error handling for empty or invalid AWS region.
region := cfg.Regionmay be empty for some credentials (e.g. environment credentials). Adding a fallback or a clearer error message may aid debugging.pkg/cli/lambda_commands.go (1)
73-149: Address the in-code TODO/FIXME references.Lines 73, 139, 140, and 141 include placeholders or open questions (e.g., compile-time flags, provisioning, ticketing configuration). Let me know if you would like assistance in refining or removing these stubs.
pkg/lambda/grpc/server.go (2)
221-223: Improve detail in error message.When the method is unknown, passing the entire
methodandservicestructs using%vmay produce unhelpful output. It's more descriptive to referencemethod.MethodNameand theserviceNamestring instead.- return ErrorResponse(status.Errorf(codes.Unimplemented, "unknown method %v for service %v", method, service)) + return ErrorResponse(status.Errorf(codes.Unimplemented, "unknown method %s for service %s", method.MethodName, serviceName))
270-273: Skip serializing the OK status if not needed.You're currently setting a status of “OK” at all times, which increases the message size and serialization overhead. Consider making the status field optional if the final status is
OK.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionEventInvokeConfigs.go (1)
17-30: Ensure consistent error handling and context usage.
The function correctly initializesparamswhen it'sniland invokes the operation with a robust middleware stack. However, there is no explicit handling of any potential cancellation from thecontext.Context. This is standard generated code behavior, but if you decide to customize it, ensure the function also checks for context cancellation or deadlines when performing long-running operations.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListEventSourceMappings.go (1)
31-77: Document input constraints.
The struct properly outlines constraints on function names and ARNs. If you wrap or extend this struct, consider validating the maximum length or format prior to dispatching the request to surface user errors earlier.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_Invoke.go (1)
60-73: Consider context cancellation checks for large payloads.
While this function is typically quick, large payloads may cause longer processing times. If customizing the invocation flow, insert context checks to respect cancellation or deadlines, especially when handling large or streaming payloads.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_InvokeWithResponseStream.go (2)
39-85: Validate required fields inInvokeWithResponseStreamInput.
FunctionNameis documented as required. However, there is no explicit runtime check for its presence (line 55). Consider adding a sanity check to return a more descriptive error ifFunctionNameis nil or empty before invoking the operation.func (c *Client) InvokeWithResponseStream(ctx context.Context, params *InvokeWithResponseStreamInput, optFns ...func(*Options)) (*InvokeWithResponseStreamOutput, error) { if params == nil { params = &InvokeWithResponseStreamInput{} } + if params.FunctionName == nil || len(*params.FunctionName) == 0 { + return nil, fmt.Errorf("InvokeWithResponseStreamInput.FunctionName is required and cannot be empty") + } result, metadata, err := c.invokeOperation(ctx, "InvokeWithResponseStream", params, optFns, c.addOperationInvokeWithResponseStreamMiddlewares) ... }
219-317: Encourage explicit use ofwaitStreamClosefor comprehensive lifecycle handling.While the event stream structure (lines 219-317) includes
waitStreamClose()(lines 290-317), consider highlighting or forcing usage to ensure thorough cleanup. This method handles both error and closed channels gracefully but might go unused if callers rely only onClose().pb/c1/transport/v1/transport.pb.validate.go (1)
341-410: Consider early checks for well-known fields inResponse.While generated validations exist for embedded messages, a pre-check of recognized fields may immediately surface user errors. This can improve error clarity.
vendor/github.com/aws/aws-lambda-go/lambda/handler.go (5)
38-69:WithContextandWithContextValueusage clarity.Using both can lead to overridden or duplicated context keys if invoked multiple times. Document how conflicts or overwrites are handled to avoid developer confusion.
71-85: Minor improvement: Clarify doc comment.Line 87 in the doc states "sets the SetEscapeHTML argument on the underlying json encoder". For uniform naming, consider rephrasing “json encoder” → “JSON encoder.”
87-102: Spelling fix: "underling" → "underlying".Doc comment at line 87 says “underling json encoder.” This is likely a minor typo.
-// WithSetIndent sets the SetIndent argument on the underling json encoder +// WithSetIndent sets the SetIndent argument on the underlying json encoder
136-155: Add doc comment on SIGTERM callback limitations.Developers might rely on these callbacks for heavy resource cleanup. Clarify (lines 136-155) that only ~500ms is guaranteed before SIGKILL.
276-395: Consider avoidingio/ioutilpackage (line 12).
io/ioutilis deprecated in newer Go versions. Replaceioutil.ReadAllwithio.ReadAllif environment allows.pb/c1/connectorapi/baton/v1/config.pb.validate.go (2)
38-66: Consider unit tests to verify generated validators
Even though these validators are automatically generated, it can be beneficial to add unit tests verifying that each rule behaves as expected, especially for critical fields. This will help ensure consistent behavior when the Proto schema evolves.Would you like help crafting an example test suite that exercises typical and edge-case validations for these messages?
420-436: Optional: Fine-tune embedded message validations
Where your embedded messages (e.g., Config fields) have constraints, you might adjust the proto definition to insert stricter rules (e.g., required fields or string length constraints) so that these validations become more comprehensive.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunction.go (1)
348-399: Add optional logging or metrics on state transitions.
The retry logic is well-implemented, but you might consider adding debug logs to capture transitions like"Active"→"Pending"to better aid debugging in production environments.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_client.go (1)
225-310: Review error wrapping approach for better debugging.
When an error is returned, it’s wrapped insmithy.OperationError. If you need more contextual logs, consider injecting debug logs or additional handling prior to returning.vendor/github.com/aws/aws-lambda-go/lambda/errors.go (1)
19-33: Consider optimizing error type determination.The error type determination logic is duplicated from
getErrorType. Consider reusing the existing function.func lambdaErrorResponse(invokeError error) *messages.InvokeResponse_Error { if ive, ok := invokeError.(messages.InvokeResponse_Error); ok { return &ive } - var errorName string - if errorType := reflect.TypeOf(invokeError); errorType.Kind() == reflect.Ptr { - errorName = errorType.Elem().Name() - } else { - errorName = errorType.Name() - } return &messages.InvokeResponse_Error{ Message: invokeError.Error(), - Type: errorName, + Type: getErrorType(invokeError), } }vendor/github.com/aws/aws-lambda-go/lambda/handlertrace/trace.go (1)
32-38: Consider adding validation for nil context.While the implementation is correct, consider adding a nil check for the context parameter to prevent potential panics.
func NewContext(ctx context.Context, trace HandlerTrace) context.Context { + if ctx == nil { + ctx = context.Background() + } existing := FromContext(ctx) return context.WithValue(ctx, handlerTraceKey{}, HandlerTrace{ RequestEvent: callbackCompose(existing.RequestEvent, trace.RequestEvent), ResponseEvent: callbackCompose(existing.ResponseEvent, trace.ResponseEvent), }) }vendor/github.com/aws/aws-lambda-go/lambda/sigterm.go (1)
14-25: Consider adding error handling for signal notification setup.The signal handling setup could fail in certain scenarios. Consider adding error handling for the
signal.Notifycall and potential channel operations.if len(sigtermHandlers) > 0 { signaled := make(chan os.Signal, 1) - signal.Notify(signaled, syscall.SIGTERM) + signal.Notify(signaled, syscall.SIGTERM) + // Ensure cleanup on function exit + defer signal.Stop(signaled) go func() { <-signaled for _, f := range sigtermHandlers { f() } }() }vendor/github.com/aws/aws-lambda-go/lambda/panic.go (1)
32-37: Consider making the frame count configurable.The default frame count is hardcoded. Consider making it configurable to handle different stack depths.
-var defaultErrorFrameCount = 32 +var defaultErrorFrameCount = 32 + +// SetErrorFrameCount allows configuring the maximum number of stack frames to capture +func SetErrorFrameCount(count int) { + if count > 0 { + defaultErrorFrameCount = count + } +}pkg/lambda/grpc/util.go (3)
20-29: Consider using iota for timeoutUnit constants.The timeoutUnit constants could be more idiomatic using iota with explicit character values.
type timeoutUnit uint8 const ( - hour timeoutUnit = 'H' - minute timeoutUnit = 'M' - second timeoutUnit = 'S' - millisecond timeoutUnit = 'm' - microsecond timeoutUnit = 'u' - nanosecond timeoutUnit = 'n' + hour timeoutUnit = timeoutUnit('H') + minute timeoutUnit = timeoutUnit('M') + second timeoutUnit = timeoutUnit('S') + millisecond timeoutUnit = timeoutUnit('m') + microsecond timeoutUnit = timeoutUnit('u') + nanosecond timeoutUnit = timeoutUnit('n') )
50-75: Add unit tests for edge cases in decodeTimeout.The decodeTimeout function handles important timeout parsing logic but could benefit from comprehensive testing of edge cases:
- Maximum hour values
- Invalid unit characters
- Empty strings
- Negative values
Would you like me to generate comprehensive unit tests for the decodeTimeout function?
153-175: Consider adding context to error messages.The ErrorResponse function could provide more context in the "unknown error" case to help with debugging.
- st = status.Newf(codes.Unknown, "unknown error: %s", err) + st = status.Newf(codes.Unknown, "failed to convert error to status: %s", err)pkg/field/defaults.go (1)
65-65: Consider documenting the purpose of empty relationships.While an empty slice is valid, consider adding a comment explaining why no relationships are needed for Lambda server fields.
pkg/config/config.go (1)
128-142: Consider extracting Lambda configuration setup to a separate function.The Lambda configuration setup in
DefineConfigurationduplicates logic fromDefineLambdaServerConfiguration. Consider extracting this to a shared function to improve maintainability.+func setupLambdaCommand(ctx context.Context, connectorName string, v *viper.Viper, connector cli.GetConnectorFunc[T], schema field.Configuration) (*cobra.Command, error) { + lambdaConfSchema := field.NewConfiguration(field.LambdaServerFields(), field.LambdaServerRelationships...) + + lambdaCommand := &cobra.Command{ + Use: "lambda", + Short: "lambda", + SilenceErrors: true, + SilenceUsage: true, + RunE: cli.MakeLambdaServerCommand(ctx, connectorName, v, connector, lambdaConfSchema, schema), + } + + return lambdaCommand, setFlagsAndConstraints(lambdaCommand, lambdaConfSchema) +}proto/c1/connectorapi/baton/v1/config.proto (2)
14-19: Request/Response Message Definitions
The messagesGetConnectorConfigRequest(empty) andGetConnectorConfigResponse(with a struct and timestamp) are straightforward. Consider adding inline comments or documentation for each field if future maintainability and clarity are desired.
21-24: SignedHeader Message Structure
TheSignedHeadermessage is simple and well defined. As a minor suggestion, if the repeated field represents multiple header values, you might consider naming itvaluesto further clarify its purpose.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/CHANGELOG.md (1)
1-734: Comprehensive Lambda Service Changelog Review
This extensive changelog for the AWS Lambda service covers numerous updates—from dependency bumps and new feature introductions (e.g., ProvisionedPollerConfig, enhanced retryers, multi-runtime support including Python 3.13, Node.js support, etc.) to bug fixes addressing issues like credential refresh and error handling. The entries are detailed and adhere to expected changelog formatting.A couple of minor nitpicks:
- Node.js Naming: Several entries refer to "Node 22.x" (and similar), but the official styling is "Node.js 22.x". Please update these instances for consistency.
- Clarity in Word Choice: In descriptive texts (if any similar to “a large number of services”), consider using “numerous” or specifying the count to reduce wordiness as per LanguageTool suggestions.
Overall, the changelog is well organized and communicates the updates effectively.
🧰 Tools
🪛 LanguageTool
[style] ~357-~357: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...entation of various input fields across a large number of services. Calling code that references ...(LARGE_NUMBER_OF)
[uncategorized] ~597-~597: The official spelling of this programming framework is “Node.js”.
Context: ...-05-12) * Feature: Lambda releases NodeJs 16 managed runtime to be available in a...(NODE_JS)
🪛 markdownlint-cli2 (0.17.2)
464-464: Bare URL used
null(MD034, no-bare-urls)
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/LICENSE.txt (2)
150-151: Punctuation Improvement: Consider Adding a CommaThe static analysis indicates a possible missing comma in the following clause:
"appropriateness of using or redistributing the Work and assume any risks associated with Your exercise of permissions under this License."
Adding a comma after "Work" will help clearly separate the clauses and improve readability.
Consider applying this change:
- appropriateness of using or redistributing the Work and assume any + appropriateness of using or redistributing the Work, and assume any🧰 Tools
🪛 LanguageTool
[uncategorized] ~151-~151: Possible missing comma found.
Context: ...iateness of using or redistributing the Work and assume any risks associated w...(AI_HYDRA_LEO_MISSING_COMMA)
161-163: Conciseness Suggestion: Revise 'any and all' PhraseThe phrase "any and all" in this context might be overly verbose. Simplifying it to just "all" could make the language more concise while preserving the intended meaning.
You may consider the following change:
- computer failure or malfunction, or any and all + computer failure or malfunction, or all🧰 Tools
🪛 LanguageTool
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumpb/c1/connectorapi/baton/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/config_grpc.pb.gois excluded by!**/*.pb.gopb/c1/transport/v1/transport.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (100)
go.mod(2 hunks)internal/connector/connector.go(3 hunks)pb/c1/connectorapi/baton/v1/config.pb.validate.go(1 hunks)pb/c1/transport/v1/transport.pb.validate.go(1 hunks)pkg/cli/lambda_commands.go(1 hunks)pkg/config/config.go(2 hunks)pkg/field/defaults.go(1 hunks)pkg/lambda/grpc/client.go(1 hunks)pkg/lambda/grpc/config/config.go(1 hunks)pkg/lambda/grpc/config/sts.go(1 hunks)pkg/lambda/grpc/server.go(1 hunks)pkg/lambda/grpc/transport.go(1 hunks)pkg/lambda/grpc/transport/local.go(1 hunks)pkg/lambda/grpc/transport/transport.go(1 hunks)pkg/lambda/grpc/util.go(1 hunks)pkg/lambda/lambda.go(1 hunks)proto/c1/connectorapi/baton/v1/config.proto(1 hunks)proto/c1/transport/v1/transport.proto(1 hunks)vendor/github.com/aws/aws-lambda-go/LICENSE(1 hunks)vendor/github.com/aws/aws-lambda-go/LICENSE-LAMBDACODE(1 hunks)vendor/github.com/aws/aws-lambda-go/LICENSE-SUMMARY(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/README.md(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/entry.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/entry_generic.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/errors.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/extensions_api_client.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/handler.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/handlertrace/trace.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/invoke_loop.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/messages/README.md(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/messages/messages.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/panic.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/rpc_function.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/runtime_api_client.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambda/sigterm.go(1 hunks)vendor/github.com/aws/aws-lambda-go/lambdacontext/README.md(1 hunks)vendor/github.com/aws/aws-lambda-go/lambdacontext/context.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/go_module_metadata.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/middleware/user_agent.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/CHANGELOG.md(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/go_module_metadata.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/message.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/aws/signer/internal/v4/headers.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/CHANGELOG.md(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/go_module_metadata.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.json(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/CHANGELOG.md(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/go_module_metadata.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/CHANGELOG.md(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/LICENSE.txt(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_client.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_AddLayerVersionPermission.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_AddPermission.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateAlias.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateEventSourceMapping.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateFunction.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateFunctionUrlConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteAlias.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteEventSourceMapping.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunction.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionConcurrency.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionEventInvokeConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionUrlConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteLayerVersion.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteProvisionedConcurrencyConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetAccountSettings.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetAlias.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetEventSourceMapping.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunction.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionConcurrency.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionConfiguration.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionEventInvokeConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionRecursionConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionUrlConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersion.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersionByArn.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersionPolicy.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetPolicy.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetProvisionedConcurrencyConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetRuntimeManagementConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_Invoke.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_InvokeAsync.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_InvokeWithResponseStream.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListAliases.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListCodeSigningConfigs.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListEventSourceMappings.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionEventInvokeConfigs.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionUrlConfigs.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctions.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionsByCodeSigningConfig.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListLayerVersions.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListLayers.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListProvisionedConcurrencyConfigs.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListTags.go(1 hunks)vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListVersionsByFunction.go(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- vendor/github.com/aws/aws-lambda-go/lambda/README.md
- vendor/github.com/aws/aws-sdk-go-v2/aws/go_module_metadata.go
- vendor/github.com/aws/aws-lambda-go/lambdacontext/README.md
- vendor/github.com/aws/aws-lambda-go/lambda/messages/README.md
- vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/go_module_metadata.go
- vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/go_module_metadata.go
- vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/go_module_metadata.go
- vendor/github.com/aws/aws-lambda-go/LICENSE-LAMBDACODE
- vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/CHANGELOG.md
- vendor/github.com/aws/aws-lambda-go/LICENSE-SUMMARY
- vendor/github.com/aws/aws-lambda-go/LICENSE
- vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/CHANGELOG.md
- vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteCodeSigningConfig.go
🧰 Additional context used
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/transport/v1/transport.proto
[failure] 3-3:
Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
[failure] 5-5:
Import "google/protobuf/timestamp.proto" is unused.
🪛 Buf (1.47.2)
proto/c1/transport/v1/transport.proto
3-3: Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
(PACKAGE_DIRECTORY_MATCH)
5-5: Import "google/protobuf/timestamp.proto" is unused.
(IMPORT_USED)
🪛 LanguageTool
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/CHANGELOG.md
[style] ~357-~357: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...entation of various input fields across a large number of services. Calling code that references ...
(LARGE_NUMBER_OF)
[uncategorized] ~597-~597: The official spelling of this programming framework is “Node.js”.
Context: ...-05-12) * Feature: Lambda releases NodeJs 16 managed runtime to be available in a...
(NODE_JS)
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/LICENSE.txt
[uncategorized] ~151-~151: Possible missing comma found.
Context: ...iateness of using or redistributing the Work and assume any risks associated w...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/CHANGELOG.md
464-464: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (214)
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetPolicy.go (6)
1-2: No action needed on auto-generated header.
These lines are automatically generated and should not be manually edited.
16-29: Straightforward implementation for invoking the GetPolicy operation.
The approach of handlingnilinput params by creating a defaultGetPolicyInput{}is consistent with the AWS SDK style. Error handling and result extraction appear correct for this operation.
31-54: Input struct definition is clear and well-documented.
The usage notes around the different function name formats are standard for Lambda ARNs. This is consistent with other AWS Lambda operations, and the inline documentation helps prevent misuse.
56-68: Output struct is straightforward and consistent.
Including a specific metadata field allows for extended response data handling. This is a standard AWS pattern, and it correctly preserves the resulting metadata.
70-168: Middleware stack construction follows AWS SDK conventions.
Each middleware is added in the correct order, and error checks are properly handled. The layering approach for serialization, deserialization, endpoint resolution, retry logic, and logging is in line with typical AWS SDK patterns.
170-176: Service metadata middleware is standard.
Registering region, service ID, and operation name ensures consistent tracking across the pipeline. No concerns from a correctness or clarity perspective.vendor/github.com/aws/aws-lambda-go/lambda/invoke_loop.go (1)
198-198: Validate the cwd retrieval logic.
This line ignores the error returned byos.Getwd(). If the working directory isn’t available, the subsequent usage can lead to incorrect data or behavior. Consider handling the error or providing a fallback.-cwd, _ := os.Getwd() +cwd, err := os.Getwd() +if err != nil { + cwd = "" // or some fallback +}pkg/lambda/grpc/transport.go (7)
21-23: No immediate issues with theRequeststruct.
The struct is straightforward, holding thepbtransport.Requestpointer. This is an idiomatic approach for bridging the message container with associated methods.
25-32: Proper usage of protojson for JSON marshalling/unmarshalling.
Overall, theUnmarshalJSONandMarshalJSONimplementations are correct. Make sure to handle or log JSON parsing errors from upstream if needed.
55-71: Constructor logic looks good.
TheNewRequestfunction properly wraps the provided proto message into ananypb.Anyand sets up headers. The error handling for the marshalling steps is clear.
73-75: TheResponsestruct usage appears consistent.
No immediate concerns here.
77-80: JSON marshalling inResponsemirrorsRequestand seems fine.
Implementation is consistent, usingprotojson.Marshalandprotojson.Unmarshal.Also applies to: 82-84
100-112: Robust status extraction logic.
The method properly handles nil checks andanypbunmarshalling. No issues found.
114-116: Metadata retrieval is straightforward.
Returning unmarshaled headers and trailers is appropriate.Also applies to: 118-120
pkg/lambda/grpc/config/config.go (2)
1-28: Global declarations and error variables are well-structured.
The named errors likeErrInvalidClientSecretandErrInvalidClientIDdocument error scenarios clearly.
89-104: Potential confusion with ignoring the path portion after splitting the client ID.
parseClientIDsplits on'/'but only returnsitems[0](the host). Confirm that discardingitems[1](the path) is intentional.vendor/github.com/aws/aws-lambda-go/lambda/rpc_function.go (5)
22-33: Init block elegantly defers to_LAMBDA_SERVER_PORT.
This approach is standard for an RPC-based lambda runtime, although it locks into net/rpc usage.
50-60: Function struct usage is consistent with the package's net/rpc approach.
Deprecation warnings are reasonable since it’s for internal runtime usage only.
62-66:Pingmethod is a basic no-op.
This is typical for health checks/ping calls; no improvements needed.
68-107: Potential concurrency concerns withos.Setenv.
Setting environment variables at runtime can cause race conditions if multiple invokes happen concurrently within the same process. If concurrency is guaranteed to be single-threaded by the runtime, this is fine; otherwise, a more concurrency-friendly approach is advised.
109-114: Base context fallback is a neat extension point.
It cleanly resolves to a background context if no custom context is provided.vendor/github.com/aws/aws-lambda-go/lambda/entry.go (2)
1-46: Doc comments and function signature rules are well-presented.
No immediate issues found; the documentation is clear and comprehensive.
72-85: ThestartFunctionstruct handles environment-based invocation cleanly.
No further concerns here; it aligns well with the initialization pattern.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListTags.go (6)
1-16: Comprehensive documentation.
The comments clearly explain the ListTags operation and related types.
17-30: Good handling of nil params.
The function defaults to a new input struct when params are nil, preventing potential panics.
32-41: Verify input validation for Resource field.
BecauseResourceis essential for the request, ensure it is not empty at runtime. You might add checks or rely on downstream validation as appropriate.
43-52: Output struct is straightforward.
Carrying tags in a map is a typical pattern. No issues.
54-152: Middleware stack is well-structured.
The ordered addition of serialization, deserialization, and logging steps follows a recognized pattern. No immediate concerns.
154-160: Service metadata registration appears correct.
The function neatly attaches region and operation metadata.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetAccountSettings.go (6)
1-16: Clear documentation of account settings retrieval.
Descriptions align with the AWS Lambda usage guidelines.
18-31: Nil input handling is robust.
The function safely defaults to a new struct if no input is provided.
33-35: Input struct is minimal and effective.
No issues identified with the definition.
37-49: Output struct usage is appropriate.
Storing limit and usage details as pointers to separate types is consistent with expected patterns.
51-146: Middleware addition follows standard layering.
No concurrency or error-handling concerns noted in the layering sequence.
148-154: Service metadata middleware is properly configured.
Operation name and region metadata appear correct.pkg/lambda/grpc/config/sts.go (4)
33-35: Struct definition is straightforward.
Theadjusterstruct properly wraps AWS config, aligning well with the usage ofdpop.ClaimsAdjuster.
46-49: Method pass-through seems intended.
AdjustBodysimply returns the provided body. This is acceptable if no transformation is required, but confirm no future claim expansions will be needed here.
50-52: Confirm if no claims adjustment is desired.
AdjustClaimsis intentionally no-op. Ensure there's no missed requirement to modify or validate JWT claims.
71-89: No issues with STS request marshalling to base64.
Generating a protobuf, wrapping inanypb, and base64-encoding is valid. Make sure the receiving end decodes with the sameRawURLEncodingvariant.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionUrlConfig.go (2)
13-28: Standard delete operation.
TheDeleteFunctionUrlConfigmethod is consistent with other AWS SDK patterns. Ensureparams.FunctionNameis validated or let the compiler/validation middleware handle it.
61-159: AWS middleware logic looks standard.
The middleware stack is comprehensive. Vendor code is auto-generated and typically not manually modified.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionRecursionConfig.go (2)
14-30: Implementation follows AWS SDK conventions.
GetFunctionRecursionConfigusage is straightforward. The parameters are properly defaulted if nil, which is standard.
62-159: Middleware stack is standard.
Auto-generated vendor code typically handles all necessary error-handling, logging, and request/response instrumentation.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_InvokeAsync.go (1)
1-183: Vendor Code – Looks GoodAll lines in this file appear to be automatically generated by Smithy for AWS Lambda’s
InvokeAsyncoperation. The structure follows standard AWS SDK patterns (e.g., function signature, middleware stack, deprecation notice). There are no obvious correctness, security, or performance concerns for this generated code.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionEventInvokeConfig.go (1)
1-171: Vendor Code – No Immediate ConcernsThis is also code generated by Smithy for the
DeleteFunctionEventInvokeConfigcall. It adheres to the typical AWS SDK layout with parameter checks, invoke operations, and comprehensive middleware insertion. Everything is consistent with the AWS SDK approach.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateCodeSigningConfig.go (1)
1-174: Vendor Code – Implementation is StandardThe newly introduced
CreateCodeSigningConfigoperation follows the same auto-generated structure, including relevant middlewares. The types and fields align with AWS Lambda’s expected parameters (e.g.,AllowedPublishers,CodeSigningPolicies). The code is consistent and correct based on the AWS SDK patterns.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetAlias.go (1)
1-196: Auto-generated vendor code.Avoid making manual edits to this file. It's generally best to let the code generator manage any changes to avoid future conflicts or inconsistencies.
vendor/github.com/aws/aws-lambda-go/lambda/runtime_api_client.go (1)
1-174: Auto-generated vendor code.No changes recommended. Typically, vendor files are not manually modified as they are managed by external packages.
pkg/cli/lambda_commands.go (3)
31-47: Looks good overall.This resolver logic is straightforward and handles endpoint parsing robustly.
49-71: Consider validating the Lambda function name.If
functionis empty or malformed, this might cause runtime errors when invoking the Lambda. Adding a quick check could prevent cryptic errors downstream.
152-207: Functionality is well-structured.The logic for retrieving and formatting metadata looks correct and straightforward.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetRuntimeManagementConfig.go (5)
21-34: ValidateFunctionNamefield usage.The implementation correctly initializes default input when
paramsis nil, aligning with standard AWS conventions. However, there is no explicit validation for the requiredFunctionNamefield; ensure that upstream or server-side checks are in place.
36-60: Struct input approach is consistent.All fields, including the required
FunctionName, follow AWS norms by relying on server or middleware validation. No changes needed.
62-78: Output struct is straightforward.The output fields are well-defined for capturing lambda runtime config details. No issues found.
80-178: Middleware stack setup looks good.The layered middlewares for serialization, endpoint resolution, retry handling, and logging appear standard and comprehensive.
180-186: Service metadata middleware is properly registered.Region, service ID, and operation name are defined as expected. No concerns.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersionByArn.go (5)
18-31: Check handling of nil input and ARN validation.Defaults are set for
paramsif nil, and there is no explicit validation for the required ARN field within the function. Confirm that upstream checks ensure a valid ARN is provided.
33-41: Input struct usage follows AWS pattern.The
Arnfield is clearly documented as required, with no local validation. This aligns with typical AWS handling.
43-87: Output struct is well-defined.All layer version metadata fields are properly exposed for further consumption. No additional concerns.
89-187: Operation middlewares are standard.The serialization, retry, and logging middlewares are properly chained. Implementation matches other AWS Lambda operations.
189-195: Service metadata function works as expected.No issues with setting region, service ID, and operation name. Seems consistent with AWS patterns.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersion.go (5)
18-31: EnsureLayerNameandVersionNumberare valid.The function sets default input if nil but does not enforce mandatory fields. This is standard for AWS, yet a reminder to validate on the caller side.
33-46: Input struct reflects required fields.
LayerNameandVersionNumberare documented as required; no local checks are performed. This aligns with expected usage.
48-92: Output struct is standard for layer metadata.The fields comprehensively capture layer version details. No changes necessary.
94-192: Middleware additions are consistent.All standard AWS middlewares—serialization, error handling, logging—are included. No performance or correctness red flags.
194-200: Service metadata middleware final check.Operation name, region, and IDs are appropriately registered. Implementation looks solid.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetProvisionedConcurrencyConfig.go (7)
1-3: Auto-generated notice.These lines indicate that this file is generated by smithy-go-codegen. Typically, manual edits in auto-generated code are discouraged unless absolutely necessary.
5-12: Import statements verified.Imports are standard references to AWS and Smithy libraries, aligning with typical AWS Go SDK patterns.
14-29: Method retrieval logic is consistent.The
GetProvisionedConcurrencyConfigmethod follows the expected pattern for retrieving concurrency configs. The invocation structure and error handling are standard. No issues found.
31-55: Input struct looks correct.
GetProvisionedConcurrencyConfigInputprovides the required fields to specify a function’s name and qualifier. Documentation is inline, and constraints are consistent with AWS guidelines.
57-86: Output struct aligns with AWS concurrency fields.
GetProvisionedConcurrencyConfigOutputcaptures concurrency details, status, and metadata. Documentation references are clear, and the type usage fromtypes.ProvisionedConcurrencyStatusEnumis appropriate.
88-186: Middleware chain additions are standard.The
addOperationGetProvisionedConcurrencyConfigMiddlewaresfunction properly serializes, deserializes, and handles request/response logic. No concerns found.
188-195: Service metadata middleware is configured correctly.
newServiceMetadataMiddleware_opGetProvisionedConcurrencyConfigsets region, service ID, and operation name as expected.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionEventInvokeConfig.go (6)
1-3: Auto-generated notice.This file is also generated by smithy-go-codegen. Manual changes may be overwritten in future updates.
5-12: Imports verified.All imported packages are standard for AWS SDK usage, including context, Smithy, and the necessary Lambda types.
15-32: Asynchronous event invocation retrieval is straightforward.
GetFunctionEventInvokeConfigaccurately sets up invocation parameters, checks for nil input, and processes errors. Pattern matches typical usage of AWS SDK.
34-57: Input struct correctness.
GetFunctionEventInvokeConfigInputfields and documentation match the standard Lambda event invoke config requirements. The requiredFunctionNamefield is clearly annotated.
59-95: Output struct covers all event invoke fields.
GetFunctionEventInvokeConfigOutputincludes destination configurations, timestamps, and maximum retries in line with AWS docs.
97-195: Middleware pipeline is complete and consistent.
addOperationGetFunctionEventInvokeConfigMiddlewaressets up the correct stack for request handling, error callbacks, logging, and finalizers. All standard for AWS usage.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateFunctionUrlConfig.go (6)
1-3: Auto-generated notice.This file is likewise smithy-go-codegen output. No manual modifications recommended unless absolutely necessary.
5-12: Import statements verified.The imports, including references to Lambda
typesand Smithy, are aligned with AWS best practices and patterns.
14-30: Method creation logic is sound.
CreateFunctionUrlConfigsets up a standard operation invocation. Input validation fornilparameters is handled gracefully.
32-82: Input struct is comprehensive.Fields for AuthType, function name, CORS, invoke mode, and qualifier match AWS documentation for creating function URLs.
84-135: Output struct matches expected fields.Fields such as
FunctionUrl,AuthType, andCorsreflect the necessary properties upon successful URL creation.
137-235: Middleware additions are complete and in the correct stack order.All relevant logic for serialization, deserialization, logging, endpoint resolution, and finalizers is included, ensuring robust operation handling.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_AddPermission.go (1)
1-244: Code generation is well-structured and consistent.The overall design of this operation aligns with typical AWS Go SDK patterns, including validation, serialization, and middleware setup. No immediate issues are apparent.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListLayerVersions.go (1)
1-281: Implementation aligns with standard AWS Go SDK pagination patterns.This file properly configures serialization, deserialization, middleware, and paginators for listing layer versions. The structure is consistent with other AWS SDK operations.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionEventInvokeConfigs.go (2)
32-58: Validate required fields at runtime.
Although the documentation indicatesFunctionNameis required, there's no runtime check enforcing it. Typically, the AWS service call will fail if the name is missing, but consider adding a pre-check if you ever extend or wrap this logic for custom business requirements.
74-172: Avoid manual modification of auto-generated middlewares.
This middleware stack is part of the AWS SDK's code generation approach. Unless you have a compelling reason to do so, refrain from adjusting these auto-generated middlewares to prevent divergence from official SDK behavior.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListEventSourceMappings.go (2)
16-29: Confirm function name and event source ARN usage.
This method gracefully initializesparamsand then callsinvokeOperation. When you implement or call this API, ensure thatFunctionNameandEventSourceArn(if used) match your Lambda configuration to avoid subtle runtime issues.
[approve]
94-189: Standard AWS error handling approach.
This block includes the standard set of middlewares (request logging, retry, close response body, etc.). No changes are recommended because these follow the official AWS SDK patterns.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_Invoke.go (2)
75-127: Validate required fields more explicitly if needed.
FunctionNameis marked as required, but the code relies on back-end enforcement. If you intend to wrap or re-use this method in custom logic, clarify or enforce theFunctionNamerequirement to catch potential errors earlier.
157-255: Adhere to auto-generated structure for optimal compatibility.
All middlewares and finalizers are standard for AWS. Modifying them could lead to incompatibilities if the SDK is upgraded in the future. Keep the auto-generated structure intact unless you're bridging a specific internal requirement.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_InvokeWithResponseStream.go (2)
24-37: Evaluate concurrency safety forInvokeWithResponseStreamusage.Although this function is auto-generated, please ensure that concurrent invocations are handled properly. In particular, verify that the defaulting of
paramsto a newInvokeWithResponseStreamInput(lines 25-26) will not introduce shared state across goroutines if the client is reused.
114-209: Ensure correct ordering of middlewares inaddOperationInvokeWithResponseStreamMiddlewares.Many middlewares are stacked here to handle serialization, deserialization, retry, logging, etc. Confirm that the final order (lines 115-208) satisfies all dependencies, especially any that must run “Before” or “After” others for correct operation.
pb/c1/transport/v1/transport.pb.validate.go (3)
38-125: Validation logic forRequestis well-structured.The generated code enforces embedded message validations (lines 61-88, 90-117) effectively. No major issues spotted.
126-195:RequestMultiErrorandRequestValidationErrorappear consistent.They provide meaningful error details (lines 132-137, 167-184). Good approach for aggregated validation errors.
196-340: Verify completeness ofResponsevalidation.Double-check that all embedded fields (like
Resp,Status,Headers,Trailers) are validated and produce descriptive errors if missing or invalid.vendor/github.com/aws/aws-lambda-go/lambda/handler.go (7)
19-34: Recommend safety checks around sharedhandlerOptions.The
handlerOptionsstruct (lines 23-34) might be reused across multiple invocations if not carefully managed. Confirm that concurrent modifications to fields (e.g.,contextValues) don’t occur at runtime.
104-118:WithUseNumber: Ensure consistent numeric parsing behaviors.Enabling
UseNumbermay cause differences in how decimals or large integers are parsed. Provide documentation or examples for large integer handling within the function’s doc comment.
120-134: EnsureWithDisallowUnknownFieldsalignment with other decoding settings.This option can raise errors for extraneous fields. If used with partial updates, confirm that the presence of additional or unrecognized fields in the event does not break standard usage.
157-187:handlerTakesContextusage.This function’s checks are thorough (lines 157-187). Consider adding test coverage to ensure correct detection of invalid handlers, especially for multiple argument scenarios.
189-206:validateReturnsensures correct error signature.Returning more than two values or a second non-error is rejected (lines 193-198). This is good validation.
208-248:newHandler: Confirm no repeated registration of SIGTERM.If
newHandler(lines 226-248) is invoked multiple times withWithEnableSIGTERM, ensure thatenableSIGTERM(h.sigtermCallbacks)is idempotent to avoid repeated signal handling registration.
250-274: Close I/O resources on errors forhandlerFunc.Invoke.Lines 254-274 handle response reading and closure. If an error occurs before reading, ensure the resource is closed to avoid leaks.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetEventSourceMapping.go (4)
17-30: **Clean AWS pattern for retrieving event source mapping **This function follows the established AWS Go SDK patterns for retrieving an event source mapping. No issues identified.
32-41: **Verify required field usage **The
UUIDfield is marked as required, but there's no explicit check here. Confirm that upstream validation ensures this field is set before calling.
42-213: **All fields appear consistent with AWS model definitions **These output fields match the AWS data model for event source mappings, and no issues are apparent.
215-313: **Middleware stack configuration is standard **The layered middleware approach is consistent with AWS Smithy-go usage, and no additional concerns emerged.
internal/connector/connector.go (5)
172-176: **Concise options struct usage **Creating
optsfromRegisterOpscentralizes registration configuration. The approach is clear and avoids scattering setup logic.
324-324: **Delegates client creation **Invoking
NewConnectorClientat this point is straightforward and helps maintain a single point of client construction logic.
355-360: **Well-defined struct for optional features **Defining
RegisterOpsfor rate limiting, provisioning, and ticketing toggles is a clean approach to feature gating.
362-398: **Modular registration with fallback **Registering actual or noop services based on flags is a good pattern for toggling features without complicating consumers.
400-418: **Centralized ConnectorClient creation **This function cleanly bundles all the gRPC service clients into a single connector client, improving maintainability.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteEventSourceMapping.go (4)
21-34: **Follows AWS delete operation conventions **This is consistent with the AWS SDK style for a delete operation, including safe handling of nil params.
36-44: **Required field check **Although
UUIDis stated as required, please verify that any upstream calling code ensures it is set.
46-217: **Output type matches AWS definitions **All fields align with typical Lambda event source mapping properties; no immediate issues found.
219-317: **Appropriate middleware stack usage **The addition of serializing, deserializing, and error-handling middlewares is standard for AWS SDK operations.
pb/c1/connectorapi/baton/v1/config.pb.validate.go (1)
1-3: Auto-generated validation code
Since this file is generated using protoc tools, manual edits are typically discouraged. Its structure and method signatures conform to conventional patterns for message validations.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateEventSourceMapping.go (2)
1-2: Vendor code notice
This file resides under thevendordirectory and is part of AWS SDK auto-generated code. Typically, direct modifications or deep code reviews are unnecessary since updates are managed via dependency versioning.
82-95: Verify pinned SDK versions and security advisories
Ensure that you are pinning a supported and secure AWS SDK version. Monitor relevant security advisories to confirm that this library remains up to date and free from known vulnerabilities.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateFunction.go (2)
1-2: Vendor code notice
This file is also part of the AWS SDK. Manual changes are generally discouraged, as they can complicate future upgrades.
74-86: Check pinned SDK versions for CreateFunction enhancements
Be sure that you are aware of any upstream changes or additional parameters introduced in newer AWS Lambda versions, particularly if you rely on new features like SnapStart or improved logging.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunction.go (5)
1-2: No issues with generated disclaimer.
This file is auto-generated, so refrain from manual edits.
21-34: Validate default parameter values more thoroughly.
Whenparamsisnil, it’s replaced with an emptyGetFunctionInput. Ensure that no required fields are silently omitted or remain unset, leading to potential unexpected results later.
91-189: Middleware addition looks comprehensive.
This function properly layers middlewares for serialization, deserialization, endpoint resolution, and logging. The structure follows AWS SDK patterns and appears robust.
233-346: Good Waiter design for FunctionActiveV2.
The exponential backoff strategy and context timeout approach align with best practices for asynchronous operation polling. No concerns noted.
558-575: Confirm error handling for ResourceNotFoundException.
When this exception occurs, the waiter continues and treats it as a retryable condition. Ensure this aligns with the intended flow (i.e., the function might not yet exist).vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_client.go (5)
1-2: No concerns with generated disclaimer.
This file is also auto-generated code, manual edits are generally discouraged.
36-61: Operation metrics approach is solid.
Capturing duration metrics at various steps (serialization, signing, etc.) is useful for performance analysis. The approach of a centraloperationMetricsstruct is consistent with instrumentation best practices.
163-214: Client initialization logic is thorough.
TheNewfunction applies standard AWS SDK defaults, resolves logger, retryer, and endpoint correctly. No issues identified.
555-567: User-agent injection is well-managed.
Appending standard fields and application-specific identifiers fosters better usage tracking and analytics.
621-643: Trace instrumentation is straightforward.
Adding a custom span for the retry loop is a pleasant addition, providing insight into repeated attempts. Implementation appears consistent with standard tracing practices.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionConfiguration.go (6)
1-2: No concerns with generated disclaimer.
This file is marked as auto-generated, so modifications should be handled with caution.
21-35: Consider usage of default values.
Like the other operations, whenparamsisnil, a freshGetFunctionConfigurationInputis used. Verify that any mandatory fields are properly handled downstream.
63-235: Comprehensive field coverage in output struct.
All relevant Lambda function configuration fields (environment, ephemeral storage, VPC config, etc.) seem to be present and match known AWS docs. No immediate concerns.
237-335: Middleware stacking is consistent.
Similar toGetFunction, the middlewares handle logging, endpoint resolution, request ID retrieval, retries, etc. The additive approach aligns well with AWS SDK design.
379-492: Waiter logic for FunctionActive includes adjustable min delay.
The default 5s interval is higher than other waiters’ 1s default. This may be intentional to reduce AWS calls, but ensure it’s acceptable for user experience.
689-725: Proper last update status checks.
You handle "Successful", "Failed", and "InProgress" states consistently. This helps to correctly identify function configuration readiness.pkg/lambda/grpc/transport/local.go (3)
9-11: LGTM! Well-structured transport type.The
localTransportstruct is well-designed with a clear single responsibility of holding the server reference.
13-15: LGTM! Clean RoundTrip implementation.The
RoundTripmethod follows good practices:
- Takes appropriate context parameter
- Uses proper types from the gRPC package
- Direct delegation to server handler without unnecessary complexity
17-22: LGTM! Well-documented constructor.The
NewLocalClientTransportfunction is well-documented and follows the constructor pattern correctly by returning the interface type.vendor/github.com/aws/aws-lambda-go/lambda/entry_generic.go (3)
1-3: LGTM! Proper build constraints.The build tags correctly specify Go 1.18 requirement for generics support.
12-15: LGTM! Well-designed generic interface.The
HandlerFuncinterface effectively uses generics to provide compile-time type safety for Lambda handlers.
17-21: LGTM! Clean generic start function.The
StartHandlerFuncfunction:
- Is well-documented
- Properly constrains generic types
- Correctly delegates to the underlying start function
vendor/github.com/aws/aws-lambda-go/lambda/errors.go (2)
11-17: LGTM! Clear error type determination.The
getErrorTypefunction correctly handles both pointer and non-pointer error types.
35-46: LGTM! Comprehensive panic handling.The
lambdaPanicResponsefunction properly:
- Handles direct
InvokeResponse_Errorcases- Captures stack traces
- Sets appropriate exit flag
vendor/github.com/aws/aws-lambda-go/lambda/messages/messages.go (6)
7-11: LGTM! Clear ping message types.Empty structs are appropriate for ping messages that don't need to carry data.
13-17: LGTM! Well-structured timestamp type.The timestamp structure correctly represents both seconds and nanoseconds components.
19-29: LGTM! Comprehensive invoke request type.The
InvokeRequeststruct properly captures all necessary Lambda invocation parameters.
31-34: LGTM! Clean response structure.The
InvokeResponsetype appropriately handles both successful and error cases.
36-46: LGTM! Well-designed error type.The error type:
- Uses appropriate JSON tags
- Includes stack trace support
- Implements the error interface
48-53: LGTM! Clear stack frame structure.The stack frame type captures all necessary debugging information with proper JSON tags.
vendor/github.com/aws/aws-lambda-go/lambda/handlertrace/trace.go (2)
11-14: LGTM! Well-structured HandlerTrace type.The HandlerTrace struct is well-designed with clear function signatures for request and response events.
16-25: LGTM! Robust callback composition.The callbackCompose function correctly handles nil checks and ensures proper execution order of callbacks.
pkg/lambda/grpc/client.go (2)
52-64:❓ Verification inconclusive
Complete the TODO regarding CallOption handling.
The comment indicates that some call options might be important (e.g., PerRPCCredsCallOption). Consider implementing these for complete gRPC compatibility.
🏁 Script executed:
#!/bin/bash # Check how other gRPC clients handle CallOptions ast-grep --pattern 'func $_(ctx context.Context, $_, $_, $_, opts ...grpc.CallOption) error { $$$ for _, opt := range opts { switch o := opt.(type) { case grpc.PerRPCCredsCallOption: $$$ } } }'Length of output: 223
Action Required: Review gRPC CallOption Handling
It appears that the current implementation only handles header and trailer options and does not address
grpc.PerRPCCredsCallOption. Our initial AST search for similar handling in other parts of the codebase (as well as patterns from comparable clients) did not yield any conclusive examples. Please review the official gRPC examples and documentation to determine the correct handling strategy forPerRPCCredsCallOption—either implementing it similarly to headers/trailers or explicitly documenting why it is omitted.
- File: pkg/lambda/grpc/client.go (Lines 52-64)
- Action: Complete the TODO by adding appropriate handling for
grpc.PerRPCCredsCallOptionor update the comment with a rationale if handling is not required.
24-26:❓ Verification inconclusive
Address the TODO comment regarding context deadline handling.
The comment suggests implementing context deadline detection and gRPC timeout settings. This is important for proper timeout handling in gRPC calls.
🏁 Script executed:
#!/bin/bash # Check if context deadline handling is implemented in similar gRPC clients ast-grep --pattern 'func $_(ctx context.Context, $_, $_, $_, $_) error { $$$ deadline, ok := ctx.Deadline() $$$ }'Length of output: 122
Subject: Address Missing Context Deadline Handling
The code in pkg/lambda/grpc/client.go currently does not implement logic to detect context deadlines or set corresponding gRPC timeout settings. Our search for usage of
ctx.Deadline()in similar gRPC client functions did not return any matches, suggesting that this functionality is likely absent elsewhere in the codebase.Recommendations:
- Add context deadline detection: Consider retrieving the deadline with code such as:
deadline, ok := ctx.Deadline() if ok { // Convert deadline to grpc timeout (e.g., via grpc-timeout metadata) and set it in the outgoing context. }- Review consistency: Verify whether other gRPC client implementations in the project should have similar logic. If not, confirm that the omission is intentional.
Please review this change manually to confirm that adding the context deadline handling aligns with the overall project design and that it effectively manages timeouts as intended.
vendor/github.com/aws/aws-lambda-go/lambdacontext/context.go (2)
30-40: LGTM! Proper initialization of Lambda environment variables.The
init()function correctly retrieves and initializes Lambda environment variables. The error handling for memory limit parsing is appropriate.
71-78: LGTM! Well-implemented context key protection.Good practice using an unexported type for the context key to prevent key collisions across packages.
vendor/github.com/aws/aws-sdk-go-v2/aws/signer/internal/v4/headers.go (1)
7-11: LGTM! Appropriate addition to ignored headers list.Adding "Transfer-Encoding" to IgnoredHeaders is correct as this header is managed by the HTTP transport layer and should not be part of the signature calculation.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetCodeSigningConfig.go (2)
14-28: LGTM! Proper parameter handling and error management.The implementation correctly handles nil parameters and properly manages operation results and metadata.
53-151: LGTM! Comprehensive middleware stack implementation.The middleware stack includes all necessary components for AWS operations:
- Request/response logging
- Error handling
- Retry logic
- Authentication
- Metrics collection
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteLayerVersion.go (3)
13-18: LGTM! Well-documented API with proper hyperlinks.The documentation clearly explains the operation's behavior and includes a helpful hyperlink to Lambda layer documentation.
33-46: LGTM! Clear parameter requirements.Input struct properly defines required fields with clear documentation.
55-153: LGTM! Comprehensive middleware stack implementation.The middleware stack includes all necessary components for AWS operations:
- Request/response logging
- Error handling
- Retry logic
- Authentication
- Metrics collection
pkg/lambda/grpc/util.go (1)
88-95: LGTM! Well-implemented generic conversion function.The Convert function is a clean implementation of a generic slice transformation.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteAlias.go (1)
1-171: LGTM! Generated code follows AWS SDK standards.The generated code for the DeleteAlias operation is well-structured and follows AWS SDK conventions.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetLayerVersionPolicy.go (1)
1-167: LGTM! Generated code follows AWS SDK standards.The generated code for the GetLayerVersionPolicy operation is well-structured and follows AWS SDK conventions.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionConcurrency.go (1)
1-164: LGTM! Generated code follows AWS SDK standards.The generated code for the DeleteFunctionConcurrency operation is well-structured and follows AWS SDK conventions.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunctionCodeSigningConfig.go (3)
13-27: LGTM! The operation implementation is well-structured.The DeleteFunctionCodeSigningConfig operation is correctly implemented with proper error handling and parameter validation.
29-55: LGTM! The input/output structs are well-defined.The structs are properly documented with clear field descriptions and validation requirements.
57-155:✅ Verification successful
Verify HTTPS disablement in middleware stack.
The middleware stack includes a potentially concerning
addDisableHTTPSMiddleware. This could affect security if HTTPS is disabled.
🏁 Script executed:
#!/bin/bash # Search for HTTPS disablement configuration rg -A 5 "addDisableHTTPSMiddleware" .Length of output: 70568
Confirmed: HTTPS disablement middleware usage is as intended.
After reviewing the code and verifying its usage across the SDK (e.g., in S3, STS, etc.), it’s clear that the call to
addDisableHTTPSMiddleware(stack, options)in the Lambda client'sDeleteFunctionCodeSigningConfigoperation is consistent with the overall design. This middleware is applied based on the configuration controlled byoptions.EndpointOptions.DisableHTTPS. In other words, HTTPS will only be disabled if explicitly configured. Ensure that your endpoint options are correctly set in production to maintain secure communications.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionConcurrency.go (2)
13-28: LGTM! The operation implementation is well-structured.The GetFunctionConcurrency operation is correctly implemented with proper error handling and parameter validation.
30-60: LGTM! The input/output structs are well-defined.The structs are properly documented with clear field descriptions and validation requirements. The ReservedConcurrentExecutions field in the output struct correctly uses int32 for concurrency limits.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteProvisionedConcurrencyConfig.go (2)
13-27: LGTM! The operation implementation is well-structured.The DeleteProvisionedConcurrencyConfig operation is correctly implemented with proper error handling and parameter validation.
29-60: LGTM! The input/output structs are well-defined.The structs are properly documented with clear field descriptions. Both required fields (FunctionName and Qualifier) are correctly marked as required.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteFunction.go (2)
13-33: LGTM! The operation implementation and documentation are comprehensive.The DeleteFunction operation is well-documented with clear explanations of the deletion behavior and dependencies. The implementation includes proper error handling and parameter validation.
35-66: LGTM! The input/output structs are well-defined.The structs are properly documented with clear field descriptions. The FunctionName field is correctly marked as required, and the Qualifier field's documentation clearly explains its impact on version deletion.
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionCodeSigningConfig.go (3)
14-27: LGTM! The GetFunctionCodeSigningConfig implementation is well-structured.The method properly handles nil parameters and includes appropriate error handling.
29-48: LGTM! Input structure is well-defined with clear documentation.The required fields are properly marked and documented with comprehensive examples.
79-177: LGTM! Comprehensive middleware setup with proper error handling.The middleware stack includes all necessary components for AWS operations:
- Serialization/deserialization
- Error handling
- Request/response logging
- Retry mechanisms
- Security features
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_AddLayerVersionPermission.go (3)
13-20: LGTM! Well-documented security implications for layer permissions.The documentation clearly explains the scope and impact of granting layer permissions to other accounts.
37-78: LGTM! Input structure has proper validation constraints.Required fields are clearly marked and documented:
- Action
- LayerName
- Principal
- StatementId
- VersionNumber
94-177: LGTM! Comprehensive middleware setup with security considerations.The middleware stack includes essential components for secure AWS operations:
- Error handling
- Request validation
- Security context
- Logging
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateAlias.go (3)
14-21: LGTM! Well-documented traffic routing capabilities.The documentation clearly explains the alias functionality and traffic shifting capabilities.
37-74: LGTM! Input structure with proper validation for routing configuration.The structure properly handles:
- Function name/ARN validation
- Version requirements
- Optional routing configuration
107-205: LGTM! Comprehensive middleware setup with proper request handling.The middleware stack includes all necessary components:
- Serialization/deserialization
- Error handling
- Request/response logging
- Security context
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetFunctionUrlConfig.go (2)
54-113: LGTM! Comprehensive output structure with security considerations.The structure properly handles:
- Authentication type (AWS_IAM/NONE)
- CORS settings
- Invocation modes (BUFFERED/RESPONSE_STREAM)
- Required timestamps in ISO-8601 format
115-213: LGTM! Robust middleware setup with security features.The middleware stack includes essential components:
- Input validation
- Error handling
- Security context
- Request/response logging
pkg/field/defaults.go (2)
51-55: LGTM! The Lambda server OAuth fields are well-defined.The fields follow consistent patterns with other fields in the file and include appropriate descriptions. The
WithRequired(true)flag correctly enforces these mandatory OAuth credentials.
58-63: LGTM! The helper function provides a clean interface.The
LambdaServerFields()function provides a clean way to access the Lambda server configuration fields.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListCodeSigningConfigs.go (1)
1-257: LGTM! Generated code follows AWS SDK conventions.The implementation correctly follows AWS SDK patterns for API operations, including:
- Proper error handling
- Middleware setup
- Pagination support
- Documentation links
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListAliases.go (1)
1-276: LGTM! Generated code follows AWS SDK conventions.The implementation correctly follows AWS SDK patterns for API operations, including:
- Proper error handling
- Middleware setup
- Pagination support
- Documentation links
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListLayers.go (1)
1-273: LGTM! Generated code follows AWS SDK conventions.The implementation correctly follows AWS SDK patterns for API operations, including:
- Proper error handling
- Middleware setup
- Pagination support
- Documentation links
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionsByCodeSigningConfig.go (1)
1-279: Implementation follows AWS SDK for Go v2 best practices.The implementation of
ListFunctionsByCodeSigningConfigoperation is well-structured and follows AWS SDK for Go v2 patterns:
- Proper error handling and middleware setup
- Consistent pagination implementation with a maximum of 50 items per response
- Required field validation through middleware
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctions.go (1)
1-275: Implementation follows AWS SDK for Go v2 best practices.The implementation of
ListFunctionsoperation is well-structured and follows AWS SDK for Go v2 patterns:
- Proper error handling and middleware setup
- Consistent pagination implementation with a maximum of 50 items per response
- Proper handling of version filtering through
FunctionVersionfieldvendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListFunctionUrlConfigs.go (1)
1-279: Implementation follows AWS SDK for Go v2 best practices.The implementation of
ListFunctionUrlConfigsoperation is well-structured and follows AWS SDK for Go v2 patterns:
- Proper error handling and middleware setup
- Consistent pagination implementation with a maximum of 50 items per response
- Required field validation through middleware
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListVersionsByFunction.go (1)
1-280: Implementation follows AWS SDK for Go v2 best practices.The implementation of
ListVersionsByFunctionoperation is well-structured and follows AWS SDK for Go v2 patterns:
- Proper error handling and middleware setup
- Consistent pagination implementation with a maximum of 50 items per response
- Required field validation through middleware
vendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_ListProvisionedConcurrencyConfigs.go (1)
1-275: Generated code looks good!The file is properly generated and follows AWS SDK patterns. The implementation includes:
- Proper error handling
- Input/output structs with documentation
- Middleware stack configuration
- Pagination support
vendor/github.com/aws/aws-sdk-go-v2/aws/middleware/user_agent.go (1)
79-111: LGTM! New user agent features are well documented.The changes add new feature flags with clear documentation about their implementation status and purpose.
pkg/config/config.go (1)
53-96: LGTM! Well-structured Lambda server configuration implementation.The new
DefineLambdaServerConfigurationfunction follows existing patterns and properly handles:
- Environment variables
- Command setup
- Flag configuration
proto/c1/transport/v1/transport.proto (1)
11-22: LGTM! Well-structured transport messages.The Request and Response message definitions are clear and use appropriate protobuf types for headers, trailers, and payloads.
proto/c1/connectorapi/baton/v1/config.proto (3)
1-9: Proto Declaration and Imports
The proto syntax, package declaration, import statements, and Go package option are correctly specified. Be sure that the Go package path ("gitlab.com/ductone/c1/pkg/pb/c1/connectorapi/baton/v1") aligns with your project’s directory structure.
10-12: Service Definition Validation
The new serviceConnectorConfigServicealong with its RPC methodGetConnectorConfigis defined clearly. Ensure that the corresponding server and client implementations will appropriately use this method.
26-31: Extended Request Message Naming
The messageSigv4SignedRequestSTSGetCallerIdentityis descriptive though lengthy. Its naming clearly indicates its purpose. Ensure that documentation elsewhere explains when and how to use this message.vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/CHANGELOG.md (1)
1-8: Changelog Updates for v1.6.10 and v1.6.9
The entries for v1.6.10 (bug fix and Go version bump to 1.22) and v1.6.9 (removal of the max limit on event stream messages) are clear and succinct. These changes appear consistent with the dependency updates made across the project.go.mod (6)
3-3: Go Version Update
The Go version is now set to1.23.4. Ensure that your build/test pipelines are compatible with this version update.
7-8: Dependency Updates: AWS Lambda & SDK Core
The addition ofgithub.com/aws/aws-lambda-go v1.47.0and the update togithub.com/aws/aws-sdk-go-v2 v1.36.2are properly noted. Confirm that the Lambda commands and client instantiations throughout the codebase correctly reference these versions.
12-12: New AWS Lambda Service Dependency
The addition ofgithub.com/aws/aws-sdk-go-v2/service/lambda v1.69.14supports new Lambda functionalities. Verify that associated client code uses the updated APIs.
16-16: Additional Dependency Additions
The dependencies forgithub.com/conductorone/dpop v0.0.1andgithub.com/google/uuid v1.6.0are now included. These additions seem to support new features.Also applies to: 23-23
27-27: Mapstructure Dependency Addition
The inclusion ofgithub.com/mitchellh/mapstructure v1.5.0is noted.
Past review comment reminder: "We don't need this, it was convenient."
Please confirm this dependency is necessary; if it duplicates existing functionality, consider its removal.
59-62: Indirect Dependency Updates
The updates in the indirect dependency block—such asgithub.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.10,github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.33, andgithub.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.33—are consistent with the core dependency upgrades. It would be good to run a full build and test suite to ensure no compatibility issues arise.vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.json (1)
226-236: Enhanced Regional Definitions for aws-iso-f Partition
The updatedregionsobject for theaws-iso-fpartition now explicitly lists "aws-iso-f-global", "us-isof-east-1", and "us-isof-south-1" along with their descriptions. This enhancement improves clarity and should allow client code to better handle region-specific logic for ISOF.vendor/github.com/aws/aws-sdk-go-v2/service/lambda/LICENSE.txt (1)
1-203: Overall License File Inclusion ApprovedThis new file provides the full Apache License, Version 2.0 text. The standard terms and sections are completely and correctly incorporated, adhering to industry requirements.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~151-~151: Possible missing comma found.
Context: ...iateness of using or redistributing the Work and assume any risks associated w...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
| func (r *TransportStream) Response() (*Response, error) { | ||
| if r.status == nil { | ||
| return nil, status.Errorf(codes.Internal, "error marshalling response: status not set") | ||
| } | ||
|
|
||
| headers, err := MarshalMetadata(r.headers) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | ||
| } | ||
| trailers, err := MarshalMetadata(r.trailers) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | ||
| } | ||
|
|
||
| var anyResp *anypb.Any | ||
| if r.response != nil { | ||
| anyResp, err = anypb.New(r.response) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "error marshalling response: %v", err) | ||
| } | ||
| } | ||
|
|
||
| anyStatus, err := anypb.New(r.status) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "error marshalling status: %v", err) | ||
| } | ||
|
|
||
| return &Response{ | ||
| msg: &pbtransport.Response{ | ||
| Resp: anyResp, | ||
| Status: anyStatus, | ||
| Headers: headers, | ||
| Trailers: trailers, | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Add concurrency locking in Response() method.
Access to the headers, trailers, response, and status fields in Response() is not guarded by the mutex, unlike the setter methods (SetHeader(), SetTrailer(), etc.). Concurrent use of these methods can lead to data races. Consider locking/unlocking around these fields to ensure thread safety.
Example fix:
func (r *TransportStream) Response() (*Response, error) {
+ r.mtx.Lock()
+ defer r.mtx.Unlock()
if r.status == nil {
return nil, status.Errorf(codes.Internal, "error marshalling response: status not set")
}
// ...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *TransportStream) Response() (*Response, error) { | |
| if r.status == nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling response: status not set") | |
| } | |
| headers, err := MarshalMetadata(r.headers) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | |
| } | |
| trailers, err := MarshalMetadata(r.trailers) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | |
| } | |
| var anyResp *anypb.Any | |
| if r.response != nil { | |
| anyResp, err = anypb.New(r.response) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling response: %v", err) | |
| } | |
| } | |
| anyStatus, err := anypb.New(r.status) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling status: %v", err) | |
| } | |
| return &Response{ | |
| msg: &pbtransport.Response{ | |
| Resp: anyResp, | |
| Status: anyStatus, | |
| Headers: headers, | |
| Trailers: trailers, | |
| }, | |
| }, nil | |
| } | |
| func (r *TransportStream) Response() (*Response, error) { | |
| r.mtx.Lock() | |
| defer r.mtx.Unlock() | |
| if r.status == nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling response: status not set") | |
| } | |
| headers, err := MarshalMetadata(r.headers) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | |
| } | |
| trailers, err := MarshalMetadata(r.trailers) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling headers: %v", err) | |
| } | |
| var anyResp *anypb.Any | |
| if r.response != nil { | |
| anyResp, err = anypb.New(r.response) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling response: %v", err) | |
| } | |
| } | |
| anyStatus, err := anypb.New(r.status) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "error marshalling status: %v", err) | |
| } | |
| return &Response{ | |
| msg: &pbtransport.Response{ | |
| Resp: anyResp, | |
| Status: anyStatus, | |
| Headers: headers, | |
| Trailers: trailers, | |
| }, | |
| }, nil | |
| } |
pkg/lambda/lambda.go
Outdated
| // NewLambdaClientTransport returns a new client transport that invokes a lambda function. | ||
| func NewLambdaClientTransport(ctx context.Context, client *lambda.Client, functionName string) (lambda_grpc.ClientTransport, error) { | ||
| return &lambdaTransport{ | ||
| lambdaClient: client, | ||
| functionName: functionName, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused context parameter.
The context parameter in NewLambdaClientTransport is not being used.
-func NewLambdaClientTransport(ctx context.Context, client *lambda.Client, functionName string) (lambda_grpc.ClientTransport, error) {
+func NewLambdaClientTransport(client *lambda.Client, functionName string) (lambda_grpc.ClientTransport, error) {
return &lambdaTransport{
lambdaClient: client,
functionName: functionName,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewLambdaClientTransport returns a new client transport that invokes a lambda function. | |
| func NewLambdaClientTransport(ctx context.Context, client *lambda.Client, functionName string) (lambda_grpc.ClientTransport, error) { | |
| return &lambdaTransport{ | |
| lambdaClient: client, | |
| functionName: functionName, | |
| }, nil | |
| } | |
| // NewLambdaClientTransport returns a new client transport that invokes a lambda function. | |
| func NewLambdaClientTransport(client *lambda.Client, functionName string) (lambda_grpc.ClientTransport, error) { | |
| return &lambdaTransport{ | |
| lambdaClient: client, | |
| functionName: functionName, | |
| }, nil | |
| } |
pkg/lambda/lambda.go
Outdated
| func (l *lambdaTransport) RoundTrip(ctx context.Context, req *lambda_grpc.Request) (*lambda_grpc.Response, error) { | ||
| payload, err := req.MarshalJSON() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("lambda_transport: failed to marshal frame: %w", err) | ||
| } | ||
|
|
||
| input := &lambda.InvokeInput{ | ||
| FunctionName: aws.String(l.functionName), | ||
| Payload: payload, | ||
| } | ||
|
|
||
| // Invoke the Lambda function. | ||
| invokeResp, err := l.lambdaClient.Invoke(ctx, input) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("lambda_transport: failed to invoke lambda function: %w", err) | ||
| } | ||
|
|
||
| // Check if the function returned an error. | ||
| if invokeResp.FunctionError != nil { | ||
| return nil, fmt.Errorf("lambda_transport: function returned error: %v", *invokeResp.FunctionError) | ||
| } | ||
|
|
||
| resp := &lambda_grpc.Response{} | ||
| err = json.Unmarshal(invokeResp.Payload, resp) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("lambda_transport: failed to unmarshal response: %w", err) | ||
| } | ||
|
|
||
| return resp, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and fix return value inconsistency.
The RoundTrip method has a few issues to address:
- The final return statement uses
errwhich could be nil at this point. - Error wrapping could be more specific for better debugging.
func (l *lambdaTransport) RoundTrip(ctx context.Context, req *lambda_grpc.Request) (*lambda_grpc.Response, error) {
payload, err := req.MarshalJSON()
if err != nil {
- return nil, fmt.Errorf("lambda_transport: failed to marshal frame: %w", err)
+ return nil, fmt.Errorf("lambda_transport: failed to marshal request: %w", err)
}
input := &lambda.InvokeInput{
FunctionName: aws.String(l.functionName),
Payload: payload,
}
// Invoke the Lambda function.
invokeResp, err := l.lambdaClient.Invoke(ctx, input)
if err != nil {
- return nil, fmt.Errorf("lambda_transport: failed to invoke lambda function: %w", err)
+ return nil, fmt.Errorf("lambda_transport: failed to invoke lambda function %s: %w", l.functionName, err)
}
// Check if the function returned an error.
if invokeResp.FunctionError != nil {
- return nil, fmt.Errorf("lambda_transport: function returned error: %v", *invokeResp.FunctionError)
+ return nil, fmt.Errorf("lambda_transport: function %s returned error: %v", l.functionName, *invokeResp.FunctionError)
}
resp := &lambda_grpc.Response{}
err = json.Unmarshal(invokeResp.Payload, resp)
if err != nil {
- return nil, fmt.Errorf("lambda_transport: failed to unmarshal response: %w", err)
+ return nil, fmt.Errorf("lambda_transport: failed to unmarshal response from %s: %w", l.functionName, err)
}
- return resp, err
+ return resp, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (l *lambdaTransport) RoundTrip(ctx context.Context, req *lambda_grpc.Request) (*lambda_grpc.Response, error) { | |
| payload, err := req.MarshalJSON() | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to marshal frame: %w", err) | |
| } | |
| input := &lambda.InvokeInput{ | |
| FunctionName: aws.String(l.functionName), | |
| Payload: payload, | |
| } | |
| // Invoke the Lambda function. | |
| invokeResp, err := l.lambdaClient.Invoke(ctx, input) | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to invoke lambda function: %w", err) | |
| } | |
| // Check if the function returned an error. | |
| if invokeResp.FunctionError != nil { | |
| return nil, fmt.Errorf("lambda_transport: function returned error: %v", *invokeResp.FunctionError) | |
| } | |
| resp := &lambda_grpc.Response{} | |
| err = json.Unmarshal(invokeResp.Payload, resp) | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to unmarshal response: %w", err) | |
| } | |
| return resp, err | |
| } | |
| func (l *lambdaTransport) RoundTrip(ctx context.Context, req *lambda_grpc.Request) (*lambda_grpc.Response, error) { | |
| payload, err := req.MarshalJSON() | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to marshal request: %w", err) | |
| } | |
| input := &lambda.InvokeInput{ | |
| FunctionName: aws.String(l.functionName), | |
| Payload: payload, | |
| } | |
| // Invoke the Lambda function. | |
| invokeResp, err := l.lambdaClient.Invoke(ctx, input) | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to invoke lambda function %s: %w", l.functionName, err) | |
| } | |
| // Check if the function returned an error. | |
| if invokeResp.FunctionError != nil { | |
| return nil, fmt.Errorf("lambda_transport: function %s returned error: %v", l.functionName, *invokeResp.FunctionError) | |
| } | |
| resp := &lambda_grpc.Response{} | |
| err = json.Unmarshal(invokeResp.Payload, resp) | |
| if err != nil { | |
| return nil, fmt.Errorf("lambda_transport: failed to unmarshal response from %s: %w", l.functionName, err) | |
| } | |
| return resp, nil | |
| } |
| func (p messagePrelude) ValidateLens() error { | ||
| if p.Length == 0 || p.Length > maxMsgLen { | ||
| if p.Length == 0 { | ||
| return LengthError{ | ||
| Part: "message prelude", | ||
| Want: maxMsgLen, | ||
| Want: minMsgLen, | ||
| Have: int(p.Length), | ||
| } | ||
| } | ||
| if p.HeadersLen > maxHeadersLen { | ||
| return LengthError{ | ||
| Part: "message headers", | ||
| Want: maxHeadersLen, | ||
| Have: int(p.HeadersLen), | ||
| } | ||
| } | ||
| if payloadLen := p.PayloadLen(); payloadLen > maxPayloadLen { | ||
| return LengthError{ | ||
| Part: "message payload", | ||
| Want: maxPayloadLen, | ||
| Have: int(payloadLen), | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern: Removal of message size limits.
The removal of message size validation (maxPayloadLen, maxHeadersLen, maxMsgLen) could expose the system to potential memory exhaustion attacks. Consider maintaining reasonable size limits or implementing alternative safeguards.
🏁 Script executed:
#!/bin/bash
# Check if there are any other size limits or validations in place
rg -A 5 "max.*len|size.*limit" .Length of output: 65573
Security Concern: Missing Maximum Message Size Validation
The current implementation in vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream/message.go (lines 81–90) only verifies that p.Length is non-zero. It no longer enforces any upper bounds (e.g. maxPayloadLen, maxHeadersLen, maxMsgLen), potentially exposing the system to memory exhaustion attacks. While related size validations exist elsewhere in the code (such as header value length checks), the absence of a maximum message size check in this context poses a risk.
Recommendations:
- Reintroduce or implement an appropriate maximum message length validation to restrict incoming message sizes.
- Evaluate whether existing safeguards in related modules (e.g., header validations) are sufficient or if additional checks should be applied at the message level.
| go func() { | ||
| _, err := client.next(id) | ||
| log.Printf("WARNING! Reached expected unreachable code! Extension /next call expected to block forever! err: %v", err) | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the goroutine.
The goroutine that calls client.next() should handle panics to prevent silent failures.
go func() {
+ defer func() {
+ if r := recover(); r != nil {
+ log.Printf("WARNING! Panic in extension next handler: %v", r)
+ }
+ }()
_, err := client.next(id)
log.Printf("WARNING! Reached expected unreachable code! Extension /next call expected to block forever! err: %v", err)
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| _, err := client.next(id) | |
| log.Printf("WARNING! Reached expected unreachable code! Extension /next call expected to block forever! err: %v", err) | |
| }() | |
| go func() { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| log.Printf("WARNING! Panic in extension next handler: %v", r) | |
| } | |
| }() | |
| _, err := client.next(id) | |
| log.Printf("WARNING! Reached expected unreachable code! Extension /next call expected to block forever! err: %v", err) | |
| }() |
| type extensionEventResponse struct { | ||
| EventType extensionAPIEventType | ||
| // ... the rest not implemented | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Complete the extensionEventResponse struct implementation.
The struct is marked as incomplete with a TODO comment. This could lead to missing important event data.
🌐 Web query:
What fields are required in AWS Lambda Extension API event response?
💡 Result:
AWS Lambda Extensions interact with specific APIs during their lifecycle phases (INITIALIZATION, INVOKE, SHUTDOWN). Here are key requirements for responses across critical Extension API interactions:
Registration Phase (/extension/register)
Required Headers:
Lambda-Extension-Name: Full filename of the extension (e.g.,my-extension) [1][7].
Response Fields:
Lambda-Extension-Identifier: Unique UUID for subsequent requests [1][8].- Optional
accountIdif requested during registration [1].
Event Polling (/extension/event/next)
Required Headers:
Lambda-Extension-Identifier: UUID from registration [1][8].
Response Structure:
Depends on whether it’s an INVOKE or SHUTDOWN event:
INVOKEEvent: Includes invocation-specific metadata (e.g., request ID) and payload data [1][9].SHUTDOWNEvent: Containsreason(SPINDOWN,TIMEOUT, etc.) anddeadlineMstimestamp [1][7].
Error Reporting (/extension/exit/error)
Required Headers:
Lambda-Extension-Function-Error-Type: Error category/reason (e.g.,Extension.ConfigInvalid) [1][6].
Request Body:
Optional JSON with errorMessage, errorType, and stackTrace [1][6]:
{
"errorMessage": "Error parsing config",
"errorType": "Extension.ConfigError",
"stackTrace": []
}Key Observations
- No Mandatory Fields for
INVOKE/SHUTDOWNresponses beyond implicit identifiers likerequestId. Event-specific data depends on use case [1][9]. - Extensions must handle blocking
/event/nextcalls indefinitely until lifecycle events occur [1][4].
For detailed implementation examples using these APIs across languages like Python or C#, refer to AWS documentation [1][4][7] and GitHub samples [4].
Citations:
- 1: https://docs.aws.amazon.com/lambda/latest/dg/runtimes-extensions-api.html
- 2: https://docs.aws.amazon.com/lambda/latest/dg/extensions-configuration.html
- 3: https://lumigo.io/blog/aws-lambda-extensions-what-are-they-and-why-do-they-matter/
- 4: https://github.com/aws-samples/aws-lambda-extensions/blob/main/csharp-example-extension/ExtensionClient.cs
- 5: https://docs.logz.io/docs/shipping/AWS/Lambda-extensions/
- 6: https://docs.aws.amazon.com/lambda/latest/dg/runtimes-api.html
- 7: https://docs.aws.amazon.com/lambda/latest/dg/lambda-extensions.html
- 8: https://www.getorchestra.io/guides/extensions-api-in-aws-lambda
- 9: https://aws.amazon.com/blogs/compute/introducing-aws-lambda-extensions-in-preview/
- 10: https://docs.aws.amazon.com/lambda/latest/dg/runtimes-logs-api.html
Action Required: Complete the extensionEventResponse Struct to Capture All Required Event Data
Based on the AWS Lambda Extensions API documentation, the event response must include additional fields beyond just EventType. For example:
-
For INVOKE events:
- Include invocation-specific metadata such as a
requestId(or similar identifier) and any payload information that accompanies the invocation.
- Include invocation-specific metadata such as a
-
For SHUTDOWN events:
- Add fields to capture the shutdown details like
reason(e.g.,SPINDOWN,TIMEOUT) anddeadlineMs(timestamp).
- Add fields to capture the shutdown details like
These additions are needed to fully support the different event types returned by /extension/event/next and to ensure essential event data isn’t lost during processing.
| i := len(path) | ||
| for n, g := 0, strings.Count(label, "/")+2; n < g; n++ { | ||
| i = strings.LastIndex(path[:i], "/") | ||
| if i == -1 { | ||
| // Something went wrong and path has less seperators than we expected | ||
| // Abort and leave i as -1 to counteract the +1 below | ||
| break | ||
| } | ||
| } | ||
|
|
||
| path = path[i+1:] // Trim the initial / | ||
|
|
||
| // Strip the path from the function name as it's already in the path | ||
| label = label[strings.LastIndex(label, "/")+1:] | ||
| // Likewise strip the package name | ||
| label = label[strings.Index(label, ".")+1:] | ||
|
|
There was a problem hiding this comment.
Add bounds checking in path manipulation.
The path and label manipulation could panic if the input strings don't match expected formats.
i := len(path)
for n, g := 0, strings.Count(label, "/")+2; n < g; n++ {
i = strings.LastIndex(path[:i], "/")
if i == -1 {
break
}
}
path = path[i+1:] // Trim the initial /
-// Strip the path from the function name as it's already in the path
-label = label[strings.LastIndex(label, "/")+1:]
-// Likewise strip the package name
-label = label[strings.Index(label, ".")+1:]
+// Safely strip the path and package name
+if lastSlash := strings.LastIndex(label, "/"); lastSlash != -1 {
+ label = label[lastSlash+1:]
+}
+if lastDot := strings.Index(label, "."); lastDot != -1 {
+ label = label[lastDot+1:]
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| i := len(path) | |
| for n, g := 0, strings.Count(label, "/")+2; n < g; n++ { | |
| i = strings.LastIndex(path[:i], "/") | |
| if i == -1 { | |
| // Something went wrong and path has less seperators than we expected | |
| // Abort and leave i as -1 to counteract the +1 below | |
| break | |
| } | |
| } | |
| path = path[i+1:] // Trim the initial / | |
| // Strip the path from the function name as it's already in the path | |
| label = label[strings.LastIndex(label, "/")+1:] | |
| // Likewise strip the package name | |
| label = label[strings.Index(label, ".")+1:] | |
| i := len(path) | |
| for n, g := 0, strings.Count(label, "/")+2; n < g; n++ { | |
| i = strings.LastIndex(path[:i], "/") | |
| if i == -1 { | |
| // Something went wrong and path has less seperators than we expected | |
| // Abort and leave i as -1 to counteract the +1 below | |
| break | |
| } | |
| } | |
| path = path[i+1:] // Trim the initial / | |
| // Safely strip the path and package name | |
| if lastSlash := strings.LastIndex(label, "/"); lastSlash != -1 { | |
| label = label[lastSlash+1:] | |
| } | |
| if lastDot := strings.Index(label, "."); lastDot != -1 { | |
| label = label[lastDot+1:] | |
| } |
| func MarshalMetadata(md metadata.MD) (*structpb.Struct, error) { | ||
| x := &structpb.Struct{Fields: make(map[string]*structpb.Value, len(md))} | ||
| for k, v := range md { | ||
| lv, err := structpb.NewList(Convert(v, func(in string) any { return in })) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| x.Fields[k] = structpb.NewListValue(lv) | ||
| } | ||
| return x, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for metadata keys.
The metadata marshaling functions should validate key names according to gRPC metadata key naming rules (must be lowercase, can only contain specific characters).
func MarshalMetadata(md metadata.MD) (*structpb.Struct, error) {
x := &structpb.Struct{Fields: make(map[string]*structpb.Value, len(md))}
for k, v := range md {
+ if !isValidMetadataKey(k) {
+ return nil, fmt.Errorf("invalid metadata key: %q", k)
+ }
lv, err := structpb.NewList(Convert(v, func(in string) any { return in }))
if err != nil {
return nil, err
}
x.Fields[k] = structpb.NewListValue(lv)
}
return x, nil
}Also applies to: 109-129
| @@ -0,0 +1,22 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package c1.svc.transport.v1; | |||
There was a problem hiding this comment.
Fix package directory structure.
The package c1.svc.transport.v1 must be within a directory c1/svc/transport/v1 relative to root, but it's currently in c1/transport/v1.
Move the file to the correct directory structure:
mkdir -p c1/svc/transport/v1
mv c1/transport/v1/transport.proto c1/svc/transport/v1/🧰 Tools
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 3-3:
Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
🪛 Buf (1.47.2)
3-3: Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
(PACKAGE_DIRECTORY_MATCH)
| import "google/protobuf/timestamp.proto"; | ||
| import "google/protobuf/struct.proto"; | ||
| import "google/protobuf/any.proto"; |
There was a problem hiding this comment.
Remove unused import.
The google/protobuf/timestamp.proto import is not used in any message definitions.
-import "google/protobuf/timestamp.proto";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import "google/protobuf/timestamp.proto"; | |
| import "google/protobuf/struct.proto"; | |
| import "google/protobuf/any.proto"; | |
| import "google/protobuf/struct.proto"; | |
| import "google/protobuf/any.proto"; |
🧰 Tools
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 5-5:
Import "google/protobuf/timestamp.proto" is unused.
🪛 Buf (1.47.2)
5-5: Import "google/protobuf/timestamp.proto" is unused.
(IMPORT_USED)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
proto/c1/transport/v1/transport.proto (1)
3-3:⚠️ Potential issuePackage Declaration vs. Directory Structure Mismatch
The package is declared as
c1.svc.transport.v1, but the file is located inproto/c1/transport/v1. According to the static analysis and previous review feedback, the file should reside within ac1/svc/transport/v1directory relative to the repository root.Suggested fix:
- proto/c1/transport/v1/transport.proto + proto/c1/svc/transport/v1/transport.protoEnsure that the package structure and file location align to avoid issues with code generation and linting.
🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 3-3:
Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
🧹 Nitpick comments (8)
pkg/lambda/grpc/config/config.go (3)
1-31: Consider adding context to error constants and environment usage.Defining descriptive error constants is good. However, consider including contextual details or structured errors (e.g., wrapping the base errors with additional info) to improve debugging. Also, be mindful that relying on environment variables for essential overrides (like
BATON_LAMBDA_TOKEN_HOST) can cause unexpected behavior if not documented or validated.
92-107: Validate strictness of the client ID format.Using
SplitNwith fixed delimiters is fragile if the input format changes. Consider providing clearer error messages or more robust parsing (e.g., regex) to ensure the client ID meets ongoing format requirements.
109-140: Enhance debug logging for secret parsing.Secret parsing is strict and returns a generic error on multiple failure points. Consider adding debug logs or wrapping errors with more detail to simplify troubleshooting.
pkg/cli/lambda_support/lambda_enabled.go (3)
34-36: Use descriptive function docstrings.
AddCommandis straightforward, but adding a succinct docstring can clarify its role in the CLI workflow for developers.
117-127: Use doc comments for the custom resolver.
staticLambdaResolveris a custom resolver and may confuse future maintainers. A brief comment explaining the behavior (resolving to a static URL, ignoring region, etc.) will reduce guesswork.
135-157: Allow toggling AWS region or credentials.
lambdaConnectorClientuses default config loading without region overrides or referencing optional AWS credentials. This might be insufficient for multi-region or specialized deployments.pkg/cli/service_unix.go (1)
22-24: Document and test the newly exported logger initialization.
InitLoggeris now an exported function. Add doc comments and consider writing a small test to ensure correct logger configuration and coverage.pkg/config/config.go (1)
54-97: Consider extracting common configuration logic.The flag handling logic is duplicated between
DefineLambdaServerConfigurationandDefineConfiguration. Consider extracting this into a shared helper function to improve maintainability.Example refactor:
+func setupViperAndFlags(v *viper.Viper, mainCMD *cobra.Command) { + v.SetEnvPrefix("BATON") + v.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + v.AutomaticEnv() + + mainCMD.PersistentFlags().VisitAll(func(f *pflag.Flag) { + if v.IsSet(f.Name) { + _ = mainCMD.PersistentFlags().Set(f.Name, v.GetString(f.Name)) + } + }) + + mainCMD.Flags().VisitAll(func(f *pflag.Flag) { + if v.IsSet(f.Name) { + _ = mainCMD.Flags().Set(f.Name, v.GetString(f.Name)) + } + }) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpb/c1/transport/v1/transport.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
.vscode/settings.json(1 hunks)go.mod(3 hunks)pkg/cli/commands.go(7 hunks)pkg/cli/lambda_support/lambda_disabled.go(1 hunks)pkg/cli/lambda_support/lambda_enabled.go(1 hunks)pkg/cli/service_unix.go(1 hunks)pkg/config/config.go(4 hunks)pkg/config/generate.go(1 hunks)pkg/field/field_options.go(1 hunks)pkg/field/fields.go(1 hunks)pkg/lambda/grpc/config/config.go(1 hunks)proto/c1/transport/v1/transport.proto(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .vscode/settings.json
- pkg/cli/lambda_support/lambda_disabled.go
🧰 Additional context used
🪛 GitHub Check: go-test (1.22.x, windows-latest)
pkg/cli/commands.go
[failure] 70-70:
undefined: InitLogger
[failure] 252-252:
undefined: InitLogger
[failure] 390-390:
undefined: InitLogger
🪛 GitHub Actions: ci
pkg/cli/commands.go
[error] 70-70: undefined: InitLogger
go.mod
[warning] 119-119: local replacement are not allowed: github.com/conductorone/dpop (gomoddirectives)
🪛 golangci-lint (1.62.2)
go.mod
119-119: local replacement are not allowed: github.com/conductorone/dpop
(gomoddirectives)
🪛 Buf (1.47.2)
proto/c1/transport/v1/transport.proto
3-3: Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/transport/v1/transport.proto
[failure] 3-3:
Files with package "c1.svc.transport.v1" must be within a directory "c1/svc/transport/v1" relative to root but were in directory "c1/transport/v1".
🔇 Additional comments (13)
proto/c1/transport/v1/transport.proto (5)
1-1: Proto Syntax Declaration is CorrectThe file correctly specifies
proto3syntax.
5-7: Import Statements Are AppropriateThe imports for
"google/protobuf/struct.proto"and"google/protobuf/any.proto"are correctly included and used in the message definitions.
9-9: Go Package Option AlignmentThe
go_packageoption is set to"github.com/conductorone/baton-sdk/pb/c1/svc/transport/v1", which is appropriate for the intended package structure. Once the file is moved to match the directoryc1/svc/transport/v1, this option will correctly reflect its location.
11-15: Message Request Definition Looks GoodThe
Requestmessage is defined with clear fields:
string method = 1google.protobuf.Any req = 2google.protobuf.Struct headers = 3This definition is clear and concise.
17-22: Message Response Definition Is CorrectThe
Responsemessage includes the fields:
google.protobuf.Any resp = 1google.protobuf.Any status = 2google.protobuf.Struct headers = 3google.protobuf.Struct trailers = 4The structure is well-defined, and the intended use for fields is straightforward.
pkg/lambda/grpc/config/config.go (1)
32-90: Verify the concurrency model and potential dial blocking behavior.While creating the gRPC client, ensure the code properly handles cases where the dial operation might block or fail under high concurrency. Verify whether a timeout or cancellation via the context is needed for production scenarios.
pkg/cli/lambda_support/lambda_enabled.go (1)
159-213: Return structured errors in metadata command.Ensure errors in
MakeLambdaMetadataCommand(e.g., line 193) carry contextual messages for faster debugging. Also, consider adding logging for user actions (like retrieving metadata) to facilitate operational insights.pkg/config/generate.go (1)
60-63: LGTM! Good use of conditional field name assignment.The logic for determining the field name is well-structured, with a clear fallback mechanism.
pkg/field/field_options.go (1)
195-200: LGTM! Clean implementation of the new field option.The
WithStructFieldNamefunction follows the established pattern of other field options and maintains consistency.pkg/field/fields.go (1)
63-66: LGTM! Clean addition of the new field.The
StructFieldNamefield is well-placed and the formatting is consistent with the rest of the struct.pkg/config/config.go (1)
149-164:✅ Verification successful
Verify Lambda command initialization.
The Lambda command initialization looks correct, but ensure that all necessary Lambda-specific configurations are properly handled.
Run the following script to verify Lambda-related configurations:
🏁 Script executed:
#!/bin/bash # Description: Check for Lambda-related configuration fields and their usage. # Search for Lambda configuration fields rg -A 5 'LambdaServerFields|LambdaServerRelationships' # Search for Lambda command usage rg -A 5 'lambda_support.MakeLambdaServerCommand'Length of output: 2203
Lambda Command Initialization Verified
The executed script confirms that the Lambda-specific configuration fields (
LambdaServerFields,LambdaServerRelationships) are defined inpkg/field/defaults.goand are correctly utilized in the Lambda command setup. Both invocations oflambda_support.MakeLambdaServerCommandare present:
- One call uses
connectorConfSchema- The other uses
schemaIf this difference is intentional for handling distinct cases, no further modifications are required. Otherwise, please double-check that the variation in the final parameter is deliberate.
- Lambda configuration fields are correctly defined and retrieved.
- The Lambda command is properly initialized in
pkg/config/config.go.- Review the usage of
schemaversusconnectorConfSchemato ensure consistency.pkg/cli/commands.go (1)
33-50: LGTM! Well-structured generic configuration handler.The implementation properly handles type validation, error cases, and uses reflection appropriately for generic configuration.
go.mod (1)
7-8: LGTM! AWS Lambda dependencies.The addition of AWS Lambda dependencies aligns with the PR objectives for implementing Lambda support.
Also applies to: 12-12
| func MakeLambdaServerCommand[T any]( | ||
| ctx context.Context, | ||
| name string, | ||
| v *viper.Viper, | ||
| getconnector cli.GetConnectorFunc[T], | ||
| lambdaSchema field.Configuration, | ||
| connectorSchema field.Configuration, | ||
| ) func(*cobra.Command, []string) error { | ||
| return func(cmd *cobra.Command, args []string) error { | ||
| err := v.BindPFlags(cmd.Flags()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| runCtx, err := cli.InitLogger( | ||
| ctx, | ||
| name, | ||
| logging.WithLogFormat(v.GetString("log-format")), | ||
| logging.WithLogLevel(v.GetString("log-level")), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // l := ctxzap.Extract(runCtx) | ||
|
|
||
| if err := field.Validate(lambdaSchema, v); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| client, err := c1_lambda_config.GetConnectorConfigServiceClient( | ||
| ctx, | ||
| v.GetString("lambda-client-id"), | ||
| v.GetString("lambda-client-secret"), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get connector manager client: %w", err) | ||
| } | ||
|
|
||
| // Get configuration, convert it to viper flag values, then proceed. | ||
| // TODO(morgabra): Should we start the lambda handler first? What are the timeouts for startup? | ||
| config, err := client.GetConnectorConfig(ctx, &pb_connector_api.GetConnectorConfigRequest{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get connector config: %w", err) | ||
| } | ||
|
|
||
| t, err := cli.MakeGenericConfiguration[T](v) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to make generic configuration: %w", err) | ||
| } | ||
|
|
||
| err = mapstructure.Decode(config.Config.AsMap(), &t) | ||
| if err != nil { | ||
| log.Fatalf("Error decoding: %v", err) | ||
| } | ||
| v := any(t).(*viper.Viper) | ||
|
|
||
| if err := field.Validate(connectorSchema, v); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := getconnector(runCtx, t) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| opts := &connector.RegisterOps{ | ||
| Ratelimiter: nil, // FIXME(morgabra/kans): ??? | ||
| ProvisioningEnabled: true, // FIXME(morgabra/kans): ??? - these are `--provisioning` flags to the server binary - do we still want to expose these via the config service? | ||
| TicketingEnabled: true, // FIXME(morgabra/kans): ??? | ||
| } | ||
|
|
||
| s := c1_lambda_grpc.NewServer(nil) | ||
| connector.Register(ctx, s, c, opts) | ||
|
|
||
| lambda.Start(s.Handler) | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using log.Fatalf in library-like code paths.
Here, log.Fatalf (line 90) immediately terminates the process, which can be disruptive if used as a library in another program. Consider returning errors or using structured logging to preserve proper flow control.
- log.Fatalf("Error decoding: %v", err)
+ return fmt.Errorf("failed to decode connector config: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func MakeLambdaServerCommand[T any]( | |
| ctx context.Context, | |
| name string, | |
| v *viper.Viper, | |
| getconnector cli.GetConnectorFunc[T], | |
| lambdaSchema field.Configuration, | |
| connectorSchema field.Configuration, | |
| ) func(*cobra.Command, []string) error { | |
| return func(cmd *cobra.Command, args []string) error { | |
| err := v.BindPFlags(cmd.Flags()) | |
| if err != nil { | |
| return err | |
| } | |
| runCtx, err := cli.InitLogger( | |
| ctx, | |
| name, | |
| logging.WithLogFormat(v.GetString("log-format")), | |
| logging.WithLogLevel(v.GetString("log-level")), | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| // l := ctxzap.Extract(runCtx) | |
| if err := field.Validate(lambdaSchema, v); err != nil { | |
| return err | |
| } | |
| client, err := c1_lambda_config.GetConnectorConfigServiceClient( | |
| ctx, | |
| v.GetString("lambda-client-id"), | |
| v.GetString("lambda-client-secret"), | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("failed to get connector manager client: %w", err) | |
| } | |
| // Get configuration, convert it to viper flag values, then proceed. | |
| // TODO(morgabra): Should we start the lambda handler first? What are the timeouts for startup? | |
| config, err := client.GetConnectorConfig(ctx, &pb_connector_api.GetConnectorConfigRequest{}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get connector config: %w", err) | |
| } | |
| t, err := cli.MakeGenericConfiguration[T](v) | |
| if err != nil { | |
| return fmt.Errorf("failed to make generic configuration: %w", err) | |
| } | |
| err = mapstructure.Decode(config.Config.AsMap(), &t) | |
| if err != nil { | |
| log.Fatalf("Error decoding: %v", err) | |
| } | |
| v := any(t).(*viper.Viper) | |
| if err := field.Validate(connectorSchema, v); err != nil { | |
| return err | |
| } | |
| c, err := getconnector(runCtx, t) | |
| if err != nil { | |
| return err | |
| } | |
| opts := &connector.RegisterOps{ | |
| Ratelimiter: nil, // FIXME(morgabra/kans): ??? | |
| ProvisioningEnabled: true, // FIXME(morgabra/kans): ??? - these are `--provisioning` flags to the server binary - do we still want to expose these via the config service? | |
| TicketingEnabled: true, // FIXME(morgabra/kans): ??? | |
| } | |
| s := c1_lambda_grpc.NewServer(nil) | |
| connector.Register(ctx, s, c, opts) | |
| lambda.Start(s.Handler) | |
| return nil | |
| } | |
| } | |
| func MakeLambdaServerCommand[T any]( | |
| ctx context.Context, | |
| name string, | |
| v *viper.Viper, | |
| getconnector cli.GetConnectorFunc[T], | |
| lambdaSchema field.Configuration, | |
| connectorSchema field.Configuration, | |
| ) func(*cobra.Command, []string) error { | |
| return func(cmd *cobra.Command, args []string) error { | |
| err := v.BindPFlags(cmd.Flags()) | |
| if err != nil { | |
| return err | |
| } | |
| runCtx, err := cli.InitLogger( | |
| ctx, | |
| name, | |
| logging.WithLogFormat(v.GetString("log-format")), | |
| logging.WithLogLevel(v.GetString("log-level")), | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| // l := ctxzap.Extract(runCtx) | |
| if err := field.Validate(lambdaSchema, v); err != nil { | |
| return err | |
| } | |
| client, err := c1_lambda_config.GetConnectorConfigServiceClient( | |
| ctx, | |
| v.GetString("lambda-client-id"), | |
| v.GetString("lambda-client-secret"), | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("failed to get connector manager client: %w", err) | |
| } | |
| // Get configuration, convert it to viper flag values, then proceed. | |
| // TODO(morgabra): Should we start the lambda handler first? What are the timeouts for startup? | |
| config, err := client.GetConnectorConfig(ctx, &pb_connector_api.GetConnectorConfigRequest{}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get connector config: %w", err) | |
| } | |
| t, err := cli.MakeGenericConfiguration[T](v) | |
| if err != nil { | |
| return fmt.Errorf("failed to make generic configuration: %w", err) | |
| } | |
| err = mapstructure.Decode(config.Config.AsMap(), &t) | |
| if err != nil { | |
| return fmt.Errorf("failed to decode connector config: %w", err) | |
| } | |
| v := any(t).(*viper.Viper) | |
| if err := field.Validate(connectorSchema, v); err != nil { | |
| return err | |
| } | |
| c, err := getconnector(runCtx, t) | |
| if err != nil { | |
| return err | |
| } | |
| opts := &connector.RegisterOps{ | |
| Ratelimiter: nil, // FIXME(morgabra/kans): ??? | |
| ProvisioningEnabled: true, // FIXME(morgabra/kans): ??? - these are `--provisioning` flags to the server binary - do we still want to expose these via the config service? | |
| TicketingEnabled: true, // FIXME(morgabra/kans): ??? | |
| } | |
| s := c1_lambda_grpc.NewServer(nil) | |
| connector.Register(ctx, s, c, opts) | |
| lambda.Start(s.Handler) | |
| return nil | |
| } | |
| } |
pkg/cli/commands.go
Outdated
| @@ -67,7 +67,7 @@ | |||
| return err | |||
| } | |||
|
|
|||
| runCtx, err := initLogger( | |||
| runCtx, err := InitLogger( | |||
There was a problem hiding this comment.
Fix undefined InitLogger function references.
The pipeline failures indicate that InitLogger is undefined. While the function has been renamed from initLogger to InitLogger to be exported, the implementation appears to be missing or not included in this PR.
Please ensure that:
- The implementation file containing
InitLoggeris included in the PR - The function is properly exported in its implementation file
- All import paths are correctly updated
Also applies to: 252-252, 390-390
🧰 Tools
🪛 GitHub Check: go-test (1.22.x, windows-latest)
[failure] 70-70:
undefined: InitLogger
🪛 GitHub Actions: ci
[error] 70-70: undefined: InitLogger
go.mod
Outdated
| @@ -112,3 +115,5 @@ require ( | |||
| modernc.org/memory v1.8.2 // indirect | |||
| modernc.org/sqlite v1.34.5 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/conductorone/dpop => ../dpop | |||
There was a problem hiding this comment.
Remove local replacement directive.
The local replacement directive for github.com/conductorone/dpop is flagged by the linter. Local replacements should not be committed to version control.
Remove the following line:
-replace github.com/conductorone/dpop => ../dpopInstead, consider:
- Publishing the dpop package to a repository
- Using a proper version tag
- If this is for development only, move the replacement to a separate
go.workfile
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| replace github.com/conductorone/dpop => ../dpop |
🧰 Tools
🪛 golangci-lint (1.62.2)
119-119: local replacement are not allowed: github.com/conductorone/dpop
(gomoddirectives)
🪛 GitHub Actions: ci
[warning] 119-119: local replacement are not allowed: github.com/conductorone/dpop (gomoddirectives)
| @@ -1,25 +1,30 @@ | |||
| module github.com/conductorone/baton-sdk | |||
|
|
|||
| go 1.23 | |||
| go 1.23.4 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Go version compatibility.
The specified Go version 1.23.4 does not exist yet. The latest stable version is 1.22.x.
Please update to a valid Go version.
🌐 Web query:
What is the latest stable version of Go?
💡 Result:
The latest stable version of Go as of February 2025 is 1.24.0, released on February 11, 2025[4][6]. This follows the established six-month release cycle for major Go versions, succeeding Go 1.23 which was released in August 2024[4].
Key details about the current release landscape:
Release Support Policy
- Each major release receives security updates and bug fixes until two subsequent versions are published[4]
- Go 1.24.x will be supported until Go 1.26 releases
Recent Version History
| Version | Release Date | Support Status |
|---|---|---|
| 1.24 | Feb 11, 2025 | Actively supported |
| 1.23 | Aug 13, 2024 | Supported |
| 1.22 | Feb 6, 2024 | Unsupported |
Developers should upgrade to Go 1.24 for improved toolchain performance and security updates from previous versions[4][6]. The Go team maintains backward compatibility through its Go 1 promise, ensuring code stability across minor versions[3][5].
Citations:
- 1: https://www.easiio.com/latest-golang-version/
- 2: https://github.com/golang/tools/releases
- 3: https://tip.golang.org/doc/go1.21
- 4: https://go.dev/doc/devel/release
- 5: https://tip.golang.org/doc/go1.23
- 6: https://go.dev/dl/
- 7: https://groups.google.com/g/golang-nuts/c/_ROx_UGCCCg
Update Go Version in go.mod & Address Local Replacement Warning
- The Go version is currently set to
"go 1.23.4", but no such version exists. As of February 2025, the latest stable Go release is 1.24.0. Please update the Go version accordingly. - Additionally, the linter is flagging the local replacement for the dpop package. Review whether this local replacement is necessary or if it can be replaced with an official dependency reference.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/cli/lambda_support.go (1)
117-120: Address theFIXMEplaceholders
Lines 117-119 referenceFIXME. Consider clarifying or removing these placeholders.Do you want me to open an issue to track the resolution of these items?
pkg/cli/commands.go (1)
29-29: Typographical improvement
Consider renamingContrainstSettertoConstraintSetterfor clarity and consistency..gitignore (1)
20-20: Confirm the intent behind ignoring.pre-commit-config.yaml.
The new line/\.pre-commit-config.yamlensures that this configuration file is now ignored by Git. Typically, many projects track their pre-commit configuration to enforce consistent linting and tooling across all environments. Please verify that excluding this file is intentional and aligns with your team's workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks).vscode/settings.json(1 hunks)pkg/cli/cli.go(1 hunks)pkg/cli/commands.go(4 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/lambda_no_support.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .vscode/settings.json
- pkg/config/config.go
🔇 Additional comments (8)
pkg/cli/lambda_support.go (5)
33-36: Looks good to me!
This function simply adds the subcommand to the main command and appears correct.
64-73: UndefinedinitLoggerfunction reference
This code referencesinitLogger, which doesn’t appear in the provided snippets. A previous review also flagged missing or undefined logger initialization.
144-153: UndefinedinitLoggerfunction reference
Similar to lines 64-69,initLoggeris called here but is not defined in the visible changes.
187-203: LGTM
The staticLambdaResolver implementation is straightforward and correctly returns an endpoint from a parsed URL.
205-227:❓ Verification inconclusive
Validate
functionargument
If thefunctionparameter is empty,transport.NewLambdaClientTransportmay fail or produce unexpected behavior. Consider verifying that it’s not empty or handling this case.
🏁 Script executed:
#!/bin/bash # Description: Search for calls to lambdaConnectorClient(...) with an empty string for the function name rg -A 5 'lambdaConnectorClient\(.*,""\)'Length of output: 42
Action Required: Validate the
functionparameter remains non-emptyOur search for invocations of
lambdaConnectorClientwith an explicit empty string for thefunctionparameter did not yield any results. However, given that the check was based solely on literal usage (i.e.,""), please manually verify that dynamic cases or indirect invocations do not inadvertently pass an empty string. In the meantime, consider adding an explicit check inpkg/cli/lambda_support.go(lines 205–227) to validate that thefunctionargument is not empty before callingtransport.NewLambdaClientTransport.pkg/cli/commands.go (3)
186-189: EnsureTis valid forMakeGenericConfiguration
CallingMakeGenericConfiguration[T]with a type other thanviper.Viperor a struct might cause unexpected behavior or a panic.
262-265: EnsureTis valid forMakeGenericConfiguration
CallingMakeGenericConfiguration[T]with a type other thanviper.Viperor a struct might cause unexpected behavior or a panic.
383-386: EnsureTis valid forMakeGenericConfiguration
CallingMakeGenericConfiguration[T]with a type other thanviper.Viperor a struct might cause unexpected behavior or a panic.
pkg/cli/lambda_support.go
Outdated
| if err != nil { | ||
| log.Fatalf("Error decoding: %v", err) | ||
| } | ||
| v := any(t).(*viper.Viper) |
There was a problem hiding this comment.
Risk of panic when casting T to *viper.Viper
If T is not actually viper.Viper, this cast will panic at runtime. Ensure it’s always safe to treat T as viper.Viper or handle other cases gracefully.
- v := any(t).(*viper.Viper)
+ vv, ok := any(t).(*viper.Viper)
+ if !ok {
+ return fmt.Errorf("expected T to be *viper.Viper")
+ }
+ // use vv📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| v := any(t).(*viper.Viper) | |
| vv, ok := any(t).(*viper.Viper) | |
| if !ok { | |
| return fmt.Errorf("expected T to be *viper.Viper") | |
| } | |
| // use vv |
pkg/cli/lambda_support.go
Outdated
| SilenceUsage: true, | ||
| } | ||
|
|
||
| constraintSetter(cmd, lambdaSchema) |
There was a problem hiding this comment.
Check error from constraintSetter
The call to constraintSetter(cmd, lambdaSchema) may return an error, but it's not captured or handled. This could lead to silent failures if any validation or setup steps fail.
- constraintSetter(cmd, lambdaSchema)
+ if err := constraintSetter(cmd, lambdaSchema); err != nil {
+ return err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constraintSetter(cmd, lambdaSchema) | |
| if err := constraintSetter(cmd, lambdaSchema); err != nil { | |
| return err | |
| } |
pkg/cli/cli.go
Outdated
| func MakeGenericConfiguration[T any](v *viper.Viper) (*T, error) { | ||
| // Create an instance of the struct type T using reflection | ||
| var config T // Create a zero-value instance of T | ||
| // Ensure T is a struct (or pointer to struct) | ||
| tType := reflect.TypeOf(config) | ||
| if tType == reflect.TypeOf(viper.Viper{}) { | ||
| return any(v).(*T), nil | ||
| } | ||
| if tType.Kind() != reflect.Struct { | ||
| return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) | ||
| } | ||
| // Unmarshal into the config struct | ||
| err := v.Unmarshal(&config) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal config: %w", err) | ||
| } | ||
| return &config, nil | ||
| } |
There was a problem hiding this comment.
Potential mismatch in MakeGenericConfiguration for Viper types
Line 19 only checks if T is viper.Viper, which may not account for pointer types or similar edge cases. This could lead to panics or incorrect type assumptions.
- if tType == reflect.TypeOf(viper.Viper{}) {
- return any(v).(*T), nil
- }
+ if tType == reflect.TypeOf(viper.Viper{}) || tType == reflect.TypeOf(&viper.Viper{}) {
+ return any(v).(*T), nil
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func MakeGenericConfiguration[T any](v *viper.Viper) (*T, error) { | |
| // Create an instance of the struct type T using reflection | |
| var config T // Create a zero-value instance of T | |
| // Ensure T is a struct (or pointer to struct) | |
| tType := reflect.TypeOf(config) | |
| if tType == reflect.TypeOf(viper.Viper{}) { | |
| return any(v).(*T), nil | |
| } | |
| if tType.Kind() != reflect.Struct { | |
| return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) | |
| } | |
| // Unmarshal into the config struct | |
| err := v.Unmarshal(&config) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal config: %w", err) | |
| } | |
| return &config, nil | |
| } | |
| func MakeGenericConfiguration[T any](v *viper.Viper) (*T, error) { | |
| // Create an instance of the struct type T using reflection | |
| var config T // Create a zero-value instance of T | |
| // Ensure T is a struct (or pointer to struct) | |
| tType := reflect.TypeOf(config) | |
| if tType == reflect.TypeOf(viper.Viper{}) || tType == reflect.TypeOf(&viper.Viper{}) { | |
| return any(v).(*T), nil | |
| } | |
| if tType.Kind() != reflect.Struct { | |
| return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) | |
| } | |
| // Unmarshal into the config struct | |
| err := v.Unmarshal(&config) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to unmarshal config: %w", err) | |
| } | |
| return &config, nil | |
| } |
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
c606398 to
8d089b7
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkg/config/config.go (5)
52-59: Consider using a more efficient field deduplication approach.The current implementation creates a temporary map and then rebuilds the slice. Consider using a more efficient approach by pre-allocating the slice capacity and using a single pass.
- uniqueFields := make(map[string]field.SchemaField) - for _, f := range confschema.Fields { - uniqueFields[f.FieldName] = f - } - confschema.Fields = make([]field.SchemaField, 0, len(uniqueFields)) - for _, f := range uniqueFields { - confschema.Fields = append(confschema.Fields, f) - } + seen := make(map[string]struct{}) + j := 0 + for _, f := range confschema.Fields { + if _, exists := seen[f.FieldName]; !exists { + seen[f.FieldName] = struct{}{} + confschema.Fields[j] = f + j++ + } + } + confschema.Fields = confschema.Fields[:j]
84-88: Enhance error handling for Lambda command integration.The error handling for Lambda command addition could be more descriptive. Consider wrapping the error with additional context.
- err = cli.OptionallyAddLambdaCommand(ctx, connectorName, v, connector, confschema, mainCMD) - if err != nil { - return nil, nil, err - } + if err = cli.OptionallyAddLambdaCommand(ctx, connectorName, v, connector, confschema, mainCMD); err != nil { + return nil, nil, fmt.Errorf("failed to add Lambda command: %w", err) + }
90-119: Consider consolidating command additions.The current implementation repeats the same error handling pattern for each command addition. Consider creating a helper function to reduce code duplication.
+func addCommandWithErrorHandling(mainCmd *cobra.Command, v *viper.Viper, schema *field.Configuration, cmd *cobra.Command) error { + _, err := cli.AddCommand(mainCmd, v, schema, cmd) + if err != nil { + return fmt.Errorf("failed to add command %s: %w", cmd.Use, err) + } + return nil +} // Usage: - _, err = cli.AddCommand(mainCMD, v, &schema, &cobra.Command{ - Use: "_connector-service", - Short: "Start the connector service", - Hidden: true, - RunE: cli.MakeGRPCServerCommand(ctx, connectorName, v, confschema, connector), - }) - if err != nil { - return nil, nil, err - } + if err := addCommandWithErrorHandling(mainCMD, v, &schema, &cobra.Command{ + Use: "_connector-service", + Short: "Start the connector service", + Hidden: true, + RunE: cli.MakeGRPCServerCommand(ctx, connectorName, v, confschema, connector), + }); err != nil { + return nil, nil, err + }
124-127: Improve function documentation.The function would benefit from clearer documentation explaining its purpose, parameters, and return values.
+// verifyStructFields ensures that every field in the provided schema has a corresponding +// struct tag in the configuration struct of type T. It handles both struct types and +// *viper.Viper types, where the latter is always considered valid. +// +// Parameters: +// - schema: The field configuration to verify against the struct +// +// Returns: +// - error: nil if verification passes, or an error describing the mismatch func verifyStructFields[T any](schema field.Configuration) error {
150-151: Remove or clarify the misleading comment.The comment about export targets seems unrelated to the field verification logic and should be removed or clarified.
- // This means a connector may not set an export target of none. + // Field is missing from the configuration structpkg/cli/cli.go (1)
208-210: Clarify treatment of 'Dependents' constraint
The code currently does nothing forfield.Dependents. If this is intentional, consider adding a comment clarifying why it’s unimplemented or implementing the constraint logic.
🛑 Comments failed to post (2)
pkg/cli/cli.go (1)
25-27:
⚠️ Potential issueAlign code with comment regarding pointer-to-struct
The comment states that T can be a struct or pointer to struct, yet this check only allows struct types. Please update this to properly handle pointers as well, or update the comment to reflect your true intent.if tType.Kind() != reflect.Struct { - return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) + if tType.Kind() == reflect.Pointer && tType.Elem().Kind() == reflect.Struct { + // handle pointer to struct + } else { + return nil, fmt.Errorf("T must be a struct or pointer to a struct, but got %s", tType.Kind()) + } }Committable suggestion skipped: line range outside the PR's diff.
pkg/cli/lambda_no_support.go (1)
14-33:
⚠️ Potential issueFix missing function body and syntax errors
The functionOptionallyAddLambdaCommandis missing its closing brace, causing compilation errors. Add the brace (and necessary logic or a placeholder) prior to definingMakeLambdaMetadataCommand.func OptionallyAddLambdaCommand[T any]( ctx context.Context, name string, v *viper.Viper, getconnector GetConnectorFunc[T], connectorSchema field.Configuration, mainCmd *cobra.Command, ) (error) { + // TODO: Implement functionality here + return nil } func MakeLambdaMetadataCommand[T any](📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func OptionallyAddLambdaCommand[T any]( ctx context.Context, name string, v *viper.Viper, getconnector GetConnectorFunc[T], connectorSchema field.Configuration, mainCmd *cobra.Command, ) (error) { // TODO: Implement functionality here return nil } func MakeLambdaMetadataCommand[T any]( ctx context.Context, name string, v *viper.Viper, getconnector GetConnectorFunc[T], confschema field.Configuration, ) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { return nil } }🧰 Tools
🪛 GitHub Check: go-lint
[failure] 23-23:
syntax error: unexpected name MakeLambdaMetadataCommand, expected ( (typecheck)
[failure] 23-23:
expected '(', found MakeLambdaMetadataCommand (typecheck)
[failure] 24-24:
expected ')', found context (typecheck)
[failure] 33-33:
expected '}', found 'EOF' (typecheck)
[failure] 23-23:
syntax error: unexpected name MakeLambdaMetadataCommand, expected () (typecheck)🪛 golangci-lint (1.62.2)
23-23: expected '(', found MakeLambdaMetadataCommand
(typecheck)
24-24: expected ')', found context
(typecheck)
33-33: expected '}', found 'EOF'
(typecheck)
🪛 GitHub Actions: ci
[error] 23-23: expected '(', found MakeLambdaMetadataCommand (and 3 more errors)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/cli/cli.go (1)
17-34:⚠️ Potential issueImprove type safety in
MakeGenericConfiguration.The function needs better type checking and safer type assertions.
Apply this diff to improve type safety:
func MakeGenericConfiguration[T any](v *viper.Viper) (*T, error) { var config T tType := reflect.TypeOf(config) - if tType == reflect.TypeOf(viper.Viper{}) { - return any(v).(*T), nil + if tType == reflect.TypeOf(viper.Viper{}) || tType == reflect.TypeOf(&viper.Viper{}) { + if result, ok := any(v).(*T); ok { + return result, nil + } + return nil, fmt.Errorf("failed to convert viper to type T") } if tType.Kind() != reflect.Struct { return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) }pkg/cli/lambda_support.go (1)
101-101:⚠️ Potential issueFix unsafe type assertion.
The type assertion can panic if the type doesn't match.
Apply this diff:
-v := any(t).(*viper.Viper) +v, ok := any(t).(*viper.Viper) +if !ok { + return fmt.Errorf("expected type *viper.Viper, got %T", t) +}
🧹 Nitpick comments (1)
pkg/config/config.go (1)
56-59: Optimize field uniqueness check.Pre-allocate the slice capacity to improve performance and reduce memory allocations.
Apply this diff:
- confschema.Fields = make([]field.SchemaField, 0, len(uniqueFields)) - for _, f := range uniqueFields { - confschema.Fields = append(confschema.Fields, f) - } + fields := make([]field.SchemaField, 0, len(uniqueFields)) + for _, f := range uniqueFields { + fields = append(fields, f) + } + confschema.Fields = fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/cli/cli.go(1 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/cli/cli.go
1-1: : # github.com/conductorone/baton-sdk/pkg/cli
pkg/cli/lambda_no_support.go:23:6: syntax error: unexpected name MakeLambdaMetadataCommand, expected (
(typecheck)
pkg/cli/lambda_no_support.go
23-23: expected '(', found MakeLambdaMetadataCommand
(typecheck)
24-24: expected ')', found context
(typecheck)
33-33: expected '}', found 'EOF'
(typecheck)
🪛 GitHub Check: go-lint
pkg/cli/lambda_no_support.go
[failure] 23-23:
syntax error: unexpected name MakeLambdaMetadataCommand, expected ( (typecheck)
[failure] 23-23:
expected '(', found MakeLambdaMetadataCommand (typecheck)
[failure] 24-24:
expected ')', found context (typecheck)
[failure] 33-33:
expected '}', found 'EOF' (typecheck)
[failure] 23-23:
syntax error: unexpected name MakeLambdaMetadataCommand, expected () (typecheck)
🪛 GitHub Actions: ci
pkg/cli/lambda_no_support.go
[error] 23-23: expected '(', found MakeLambdaMetadataCommand (and 3 more errors)
🔇 Additional comments (1)
pkg/cli/lambda_support.go (1)
86-86:❓ Verification inconclusive
Address TODO comment about Lambda handler timeouts.
The TODO comment raises a valid concern about Lambda startup timeouts that needs to be addressed.
Please verify the AWS Lambda function timeout configuration and startup behavior:
🌐 Web query:
What is the default timeout for AWS Lambda functions and how does cold start affect the timeout?💡 Result:
AWS Lambda functions have a default timeout of 3 seconds[1][7], but this can be configured up to a maximum of 15 minutes (900 seconds)[4][7]. The actual timeout value impacts whether cold starts affect function execution:
Default Timeout and Configuration
- Default: 3 seconds[1][7]
- Maximum Configurable: 15 minutes (900 seconds)[4]
- Best Practices:
- For synchronous APIs (e.g., API Gateway): Set to 3–6 seconds to avoid user experience degradation[1][7].
- For asynchronous/batch workloads: Align with task duration (monitor metrics)[1].
Cold Start Impact on Timeout
Cold starts add latency during function initialization (typically 100 ms–1+ second[2][3]), which counts toward the total execution time. This can cause unexpected timeouts if:
Timeouts are set too tightly:
- Example: A function with a 1-second timeout and a 700ms cold start leaves only 300ms for business logic[2][6].
- Solutions:
- Use
context.getRemainingTimeInMillis()to dynamically adjust downstream timeouts[7].- Allocate more memory (reduces cold start duration)[2][5].
Short-lived functions:
- Functions with sub-second execution times are disproportionately affected by cold starts[6].
Concurrency spikes:
- Sudden traffic increases trigger new execution environments (more cold starts)[5][8].
Mitigation Strategies
- Provisioned Concurrency: Pre-warms execution environments (eliminates cold starts for critical paths)[5].
- Optimize initialization:
- Move static logic (e.g., database connections) outside the handler[10].
- Use lightweight runtimes (e.g., Python/Node.js vs. Java)[2][5].
- Monitor cold start rates using tools like AWS X-Ray or Lumigo[1][5].
Cold starts don’t directly reduce the configured timeout but consume part of it during initialization. Proper timeout configuration and cold start mitigation are essential for latency-sensitive applications[1][6][10].
Citations:
- 1: https://lumigo.io/debugging-aws-lambda-serverless-applications/serverless-timeouts/
- 2: https://www.ranthebuilder.cloud/post/is-aws-lambda-cold-start-still-an-issue-in-2024
- 3: https://www.pluralsight.com/resources/blog/cloud/how-long-does-aws-lambda-keep-your-idle-functions-around-before-a-cold-start
- 4: https://aws.amazon.com/about-aws/whats-new/2018/10/aws-lambda-supports-functions-that-can-run-up-to-15-minutes/
- 5: https://repost.aws/questions/QUqM07iC91T4Wvw9h-DiDT1g/frequent-lambda-cold-starts
- 6: https://aaronstuyvenberg.com/posts/lambda-timeout-doom-loop
- 7: https://lumigo.io/aws-lambda-performance-optimization/aws-lambda-timeout-best-practices/
- 8: https://repost.aws/questions/QUKdeptBaRT5OKa-5ZCY3Bpg/how-long-does-a-lambda-instance-can-keep-warm
- 9: Include cold start duration in http API Gateway timeout computation serverless/serverless#11222
- 10: https://repost.aws/knowledge-center/lambda-troubleshoot-invocation-timeouts
Review on Lambda Handler Timeout Configuration: Verify Cold Start Implications
The AWS Lambda default timeout is 3 seconds, and cold start delays (typically between 100 ms and over 1 second) can consume a significant portion of that time. Please verify the following:
- Confirm if the current handler initialization order is optimal—starting the lambda handler earlier might mitigate cold start latency.
- Ensure the configured timeout (default or custom) provides a sufficient buffer for the added initialization delay from cold starts.
- Consider strategies like provisioned concurrency or moving static initializations outside the handler to further reduce cold start impact.
pkg/cli/lambda_no_support.go
Outdated
| func OptionallyAddLambdaCommand[T any]( | ||
| ctx context.Context, | ||
| name string, | ||
| v *viper.Viper, | ||
| getconnector GetConnectorFunc[T], | ||
| connectorSchema field.Configuration, | ||
| mainCmd *cobra.Command, | ||
| ) (error) { | ||
|
|
There was a problem hiding this comment.
Fix syntax error in OptionallyAddLambdaCommand function.
The function declaration is incomplete and missing its body, which causes compilation errors.
Apply this diff to fix the syntax error:
func OptionallyAddLambdaCommand[T any](
ctx context.Context,
name string,
v *viper.Viper,
getconnector GetConnectorFunc[T],
connectorSchema field.Configuration,
mainCmd *cobra.Command,
) (error) {
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func OptionallyAddLambdaCommand[T any]( | |
| ctx context.Context, | |
| name string, | |
| v *viper.Viper, | |
| getconnector GetConnectorFunc[T], | |
| connectorSchema field.Configuration, | |
| mainCmd *cobra.Command, | |
| ) (error) { | |
| func OptionallyAddLambdaCommand[T any]( | |
| ctx context.Context, | |
| name string, | |
| v *viper.Viper, | |
| getconnector GetConnectorFunc[T], | |
| connectorSchema field.Configuration, | |
| mainCmd *cobra.Command, | |
| ) (error) { | |
| return nil | |
| } |
pkg/cli/lambda_support.go
Outdated
| Ratelimiter: nil, // FIXME(morgabra/kans): ??? | ||
| ProvisioningEnabled: true, // FIXME(morgabra/kans): ??? - these are `--provisioning` flags to the server binary - do we still want to expose these via the config service? | ||
| TicketingEnabled: true, // FIXME(morgabra/kans): ??? | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address FIXME comments about provisioning flags.
The FIXME comments indicate uncertainty about rate limiting and provisioning configuration.
Run this script to check for any existing rate limiting or provisioning configurations:
🏁 Script executed:
#!/bin/bash
# Search for rate limiting and provisioning configurations
rg -A 5 'Ratelimiter|ProvisioningEnabled|TicketingEnabled' --type goLength of output: 10193
Clarify and Update Provisioning and Rate Limiter Configuration in Lambda Support
In pkg/cli/lambda_support.go (lines 113–116), the configuration is hardcoded as follows:
- Ratelimiter: set to nil (with a FIXME indicating uncertainty)
- ProvisioningEnabled: set to true (with a FIXME questioning if these
--provisioningflags should be exposed via the config service) - TicketingEnabled: set to true (also marked with a FIXME)
The search across the repository shows that similar flags are dynamically enabled elsewhere (for example, via connectorrunner.WithProvisioningEnabled() in both pkg/cli/commands.go and pkg/connectorrunner/runner.go). This suggests that the provisioning and ticketing behaviors are intended to be configurable, whereas here they are hardcoded and the ratelimiter remains unconfigured.
Please reconcile these discrepancies by one of the following:
- Option 1: Expose these values as configurable (driven by CLI flags or environment variables) to align with how they’re set up in the rest of the codebase.
- Option 2: If lambdas are meant to behave differently (e.g., not supporting rate limiting), update the FIXME comments to document this design decision and ensure it doesn’t lead to confusion.
Review and update the hardcoded configurations and comments accordingly.
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
8d089b7 to
9f25721
Compare
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
9f25721 to
906a393
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cli/lambda_support.go (1)
101-101:⚠️ Potential issueRisk of panic when casting
Tto*viper.ViperIf
Tis not actuallyviper.Viper, this cast will panic at runtime. Ensure it's always safe to treatTasviper.Viperor handle other cases gracefully.Apply this diff to add proper type assertion handling:
- v := any(t).(*viper.Viper) + vv, ok := any(t).(*viper.Viper) + if !ok { + return fmt.Errorf("expected T to be *viper.Viper") + } + // use vv
🧹 Nitpick comments (4)
pkg/config/config.go (2)
84-89: Enhance error handling for Lambda command integration.The Lambda command integration could fail silently if the connector doesn't support Lambda functionality. Consider adding validation or logging to help diagnose integration issues.
- err = cli.OptionallyAddLambdaCommand(ctx, connectorName, v, connector, confschema, mainCMD) + err = cli.OptionallyAddLambdaCommand(ctx, connectorName, v, connector, confschema, mainCMD) + if err != nil { + // Log the error but continue since this is optional + fmt.Fprintf(os.Stderr, "Warning: Failed to add Lambda command: %v\n", err) + err = nil + }
124-155: Remove or clarify the misleading comment.The comment "This means a connector may not set an export target of none" on line 150 seems unrelated to the field validation context. Consider removing it or providing clearer explanation.
- // This means a connector may not set an export target of none. + // Field validation failed: schema field not found in structOtherwise, the implementation is well-structured with proper type safety and validation.
pkg/cli/lambda_support.go (1)
174-177: Improve error handling for stdout write operationThe error from
fmt.Fprintis properly captured but could provide more context about the failure.Apply this diff to improve error handling:
- _, err = fmt.Fprint(os.Stdout, string(outBytes)) - if err != nil { - return err - } + if _, err = fmt.Fprint(os.Stdout, string(outBytes)); err != nil { + return fmt.Errorf("failed to write metadata to stdout: %w", err) + }pkg/field/defaults.go (1)
52-65: Consider adding package-level documentation for Lambda server fieldsWhile the field descriptions are clear, consider adding package-level documentation to explain the purpose and usage of these Lambda server fields and relationships.
Add this documentation above the field declarations:
+// Lambda server configuration fields for OAuth authentication. +// These fields are required when running in Lambda mode and are used +// to authenticate with the configuration endpoint. lambdaServerClientIDField = StringField("lambda-client-id", ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/cli/cli.go(1 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(2 hunks)pkg/field/defaults.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/lambda_no_support.go
- pkg/cli/cli.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (6)
pkg/config/config.go (3)
70-70: LGTM! Improved modularity by moving flag handling to the CLI package.The refactoring to use
cli.SetFlagsAndConstraintsimproves code organization and maintainability.Also applies to: 76-76
90-95: LGTM! Well-structured gRPC server command integration.The hidden connector service command is properly integrated with clear error handling.
111-115: LGTM! Improved command description clarity.The updated description "Get the connector config schema" is more precise and better reflects the command's purpose.
pkg/cli/lambda_support.go (3)
113-116: Clarify and Update Provisioning and Rate Limiter Configuration in Lambda SupportThe configuration is hardcoded with FIXME comments indicating uncertainty about rate limiting and provisioning configuration. This needs to be clarified and properly configured.
Run this script to check for any existing rate limiting and provisioning configurations:
#!/bin/bash # Search for rate limiting and provisioning configurations rg -A 5 'Ratelimiter|ProvisioningEnabled|TicketingEnabled' --type go
183-199: Well-implemented endpoint resolver!The
staticLambdaResolverimplementation is clean, properly handles URL parsing errors, and follows the AWS SDK interface requirements.
201-223: Well-structured Lambda client setup!The function properly handles AWS configuration loading, endpoint resolution, and transport setup with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
pkg/cli/lambda_support.go (3)
91-99: Avoid abrupt termination withlog.Fatalf.
Usinglog.Fatalf("Error decoding: %v", err)on line 98 abruptly terminates the process, which may not be desirable in library or CLI code. Consider returning the error instead, so that callers or higher-level flows can determine how to handle it.- log.Fatalf("Error decoding: %v", err) + return fmt.Errorf("failed to decode configuration: %w", err)
110-114: Resolve theFIXMElines for provisioning, ticketing, and rate-limiting.
Theoptsobject setsRatelimitertonilandProvisioningEnabled/TicketingEnabledtotrueby default, but theFIXMEcomments imply uncertainty about making them configurable. Clarify or remove theFIXMEcomments to ensure maintainability and transparency.
125-179: Validate large metadata outputs or restricted runtime environments.
MakeLambdaMetadataCommandprints potentially large metadata to stdout. Ensure that, if metadata grows large or runs in a restricted file descriptor environment, this output remains stable and does not exceed function time limits.pkg/cli/cli.go (3)
24-41: Strengthen type-check inMakeGenericConfiguration.
Currently, line 29 checks only for an exact match toviper.Viper{}, ignoring pointer types or alternative usage. This might cause runtime panics ifTis something like*viper.Viper. Consider a refined type check or fallback error message.
67-220: Consider splittingSetFlagsAndConstraintsinto multiple smaller functions.
This function handles flag creation, default assignment, hidden/required logic, and relationship constraints in a single pass. Splitting it can improve maintainability and clarity.
203-217: Caution in applying constraints.
If future constraints become more complex (e.g., dynamic or conditional constraints), the switch statement might grow unwieldy. Plan for a more robust constraint-handling pattern if additional constraint types are expected.pkg/cli/commands.go (3)
29-29: Rename “ContrainstSetter” to “ConstraintSetter” for clarity and correctness.
The current spelling could cause confusion for future maintainers and might be interpreted as a typo.- type ContrainstSetter func(*cobra.Command, field.Configuration) error + type ConstraintSetter func(*cobra.Command, field.Configuration) error
352-425: Confirm that capabilities retrieval is supported by all connectors.
Line 399 indicates an error if capabilities are not supported. Ensure it’s intentional for connectors that omit or do not implement such metadata.
427-446: Exporting schema as JSON.
MakeConfigSchemaCommandsimply marshals the schema to JSON. This simplistic approach is fine, but consider how you will handle future expansions (e.g., field categories, localized descriptions, or nested structures).pkg/field/validation.go (3)
314-349: Consider enhancing error context.While the error handling is functional, consider adding more context to validation errors by including:
- The actual validation rule that failed
- The expected value range or format
This would make debugging configuration issues easier.Example enhancement:
func Validate(c Configuration, v cli.Configurable) error { present := make(map[string]int) - validationErrors := &ErrConfigurationMissingFields{} + validationErrors := &ErrConfigurationMissingFields{ + configName: c.Name, // Add config name for context + }
133-242: Consider refactoringValidateStringRulesfor better maintainability.The function is quite long and handles multiple responsibilities. Consider:
- Extracting well-known string validation into a separate function
- Using a map for well-known validators
- Making error messages more consistent
Example refactoring:
+var wellKnownValidators = map[v1_conf.WellKnownString]func(string) error{ + v1_conf.WellKnownString_WELL_KNOWN_STRING_EMAIL: validateEmail, + v1_conf.WellKnownString_WELL_KNOWN_STRING_URI: validateURI, + // ... other validators +} + +func validateWellKnownString(v string, wellKnown v1_conf.WellKnownString, name string) error { + if validator, ok := wellKnownValidators[wellKnown]; ok { + if err := validator(v); err != nil { + return fmt.Errorf("field %s: %w", name, err) + } + return nil + } + return fmt.Errorf("field %s: unknown well-known validation rule: %T", name, wellKnown) +}
83-91: Enhance hostname validation error reporting.While the hostname validation is thorough, it could provide more specific error messages to help users understand why their hostname is invalid.
Example enhancement:
-func isValidDomain(hostname string) bool { +func isValidDomain(hostname string) (bool, error) { if len(hostname) == 0 || len(hostname) > 253 { - return false + return false, fmt.Errorf("hostname length must be between 1 and 253 characters") } // ... rest of validation with specific errors - return true + return true, nil } -func validateHostname(hostname string) bool { +func validateHostname(hostname string) (bool, error) { if net.ParseIP(hostname) != nil { - return true + return true, nil } - return isValidDomain(hostname) + return isValidDomain(hostname) }Also applies to: 94-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/cli/cli.go(1 hunks)pkg/cli/commands.go(7 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/generate.go(5 hunks)pkg/field/validation.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/generate.go
- pkg/cli/lambda_no_support.go
- pkg/config/config.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/cli/cli.go
1-1: : import cycle not allowed: import stack: [github.com/conductorone/baton-sdk/internal/tests github.com/conductorone/baton-sdk/pkg/config github.com/conductorone/baton-sdk/pkg/cli github.com/conductorone/baton-sdk/pkg/field github.com/conductorone/baton-sdk/pkg/cli]
(typecheck)
🪛 GitHub Actions: ci
pkg/cli/cli.go
[error] 1-1: Import cycle not allowed: import stack: [github.com/conductorone/baton-sdk/internal/tests github.com/conductorone/baton-sdk/pkg/config github.com/conductorone/baton-sdk/pkg/cli github.com/conductorone/baton-sdk/pkg/field github.com/conductorone/baton-sdk/pkg/cli] (typecheck)
🔇 Additional comments (9)
pkg/cli/lambda_support.go (4)
33-49: Consider capturing and handling errors from AddCommand more proactively.
While you already handle the error fromAddCommandon line 50, ensure that any subsequent setup tasks (e.g., binding flags, validating schema) also consider recovery or cleanup if this step fails. Otherwise, partial initialization might lead to a fragile command state.
116-119: Double-check concurrency behavior oflambda.Start(s.Handler).
Invokinglambda.Startblocks the current goroutine. Ensure your CLI architecture accounts for this blocking behavior (e.g., no additional tasks need to run concurrently after the Lambda loop starts).
185-189: No issues found for staticLambdaResolver.
Implementation cleanly returns the parsed endpoint. Looks good!
199-221: Ensure robust error handling when creating the Lambda client transport.
If the chosen AWS region or credentials are invalid,NewLambdaClientTransportcan fail. To improve user experience, consider surfacing a more contextual error message or fallback.pkg/cli/cli.go (1)
47-53: VisitFlags may override user-defined flags.
By unconditionally setting flags from Viper values if set,VisitFlagscan silently clobber flags set programmatically. If that's intentional, document this behavior. Otherwise, prompt a check to only overwrite default or empty values.pkg/cli/commands.go (3)
31-38: Validate injection points for MakeMainCommand.
Ensure all external dependencies, such asgetconnector, are tracked and tested. Missing or invalid dependencies can lead to runtime failures in the CLI flow.
186-190: Safe unmarshal in MakeMainCommand.
CallingMakeGenericConfiguration[T]returns an error when unmarshalling fails. The subsequent error check is correct. No issues found.
214-350: Check concurrency model in MakeGRPCServerCommand.
When running a gRPC server function, ensure that the parent command’s concurrency model is well-defined, and that shutting down viaos.Stdin.Read()or similar signals does not leave partial states.pkg/field/validation.go (1)
14-15: LGTM! Good abstraction withcli.Configurable.The change from
*viper.Vipertocli.Configurableimproves modularity by decoupling the validation logic from the specific configuration implementation. This makes the code more flexible and easier to test.Also applies to: 314-314
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
721dcaf to
ec4673b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cli/cli.go (1)
21-24:⚠️ Potential issueHandle pointer types to
viper.Viperto avoid potential panics.
IfTis defined as*viper.Viper, the current check compares against the non-pointer typeviper.Viper{}, which can cause runtime issues. This concern was mentioned in a previous review.if tType == reflect.TypeOf(viper.Viper{}) { + // Also account for *viper.Viper + if tType == reflect.TypeOf(&viper.Viper{}) { + return any(v).(T), nil + } return any(v).(T), nil }
🧹 Nitpick comments (6)
pkg/cli/lambda_support.go (2)
43-48: Consider validating subcommand creation errors more explicitly.
Currently, the creation oflambdaCmdrelies onAddCommand. If there is an issue with subcommand registration or flag binding, it will return an error. While the code checkserr != nil, ensure that the CLI user gets sufficient context about which subcommand failed. This could be done by wrapping the error with details about the subcommand name or by logging subcommand info before returning.
110-114: Clarify usage of Provisioning and Ticketing flags.
TheFIXMEcomments indicate uncertainty about whether or how to expose these feature flags via the config service. If these are intended to be user-configurable, consider reading them fromvor from the schema. If they are truly fixed for Lambda usage, document this design decision.Do you want me to update and propose a revised approach or open a new issue to track the resolution of these flags?
pkg/config/config.go (1)
128-158: Account for nested fields and pointer-pointer edge cases inverifyStructFields.
The current check only ensures that top-level fields match by name in a direct struct or single pointer to struct. Consider whether nested struct fields or pointer-pointer types need coverage. If not, explicitly document this limitation to prevent usage confusion.pkg/cli/cli.go (1)
196-207: Implement logic forDependentsconstraints if required.
Currently, the code does nothing forfield.Dependents. If you intend to enforce relationships (e.g., “field A requires B to be set”), consider integrating a check similar toMarkFlagsRequiredTogether. Otherwise, clarify that dependent constraints are intentionally ignored.pkg/cli/commands.go (1)
29-29: Fix typo in type name.The type name has a typo: "ContrainstSetter" should be "ConstraintSetter".
-type ContrainstSetter func(*cobra.Command, field.Configuration) error +type ConstraintSetter func(*cobra.Command, field.Configuration) errorpkg/field/validation.go (1)
308-313: Add documentation for the Configurable interface.The interface would benefit from documentation explaining its purpose and usage.
+// Configurable defines the interface for types that can provide configuration values. +// This interface abstracts the configuration source, allowing for different implementations +// beyond Viper. type Configurable interface { + // GetString returns the string value associated with the given key. GetString(key string) string + // GetBool returns the boolean value associated with the given key. GetBool(key string) bool + // GetInt returns the integer value associated with the given key. GetInt(key string) int + // GetStringSlice returns the string slice value associated with the given key. GetStringSlice(key string) []string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/cli/cli.go(1 hunks)pkg/cli/commands.go(7 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/generate.go(5 hunks)pkg/field/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/config/generate.go
- pkg/cli/lambda_no_support.go
🔇 Additional comments (7)
pkg/cli/lambda_support.go (1)
119-119: Confirm Lambda server blocking behavior.
lambda.Start(s.Handler)is a blocking call. Verify that this command is intended to be the final active step in your CLI flow. If more CLI subcommands or tasks need to run afterward, they won't execute until the Lambda process ends.pkg/config/config.go (1)
50-59: Ensure consistent merging of default fields.
By doing:confschema.Fields = append(field.DefaultFields, confschema.Fields...)and then deduplicating, any changes or patches to
DefaultFieldsrisk conflicting with user-defined fields if they share the same key. Make sure that overwriting the user’s configuration is always the desired outcome, or reverse the order if you want the user’s fields to take precedence.pkg/cli/commands.go (4)
31-212: LGTM!The function signature has been correctly updated to use the new
field.Configurableinterface, improving type safety while maintaining the existing functionality.
214-350: Fix undefined InitLogger function references.The pipeline failures indicate that
InitLoggeris undefined. While the function has been renamed frominitLoggertoInitLoggerto be exported, the implementation appears to be missing or not included in this PR.
352-425: LGTM!The function signature has been correctly updated to use the new
field.Configurableinterface, improving type safety while maintaining the existing functionality.
427-445: LGTM!The function signature has been correctly updated to use the new
field.Configurableinterface, improving type safety while maintaining the existing functionality.pkg/field/validation.go (1)
320-320: LGTM!The function signature has been correctly updated to use the new
Configurableinterface, reducing coupling to Viper while maintaining the existing functionality.
pkg/cli/lambda_support.go
Outdated
| err = mapstructure.Decode(config.Config.AsMap(), t) | ||
| if err != nil { | ||
| log.Fatalf("Error decoding: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid calling log.Fatalf to allow graceful error handling.
Using log.Fatalf("Error decoding: %v", err) will terminate the entire program immediately. This might be acceptable in certain CLI flows, but consider returning the error to maintain consistency with other places in the function where errors are returned. This will let upstream callers manage the failure more gracefully.
- log.Fatalf("Error decoding: %v", err)
+ return fmt.Errorf("error decoding: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = mapstructure.Decode(config.Config.AsMap(), t) | |
| if err != nil { | |
| log.Fatalf("Error decoding: %v", err) | |
| } | |
| err = mapstructure.Decode(config.Config.AsMap(), t) | |
| if err != nil { | |
| return fmt.Errorf("error decoding: %w", err) | |
| } |
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
ec4673b to
cc16def
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/cli/cli.go (1)
17-34:⚠️ Potential issueFix type checking in
MakeGenericConfiguration.The type checking logic has potential issues:
- The commented-out struct type check would prevent pointer types from working.
- The current implementation might panic when casting to interface.
Apply this diff to improve type safety:
func MakeGenericConfiguration[T field.Configurable](v *viper.Viper) (T, error) { var config T tType := reflect.TypeOf(config) - if tType == reflect.TypeOf(viper.Viper{}) { - return any(v).(T), nil + if tType == reflect.TypeOf(viper.Viper{}) || tType == reflect.TypeOf(&viper.Viper{}) { + if val, ok := any(v).(T); ok { + return val, nil + } + return config, fmt.Errorf("failed to cast viper to type %T", config) } err := v.Unmarshal(&config) if err != nil { return config, fmt.Errorf("failed to unmarshal config: %w", err) } return config, nil }pkg/cli/lambda_support.go (1)
96-99:⚠️ Potential issueReplace
log.Fatalfwith proper error handling.Using
log.Fatalfforces program termination and bypasses proper error handling.Apply this diff to maintain consistent error handling:
- log.Fatalf("Error decoding: %v", err) + return fmt.Errorf("error decoding: %w", err)
🧹 Nitpick comments (5)
pkg/cli/cli.go (2)
40-46: Consider handling errors from flag setting.The
VisitFlagsfunction silently ignores errors when setting flag values.Apply this diff to handle errors:
func VisitFlags(cmd *cobra.Command, v *viper.Viper) { + var errs []error cmd.Flags().VisitAll(func(f *pflag.Flag) { if v.IsSet(f.Name) { - _ = cmd.Flags().Set(f.Name, v.GetString(f.Name)) + if err := cmd.Flags().Set(f.Name, v.GetString(f.Name)); err != nil { + errs = append(errs, fmt.Errorf("failed to set flag %s: %w", f.Name, err)) + } } }) + if len(errs) > 0 { + for _, err := range errs { + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } + } }
196-213: Improve error message for invalid constraint kind.The error message for an invalid constraint kind is too generic.
Apply this diff to provide more context:
default: - return fmt.Errorf("invalid config") + return fmt.Errorf("invalid constraint kind: %v", constrain.Kind)pkg/cli/lambda_support.go (1)
199-221: Add error handling for AWS configuration loading.The function doesn't provide context when AWS configuration loading fails.
Apply this diff to improve error handling:
func lambdaConnectorClient(ctx context.Context, endpoint string, function string) (types.ConnectorClient, error) { cfg, err := aws_config.LoadDefaultConfig(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to load AWS config: %w", err) } var opts []func(*lambda_sdk.Options) if endpoint != "" { resolver, err := newStaticLambdaResolver(endpoint) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create endpoint resolver: %w", err) } opts = append(opts, lambda_sdk.WithEndpointResolverV2(resolver)) } lambdaClient := lambda_sdk.NewFromConfig(cfg, opts...) lambdaTransport, err := transport.NewLambdaClientTransport(ctx, lambdaClient, function) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create lambda transport: %w", err) } cc := c1_lambda_grpc.NewClientConn(lambdaTransport) return connector.NewConnectorClient(ctx, cc), nil }internal/connector/connector.go (1)
355-359: Document the RegisterOps struct fields.The struct fields lack documentation explaining their purpose and usage.
Add field documentation:
type RegisterOps struct { + // Ratelimiter is the rate limiting service implementation Ratelimiter ratelimitV1.RateLimiterServiceServer + // ProvisioningEnabled enables provisioning-related services ProvisioningEnabled bool + // TicketingEnabled enables ticketing-related services TicketingEnabled bool }pkg/cli/commands.go (1)
427-445: Add error handling for JSON marshaling.The error message for JSON marshaling failure lacks context.
Apply this diff to improve error handling:
func MakeConfigSchemaCommand[T field.Configurable]( ctx context.Context, name string, v *viper.Viper, confschema field.Configuration, getconnector GetConnectorFunc[T], ) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { pb, err := json.Marshal(&confschema) if err != nil { - return err + return fmt.Errorf("failed to marshal config schema: %w", err) } _, err = fmt.Fprint(os.Stdout, string(pb)) if err != nil { - return err + return fmt.Errorf("failed to write config schema: %w", err) } return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/connector/connector.go(3 hunks)pkg/cli/cli.go(1 hunks)pkg/cli/commands.go(7 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/generate.go(5 hunks)pkg/field/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/config/generate.go
- pkg/field/validation.go
- pkg/cli/lambda_no_support.go
- pkg/config/config.go
🔇 Additional comments (2)
pkg/cli/lambda_support.go (1)
110-114: Clarify and document hardcoded configuration values.The FIXME comments indicate uncertainty about rate limiting and provisioning configuration.
Run this script to check for any existing rate limiting and provisioning configurations:
#!/bin/bash # Search for rate limiting and provisioning configurations rg -A 5 'Ratelimiter|ProvisioningEnabled|TicketingEnabled' --type gopkg/cli/commands.go (1)
49-57:⚠️ Potential issueFix undefined
initLoggerfunction reference.The code calls an undefined
initLoggerfunction.Ensure that:
- The implementation file containing
initLoggeris included in the PR- The function is properly exported in its implementation file
- All import paths are correctly updated
kans
force-pushed
the
kans/morgabra/lambdaize
branch
from
cc16def to
53a48de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/cli/lambda_support.go (2)
96-99:⚠️ Potential issueFix potential panic when decoding configuration.
The
mapstructure.Decodeerror is handled by callinglog.Fatalf, which will terminate the program immediately. This prevents graceful error handling and could lead to a poor user experience.Apply this diff to handle the error gracefully:
- err = mapstructure.Decode(config.Config.AsMap(), t) - if err != nil { - log.Fatalf("Error decoding: %v", err) - } + err = mapstructure.Decode(config.Config.AsMap(), t) + if err != nil { + return fmt.Errorf("error decoding configuration: %w", err) + }
111-114: 🛠️ Refactor suggestionClarify and update provisioning and rate limiter configuration.
The hardcoded values for rate limiting and provisioning flags need to be clarified or made configurable.
Consider one of these options:
- Expose these values as configurable flags to align with the rest of the codebase.
- Document why Lambda functions have different behavior regarding rate limiting and provisioning.
internal/connector/connector.go (2)
361-397:⚠️ Potential issueAdd validation for nil ServiceRegistrar and ConnectorServer.
The function should validate its input parameters to prevent potential panics.
Apply this diff to add validation:
func Register(ctx context.Context, s grpc.ServiceRegistrar, srv types.ConnectorServer, opts *RegisterOps) { + if s == nil { + panic("ServiceRegistrar cannot be nil") + } + if srv == nil { + panic("ConnectorServer cannot be nil") + } if opts == nil { opts = &RegisterOps{} }
399-417:⚠️ Potential issueAdd validation for nil ClientConnInterface.
The function should validate its input parameter to prevent potential panics.
Apply this diff to add validation:
func NewConnectorClient(ctx context.Context, cc grpc.ClientConnInterface) types.ConnectorClient { + if cc == nil { + panic("ClientConnInterface cannot be nil") + } return &connectorClient{ ResourceTypesServiceClient: connectorV2.NewResourceTypesServiceClient(cc), ResourcesServiceClient: connectorV2.NewResourcesServiceClient(cc),
🧹 Nitpick comments (6)
pkg/cli/cli.go (5)
25-27: Remove commented out code.The commented out code should be removed to maintain code cleanliness.
- // if tType.Kind() != reflect.Struct { - // return nil, fmt.Errorf("T must be a struct, but got %s", tType.Kind()) - // }
29-32: Improve error message for unmarshal failure.The error message could be more descriptive by including the type information.
- return config, fmt.Errorf("failed to unmarshal config: %w", err) + return config, fmt.Errorf("failed to unmarshal config of type %T: %w", config, err)
60-213: Refactor duplicated flag setup logic.The flag setup logic is duplicated across different flag types. Consider extracting common patterns into helper functions.
+type flagSetter interface { + BoolP(name, shorthand string, value bool, usage string) *pflag.Flag + IntP(name, shorthand string, value int, usage string) *pflag.Flag + StringP(name, shorthand string, value string, usage string) *pflag.Flag + StringSliceP(name, shorthand string, value []string, usage string) *pflag.Flag +} + +func setFlag[T any](f *field.Field, fs flagSetter, value *T, setter func(flagSetter, string, string, T, string) *pflag.Flag) error { + if f.IsPersistent() { + _ = setter(fs, f.FieldName, f.GetCLIShortHand(), *value, f.GetDescription()) + } else { + _ = setter(fs, f.FieldName, f.GetCLIShortHand(), *value, f.GetDescription()) + } + return nil +}This would allow simplifying the switch cases:
switch f.Variant { case field.BoolVariant: value, err := field.GetDefaultValue[bool](f) if err != nil { return fmt.Errorf("field %s, %s: %w", f.FieldName, f.Variant, err) } - if f.IsPersistent() { - command.PersistentFlags().BoolP(f.FieldName, f.GetCLIShortHand(), *value, f.GetDescription()) - } else { - command.Flags().BoolP(f.FieldName, f.GetCLIShortHand(), *value, f.GetDescription()) - } + err = setFlag(f, command.Flags(), value, func(fs flagSetter, name, short string, val bool, usage string) *pflag.Flag { + return fs.BoolP(name, short, val, usage) + })
208-208: Improve error message for invalid constraint kind.The error message "invalid config" is too generic. Consider including the invalid constraint kind in the error message.
- return fmt.Errorf("invalid config") + return fmt.Errorf("unsupported constraint kind: %v", constrain.Kind)
215-222: Fix typo in function name.The function name has a typo: "constrains" should be "constraints".
-func listFieldConstrainsAsStrings(constrains field.SchemaFieldRelationship) []string { +func listFieldConstraintsAsStrings(constraints field.SchemaFieldRelationship) []string {pkg/cli/commands.go (1)
29-29: Fix typo in type name.The type name
ContrainstSettercontains a typo and should beConstraintSetter.Apply this diff to fix the typo:
-type ContrainstSetter func(*cobra.Command, field.Configuration) error +type ConstraintSetter func(*cobra.Command, field.Configuration) error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.mod(2 hunks)internal/connector/connector.go(3 hunks)pkg/cli/cli.go(1 hunks)pkg/cli/commands.go(7 hunks)pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/generate.go(5 hunks)pkg/field/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/cli/lambda_no_support.go
- pkg/config/generate.go
- pkg/field/validation.go
- pkg/config/config.go
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (6)
pkg/cli/cli.go (1)
22-24: Fix incomplete type checking for viper.Viper.The type checking only handles
viper.Viper{}but not*viper.Viper.- if tType == reflect.TypeOf(viper.Viper{}) { + if tType == reflect.TypeOf(viper.Viper{}) || tType == reflect.TypeOf(&viper.Viper{}) {pkg/cli/lambda_support.go (3)
125-179: LGTM!The function is well-structured, with proper error handling, validation, and output formatting.
191-197: LGTM!The function correctly implements a static resolver for Lambda endpoints with proper error handling.
199-221: LGTM!The function correctly implements a Lambda client with proper error handling and optional endpoint resolution.
internal/connector/connector.go (1)
355-359: LGTM!The
RegisterOpsstruct provides a clean way to encapsulate service registration options.pkg/cli/commands.go (1)
49-54:⚠️ Potential issueFix undefined
initLoggerfunction references.The
initLoggerfunction is undefined, which will cause compilation errors.Ensure that:
- The implementation file containing
initLoggeris included in the PR- The function is properly exported in its implementation file
- All import paths are correctly updated
Also applies to: 138-143, 231-236, 369-374
| func VisitFlags(cmd *cobra.Command, v *viper.Viper) { | ||
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | ||
| if v.IsSet(f.Name) { | ||
| _ = cmd.Flags().Set(f.Name, v.GetString(f.Name)) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Handle potential errors from flag Set operation.
The error from Set operation is being ignored. This could mask important issues during flag value setting.
func VisitFlags(cmd *cobra.Command, v *viper.Viper) {
cmd.Flags().VisitAll(func(f *pflag.Flag) {
if v.IsSet(f.Name) {
- _ = cmd.Flags().Set(f.Name, v.GetString(f.Name))
+ if err := cmd.Flags().Set(f.Name, v.GetString(f.Name)); err != nil {
+ // Consider how to handle this error - could use a logger or return error through a channel
+ fmt.Printf("Warning: failed to set flag %s: %v\n", f.Name, err)
+ }
}
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func VisitFlags(cmd *cobra.Command, v *viper.Viper) { | |
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | |
| if v.IsSet(f.Name) { | |
| _ = cmd.Flags().Set(f.Name, v.GetString(f.Name)) | |
| } | |
| }) | |
| } | |
| func VisitFlags(cmd *cobra.Command, v *viper.Viper) { | |
| cmd.Flags().VisitAll(func(f *pflag.Flag) { | |
| if v.IsSet(f.Name) { | |
| if err := cmd.Flags().Set(f.Name, v.GetString(f.Name)); err != nil { | |
| // Consider how to handle this error - could use a logger or return error through a channel | |
| fmt.Printf("Warning: failed to set flag %s: %v\n", f.Name, err) | |
| } | |
| } | |
| }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cli/lambda_support.go (1)
87-90:⚠️ Potential issueReplace
log.Fatalfwith proper error handling.Using
log.Fatalfterminates the program immediately and bypasses proper error handling. This is inconsistent with the rest of the function where errors are returned for graceful handling.Apply this diff to fix the error handling:
err = mapstructure.Decode(config.Config.AsMap(), t) if err != nil { - log.Fatalf("Error decoding: %v", err) + return fmt.Errorf("error decoding config: %w", err) }
🧹 Nitpick comments (1)
pkg/cli/lambda_support.go (1)
61-61: Remove unused commented code.The line
l := ctxzap.Extract(runCtx)is commented out and not used. Either remove it or document why it's kept for future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cli/lambda_no_support.go(1 hunks)pkg/cli/lambda_support.go(1 hunks)pkg/lambda/grpc/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/lambda/grpc/util.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (3)
pkg/cli/lambda_no_support.go (1)
14-23: LGTM! The no-op implementation is correct.The function provides a clean fallback implementation when Lambda support is not built, following Go's best practices for conditional compilation.
pkg/cli/lambda_support.go (2)
24-43: LGTM! The function signature and Lambda schema setup are correct.The implementation follows best practices:
- Uses proper generic constraints with
field.Configurable- Sets up Lambda schema correctly
- Handles command setup errors appropriately
101-106: Clarify registration options configuration.The registration options are hardcoded with:
Ratelimiter: nilProvisioningEnabled: trueTicketingEnabled: trueThis needs clarification:
- Should these be configurable via flags or environment variables?
- If hardcoding is intentional, document the rationale in the comment.
Consider one of these options:
- Make the options configurable:
opts := &connector.RegisterOps{ - Ratelimiter: nil, - ProvisioningEnabled: true, - TicketingEnabled: true, + Ratelimiter: getRateLimiter(v), + ProvisioningEnabled: v.GetBool("provisioning-enabled"), + TicketingEnabled: v.GetBool("ticketing-enabled"), }
- Document the design decision:
-// TODO(morgabra/kans): This seems to be OK in practice - just don't invoke the unimplemented methods +// Note: Registration options are intentionally hardcoded for Lambda functions: +// - Rate limiting is disabled as Lambda has built-in concurrency control +// - Provisioning and ticketing are always enabled for Lambda functions
| return nil, err | ||
| } | ||
|
|
||
| if envHost, ok := os.LookupEnv("BATON_LAMBDA_TOKEN_HOST"); ok { |
There was a problem hiding this comment.
This should be part of the lambda config proper.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
pkg/lambda/grpc/config/claims.go (5)
23-26: Consider a more idiomatic function name.While the function is well-implemented, consider renaming it to follow Go's naming conventions:
-func sha256AndHexEncode(input string) string { +func hexEncodeSHA256(input string) string {
32-36: Fix error message context.The error message references "create-sigv4-sts-get-caller-identity-request" but this is in the
NewAdjusterfunction context.- return nil, fmt.Errorf("create-sigv4-sts-get-caller-identity-request: failed to load AWS configuration: %w", err) + return nil, fmt.Errorf("new adjuster: failed to load AWS configuration: %w", err)
41-47: Document the no-op implementations.Consider adding comments explaining why these methods are no-op implementations.
+// AdjustBody returns the body unmodified as required by the dpop.ClaimsAdjuster interface. func (a *adjuster) AdjustBody(body *url.Values) (dpop.Encodable, error) { return body, nil } +// AdjustClaims performs no adjustments as required by the dpop.ClaimsAdjuster interface. func (a *adjuster) AdjustClaims(claims *jwt.Claims) error { return nil }
49-53: Fix inconsistent error message prefix.The error message uses "c1-credential-provider" but other errors use different prefixes.
- return nil, fmt.Errorf("c1-credential-provider: failed to get identity attestation: %w", err) + return nil, fmt.Errorf("adjuster marshal: failed to get identity attestation: %w", err)
66-81: Improve error handling clarity.The error messages use inconsistent prefixes and have duplicate messages for different error cases.
func (a *adjuster) getIdentityAttestation() (string, error) { req, err := createSigv4STSGetCallerIdentityRequest(context.Background(), a.awsConfig) if err != nil { - return "", fmt.Errorf("c1-credential-provider: failed to create sigv4 sts get caller identity request: %w", err) + return "", fmt.Errorf("get identity attestation: failed to create STS request: %w", err) } anyReq, err := anypb.New(req) if err != nil { - return "", fmt.Errorf("c1-credential-provider: failed to create sigv4 sts get caller identity request: %w", err) + return "", fmt.Errorf("get identity attestation: failed to create protobuf Any message: %w", err) }pkg/cli/lambda_server__added.go (2)
97-102: Provide safe handling or stubs for unimplemented connector methods
Relying on “don’t invoke the unimplemented methods” may lead to runtime surprises. Consider returning acodes.Unimplementederror or logging a warning if those methods are called inadvertently.
104-107: Add logging or cleanup around Lambda server start
Onceaws_lambda.Start(...)is called, the flow will remain in the Lambda entry handler. If any error occurs in the server (e.g., an unhandled panic), it can be difficult to diagnose. Consider adding dedicated startup logs and a graceful shutdown strategy if possible.pkg/lambda/grpc/config/config.go (1)
134-137: Confirm Ed25519-only restriction
This logic enforces that the provided key is specifically aned25519.PrivateKey. If alternative key types (e.g., RSA, ECDSA) are needed in the future, this may need refactoring.pkg/field/defaults.go (2)
52-63: Unify naming for Lambda-specific client fields
Defining dedicated fields (lambda-client-idandlambda-client-secret) clarifies usage in Lambda contexts. However, you might consider unifying them with the existingclient-idandclient-secretfields to reduce duplicated code or confusion about which client credentials to use.
65-65: Consider removing or populating the unused relationships
LambdaServerRelationshipsis currently an empty slice. If no relationships are required, removing it could reduce confusion. Otherwise, adding placeholders or documentation clarifies future usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.mod(2 hunks)pkg/cli/lambda_server__added.go(1 hunks)pkg/cli/lambda_server_omitted.go(1 hunks)pkg/field/defaults.go(1 hunks)pkg/lambda/grpc/config/claims.go(1 hunks)pkg/lambda/grpc/config/config.go(1 hunks)pkg/lambda/grpc/config/sts.go(1 hunks)vendor/modules.txt(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/lambda_server_omitted.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/lambda/grpc/config/sts.go
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (9)
vendor/modules.txt (5)
12-17: AWS Lambda Go Modules Added.
These lines introduce the AWS Lambda Go modules (v1.47.0) along with their sub-packages (lambda, lambda/handlertrace, lambda/messages, and lambdacontext). This update aligns with the new AWS Lambda functionality you’re integrating. Please ensure that the corresponding changes in your go.mod file reflect these updates and that they have been tested with your Lambda runtime environment.
46-46: Update to Eventstream Version.
The version for the packagegithub.com/aws/aws-sdk-go-v2/aws/protocol/eventstreamhas been updated to v1.6.10. Verify that this version change is compatible with all parts of your codebase consuming eventstream functionality and that there are no breaking changes.
69-69: Update for Internal Configsources Dependency.
The internal packagegithub.com/aws/aws-sdk-go-v2/internal/configsourceshas been updated to v1.3.33. Ensure that this update is consistent with your go.mod and that any API changes in this new version are handled appropriately in your integration tests or downstream consumers.
73-73: Endpoints/v2 Version Bump.
Thegithub.com/aws/aws-sdk-go-v2/internal/endpoints/v2package has been updated to v2.6.33. It is important to verify that this update doesn’t introduce any incompatibilities with your usage of endpoints in your project, especially if the internal API has been modified.
154-157: New ConductorOne DPOP Dependency Introduced.
These lines add the new dependencygithub.com/conductorone/dpopat version v0.0.2 along with its sub-packages (pkgandpkg/ugrpc), and update the explicit Go version to 1.23.4. Confirm that this new dependency meets your requirements, that its APIs are used correctly in your code, and that any migration from v0.0.1 (if applicable) has been properly addressed.pkg/lambda/grpc/config/claims.go (1)
1-21: LGTM! Well-organized imports with compile-time interface verification.The imports are properly organized and the interface compliance check ensures type safety at compile time.
pkg/cli/lambda_server__added.go (1)
22-29: Nice approach to conditionally add the Lambda command
Using a build tag and a generic type parameter for configuration is a clean solution for modular builds. The signature is well-structured and keeps the existing CLI intact.pkg/lambda/grpc/config/config.go (2)
38-40: Centralize token host configuration
Overriding the token host via the environment variableBATON_LAMBDA_TOKEN_HOSTduplicates the configuration logic. Moving this into a top-level Lambda configuration might simplify maintainability and avoid confusion.
100-107: Revisit leftover portion of parsed client ID
After splitting by “/”, onlyitems[0]is used anditems[1]is discarded. If the second part was intended for further usage (e.g. tenant or environment info), consider handling or validating it to avoid silent data loss.
| // getIdentityAttestation creates a signed sts GetCallerIdentity request and marshals it to a base64 encoded string. | ||
| func (a *adjuster) getIdentityAttestation() (string, error) { | ||
| req, err := createSigv4STSGetCallerIdentityRequest(context.Background(), a.awsConfig) | ||
| if err != nil { |
There was a problem hiding this comment.
Use propagated context instead of background context.
Using context.Background() breaks context propagation. The context should be passed through from the caller.
-func (a *adjuster) getIdentityAttestation() (string, error) {
- req, err := createSigv4STSGetCallerIdentityRequest(context.Background(), a.awsConfig)
+func (a *adjuster) getIdentityAttestation(ctx context.Context) (string, error) {
+ req, err := createSigv4STSGetCallerIdentityRequest(ctx, a.awsConfig)This change will require updating the Marshal method signature and call site as well:
-func (a *adjuster) Marshal(claims *jwt.Claims) ([]byte, error) {
+func (a *adjuster) Marshal(ctx context.Context, claims *jwt.Claims) ([]byte, error) {
- getIdentityAttestation, err := a.getIdentityAttestation()
+ getIdentityAttestation, err := a.getIdentityAttestation(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // getIdentityAttestation creates a signed sts GetCallerIdentity request and marshals it to a base64 encoded string. | |
| func (a *adjuster) getIdentityAttestation() (string, error) { | |
| req, err := createSigv4STSGetCallerIdentityRequest(context.Background(), a.awsConfig) | |
| if err != nil { | |
| // getIdentityAttestation creates a signed sts GetCallerIdentity request and marshals it to a base64 encoded string. | |
| func (a *adjuster) getIdentityAttestation(ctx context.Context) (string, error) { | |
| req, err := createSigv4STSGetCallerIdentityRequest(ctx, a.awsConfig) | |
| if err != nil { | |
| return "", err | |
| } | |
| // ... rest of the implementation | |
| } | |
| // Marshal serializes the claims and attaches the identity attestation. | |
| func (a *adjuster) Marshal(ctx context.Context, claims *jwt.Claims) ([]byte, error) { | |
| getIdentityAttestation, err := a.getIdentityAttestation(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // ... rest of the implementation | |
| } |
wip wip wip
Summary by CodeRabbit
New Features
Refactor
Chores