Skip to content

(feat): adds publish support for aws secrets manager and parameter store #1953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
bruce-szalwinski-he wants to merge 28 commits into
base: main
Choose a base branch
from
Open

(feat): adds publish support for aws secrets manager and parameter store #1953

bruce-szalwinski-he wants to merge 28 commits into from

Conversation

@bruce-szalwinski-he
Copy link
Contributor

@bruce-szalwinski-he bruce-szalwinski-he commented Sep 20, 2025

Adds AWS Secrets Manager and Parameter Store publish support. Works same as vault in that it uploads unencrypted results as plain text JSON.

fixes #1942, #1105

bruce-szalwinski-he and others added 8 commits September 19, 2025 22:12
@bruce-szalwinski-he
support for aws secrets manager and parameter store
05e6728 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
parameter store is json-based storage
40b95a1 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
feat: add AWS Secrets Manager key/value format testing and comprehens…
047e3b6 
…ive destination validation

- Add comprehensive destination conflict validation tests for all 5 destinations
  (S3, GCS, Vault, AWS Secrets Manager, AWS Parameter Store) in config_test.go
- Add keyvalue-secrets.yaml example showing optimal format for AWS console key/value editor
- Add aws-secrets-manager-keyvalue-format.md documentation explaining JSON vs key/value formats
- Add destination-test-coverage.md documenting complete test matrix (10 conflict scenarios)

This ensures proper validation of destination conflicts and provides clear guidance
for using AWS Secrets Manager key/value format to enable the AWS console editor.

Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
remove unused
bdd4676 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
fix: make AWS Parameter Store Upload method consistent with other des…
09d51b7 
…tinations

- Add test to verify Upload method returns NotImplementedError
- Replace Parameter Store Upload implementation with NotImplementedError
- Ensure consistency with Vault and Secrets Manager destinations
- The publish command uses UploadUnencrypted for all structured destinations

This removes unreachable code and aligns with the current design where:
- S3/GCS: Upload encrypted files (Upload method)
- Vault/Secrets Manager/Parameter Store: Upload decrypted JSON (UploadUnencrypted method)

Test-driven fix: wrote failing test, then implemented the fix.

Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
throw not implemented for upload
217cb1b 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
test: update Parameter Store integration test to match new Upload beh…
7dd858e 
…avior

- Update TestAWSParameterStoreDestination_EncryptedFile_Integration to test NotImplementedError
- Remove test of legacy Upload functionality that's not used by publish command
- Verify Upload method now returns NotImplementedError consistently with other destinations
- All integration tests pass with real AWS credentials

The publish command uses UploadUnencrypted for Parameter Store, so Upload method
should return NotImplementedError like Vault and Secrets Manager destinations.

Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
remove unused
292c9b0 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he bruce-szalwinski-he force-pushed the feature/aws-publishing-support branch from 094d6c9 to 292c9b0 Compare September 20, 2025 03:14
bruce-szalwinski-he
bruce-szalwinski-he commented Sep 20, 2025
View reviewed changes
config/config_aws_test.go
@@ -0,0 +1,123 @@
package config
Copy link
Contributor Author

@bruce-szalwinski-he bruce-szalwinski-he Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config_test was getting a little long, so added aws config tests as separate file.

@bruce-szalwinski-he
remove unused
88fe4ae 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@felixfontein felixfontein requested a review from a team September 20, 2025 12:59
bruce-szalwinski-he
bruce-szalwinski-he commented Sep 20, 2025
View reviewed changes
publish/aws_parameter_store.go
parameterType = "SecureString"
}

// Ensure parameter path starts with /
Copy link
Contributor Author

@bruce-szalwinski-he bruce-szalwinski-he Sep 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.aws.amazon.com/systems-manager/latest/userguide/sysman-paramstore-hierarchies.html - parameter hierarchies require leading /. parameter hierarchies was introduced in 2017, https://aws.amazon.com/about-aws/whats-new/2017/06/amazon-ec2-systems-manager-adds-hierarchy-tagging-and-notification-support-for-parameter-store/

@bruce-szalwinski-he
docs on systems manager and parameter store
8a54645 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
parameter store is always secureString
cf02b0b 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
words matter
9427fab 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he bruce-szalwinski-he force-pushed the feature/aws-publishing-support branch from ca8cf9f to 9427fab Compare September 22, 2025 14:26
@bruce-szalwinski-he
Copy link
Contributor Author

bruce-szalwinski-he commented Oct 1, 2025

@felixfontein I see that you have been busy preparing the 3.11.0 release, so may not have had time to look at this one. Just checking in to see if there is anything else I need to provide for this PR.

bruce-szalwinski-he and others added 6 commits October 12, 2025 10:40
@bruce-szalwinski-he
Merge branch 'main' into feature/aws-publishing-support
e5ea0d9 
Signed-off-by: bruce-szalwinski-he <150711512+bruce-szalwinski-he@users.noreply.github.com>
@bruce-szalwinski-he
Merge branch 'main' into feature/aws-publishing-support
2a9721a 
Signed-off-by: bruce-szalwinski-he <150711512+bruce-szalwinski-he@users.noreply.github.com>
@bruce-szalwinski-he
Merge branch 'main' into feature/aws-publishing-support
5510949 
Signed-off-by: bruce-szalwinski-he <150711512+bruce-szalwinski-he@users.noreply.github.com>
@bruce-szalwinski-he
Merge branch 'main' into feature/aws-publishing-support
e6f0dc4 
Signed-off-by: bruce-szalwinski-he <150711512+bruce-szalwinski-he@users.noreply.github.com>
@bruce-szalwinski-he
tidy
7d5dddd 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
Merge branch 'main' into feature/aws-publishing-support
91b5bb7 
Signed-off-by: bruce-szalwinski-he <150711512+bruce-szalwinski-he@users.noreply.github.com>
@bruce-szalwinski-he
tidy
fc0767d 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
sabre1041
sabre1041 reviewed Nov 25, 2025
View reviewed changes
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the functionality of both the AWS Secrets Manager and AWS Parameter Store

A question that this integration has raised is how to handle the AWS region. The existing S3 integration does not include a dedicated property and instead relies on the default properties that the golang AWS library leverages.

Why not use an aws_region property to each of the AWS related publishers instead of specific parameters. This simplifies the end user experience, but also adds functionality for the S3 publisher. If it is not included, default to the logic of the AWS library

@sabre1041
Copy link
Contributor

sabre1041 commented Dec 7, 2025

@bruce-szalwinski-he any chance that you might be able to resolve the conflicts?

@bruce-szalwinski-he
Copy link
Contributor Author

bruce-szalwinski-he commented Dec 8, 2025

@bruce-szalwinski-he any chance that you might be able to resolve the conflicts?

yes. I'll resolve when I align the region as you have suggested above.

@bruce-szalwinski-he
Merge upstream/main and resolve go.mod/go.sum conflicts
5804832 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
Unify AWS region handling for publish destinations
f0fb356 
- Replace service-specific region fields (aws_secrets_manager_region,
  aws_parameter_store_region) with a single optional aws_region field
- Use aws_secrets_manager_secret_name and aws_parameter_store_path for
  destination detection instead of region fields
- Make region optional, falling back to AWS SDK defaults (env vars,
  config file, IAM role) - consistent with S3 behavior
- Update documentation and tests

Addresses feedback from PR review regarding inconsistent region handling
across AWS publish destinations.

Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@bruce-szalwinski-he
Copy link
Contributor Author

bruce-szalwinski-he commented Dec 9, 2025

Confirmed the functionality of both the AWS Secrets Manager and AWS Parameter Store

A question that this integration has raised is how to handle the AWS region. The existing S3 integration does not include a dedicated property and instead relies on the default properties that the golang AWS library leverages.

Why not use an aws_region property to each of the AWS related publishers instead of specific parameters. This simplifies the end user experience, but also adds functionality for the S3 publisher. If it is not included, default to the logic of the AWS library

went with consistently named aws_region as optional element on secrets manager and parameter store. can follow up with PR against s3 if needed.

sabre1041
sabre1041 reviewed Dec 14, 2025
View reviewed changes
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Theres one final comment related to the description that is populated on the backend

@bruce-szalwinski-he
remove description
84c3124 
Signed-off-by: bruce-szalwinski-he <bruce.szalwinski@engine.com>
@sabre1041
Copy link
Contributor

sabre1041 commented Dec 15, 2025 •
edited
Loading

@bruce-szalwinski-he This PR actually enabled me to investigate the full functionality of the publish subcommand. I discovered that the --recursive option was not working properly. I have submitted a PR to resolve the issue. However, it does not appear that this PR will support publishing multiple files as the latest content will overwrite previously configured/published content

@bruce-szalwinski-he
Copy link
Contributor Author

bruce-szalwinski-he commented Dec 15, 2025

@bruce-szalwinski-he This PR actually enabled me to investigate the full functionality of the publish subcommand. I discovered that the --recursive option was not working properly. I have submitted a PR to resolve the issue. However, it does not appear that this PR will support publishing multiple files as the latest content will overwrite previously configured/published content

I'll take a peek and see if there is a way to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sabre1041 sabre1041 Awaiting requested review from sabre1041

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add support for AWS Secrets Manager

2 participants

@bruce-szalwinski-he @sabre1041