Skip to content

Fix proxy-configuration being ignored on docker create - #1573

Merged
silvin-lubecki merged 1 commit into
docker:masterfrom
thaJeztah:fix_proxy_on_create
Jan 29, 2019
Merged

Fix proxy-configuration being ignored on docker create#1573
silvin-lubecki merged 1 commit into
docker:masterfrom
thaJeztah:fix_proxy_on_create

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Dec 13, 2018•
edited
Loading

Copy link
Copy Markdown
Member

relates to: #93
fixesdocker/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 Silvanoc@users.noreply.github.com
Signed-off-by: Sebastiaan van Stijn github@gone.nl

- 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)

@thaJeztahthaJeztah mentioned this pull request Dec 13, 2018
Closed
3 tasks
@thaJeztah

thaJeztah commented Dec 13, 2018

Copy link
Copy Markdown
MemberAuthor

ping @dave-tucker PTAL 😅

@thaJeztahthaJeztah mentioned this pull request Dec 13, 2018
Open
@codecov-io

codecov-io commented Dec 13, 2018•
edited
Loading

Copy link
Copy Markdown

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 <Silvanoc@users.noreply.github.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

thaJeztah commented Jan 19, 2019

Copy link
Copy Markdown
MemberAuthor

ping @silvin-lubecki@vdemeester PTAL

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

@dave-tuckerdave-tucker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thaJeztah

thaJeztah commented Jan 29, 2019

Copy link
Copy Markdown
MemberAuthor

ping @silvin-lubecki@vdemeester PTAL 🙏

vdemeester
vdemeester approved these changes Jan 29, 2019
View reviewed changes

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🐯

@thaJeztahthaJeztah mentioned this pull request Jan 29, 2019
Merged
silvin-lubecki
silvin-lubecki reviewed Jan 29, 2019
View reviewed changes
Comment threadcli/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{}

@silvin-lubeckisilvin-lubeckiJan 29, 2019

Copy link
Copy Markdown
Contributor

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

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

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

thaJeztah commented Jan 29, 2019

Copy link
Copy Markdown
MemberAuthor

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

silvin-lubecki commented Jan 29, 2019

Copy link
Copy Markdown
Contributor

That makes sense, thank you @thaJeztah 👍

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

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

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:masterJan 29, 2019
@GordonTheTurtleGordonTheTurtle 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@silvin-lubeckisilvin-lubeckisilvin-lubecki approved these changes
+2 more reviewers
@vdemeestervdemeestervdemeester approved these changes
@dave-tuckerdave-tuckerdave-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