feat: add variable resolvers for GCP Secret Manager, Azure Key Vault,… - #13497
Open
abderbejaoui wants to merge 6 commits into
Open
feat: add variable resolvers for GCP Secret Manager, Azure Key Vault,…#13497abderbejaoui wants to merge 6 commits into
abderbejaoui wants to merge 6 commits into
Conversation
… Consul KV, Cloudflare KV, and AWS AppConfig Implements additional variable resolvers as proposed in serverless#13161. New standalone providers: - GCP Secret Manager (${gcpSecretManager:project/secretName}) - Azure Key Vault (${azureKeyVault:vaultName/secretName}) - Consul KV (${consul:path/to/key}) - Cloudflare KV (${cloudflareKv:namespaceId/keyName}) New resolver added to existing AWS provider: - AWS AppConfig (${aws:appconfig:appId/envId/profileId}) Each resolver includes: - Zod config validation - Credential resolution with env var fallbacks - Comprehensive unit tests Closes serverless#13161
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
✅ 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. |
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds five new variable resolvers (AWS AppConfig, Azure Key Vault, GCP Secret Manager, Consul KV, Cloudflare KV), registers them in the provider registry, adds an AWS SDK dependency, and includes comprehensive unit tests for each resolver. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
Author
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sf-core/src/lib/resolvers/providers/aws/aws.js (1)
137-157:⚠️ Potential issue | 🔴 CriticalError handling swallows non-ExpiredToken errors.
When
error.name !== 'ExpiredToken', the code setserrtoundefinedand then throwserr(line 156), which throwsundefined. This breaks error propagation for all non-ExpiredToken errors from the resolver functions.🐛 Proposed fix to preserve original error
} catch (error) { let err if (error.name === 'ExpiredToken') { const errorMessage = `AWS credentials appear to have expired. This is likely due to the use of temporary credentials (e.g. AWS SSO, AWS IAM STS). Original error from AWS: "${error.message}"` err = Object.assign( new ServerlessError( errorMessage, ServerlessErrorCodes.general.AWS_CREDENTIALS_MISSING, { originalMessage: error.message, originalName: error.name, stack: false, }, ), { providerError: error, }, ) + } else { + err = error } throw err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sf-core/src/lib/resolvers/providers/aws/aws.js` around lines 137 - 157, The catch block currently assigns err only when error.name === 'ExpiredToken' and then unconditionally throws err, causing non-ExpiredToken errors to become undefined; update the catch in aws.js (the catch that constructs a ServerlessError with ServerlessErrorCodes.general.AWS_CREDENTIALS_MISSING and adds providerError) so that for non-ExpiredToken cases you rethrow the original error (or assign err = error) instead of throwing undefined, preserving the original error's stack and message; ensure any wrapped ServerlessError still sets providerError and originalMessage/originalName when used.
🧹 Nitpick comments (3)
packages/sf-core/tests/unit/resolvers/aws-appconfig.test.js (1)
116-135: Consider adding test for deeply nested JSON path.The test covers
missing.keybut doesn't test traversal through intermediatenull/undefinedvalues. Given the issue flagged in the implementation about JSON path traversal, a test likeexisting.nested.missingwhereexisting.nestedisnullwould help validate the error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sf-core/tests/unit/resolvers/aws-appconfig.test.js` around lines 116 - 135, Add a unit test to verify traversal fails when an intermediate JSON node is null: in packages/sf-core/tests/unit/resolvers/aws-appconfig.test.js add a case using resolveValueFromAppConfig with a configuration JSON like { "existing": { "nested": null } } and requesting the path "existing.nested.missing"; mockSend should return that JSON (same pattern as the existing test using InitialConfigurationToken and Configuration) and the assertion should expect a rejection with the same error text 'Key "existing.nested.missing" not found in AWS AppConfig configuration' to ensure resolveValueFromAppConfig correctly handles null/undefined intermediate nodes.packages/sf-core/src/lib/resolvers/providers/aws/appconfig.js (1)
84-91: Potential null/undefined dereference in JSON path traversal.The loop checks
value === undefined || value === nullat the start of each iteration, but then unconditionally accessesvalue[k]. Ifvalueis not an object (e.g., a primitive like a string or number), accessingvalue[k]may returnundefinedwithout throwing an appropriate error, or worse, ifvalueisnullafter being set tovalue[k]in a previous iteration, the check happens too late.Consider moving the check after the assignment to catch when a path segment doesn't exist:
♻️ Proposed fix for safer path traversal
const keys = jsonPath.split('.') let value = configData for (const k of keys) { - if (value === undefined || value === null) { + if (value === undefined || value === null || typeof value !== 'object') { throw new Error( `Key "${jsonPath}" not found in AWS AppConfig configuration`, ) } value = value[k] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sf-core/src/lib/resolvers/providers/aws/appconfig.js` around lines 84 - 91, The JSON path traversal loop using variables keys, value and jsonPath can dereference non-object or null values; change the loop to assign value = value[k] first, then immediately check if value === undefined || value === null (or typeof value !== 'object' when you expect an object for further traversal) and throw the Error(`Key "${jsonPath}" not found in AWS AppConfig configuration`) when the segment is missing, ensuring you validate after each assignment so you never access a property on null/undefined in subsequent iterations.packages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js (1)
66-70: Slash in secret names could cause incorrect project extraction.If a secret name contains a slash (e.g.,
my/nested/secret), the current logic would incorrectly treat the first part as a project. GCP Secret Manager does allow slashes in secret names.Consider documenting this limitation or adding a more explicit syntax (e.g.,
project:secretNameinstead ofproject/secretName).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js` around lines 66 - 70, The current parsing in gcp-secret-manager.js incorrectly treats the part before the first '/' as a GCP project which breaks when secret names contain slashes; change the parsing to recognize an explicit project separator (e.g., "project:secretName") instead of '/', so update the logic that inspects key (the variables project and secretName) to: if key contains ':' split on the first ':' to set project and secretName (use a single split with a limit of 2), otherwise treat the entire key as secretName and leave project unset/default; also update any related code/comments to document the new "project:secretName" syntax and preserve backward compatibility by not splitting on '/' anymore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/sf-core/src/lib/resolvers/providers/azure-key-vault/azure-key-vault.js`:
- Around line 42-43: The call to the parent async method is missing an await, so
change the invocation in resolveVariable to await super.resolveVariable({
resolverType, resolutionDetails, key }) so the parent promise is properly
awaited and errors propagate; update only the call site in the resolveVariable
method (reference: resolveVariable and super.resolveVariable).
In `@packages/sf-core/src/lib/resolvers/providers/cloudflare-kv/cloudflare-kv.js`:
- Around line 42-46: The call to super.resolveVariable(...) in resolveVariable
is missing an await, so the parent's async credential caching may not complete
before this provider proceeds; update resolveVariable to await
super.resolveVariable({ resolverType, resolutionDetails, key }) and then use the
cached this.credentials (populated by AbstractProvider.resolveVariable) instead
of calling this.resolveCredentials() again (or at minimum avoid redundant
resolveCredentials() calls when resolverType === 'cloudflareKv').
In `@packages/sf-core/src/lib/resolvers/providers/consul/consul.js`:
- Around line 46-50: In resolveVariable (the async method on the Consul
provider) you must await the parent call so credential caching is applied before
calling this.resolveCredentials(): change the super.resolveVariable({
resolverType, resolutionDetails, key }) invocation to await
super.resolveVariable(...). Ensure the await is used inside the async
resolveVariable method (where resolveCredentials() is called) so the parent's
async caching logic runs first.
In
`@packages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js`:
- Around line 42-43: The call to the async parent method is not awaited in
resolveVariable; update the resolveVariable implementation to await
super.resolveVariable({ resolverType, resolutionDetails, key }) so the
superclass resolution completes before continuing/returning, ensuring any
returned value or thrown errors are propagated correctly from the
resolveVariable method.
---
Outside diff comments:
In `@packages/sf-core/src/lib/resolvers/providers/aws/aws.js`:
- Around line 137-157: The catch block currently assigns err only when
error.name === 'ExpiredToken' and then unconditionally throws err, causing
non-ExpiredToken errors to become undefined; update the catch in aws.js (the
catch that constructs a ServerlessError with
ServerlessErrorCodes.general.AWS_CREDENTIALS_MISSING and adds providerError) so
that for non-ExpiredToken cases you rethrow the original error (or assign err =
error) instead of throwing undefined, preserving the original error's stack and
message; ensure any wrapped ServerlessError still sets providerError and
originalMessage/originalName when used.
---
Nitpick comments:
In `@packages/sf-core/src/lib/resolvers/providers/aws/appconfig.js`:
- Around line 84-91: The JSON path traversal loop using variables keys, value
and jsonPath can dereference non-object or null values; change the loop to
assign value = value[k] first, then immediately check if value === undefined ||
value === null (or typeof value !== 'object' when you expect an object for
further traversal) and throw the Error(`Key "${jsonPath}" not found in AWS
AppConfig configuration`) when the segment is missing, ensuring you validate
after each assignment so you never access a property on null/undefined in
subsequent iterations.
In
`@packages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js`:
- Around line 66-70: The current parsing in gcp-secret-manager.js incorrectly
treats the part before the first '/' as a GCP project which breaks when secret
names contain slashes; change the parsing to recognize an explicit project
separator (e.g., "project:secretName") instead of '/', so update the logic that
inspects key (the variables project and secretName) to: if key contains ':'
split on the first ':' to set project and secretName (use a single split with a
limit of 2), otherwise treat the entire key as secretName and leave project
unset/default; also update any related code/comments to document the new
"project:secretName" syntax and preserve backward compatibility by not splitting
on '/' anymore.
In `@packages/sf-core/tests/unit/resolvers/aws-appconfig.test.js`:
- Around line 116-135: Add a unit test to verify traversal fails when an
intermediate JSON node is null: in
packages/sf-core/tests/unit/resolvers/aws-appconfig.test.js add a case using
resolveValueFromAppConfig with a configuration JSON like { "existing": {
"nested": null } } and requesting the path "existing.nested.missing"; mockSend
should return that JSON (same pattern as the existing test using
InitialConfigurationToken and Configuration) and the assertion should expect a
rejection with the same error text 'Key "existing.nested.missing" not found in
AWS AppConfig configuration' to ensure resolveValueFromAppConfig correctly
handles null/undefined intermediate nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2dd431d-02cb-4b8e-b15a-4c15a5cd04a0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
packages/sf-core/package.jsonpackages/sf-core/src/lib/resolvers/providers/aws/appconfig.jspackages/sf-core/src/lib/resolvers/providers/aws/aws.jspackages/sf-core/src/lib/resolvers/providers/azure-key-vault/azure-key-vault.jspackages/sf-core/src/lib/resolvers/providers/cloudflare-kv/cloudflare-kv.jspackages/sf-core/src/lib/resolvers/providers/consul/consul.jspackages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.jspackages/sf-core/src/lib/resolvers/registry/index.jspackages/sf-core/tests/unit/resolvers/aws-appconfig.test.jspackages/sf-core/tests/unit/resolvers/aws.test.jspackages/sf-core/tests/unit/resolvers/azure-key-vault.test.jspackages/sf-core/tests/unit/resolvers/cloudflare-kv.test.jspackages/sf-core/tests/unit/resolvers/consul.test.jspackages/sf-core/tests/unit/resolvers/gcp-secret-manager.test.js
Add missing await to super.resolveVariable calls in provider implementations (azure-key-vault, cloudflare-kv, consul, gcp-secret-manager). This ensures the asynchronous base-class resolution completes before running provider-specific logic (e.g. resolveCredentials), preventing potential race conditions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sf-core/src/lib/resolvers/providers/consul/consul.js (1)
47-55: Use cached credentials fromsuper.resolveVariable()instead of resolving again.After
await super.resolveVariable(...), credentials are already resolved/cached onthis.credentials. Callingthis.resolveCredentials()again is redundant and weakens the base-class caching flow.♻️ Proposed refactor
resolveVariable = async ({ resolverType, resolutionDetails, key }) => { await super.resolveVariable({ resolverType, resolutionDetails, key }) if (resolverType === 'consul') { - const credentials = await this.resolveCredentials() + const { address, token } = this.credentials return await resolveValueFromConsul({ - address: credentials.address, - token: credentials.token, + address, + token, key, datacenter: this.config.datacenter, namespace: this.config.namespace, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sf-core/src/lib/resolvers/providers/consul/consul.js` around lines 47 - 55, The code currently calls this.resolveCredentials() after await super.resolveVariable(...), which defeats the base-class caching; instead, read credentials from the cached this.credentials set by super.resolveVariable and pass its address and token into resolveValueFromConsul (in the branch where resolverType === 'consul'), removing the redundant this.resolveCredentials() call and keeping the call to resolveValueFromConsul({ address: this.credentials.address, token: this.credentials.token, key }) so the base-class cache is respected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sf-core/src/lib/resolvers/providers/cloudflare-kv/cloudflare-kv.js`:
- Around line 68-72: The code currently treats any key containing '/' as a
namespace override by splitting key into namespaceId and keyName (symbols: key,
namespaceId, keyName), which wrongly overrides an already-configured
namespaceId; change the parsing so you only split and override namespaceId when
namespaceId is not already set (i.e., if (!namespaceId && key.includes('/')) {
... }), and apply the same guard to the other similar block handling keys at
lines 74-78 so existing namespaceId is preserved unless unset.
In `@packages/sf-core/src/lib/resolvers/providers/consul/consul.js`:
- Line 81: The Consul key is interpolated raw into the request path and must be
percent-encoded to avoid malformed URLs; update the construction of the url (the
line creating const url using baseUrl, key and params.toString()) to use an
encoded path segment for key (e.g., replace key with the result of
encodeURIComponent(key) or equivalent) so the path portion is safely encoded
before appending the query string.
---
Nitpick comments:
In `@packages/sf-core/src/lib/resolvers/providers/consul/consul.js`:
- Around line 47-55: The code currently calls this.resolveCredentials() after
await super.resolveVariable(...), which defeats the base-class caching; instead,
read credentials from the cached this.credentials set by super.resolveVariable
and pass its address and token into resolveValueFromConsul (in the branch where
resolverType === 'consul'), removing the redundant this.resolveCredentials()
call and keeping the call to resolveValueFromConsul({ address:
this.credentials.address, token: this.credentials.token, key }) so the
base-class cache is respected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 938c3cbe-ffb8-4799-a875-2af772246eb8
📒 Files selected for processing (4)
packages/sf-core/src/lib/resolvers/providers/azure-key-vault/azure-key-vault.jspackages/sf-core/src/lib/resolvers/providers/cloudflare-kv/cloudflare-kv.jspackages/sf-core/src/lib/resolvers/providers/consul/consul.jspackages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sf-core/src/lib/resolvers/providers/gcp-secret-manager/gcp-secret-manager.js
- packages/sf-core/src/lib/resolvers/providers/azure-key-vault/azure-key-vault.js
Comment on lines
+68
to
+72
| if (key.includes('/')) { | ||
| const parts = key.split('/') | ||
| namespaceId = parts[0] | ||
| keyName = parts.slice(1).join('/') | ||
| } |
Contributor
There was a problem hiding this comment.
Slash-containing key names are parsed as namespace overrides
Line 68 always treats / as a namespace delimiter. If namespaceId is already configured, a key like folder/a is reinterpreted as namespaceId=folder, which can fetch from the wrong namespace.
Proposed fix
let namespaceId = this.config.namespaceId
let keyName = key
- if (key.includes('/')) {
+ if (!namespaceId && key.includes('/')) {
const parts = key.split('/')
namespaceId = parts[0]
keyName = parts.slice(1).join('/')
}
if (!namespaceId) {
throw new Error(
'No namespace ID specified. Either include namespaceId in the resolver config, or reference it in the variable string as "${cloudflareKv:namespaceId/keyName}"',
)
}
+
+ if (!keyName) {
+ throw new Error(
+ 'No key name specified. Provide a key as "${cloudflareKv:keyName}" or "${cloudflareKv:namespaceId/keyName}".',
+ )
+ }Also applies to: 74-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sf-core/src/lib/resolvers/providers/cloudflare-kv/cloudflare-kv.js`
around lines 68 - 72, The code currently treats any key containing '/' as a
namespace override by splitting key into namespaceId and keyName (symbols: key,
namespaceId, keyName), which wrongly overrides an already-configured
namespaceId; change the parsing so you only split and override namespaceId when
namespaceId is not already set (i.e., if (!namespaceId && key.includes('/')) {
... }), and apply the same guard to the other similar block handling keys at
lines 74-78 so existing namespaceId is preserved unless unset.
| } | ||
|
|
||
| const baseUrl = address.replace(/\/+$/, '') | ||
| const url = `${baseUrl}/v1/kv/${key}?${params.toString()}` |
Contributor
There was a problem hiding this comment.
Encode the Consul key before building the request URL.
key is interpolated directly into the path. Special characters can produce malformed URLs or alter query semantics unexpectedly.
🔧 Proposed fix
- const url = `${baseUrl}/v1/kv/${key}?${params.toString()}`
+ if (!key) {
+ throw new Error('Consul key is required')
+ }
+ const encodedKey = key
+ .split('/')
+ .map((segment) => encodeURIComponent(segment))
+ .join('/')
+ const url = `${baseUrl}/v1/kv/${encodedKey}?${params.toString()}`📝 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.
Suggested change
| const url = `${baseUrl}/v1/kv/${key}?${params.toString()}` | |
| if (!key) { | |
| throw new Error('Consul key is required') | |
| } | |
| const encodedKey = key | |
| .split('/') | |
| .map((segment) => encodeURIComponent(segment)) | |
| .join('/') | |
| const url = `${baseUrl}/v1/kv/${encodedKey}?${params.toString()}` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sf-core/src/lib/resolvers/providers/consul/consul.js` at line 81,
The Consul key is interpolated raw into the request path and must be
percent-encoded to avoid malformed URLs; update the construction of the url (the
line creating const url using baseUrl, key and params.toString()) to use an
encoded path segment for key (e.g., replace key with the result of
encodeURIComponent(key) or equivalent) so the path portion is safely encoded
before appending the query string.
Author
|
@Mmarzex does this PR need any further changes? |
Author
|
Hi @Mmarzex, just following up on this PR. I addressed the review comments that were raised earlier, including: awaiting super.resolveVariable in the providers The remaining checks are passing, and the only pending item seems to be maintainer approval / final review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… Consul KV, Cloudflare KV, and AWS AppConfig
Implements additional variable resolvers as proposed in #13161.
New standalone providers:
New resolver added to existing AWS provider:
Each resolver includes:
Closes #13161
Summary by CodeRabbit
New Features
Chores
Tests