feat: support SecretsManagerConfiguration for Firehose API key - #238
cx-ryu[bot] wants to merge 5 commits into
Conversation
|
|
![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: 26ccb35bb8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - UseSecretsManager | ||
| - !Ref 'AWS::NoValue' | ||
| - !Ref ApiKey | ||
| SecretsManagerConfiguration: |
There was a problem hiding this comment.
When ApiKeySecretArn is set, this block is emitted inside EndpointConfiguration, but the CloudFormation schema defines SecretsManagerConfiguration on HttpEndpointDestinationConfiguration as a sibling of EndpointConfiguration, not as part of HttpEndpointConfiguration (AWS docs). Stacks using the new secret parameter will be rejected instead of enabling secret-based auth; move this block out one level.
Useful? React with 👍 / 👎.
| - UseSecretsManager | ||
| - !Ref 'AWS::NoValue' | ||
| - !Ref ApiKey | ||
| SecretsManagerConfiguration: |
There was a problem hiding this comment.
When ApiKeySecretArn is set, this block is emitted inside EndpointConfiguration, but the CloudFormation schema defines SecretsManagerConfiguration on HttpEndpointDestinationConfiguration as a sibling of EndpointConfiguration, not as part of HttpEndpointConfiguration (AWS docs). Stacks using the new secret parameter will be rejected instead of enabling secret-based auth; move this block out one level.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87da363538
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <!-- ### version / full date --> | ||
| <!-- * [Update/Bug fix] message that describes the changes that you apply --> | ||
|
|
||
| ### 0.0.14 / 4 Aug 2026 |
There was a problem hiding this comment.
This new 0.0.14 changelog entry is not matched by Metadata.SemanticVersion in aws-integrations/firehose-metrics/template.yaml, which still reads 0.0.13; the same mismatch exists for the new firehose-logs 0.0.8 entry versus its template's 0.0.5. The sync workflow derives the destination version from Metadata.SemanticVersion, so merging this will publish the changed templates under the old revision paths rather than the announced versions, leaving the Secrets Manager support/fix unavailable as a new release.
Useful? React with 👍 / 👎.
| Type: String | ||
| Description: The Coralogix Api key which is used to validate your authenticity. | ||
| NoEcho: true | ||
| Default: "" |
There was a problem hiding this comment.
Adding an empty default makes ApiKey optional, but there is no rule requiring either ApiKey or ApiKeySecretArn; if a stack is launched without both values, UseSecretsManager is false and Firehose is created with an empty HTTP endpoint access key, so Coralogix delivery will fail instead of CloudFormation stopping the deployment. The same empty default was added to the metrics template, so add a validation rule in both templates that requires one credential source.
Useful? React with 👍 / 👎.
| Default: "" | ||
| ApiKeySecretArn: | ||
| Type: String | ||
| Description: Optional - The ARN of an AWS Secrets Manager secret containing the Coralogix Api key. If set, the API key is read from Secrets Manager at runtime instead of the ApiKey parameter. |
There was a problem hiding this comment.
For the new ApiKeySecretArn path, a Secrets Manager secret that simply contains the raw key will not work: Amazon Data Firehose requires HTTP endpoint/Coralogix secrets to use an api_key JSON field (AWS docs). This description tells users only to provide a secret “containing the Coralogix Api key” (same in the metrics template), which is likely to produce deployments that create successfully but cannot authenticate; specify the required JSON shape here or in the template docs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41a9d46a12
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - Enabled: true | ||
| SecretARN: !Ref ApiKeySecretArn | ||
| RoleARN: !GetAtt FirehoseLogsRole.Arn |
There was a problem hiding this comment.
When an existing stack switches from ApiKey to ApiKeySecretArn, this conditional block adds SecretsManagerConfiguration; its fields are replacement-only in CloudFormation, and this delivery stream is custom-named as ${AWS::StackName}-logs. CloudFormation does not allow replacement of custom-named resources without a new name, so the migration path for existing log streams will fail; document this as recreate-only or provide a safe replacement name/path. AWS SecretsManagerConfiguration docs, custom name docs.
Useful? React with 👍 / 👎.
| - Enabled: true | ||
| SecretARN: !Ref ApiKeySecretArn | ||
| RoleARN: !GetAtt FirehoseMetricsRole.Arn |
There was a problem hiding this comment.
When an existing stack switches from ApiKey to ApiKeySecretArn, this conditional block adds SecretsManagerConfiguration; its fields are replacement-only in CloudFormation, and this delivery stream is custom-named as ${AWS::StackName}-metrics. CloudFormation does not allow replacement of custom-named resources without a new name, so the migration path for existing metric streams will fail; document this as recreate-only or provide a safe replacement name/path. AWS SecretsManagerConfiguration docs, custom name docs.
Useful? React with 👍 / 👎.
… firehose logs and metrics
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617d202877
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Action: | ||
| - kms:Decrypt | ||
| Resource: | ||
| - !Sub 'arn:aws:kms:${AWS::Region}:${AWS::AccountId}:key/*' |
There was a problem hiding this comment.
When ApiKeySecretArn points at a secret in another account encrypted with a customer-managed KMS key, Firehose's role still needs kms:Decrypt on that key, but this resource pattern only covers keys in the stack account. AWS supports cross-account Firehose secrets only with a CMK and the other account's key policy/resource policy, so delivery will fail after deployment unless this policy allows the secret account's key ARN or the template explicitly rejects/documents same-account-only secrets.
Useful? React with 👍 / 👎.
| Action: | ||
| - kms:Decrypt | ||
| Resource: | ||
| - !Sub 'arn:aws:kms:${AWS::Region}:${AWS::AccountId}:key/*' |
There was a problem hiding this comment.
When ApiKeySecretArn points at a secret in another account encrypted with a customer-managed KMS key, Firehose's role still needs kms:Decrypt on that key, but this resource pattern only covers keys in the stack account. AWS supports cross-account Firehose secrets only with a CMK and the other account's key policy/resource policy, so delivery will fail after deployment unless this policy allows the secret account's key ARN or the template explicitly rejects/documents same-account-only secrets.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…e Secrets Manager secrets
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Adds optional Secrets Manager support for the Coralogix API key in the firehose-logs and firehose-metrics templates, resolving #237.
This is a backward-compatible, opt-in, non-breaking change. A new ApiKeySecretArn parameter lets Amazon Data Firehose read the API key from Secrets Manager at runtime via SecretsManagerConfiguration on the HTTP endpoint destination, instead of storing it inline as AccessKey.
Requested by: <@U07V5PPAHBB>