Let Vault modify dynamodb tables and use pay-per-request billing mode. #25115
Conversation
This file contains 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
|
|
schavis
requested changes
Feb 21, 2024
schavis
added
the
content-lgtm
|
Content LGTM. Feel free to merge once ENG signs off on the code changes |
VioletHynes
reviewed
Jan 20, 2025
There was a problem hiding this comment.
Hi there! Sorry it took us some time to get to this PR, but it broadly looks good. I appreciate all of the new tests for the functionality. There's some small changes I'd like to see before it gets merged, e.g. the typo in the changelog. Thank you for this contribution, and I'm eager to try and see this one to merge.
VioletHynes
approved these changes
Jan 20, 2025
There was a problem hiding this comment.
Thank you! This looks great. Again, apologies it took us a while to get eyes on this. Appreciate your patience.
I'll wait for CI to finish on this PR and I need to get the 'requested changes' unflagged so it can be merged, but I'll try and get this merged as soon as everything's green.
No problem and thanks for the review! I've updated the PR based on your comments. There's a small annoying issue where some of my earlier commits had the incorrect commit author which the CLA check doesn't like (an old work account that has since been deactivated). |
* Update vault-plugin-auth-azure to v0.19.2 * Add changelog --------- Co-authored-by: hc-github-team-secure-vault-ecosystem <[email protected]>
…ndbys (#28807) * return 500 for RPC failures during perf standby logins * godoc * changelog
… latest (#28855) * Pull versioned golang images in Zlint testsuite to avoid pulling with latest - Leverage the versioned golang images which should be more static avoiding issues we somtimes encounter pulling latest images from our docker mirror. - We use the golang runtime version to avoid having to update this test continuously. * Fallback to latest if the version tag isn't a release tag
* Content conversion from tutorial to doc - seal * Add AppRole best practices * Clean up the reference list * Updated the title * match the titles * Add namespaces best practices * Update the table style
* adding ce changes * using oss patch
…28862) * add fragment locks to GetActiveLocalFragment and GetActiveFragment * update locks for all functions
Fix typo and text formatting
* changes then onto tests * fix wif test failures * changelog * clean up * address pr comments * only test one wif engine for relevant tests * add back engine loop for tests that depend on type
* initial changes need to add test coverage * change icon * replace original idea with hds::codeblock on kvv2 details view * changelog * fixing edit view by addressing viewportMargin * fix failing test * missedone * Update 28808.txt * Update json-editor.js * test coverage * update codeblock selector * Update general-selectors.ts * Update kv-data-fields.js * Update ui/lib/core/addon/components/json-editor.js Co-authored-by: claire bontempo <[email protected]> * Update ui/lib/kv/addon/components/kv-data-fields.js Co-authored-by: claire bontempo <[email protected]> * update test name * add default to modifier --------- Co-authored-by: claire bontempo <[email protected]>
* chore: update reference consul-helm url to consul-k8s * docs: add changelog --------- Co-authored-by: Yoko Hyakuna <[email protected]>
* Make identity store loading and alias merging deterministic * Add CHANGELOG * Refactor our Ent-only logic from determinism test * Use stub-maker * Add test godoc
* Move helper funcs out of tests to fix build errors * Rename identiy->identity
* ce changes for vault-31750 * add changelog * make proto * refactor naming * clarify error message * update changelog * one more time * make proto AGAIN
Signed-off-by: Ryan Cragun <[email protected]>
* add api docs for pqc key types * add pqc key types to docs * remove slh-dsa and add hybrid
…29334) * replace keyring dependency to address zombie dbus-daemons processes * changelog
* docs/vault-k8s: updates for v1.6.0 release * Apply suggestions from code review Co-authored-by: Sarah Chavis <[email protected]> * updating whitespace and an extra "injector" --------- Co-authored-by: Sarah Chavis <[email protected]>
…conciliation logic (#29328)
* check if not initialized * add comment and fix flake
* string-to-camel helper * fix: * Update string-to-camel-test.js * update comment * rename and clarify comment * welp, forgot to update test
* Add 'how to run' instructions for each scenario
* initial things without helper changes * adjust test for clean up of secret-engine-helper * remove added line thats better in next pr * remove extra check * 🧹 * replace return with continue within loops
* docs: DB skip auto import rotation * add usage section * add password field; mark self_managed_password as deprecated
This PR introduces a new type of conflict resolution for duplicate Entities and Groups. Renaming provides a way of preventing Vault from entering case-sensitive mode, which is the current behavior for any kind of duplicate. Renames append the conflicting identity artifact's UUID to its name and updates a metadata field to indicate the pre-existing artifact's UUID. The feature is gated by the force-identity-deduplication activation flag. In order to maintain consistent behavior between the reporting resolver and the rename operation, we need to adjust the behavior of generated reports. Previously, they intentionally preserved existing Group merge determinism, wherein the last MemDB update would win and all others would be renamed. This approach is more complicated for the rename resolver, since we would need to update any duplicated entity in the cache while inserting the new duplicate (resulting in two MemDB operations). Though we can ensure atomic updates of the two identity artifacts with transactions (which we could get for groups with a minor adjustment, and we will get along with batching of Entity upserts on load), it's far simpler to just rename all but the first insert as proposed in the current PR. Since the feature is gated by an activation flag with appropriate warnings of potential changes via the reporting resolver, we opt for simplicity over maintaining pre-existing behavior. We can revisit this assumption later if we think alignment with existing behavior outweighs any potential complexity in the rename operation. Entity alias resolution is left alone as a destructive merge operation to prevent a potentially high-impact change in existing behavior.
Typo in client count references.
* Updated the PostgreSQL database creation command to ensure the static role name is consistent. The role name specified in allowed_roles="my-role" under the section "Rootless Configuration and Password Rotation for Static Roles" should align with the static role name in step #3. Previously, the command incorrectly used "my-static-role"; it should be "my-role" to match the earlier step. The same role name should also be used when reading the static credentials in step #4 * Added the file changelog/29138.txt * Delete changelog/29138.txt --------- Co-authored-by: Yoko Hyakuna <[email protected]> Co-authored-by: akshya96 <[email protected]> Co-authored-by: Violet Hynes <[email protected]>
michael-diggin
force-pushed
the
let-vault-modify-dynamodb-tables
branch
from
229d3ad to
ecd9d7c
Compare
|
Ah I've made a mess of this trying to sort out those commits - let me close it and create a new one to save a huge headache, sorry! |
|
Hehe, it seems so! No matter, feel free to tag me (and @schavis) in the next one you open and I'll re-review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes Issue #25114.
This PR introduces support for creating the DynamoDB table that Vault would use with PAY_PER_REQUEST billing mode, and introduces support for allowing Vault to modify the billing mode and read/write capacity of the table if it already exists. This is done with new environment variables, and the current default behaviour is left unchanged (no-ops if the table already exists).