-
Notifications
You must be signed in to change notification settings - Fork 164
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
Per container ingress network override #480
Conversation
Hey, thanks @kiwiz for working on that! The place you started extending is called only once when caddy is initialized and it will not update if new containers are created with new networks. It will also share the ingress network with all containers, instead of using it only for the container that declares the label. My recommendation is to do it in https://github.com/lucaslorentz/caddy-docker-proxy/blob/master/generator/containers.go#L17 and https://github.com/lucaslorentz/caddy-docker-proxy/blob/master/generator/services.go#L52 For containers.goI did a docker inspect in a container and found the network name is in the key of the network map, as per the image below. I think this will hold true for docker, not sure if podman will be like that, but it should be fine for now. So, I think changing this block:
Into something like:
Might be enough, but I haven't tested. For services.goI didn't sketch any code for this one, but I did inspect to check where the network name is: We could write similar code for this as well. If you're not using docker swarm, feel free to implement it only for containers, I don't mind merging it without support for services for now. |
5403486
to
1ce5de7
Compare
Thanks for the pointers! I updated the code and tested locally. Can confirm that the non-swarm side of things works. It's a bit different from your proposed solution, so feedback welcome. I also implement similar logic on the Swarm side, but it's totally untested atm. |
ingressNetworkFromLabel, overrideNetwork := container.Labels[IngressNetworkLabel] | ||
|
||
for networkName, network := range container.NetworkSettings.Networks { | ||
include := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a simpler way to express this logic, but I believe the caddy_ingress_network
label needs to be a complete override of the allowed networks. Otherwise, we could still return IPs from another network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense, can you please let param ingress
overrule everything else? Basically, there is a place that calls this function to get all IPs, not only IPs for ingress.
Maybe doing an if !ingress
before if overrideNetwork
Feel free to rename the ingress param as well to something more clear, maybe forIngress
or onlyIngressIps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will go with onlyIngressIps
I've added Docker tests for swarm and standalone usage. Still working on the Go tests - unsure why they're failing atm. |
OK, tests fixed. However, I don't think |
@kiwiz Looking very good! |
generator/services_test.go
Outdated
const expectedLogs = otherIngressNetworksMapLog + swarmIsAvailableLog | ||
|
||
testGeneration(t, dockerClient, func(options *config.Options) { | ||
options.IngressNetworks = []string{"other-network-name"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this test is not meaningful as it is right now.
But swarm service proxying supports 2 modes, proxying to virutal service using its dns name, or proxying to all service tasks (containers) IPs.
Add another option after this line like:
options.ProxyServiceTasks = true
This will swap the behavior to IPs, and then adjust the expectedCaddyfile
to match the IP addresses configured in dockerClient.ServicesData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. If I understand correctly, I should use TestServiceTasks_ManualIngressNetwork
as a reference. Is overriding the ingress network to a virtual service something that should be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need
Thanks @kiwiz! |
🎉 Thanks! |
https://github.com/lucaslorentz/caddy-docker-proxy/releases/tag/v2.8.4 |
This PR adds support for overriding the ingress network for a container via a
caddy_ingress_network
label. Relates to #157Pushing this up for any early feedback - the code compiles, but I still need to test & add docs and tests are ready.Should be ready to go.