-
Notifications
You must be signed in to change notification settings - Fork 5
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
Enable AWS ENV VAR key auth #9
Conversation
pkg/provider/aws/aws.go
Outdated
@@ -83,7 +83,7 @@ type Client struct { | |||
|
|||
func New(log *logger.FunLogger, env v1alpha1.Environment, cacheFile string) (*Client, error) { | |||
// Create an AWS session and configure the EC2 client | |||
cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRegion(env.Spec.Region)) | |||
cfg, err := config.LoadDefaultConfig(context.TODO()) |
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.
Does this mechanism not work to set these options?
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.
with that set, it default to config file first, then env vars.
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.
I don't quite understand. There are three ways to set the AWS region here.
- The config file as defined in this project.
- The AWS config file.
- The
AWS_REGION
environment variable.
In what order to we want to apply them? Why is the region special compared to other environment variables?
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.
before this PR, this project was only taking into account the region in the config file.
This PR fixes that by moving to
- ENV AWS_REGION
- Config file
aws config file is not taken into account, since the idea is to be used by a github action or manual testing.
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.
Ok, but it isn't required to call os.Setenv("AWS_REGION")
to do this? Would something like the following:
region := env.Spec.Region
if envRegion := os.Getenv("AWS_REGION"); envRegion != "" {
region = envRegion
}
cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRegion(region))
not be more explicit, and not assume that the os
env is being read?
Note that if we take this further and resolve a "merged" spec where we load env.Spec
from file, then we only ever need to pass around env.Spec
.
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 suggestion, change added
a9a08c1
to
c790a11
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
c790a11
to
8d9ac35
Compare
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 @ArangoGutierrez
I think this looks good now. It may be good to consider how to handle envvars that override options in the config spec more generally at some point (like we do in the device plugin or GFD perhaps?), but that's not a blocker for this PR.
Before this PR, this project was only taking into account the region in the config file.
This PR fixes that by moving to (in descending order) :
The aws config file is not taken into account by design. The idea is to be used by a GitHub action or manual testing.