dns-proxy-server icon indicating copy to clipboard operation
dns-proxy-server copied to clipboard

Add config/environment/cli options to configure docker socket URL

Open imartinezortiz opened this issue 5 years ago • 5 comments

The main objective of this PR is to add a couple configuration options to specify the URL to connect to the docker daemon and to specify the docker API version to use. The idea is to use tecnativa/docker-socket-proxy to restrict and control the access of DPS to the docker demon.

I also created some tests, updated the documentation and provided an example docker compose that shows how to use the new feature.

Besides the PR also does:

  • [X] Update DPS to use go 1.14
  • [X] Migrate DPS govendor modules to go native modules
  • [X] Migrate the deprecated docker cli / types dependencies to use the current ones.

If you prefer / think that these additional changes require a separate PR let me know.

imartinezortiz avatar May 04 '20 20:05 imartinezortiz

I just updated the PR, to enable the ability use a hostname / container_name to in MG_DOCKER_HOST that is resolved before launching dns-proxy-server instead of of using an IP

imartinezortiz avatar May 04 '20 22:05 imartinezortiz

@mageddo any update on this ?

imartinezortiz avatar Jul 06 '20 12:07 imartinezortiz

Well, these are a lot of changes to be done at one shot, not sure if they are dependent. I think we may try isolate the Add config/environment/cli options to configure docker socket URL feature, then we can merge it ASAP.

Do you wanna do it by yourself or prefer me to do the job @imartinezortiz ?

mageddo avatar Jul 09 '20 23:07 mageddo

Let me do a test and I'll get back to you. If I remember correctly the main reason of all these changes were that the docket docket proxy requires at least docker API v 1.27 and the dependency that DPS is currently using it is compatible up to v1.21 because it is deprecated.

On Fri, Jul 10, 2020, 01:12 Elvis Souza [email protected] wrote:

Well, these are a lot of changes to be done at one shot, not sure if they are dependent. I think we may try isolate the Add config/environment/cli options to configure docker socket URL feature, then we can merge it ASAP.

Do you wanna do it by yourself or prefer me to do the job @imartinezortiz https://github.com/imartinezortiz ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mageddo/dns-proxy-server/pull/195#issuecomment-656394473, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALF7P2NU5ZURGPKF2YRYHDR2ZFETANCNFSM4MZBSXOQ .

imartinezortiz avatar Jul 10 '20 21:07 imartinezortiz

I've done some tests just adding the configuration options and seems to work at least for my use case. I'm getting some panics from time to time and that is the reason I tried to update the API, but to be honest, I also got some panics after doing the update (golang and docker API dependency).

Reviewing both DPS and tecnativa/docker-socket-proxy, maybe the problem is related due to the fact docker-socket-proxy return a HTML response if the request it is forbidden and DPS panics.

To sum up:

  • I can change the PR just to include the changes to add the two command line options.

  • Seems that at least it is needed to change mustInspectContainer to not panic, but I do not know the impact that may have this change. Seems that there is a design decision in DPS about panicking and exit instead of handling issues in other ways.

    Besides, what about the other changes: go update, migration to google modules, etc. ? should I open a different PR(s).

# This is the panic I got today 
dns-proxy-server_1  | panic: status=inspect-error, id=: json: cannot unmarshal array into Go value of type types.ContainerJSON
dns-proxy-server_1  | 
dns-proxy-server_1  | goroutine 19 [running]:
dns-proxy-server_1  | github.com/mageddo/dns-proxy-server/events/docker.mustInspectContainer(0x8fff60, 0xc00034cf00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
dns-proxy-server_1  | 	/app/src/github.com/mageddo/dns-proxy-server/events/docker/docker.go:116 +0x24b
dns-proxy-server_1  | github.com/mageddo/dns-proxy-server/events/docker.HandleDockerEvents()
dns-proxy-server_1  | 	/app/src/github.com/mageddo/dns-proxy-server/events/docker/docker.go:90 +0xd40
dns-proxy-server_1  | created by main.main
dns-proxy-server_1  | 	/app/src/github.com/mageddo/dns-proxy-server/dns.go:126 +0xe6

# This is the panic I got during the last two months using all the changes included in this PR.
panic: status=inspect-error, id=7134e8ae0488f877d11d1b2ac48846db920d4a6612bb4e828075907bee961d7d: Error: No such container: 7134e8ae0488f877d11d1b2ac48846db920d4a6612bb4e828075907bee961d7d

goroutine 18 [running]:
github.com/mageddo/dns-proxy-server/events/docker.mustInspectContainer(0x918500, 0xc00010e090, 0xc00011c040, 0x40, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/app/src/github.com/mageddo/dns-proxy-server/events/docker/docker.go:113 +0x255
github.com/mageddo/dns-proxy-server/events/docker.HandleDockerEvents()
	/app/src/github.com/mageddo/dns-proxy-server/events/docker/docker.go:82 +0xce3
created by main.main
	/app/src/github.com/mageddo/dns-proxy-server/dns.go:137 +0x175

imartinezortiz avatar Jul 13 '20 10:07 imartinezortiz

Hey, I was stale for some time, below some updates:

  • DPS was refactored on 3.x.x so this PR isn't compatible anymore , see #267
  • This feature was implemented at #379

So I'm closing this one, thanks for the help, sorry we didn't solve it earlier.

mageddo avatar Mar 19 '23 17:03 mageddo