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

Remove CMD host flag from Dockerfile #4235

Closed
wants to merge 2 commits into from

Conversation

agneevX
Copy link
Contributor

@agneevX agneevX commented Jan 31, 2022

This flag served no real purpose other than listening to all interfaces, despite selecting a specific interface during onboarding.

Also use long flags instead of shorthands

Closes #4231

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2022

Codecov Report

Merging #4235 (3863cde) into master (b04d1ed) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4235   +/-   ##
=======================================
  Coverage   43.37%   43.37%           
=======================================
  Files         104      104           
  Lines       11938    11938           
=======================================
  Hits         5178     5178           
  Misses       6214     6214           
  Partials      546      546           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04d1ed...3863cde. Read the comment docs.

@EugeneOne1 EugeneOne1 added this to the v0.108.0 milestone Jan 31, 2022
@ainar-g ainar-g self-assigned this Jan 31, 2022
@ainar-g ainar-g self-requested a review January 31, 2022 17:19
@ainar-g
Copy link
Contributor

ainar-g commented Jan 31, 2022

Thanks for the contribution! We'll probably merge it this week, but the final decision is yet to be made. In any way, this is going to be in v0.108.0, since this will definitely break some people's installations if we're not careful.

Copy link
Contributor

@ainar-g ainar-g left a comment

Choose a reason for hiding this comment

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

@‌ameshkov thinks that this change as-is could break too many installations. To prevent a breaking change like that, could you add the -h back, but put its value into an ENV instruction? I.e. ENV AGH_BIND_HOST=0.0.0.0. We'll then update the documentation to mention that people who want to bind to a specific address inside their containers should use --env AGH_BIND_HOST=192.168.1.1.

@agneevX
Copy link
Contributor Author

agneevX commented Feb 1, 2022

OK, but exec form does not allow for shell expansion.

Shell form can be used but does have caveats:

The shell form prevents any CMD or run command line arguments from being used, but has the disadvantage that your ENTRYPOINT will be started as a subcommand of /bin/sh -c, which does not pass signals. This means that the executable will not be the container’s PID 1 - and will not receive Unix signals - so your executable will not receive a SIGTERM from docker stop .

@ainar-g
Copy link
Contributor

ainar-g commented Feb 1, 2022

Sigh. Docker being Docker, heh. Something like this should probably work, albeit quite a hack:

ENV AGH_BIND_HOST='0.0.0.0'

ENTRYPOINT [ \
	"/bin/sh", \
	"-c", \
	"exec /opt/adguardhome/AdGuardHome --host \"$AGH_BIND_HOST\" \"$@\"", \
	"--" \
]

CMD [ \
	"--config", "/opt/adguardhome/conf/AdGuardHome.yaml", \
	"--no-check-update", \
	"--work-dir", "/opt/adguardhome/work" \
]

@agneevX
Copy link
Contributor Author

agneevX commented Feb 1, 2022

Ehh that could work but do you want to have your Dockerfile looking like that?

I'd say supporting environment variables is something that the binary itself should understand, without the need for flags in the middle.

This does not feel like the correct way to do this. Since this is env vars, it might not be fair to restrict it to a single item.

@ainar-g
Copy link
Contributor

ainar-g commented Feb 1, 2022

I think, we've had several issues about using the environment more actively. We'll need to think about it. But until then, this PR will remain open, if you don't mind.

@jonathanjsimon
Copy link

Necrobumping

With the -h 0.0.0.0 in the Dockerfile, AdGuard becomes unusable in my use case as I have two interfaces and port 80 is in use on one of them. Specifying the other interface's IP is the only way to use AdGuard and have individual client IPs listed in the interface.

@jonathanjsimon
Copy link

Also, since this setting will have previously configured a user's AdGuardHome.yaml file with 0.0.0.0, how does removing the flag break an installation? It's already configured to listen on all interfaces. For further protection, couldn't the application itself be altered to presume 0.0.0.0 if no other value is present from switches or config files?

@jonathanjsimon
Copy link

For anyone wanting to do this now, https://hub.docker.com/r/jsimon0/adguardhome

@Mizzick
Copy link
Contributor

Mizzick commented Jun 22, 2023

Hello,

With the upcoming command-line flags changes, we are going to deprecate the -h flag. So I think now, we are ready to merge this pull request.

@Mizzick Mizzick self-requested a review June 22, 2023 07:30
@Mizzick
Copy link
Contributor

Mizzick commented Jun 23, 2023

Hello again,

We have implemented the requested changes in e54fc9b.

Thank you for you help!

adguard pushed a commit that referenced this pull request Jun 23, 2023
Merge in DNS/adguard-home from cmdline-flag-docs to master

Updates #4235.

Squashed commit of the following:

commit 1d68255
Author: Dimitry Kolyshev <dkolyshev@adguard.com>
Date:   Fri Jun 23 16:04:15 2023 +0400

    all: docs
@ainar-g ainar-g closed this Jun 27, 2023
@ainar-g ainar-g removed this from the v0.108.0 milestone Jul 13, 2023
@ainar-g ainar-g removed their assignment Jul 13, 2023
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.

Cannot set bind_host in AdGuardHome.yaml (docker version)
7 participants