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

Default EC2RoleProvider ignores HTTPClient configuration #504

Closed
pwaller opened this issue Jan 14, 2016 · 8 comments
Closed

Default EC2RoleProvider ignores HTTPClient configuration #504

pwaller opened this issue Jan 14, 2016 · 8 comments

Comments

@pwaller
Copy link
Contributor

pwaller commented Jan 14, 2016

I don't know if my desire is reasonable here, but I have some code which tunnels AWS commands over an SSH connection to an EC2 instance by switching out the http.Transport.Dial function. I had this (conceptual) code working in the distant past but at some point it stopped working, I think some time around when aws-sdk-go moved out of labs.

s := session.New()
s.Config.HTTPClient.Transport = &http.Transport{Dial: viaBastionDialer}

ec2.New(s).DoSomething()

I expected the command to use the AWS credentials available from the perspective of viaBastionDialer, however with some debugging I determined that it was using the default HTTP dialer.

Reading the code I think the reason is that the default credentials provider is constructed by copying a the aws.Config the EC2RoleProvider, rather than sharing a reference.

I can work around this by over-riding the Credentials on my session, but I'm more concerned about writing future code where my HTTPClient choices are not respected. I write this up here at least so that there is something to search for if I hit this issue again.

Is there a good workaround, or way of setting HTTPClient so that it takes effect everywhere, or not?

@pwaller
Copy link
Contributor Author

pwaller commented Jan 14, 2016

For some reason the provider still doesn't work. Now I use this:

s.Config.HTTPClient.Transport = &http.Transport{Dial: viaBastionDialer}
// Copy HTTPClient configuration for EC2RoleProvider
s.Config.Credentials = defaults.CredChain(s.Config, defaults.Handlers())

With debugging enabled:

2016/01/14 15:28:24 DEBUG: Request ec2metadata/GetMetadata Details:
---[ REQUEST POST-SIGN ]-----------------------------
GET /latest/meta-data/iam/security-credentials HTTP/1.1
Host: 169.254.169.254
User-Agent: aws-sdk-go/1.0.8 (go1.6beta2; linux; amd64)
Accept-Encoding: gzip


-----------------------------------------------------
2016/01/14 15:28:27 DEBUG: Response ec2metadata/GetMetadata Details:
---[ RESPONSE ]--------------------------------------
HTTP/0.0 0 status code 0

-----------------------------------------------------

@pwaller
Copy link
Contributor Author

pwaller commented Jan 14, 2016

@pwaller
Copy link
Contributor Author

pwaller commented Jan 14, 2016

I got it working in the end with this:

useClient := *http.DefaultClient
useClient.Transport = &http.Transport{Dial: viaBastionDialer}
s.Config.HTTPClient = &useClient
s.Config.Credentials = defaults.CredChain(s.Config, defaults.Handlers())

Are there likely to be any other HTTPClients that I've missed?

@jasdel
Copy link
Contributor

jasdel commented Jan 14, 2016

Hi @pwaller thanks for contacting us. The condition you found is limited to the EC2Metadata client used by EC2RoleProvider. The SDK did not expect the default http client to be the one modified. But clients to create new http clients based on their need.

The deadlines are only intended to be used if the HTTP client is not the default client. This was done with the intention that the default client wouldn't be modified. If there is any other type of check if the that would be helpful for the SDK to use I'd definitely be up to consider it.

@jasdel
Copy link
Contributor

jasdel commented Jan 14, 2016

Digging into this a bit deeper the SDK could be updated to determine if the passed in client has been modified from the zero state http.Client. Which is the original form of http.DefaultClient. If the HTTP client was modified in anyway from the default http client state the SDK should not make any additions and use the passed in client as is.

reflect.DeepEqual(*cfg.HTTPClient, http.Client{})

jasdel added a commit that referenced this issue Jan 14, 2016
The EC2Metadata Client was incorrectly overriding the http.DefaultClient
when users had modified the default client's parameters.  The metadata
client will now only set its alternate dial timeout if not http client
was provided, or if the provided client was never modified from the
original http.Client{} form.

Also updates the logic so DefaultTransport is reused instead of
hardcoded.

Fix #504
@xibz xibz closed this as completed in 7d7147d Jan 14, 2016
jasdel added a commit that referenced this issue Jan 14, 2016
@pwaller
Copy link
Contributor Author

pwaller commented Jan 15, 2016

Thanks for the fix there. I guess there are still two issues remaining for anyone hitting this problem to beware:

  1. Just replacing s.HTTPClient after session.New() does not affect the EC2RoleProvider.
  2. If you replace http.DefaultTransport.(*Transport).Dial, it will not be respected by EC2RoleProvider.

Workarounds to these issues:

  1. After setting s.HTTPClient, remake s.Credentials to its default with something like s.Config.Credentials = defaults.CredChain(s.Config, defaults.Handlers()).
  2. If you want to modify http.DefaultTransport and have it affect EC2RoleProvider, you must instead modify the transport the s.HTTPClient.

These are hard to fix in the Go AWS SDK because:

  1. Would require refactoring EC2RoleProvider so that it instead held a pointer to the config, rather than taking a copy of the configuration at the moment of construction.
  2. There is no way that I'm aware of to test to see if http.DefaultTransport's dialer has been modified from the default, since it's a) not possible to compare function pointers b) not possible to obtain a reference to the dialer before any libraries might modify it anyway (even in init).

To conclude: Could we fix (1) in aws-sdk-go, or would that be too much breakage? (2) I guess just requires that you don't expect modifications to DefaultTransport to have effect, unless a smart gopher can figure out a way to achieve that, or such a mechanism is put in the standard library.

@jasdel
Copy link
Contributor

jasdel commented Jan 15, 2016

@pwaller you are correct. Since the EC2RoleProvider is created when a session.New() call is made any changes to the HTTPClient or http.DefaultClient would not be reflected in the EC2RoleProvider.

#509 was opened which touch on this issue, and the data race that was introduced because of this change.

@pwaller
Copy link
Contributor Author

pwaller commented Jan 17, 2016

Aha. So with #511 now the transport is not touched. So (2) no longer applies from the above post. That's a great start.

I see two remaining problems:

  1. The first problem from the above post still applies (you have to re-initialise .Credentials).
  2. If http.DefaultClient is modified, those choices won't be respected, because a new http.Client is constructed from blank rather than copying the http.DefaultClient.

skotambkar pushed a commit to skotambkar/aws-sdk-go that referenced this issue May 20, 2021
Fixes a broken unit test missed when implementing aws#487.
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

No branches or pull requests

2 participants