Skip to content

Fix TLS from environment variables in client #36270

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

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

dperny
Copy link
Contributor

@dperny dperny commented Feb 9, 2018

- What I did

A recent change accidently caused any TLS configuration in FromEnv to be ignored. I fixed this so TLS works again.

- How I did it

This change alters WithHost to create a new http client only if one doesn't already exist, and otherwise applies the logic to the transport on the existing client. This preserves the TLS configuration that might already be on the client.

- How to verify it

Before change: use NewEnvClient to connect to a TLS-enabled daemon, note the TLS errors.
After change: note the lack of TLS errors.

- Description for the changelog

N/A, I don't think the bug was ever shipped

/cc @vdemeester for review, who git blame says committed the original changes to the client.

@thaJeztah
Copy link
Member

Introduced in #36140

A recent change accidently caused any TLS configuration in FromEnv to be
ignored. This change alters WithHost to create a new http client only if
one doesn't already exist, and otherwise applies the logic to the
transport on the existing client. This preserves the TLS configuration
that might already be on the client.

Signed-off-by: Drew Erny <drew.erny@docker.com>
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I guess our test coverage isn't too extensive here.

LGTM

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

Successfully merging this pull request may close these issues.

5 participants