-
Notifications
You must be signed in to change notification settings - Fork 459
DynamoDB storage backend #190
base: master
Are you sure you want to change the base?
Conversation
brikis98
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! A few minor fixes, and I can kick off tests.
| * `--s3-bucket` (optional): Specifies the S3 bucket to use to store Vault data. Only used if `--enable-s3-backend` is set. | ||
| * `--s3-bucket-path` (optional): Specifies the S3 bucket path to use to store Vault data. Default is `""`. Only used if `--enable-s3-backend` is set. | ||
| * `--s3-bucket-region` (optional): Specifies the AWS region where `--s3-bucket` lives. Only used if `--enable-s3-backend` is set. | ||
| * `--storage-backend` (optional): If this flag is set, the backend will be enabled in addition to the HA backend. Default is `consul`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Should list all valid values (consul, s3, dynamo) for this param and the ones below.
| echo -e " --enable-dynamo-backend\tIf this flag is set, DynamoDB will be enabled as the backend storage (HA)" | ||
| echo -e " --dynamo-region\tSpecifies the AWS region where --dynamo-table lives. Only used if '--enable-dynamo-backend is on'" | ||
| echo -e " --dynamo--table\tSpecifies the DynamoDB table to use for HA Storage. Only used if '--enable-dynamo-backend is on'" | ||
| echo -e " --storage-backend\tStorage backend type to use for secrets. Default is consul" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Same here. Should list all supported values for the backend params.
| if [[ "$enable_s3_backend" == "true" ]]; then | ||
| s3_config=$(cat <<EOF | ||
| local vault_ha_storage_backend="" | ||
| if [[ "$storage_backend" == "consul" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: to keep this function from growing bigger and bigger, we should extract the code that sets the storage backend into a separate function that uses a case statement and returns the proper value by writing it to stdout. Same for HA storage.
| local skip_vault_config="false" | ||
| local enable_s3_backend="false" | ||
| local storage_backend="consul" | ||
| local ha_storage_backend="consul" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Store these values in readonly variables at the top. Use the variables here and in the docs.
Description
Change
run-vaultto support dynamo backend as either storage or HA storage.Why
The implementation of DynamoDB has limited support only allowing DynamoDB as both the
storageandha_storagebackend.It is valid to use DynamoDB as either. These combinations should all be valid but not currently possible
DynamoDBConsulDynamoDBThis change enables you to specify the
--storage-backendas any ofs3|dynamodb|consuland the--ha-storage-backendasdynamodb|consul.Consul is the default for both to maintain expected behavior