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

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

Closed

Conversation

imartinezortiz
Copy link
Contributor

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:

  • Update DPS to use go 1.14
  • Migrate DPS govendor modules to go native modules
  • 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
Copy link
Contributor Author

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
Copy link
Contributor Author

@mageddo any update on this ?

@mageddo
Copy link
Owner

mageddo commented Jul 9, 2020

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 ?

@imartinezortiz
Copy link
Contributor Author

imartinezortiz commented Jul 10, 2020 via email

@imartinezortiz
Copy link
Contributor Author

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 added a commit to e-ucm/simva-infra that referenced this pull request Oct 1, 2020
@mageddo mageddo added this to the 3.14.2-snapshot milestone Mar 19, 2023
@mageddo
Copy link
Owner

mageddo commented Mar 19, 2023

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

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

@mageddo mageddo closed this Mar 19, 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.

2 participants