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

[v2] Add configuration to disable host prefix injection #9268

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

kdaily
Copy link
Member

@kdaily kdaily commented Jan 31, 2025

Issue #, if available:

Port of boto/botocore#3405

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 object DEFAULT_TRUE (from True) 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:

  1. Default case (no disable host prefix configuration set) results in the host prefix being added:
$ aws omics create-sequence-store --name "test-sequence-store1" --description "Test store" --endpoint-url https://localhost:1234/ --tags "Environment=Test"

Could not connect to the endpoint URL: "https://control-storage-localhost:1234/sequencestore"
  1. Set config environment variable to False, which is the same as the default behavior:
AWS_DISABLE_HOST_PREFIX_INJECTION=False aws omics create-sequence-store --name "test-sequence-store1" --description "Test store" --endpoint-url https://localhost:1234/ --tags "Environment=Test"

Could not connect to the endpoint URL: "https://control-storage-localhost:1234/sequencestore"
  1. Set config environment variable to True, which prevents the host prefix from being prepended to the endpoint URL:
$ AWS_DISABLE_HOST_PREFIX_INJECTION=True aws omics create-sequence-store --name "test-sequence-store1" --description "Test store" --endpoint-url https://localhost:1234/ --tags "Environment=Test"

Could not connect to the endpoint URL: "https://localhost:1234/sequencestore"

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.

@kdaily kdaily requested review from hssyoo and a team January 31, 2025 22:07
Copy link
Contributor

@hssyoo hssyoo left a 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.

Edit: Chatted offline about setting env vars/config file. Decided we're okay with current tests.

Copy link
Member

@ashovlin ashovlin left a 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.

@kdaily kdaily force-pushed the disable-host-prefix-injection-config-v2 branch from 4ab1cc6 to e764f66 Compare February 11, 2025 20:58
@kdaily kdaily requested review from ashovlin, hssyoo and a team February 11, 2025 20:59
@kdaily
Copy link
Member Author

kdaily commented Feb 11, 2025

Could we wait to merge until we figure out the plan for botocore? Want to understand where V1 vs. V2 will land.

We should still wait to merge once we confirm that this is going to be fine for botocore.

@kdaily
Copy link
Member Author

kdaily commented Feb 11, 2025

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.

I think we can - this should have been done for configured endpoint URLs. I'll investigate the same still.

@kdaily
Copy link
Member Author

kdaily commented Feb 11, 2025

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 services section of the config file. Other params are not tested as extensively beyond what I already added here.

@kdaily kdaily marked this pull request as draft February 25, 2025 18:26
@kdaily
Copy link
Member Author

kdaily commented Feb 25, 2025

Marking as draft pending review and approval of PR in botocore:

boto/botocore#3405

This change allows for host prefix injection to be disabled through an
environment variable `AWS_DISABLE_HOST_PREFIX_INJECTION` or a shared
configuration file parameter `disable_host_prefix_injection`. This is
in addition to the existing Config parameter `inject_host_prefix`.

This change also changes the default value of the Config object's
`inject_host_prefix` from `True` to `None`. A property is used to
preserve the existing behavior when a user does not specify the
parameter to set it to `True`.

This preserves the behavior of a user setting inject_host_prefix=None
while keeping the logic internal.
@kdaily kdaily force-pushed the disable-host-prefix-injection-config-v2 branch from e764f66 to 5366eda Compare February 27, 2025 19:26
@kdaily kdaily requested a review from hssyoo February 27, 2025 21:40
@kdaily kdaily marked this pull request as ready for review February 27, 2025 21:40
@kdaily kdaily merged commit 2154122 into aws:v2 Mar 3, 2025
45 checks passed
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (619640c) to head (b729c01).
Report is 36 commits behind head on v2.

Additional details and impacted files
@@    Coverage Diff     @@
##   v2   #9268   +/-   ##
==========================
==========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants