Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughAdds support to update existing AWS API Gateway custom domains (V1 and V2), introduces Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin as ServerlessCustomDomain
participant WrapperV as APIGatewayV1/V2 Wrapper
participant AWS as AWS API Gateway SDK
User->>Plugin: Deploy domain config (name, apiType, securityPolicy, accessMode?)
Plugin->>Plugin: Determine API type (usesApiGatewayV2)
Plugin->>Plugin: Check existing domain via SDK
alt domain exists & configured fields differ
Plugin->>WrapperV: updateCustomDomain(domain)
WrapperV->>WrapperV: validate inputs (cert ARN / accessMode / securityPolicy)
WrapperV->>AWS: UpdateDomainNameCommand / Patch operations
AWS-->>WrapperV: DomainInfo response
WrapperV-->>Plugin: DomainInfo (updated)
Plugin->>Plugin: Log "was updated"
else domain exists & no drift
Plugin->>Plugin: Log "already exists"
else domain missing
Plugin->>WrapperV: createCustomDomain(domain)
WrapperV->>AWS: CreateDomainNameCommand
AWS-->>WrapperV: DomainInfo response
WrapperV-->>Plugin: DomainInfo (created)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@cursor review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5590f74c4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/sf/providers/aws/guide/domains.md (1)
226-253:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency:
apiType: restwithhttpApievent.Line 232 was changed to
apiType: rest, but the function event at line 251 still useshttpApi(which corresponds to HTTP APIs, not REST APIs). REST API events use- http:, not- httpApi:. This will confuse users.Proposed fix
Either revert
apiTypeback tohttp:- apiType: rest + apiType: httpOr change the event to match a REST API:
events: - - httpApi: - path: /users - method: get + - http: + path: /users + method: get
🤖 Fix all issues with AI agents
In `@packages/serverless/lib/plugins/aws/domains/models/domain-config.js`:
- Around line 15-16: The two boolean flags this.hasSecurityPolicyConfigured and
this.hasAccessModeConfigured should treat null and undefined the same; change
their assignments to check for != null (e.g., set
this.hasSecurityPolicyConfigured = config.securityPolicy != null and
this.hasAccessModeConfigured = config.accessMode != null) so that both null and
undefined are considered "not configured" and avoid mismatches with
_getAccessMode when deciding whether to patch values.
🧹 Nitpick comments (1)
packages/serverless/lib/plugins/aws/domains/index.js (1)
187-209: Validation logic looks correct and produces helpful error messages.The dynamic error message at line 204 that explains why a domain resolves to V2 (multi-segment basePath vs non-REST apiType) is a nice touch for developer experience.
One observation: the securityPolicy validation (line 188) is gated on
hasSecurityPolicyConfigured, but the accessMode validation (line 200) checksdomain.accessModedirectly. This is fine because_getAccessModereturnsundefinedwhen not configured, so the truthiness check works. But for consistency with the explicit-tracking pattern,domain.hasAccessModeConfiguredwould make the intent clearer:- if (domain.accessMode && this.usesApiGatewayV2(domain)) { + if (domain.hasAccessModeConfigured && this.usesApiGatewayV2(domain)) {
packages/serverless/lib/plugins/aws/domains/models/domain-config.js
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/serverless/lib/plugins/aws/domains/aws/api-gateway-v2-wrapper.js
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05a6001de3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/serverless/lib/plugins/aws/domains/aws/api-gateway-v1-wrapper.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/sf/providers/aws/guide/domains.md (1)
229-253:⚠️ Potential issue | 🟡 MinorExample configuration is invalid —
accessModerequires an enhancedsecurityPolicy.The example shows
securityPolicy: TLS_1_2withaccessMode: strict, but the validation logic inindex.js(Lines 211–222) requiressecurityPolicyto start withSecurityPolicy_whenaccessModeis set. This example would throw aDOMAIN_VALIDATION_INCOMPATIBLE_ACCESS_MODEerror at runtime. The reconciliation notes section at Line 425 correctly documents this requirement, but the example contradicts it.Fix the example to use an enhanced security policy
securityPolicy: TLS_1_2 + securityPolicy: SecurityPolicy_TLS13_2025_EDGE accessMode: strict
🤖 Fix all issues with AI agents
In `@packages/serverless/lib/plugins/aws/domains/index.js`:
- Around line 399-411: The catch currently wraps all errors with a
DOMAIN_CREATION_FAILED/“Unable to create domain” message which is misleading for
failures from apiGateway.updateCustomDomain; update the flow so failures thrown
by updateCustomDomain (called as updateCustomDomain(domain)) produce a distinct
or more accurate error message (e.g., “Unable to update domain” or “Unable to
create or update domain”) before rethrowing, by tracking whether you attempted
an update (set a local flag like isUpdate when calling updateCustomDomain or
inspect whether updatedDomainInfo path ran) and using that flag in the catch to
choose the appropriate error text; ensure domain.domainInfo assignment and
Logging.logInfo remain unchanged and only the error wrapping message is
adjusted.
|
@cursor review |
3 participants
Overview
Adds support for
accessModeconfiguration on custom domains, allowing users to set API Gateway endpoint access modes (BASIC or STRICT) for REST APIs managed through API Gateway V1.Changes
Core Features
basicandstrictvalues for REST domains with single-level basePathupdateCustomDomain()method for both V1 and V2 API Gateway wrappers to reconcile configuration driftaccessModeis restricted to REST APIs with single-level basePath; multi-level basePath and HTTP/WebSocket APIs use V2 and don't support this settingValidation & Error Handling
accessModeconfiguration on incompatible domain types (HTTP, WebSocket, multi-level REST)Explicit Configuration Tracking
hasSecurityPolicyConfiguredandhasAccessModeConfiguredflags to implement reconciliation semantics: only explicitly configured fields are updated; omitted fields remain unchangedPerformance
Tests
Documentation
Fixes #13329
Fixes #13332
Note
Medium Risk
Touches custom domain provisioning/update paths and adds reconciliation that can change live API Gateway domain settings; validation reduces misconfig risk but rollout could affect existing domain configurations.
Overview
Adds a new optional custom-domain setting,
accessMode(basic/strict), wired to API Gateway v1 domain creation and schema-validated inprovider.domainconfig.Implements
updateCustomDomain()for both API Gateway wrappers and updates the domain creation flow to reconcile existing domains: only explicitly configuredsecurityPolicyandaccessModeare updated (including allowing explicit unsetting ofaccessModeon v1), otherwise existing values are left untouched.Tightens validation around security settings: API Gateway v2-managed domains only allow
TLS_1_2securityPolicy, andaccessModeis rejected for v2-managed domains (non-REST or multi-segment RESTbasePath) and requires enhancedSecurityPolicy_*on v1. Docs and tests are updated accordingly, with new domain-related error codes.Written by Cursor Bugbot for commit 05a6001. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Validation
Documentation