-
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
[v2] Add configuration to disable host prefix injection #9268
base: v2
Are you sure you want to change the base?
Conversation
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.
Looks good, just a couple small suggestions. I'm also curious if there are existing unit/functional tests where you can actually set the environment variable or create a config file to test that they resolve correctly E2E.
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.
Could we wait to merge until we figure out the plan for botocore? Want to understand where V1 vs. V2 will land.
{ | ||
"type": "feature", | ||
"category": "configuration", | ||
"description": "Configure if the host prefix is prepended to the request URL in the shared configuration file or via an environment variable" |
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.
Could we document the specific environment variable and setting name here?
"description": "Configure if the host prefix is prepended to the request URL in the shared configuration file or via an environment variable" | |
"description": "Allow disabling the prepending of the host prefix to the request URL via ``disable_host_prefix_injection`` in the shared configuration file or the ``AWS_DISABLE_HOST_PREFIX_INJECTION`` environment variable" |
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.
Still need to do this - need to regenerate changelog after refactoring.
4ab1cc6
to
e764f66
Compare
We should still wait to merge once we confirm that this is going to be fine for |
I think we can - this should have been done for configured endpoint URLs. I'll investigate the same still. |
Re: functional testing when actually setting in config file or as an env variable: here's the examples we did for the configured endpoint URL. As I recall we did this specifically because this change also introduced complexity with the |
Issue #, if available:
Description of changes:
This change adds the ability to disable host prefix injection. It changes the default for
inject_host_prefix
in the Config object to be a True-like objectDEFAULT_TRUE
(fromTrue
) so that the value can be differentiated from a user-set value using the configuration precedence.Testing
Tested with the following commands which uses an operation-specific host prefix:
False
, which is the same as the default behavior:True
, which prevents the host prefix from being prepended to the endpoint URL:I also confirmed the behavior applies when using the configuration file setting
disable_host_prefix_injection
, and even when not using a custom endpoint URL.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.