Skip to content

Fix proxy-configuration being ignored on docker create #1573

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.

Sign up for GitHub

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

Jump to bottom
Merged
silvin-lubecki merged 1 commit into from
Jan 29, 2019
Merged

Fix proxy-configuration being ignored on docker create #1573

silvin-lubecki merged 1 commit into from
Jan 29, 2019

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Dec 13, 2018 •
edited
Loading

relates to: #93
fixes docker/for-linux#369

Proxies configured in config.json were only taking effect when using docker run, but were being ignored when using docker create.

Before this change:

echo '{"proxies":{"default":{"httpProxy":"httpProxy","httpsProxy":"httpsProxy","noProxy":"noProxy","ftpProxy":"ftpProxy"}}}' > config.json
docker inspect --format '{{.Config.Env}}' $(docker --config=./ create busybox)
[PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]

With this change applied:

echo '{"proxies":{"default":{"httpProxy":"httpProxy","httpsProxy":"httpsProxy","noProxy":"noProxy","ftpProxy":"ftpProxy"}}}' > config.json
docker inspect --format '{{.Config.Env}}' $(docker --config=./ create busybox)
[NO_PROXY=noProxy no_proxy=noProxy FTP_PROXY=ftpProxy ftp_proxy=ftpProxy HTTP_PROXY=httpProxy http_proxy=httpProxy HTTPS_PROXY=httpsProxy https_proxy=httpsProxy PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]

Reported-by: Silvano Cirujano Cuesta [email protected]
Signed-off-by: Sebastiaan van Stijn [email protected]

- Description for the changelog

- Fix proxy-configuration being ignored on docker create [docker/cli#1573](https://github.com/docker/cli/pull/1573)

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah mentioned this pull request Dec 13, 2018
Closed
3 tasks
@thaJeztah
Copy link
Member Author

thaJeztah commented Dec 13, 2018

ping @dave-tucker PTAL 😅

@thaJeztah thaJeztah requested a review from vdemeester December 13, 2018 21:41
@thaJeztah thaJeztah mentioned this pull request Dec 13, 2018
Open
@codecov-io
Copy link

codecov-io commented Dec 13, 2018 •
edited
Loading

Codecov Report

Merging #1573 into master will increase coverage by <.01%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #1573      +/-   ##
==========================================
+ Coverage   55.16%   55.16%   +<.01%     
==========================================
  Files         301      301              
  Lines       20384    20392       +8     
==========================================
+ Hits        11244    11249       +5     
- Misses       8335     8336       +1     
- Partials      805      807       +2

@thaJeztah
Fix: proxy-configuration being ignored on docker create
91bc4dd 
Proxies configured in config.json were only taking effect
when using `docker run`, but were being ignored when
using `docker create`.

Before this change:

    echo '{"proxies":{"default":{"httpProxy":"httpProxy","httpsProxy":"httpsProxy","noProxy":"noProxy","ftpProxy":"ftpProxy"}}}' > config.json
    docker inspect --format '{{.Config.Env}}' $(docker --config=./ create busybox)
    [PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]

With this change applied:

    echo '{"proxies":{"default":{"httpProxy":"httpProxy","httpsProxy":"httpsProxy","noProxy":"noProxy","ftpProxy":"ftpProxy"}}}' > config.json
    docker inspect --format '{{.Config.Env}}' $(docker --config=./ create busybox)
    [NO_PROXY=noProxy no_proxy=noProxy FTP_PROXY=ftpProxy ftp_proxy=ftpProxy HTTP_PROXY=httpProxy http_proxy=httpProxy HTTPS_PROXY=httpsProxy https_proxy=httpsProxy PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]

Reported-by: Silvano Cirujano Cuesta <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the fix_proxy_on_create branch from cc6a92c to 91bc4dd Compare January 19, 2019 11:33
@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 19, 2019

ping @silvin-lubecki @vdemeester PTAL

dave-tucker
dave-tucker approved these changes Jan 20, 2019
View reviewed changes
Copy link
Contributor

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 29, 2019

ping @silvin-lubecki @vdemeester PTAL 🙏

vdemeester
vdemeester approved these changes Jan 29, 2019
View reviewed changes
Copy link
Collaborator

@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 🐯

@thaJeztah thaJeztah mentioned this pull request Jan 29, 2019
Merged
silvin-lubecki
silvin-lubecki reviewed Jan 29, 2019
View reviewed changes
cli/command/container/create.go
func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *createOptions, copts *containerOptions) error {
func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error {
proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), copts.env.GetAll())
newEnv := []string{}
Copy link
Contributor

@silvin-lubecki silvin-lubecki Jan 29, 2019

Choose a reason for hiding this comment

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

nit: var newEnv []string

silvin-lubecki
silvin-lubecki reviewed Jan 29, 2019
View reviewed changes
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

As this proxy configuration is now handled by runCreate, shouldn't we remove the exact same code from runRun ? Or if not, at least factorize the common code between the two commands?

@thaJeztah
Copy link
Member Author

thaJeztah commented Jan 29, 2019

Or if not, at least factorize the common code between the two commands?

Ideally, we would be able to share more code; the reason I did not do this (and instead went for some small amount of duplication), is that I wanted to keep the code changes minimal (in case we wanted to backport), and didn't want to put the cli-configuration handling too deep into the stack;

  • runRun (cli specific code) converts the CLI options into a more generic containeroptions
    • calls runContainer (apply "run" specific options)
    • calls createContainer with the given options and container-configuration
  • runCreate (cli specific code) converts the CLI options into a more generic containeroptions
    • calls createContainer with the given options and container-configuration

In the above, possibly runRun and runContainer could be (partially) combined, but perhaps best left to a bigger refactor (we should more clearly define the "hand-off" point between CLI and "API/client" code).

@silvin-lubecki
Copy link
Contributor

silvin-lubecki commented Jan 29, 2019

That makes sense, thank you @thaJeztah 👍

silvin-lubecki
silvin-lubecki approved these changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@silvin-lubecki silvin-lubecki merged commit b258f45 into docker:master Jan 29, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Jan 29, 2019
@thaJeztah thaJeztah deleted the fix_proxy_on_create branch April 23, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdemeester vdemeester vdemeester approved these changes

@silvin-lubecki silvin-lubecki silvin-lubecki approved these changes

+1 more reviewer

@dave-tucker dave-tucker dave-tucker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

19.03.0

Development

Successfully merging this pull request may close these issues.

User proxy configuration doesn't work with docker create + start

6 participants

@thaJeztah @codecov-io @silvin-lubecki @vdemeester @dave-tucker @GordonTheTurtle