Skip to content
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

config: Design of LoadDefaultConfig options is very generic, allows invalid loading options #926

Closed
jasdel opened this issue Nov 23, 2020 · 3 comments
Labels
breaking-change Issue requires a breaking change to remediate. refactor

Comments

@jasdel
Copy link
Contributor

jasdel commented Nov 23, 2020

The v2 SDK's design for LoadDefaultConfig options are very generic, and will accept any value since Config is an alias for interface{}. This means there are no direct links between the configuration loading utility and the options a user can provide. Other than looking for With... types/functions in the config module. In addition, since Config is an empty interface, users could provide any value here regardless if it is valid and usable or not.

To address these issues, I propose the config.LoadDefaultConfig function signature be changed to take function options for a LoadOptions struct instead of abstract Config empty interface type. Instead of LoadDefaultConfig taking an abstract set of configuration provider getters, LoadDefaultConfig would take functional options of a discrete set of options that are valid for loading the configuration. Helpers like WithRegion, WithSharedConfigProfile, etc would be functional options that set fields on the LoadOptions struct instead of value getters.

Usage:

cfg, err := config.LoadDefaultConfig(config.WithRegion("us-west-2"),
     func(o *config.LoadOptions) {
          o.SharedConfigProfile = "foo"
     })

config Package type definitions:

type LoadOptions struct {
     aws.Config
     
     SharedConfigProfile string
     CustomCABundle io.Reader // or string?
}

func LoadDefaultConfig(optFns ...func(*LoadOptions)) (aws.Config, error) {}

The embedded aws.Config allows configuration of default configuration options such as Region, and EndpointResolver. the value of the embedded aws.Config would be initialized with the config's current resolveDefaultConfig function before the user provided option functions are invoked. This design also removes the needed for resolvers like resolveEndpointResolvers, resolveRegion, and resolveHTTPClient.

The LoadOptions type would implement the provider getters that are used by the config loading resolvers.

@jasdel jasdel added refactor breaking-change Issue requires a breaking change to remediate. labels Nov 23, 2020
@jasdel jasdel added this to the v1.0 Release Candidate milestone Nov 23, 2020
@jasdel
Copy link
Contributor Author

jasdel commented Nov 23, 2020

resolveRegion resolver would still need to exist, since region can come from additional sources, such as environment variables and shared config. In addition to ec2 region if the SDK ever supports that.

@skotambkar
Copy link
Contributor

Fixed in #951

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issue requires a breaking change to remediate. refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants