-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add AWS_DYNAMODB_REGION Environment variable #15054
Conversation
Hi! Can you please also open and link a GitHub issue? Please don't forget a changelog entry, too. Thanks! :) |
@hsimon-hashicorp Added issue. How I can add changelog? can you point me to example? |
Thank you for the issue! Adding a changelog entry is described in the CONTRIBUTING.md. Thanks! :) |
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.
LGTM
This seems pretty reasonable to me. Thanks for this!
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.
Oops, I just saw the extra empty lines in the changelog file. Please remove those.
Done |
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.
LGTM
@@ -163,13 +163,16 @@ func NewDynamoDBBackend(conf map[string]string, logger log.Logger) (physical.Bac | |||
if endpoint == "" { | |||
endpoint = conf["endpoint"] | |||
} | |||
region := os.Getenv("AWS_REGION") | |||
region := os.Getenv("AWS_DYNAMODB_REGION") |
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.
@swenson I wonder if using a switch
is cleaner instead of deep nesting. I know that we have this pattern for the other config params, but since we're evaluating several env vars for this one it might not be a bad idea?
var region string
switch {
case os.Getenv("AWS_DYNAMODB_REGION") != "":
region = os.Getenv("AWS_DYNAMODB_REGION")
case os.Getenv("AWS_REGION") != "":
region = os.Getenv("AWS_REGION")
case os.Getenv("AWS_DEFAULT_REGION") != "":
region = os.Getenv("AWS_DEFAULT_REGION")
case conf["region"] != "":
region = conf["region"]
default:
region = DefaultDynamoDBRegion
}
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.
Oh, I was thinking that the deep nesting was a little bit much. The switch is much cleaner.
From eks 1.18
AWS_REGION
andAWS_DEFAULT_REGION
auto inject to pods. So if you can't use dynamodb table from other region it's read fromAWS_REGION
always and override on config file. Reorder setting can be break current vault settings. I added one new withAWS_DYNAMODB_REGION
aws/amazon-eks-pod-identity-webhook#40
fix #15071
changelog