Route also running services (no service restart required)

[markfqs]

Just added a couple of lines so the daemon checks for already running services needing to be rerouted. This removes the need to restart services after docker-ingress-daemon start.

I tried hard to be less intrusive as possible on the upstream code for and ease merge. This could also be implemented differently (also tried thru a function which could also allow 'uninstalling' the rules on containers).

Let me know of any suggestion or change you consider

[struanb]

Hi @markfqs. Thank you for your pull request. Support for configuring routing within running containers without a service restart is a very interesting idea.

It being the holiday season, please allow me time to consider this more fully over the coming few days. But my first (and not fully-considered) thoughts, in case you'd like to address them, are:

1. What ensures that when DIND restarts, routing+firewall rules won't be added to containers that already have the rules added? Should a check be made before rules are added, or should each container's rules be torn down before being readded?
2. While your patch is certainly neat and simple, does docker ps need to be run in the subprocess (which I am concerned might reduce reliability in cases where docker events exits), or could its output be processed instead once on script startup? Could this be done in a modular way by breaking the while read ID SERVICE loop out into a function?

[markfqs]

Thanks for your quick response, take the time you need.

I also thought on the items you point, but my first attempt was mainly to be a less intrussive code change and then advance in small steps from there (as I've seen your actively working on another indexed-ids branch which adds a bit more complexity). Let me comment:

1- Right, nothing prevents the routing to be re-applied. (Although has no impact at all, container continue working with proper routing, just more garbage in its tables). Note also that current implementation, leaves these rules in the already running services even when uninstalling (again with no impact).

I will review the code to avoid re-applying rules as I see a problem removing them unconditionally when unistalling/restarting:

I'm thinking on what will happen if in a scenario where one node 'uninstalled' (A), while another LB is still running (B).

Node B will receive incoming connections and apply your rules so the traffic is sent to container running in A (with original srcIP). However the container in A has lost its rules to proper reply that original srcIP as DIND is removed.

So is good idea to code a uninstall step in containers, however I'm not totally sure in which scenarios to use it. Maybe adding a flag to force container rules removal? Ideas?

2- Indeed my first attempt was replacing the whole while loop by a function and call it separtedly: One for docker ps (one shot), and in a loop listening for events. However I opted for this approach to be less code-intrusive.

So give me some days and I will propose a new code where this will be done in a modular way.

Just let me know your thoughts on item 1 and if you prefer I submit a patch, Pull Request from my fork or a new branch on your repo.

[struanb]

Thanks @markfqs sounds like you've given this some considerable thought.

Should have asked this first, but for avoidance of doubt: please would you outline the use case(s) you see for having a DIND update existing running service containers? Is this just for when you want DIND to take effect on a currently-running service, and you don't want to break the service? (In this case, I do wonder how well it will work unless DIND is launched on all service and load balancer nodes simultaneously; but maybe that's fine for services receiving a low rate of requests).

Also, re 'uninstalling', for avoidance of doubt it was never the intention that running services relying on DIND would continue to work after 'uninstalling' - just that there was a way to undo the firewall rule changes the script makes in the ingress namespace. Are you seeing use case(s) for this, and if so what are they? (Again, I wonder if it's straightforward to support the hybrid state where some nodes' containers have DIND rules uninstalled, but other nodes' containers still have the rules installed).

Re (1), I agree, duplicates in the tables unlikely to do any harm. But if there's a good way to avoid that and keep things clean, that's preferable, as it's more correct and makes what you see when inspecting a container's namespace more predictable.

Re (2), I'm happier with more intrusive code where that is more correct/reliable, as I think what you're now proposing will be.

Last but not least, I would much prefer we branch off the indexeds-ids branch. That way, when your patch is tested, the indexeds-ids branch will also be better tested and they can be merged into main together.

Pull requests are fine for now.

[markfqs]

Yes, a possible use case is to make DIND take effect on a running service without needing to restart, my services have low rate of requests and I can perfectly afford this. I also think in case of failure of DIND in one node, be able to restart it without redeploying the services (as per docker swarm the services may be fine, but not being properly routed). Current implementation breaks already running services ingress.

Rethinking about uninstalling, I agree with you as:

A) having rules in the node ingress-sbox (the uninstallable part) disrupts traffic to containers not having the ip rules to manage the TOS mark (no DIND setup/running).

B) Leaving the rules in the containers allows them to both reply original srcIP traffic marked with TOS by LB nodes running DIND. and still properly reply to masqueraded traffic (SNAT) sent by LB nodes not running DIND (or uninstalled).

So summarizing (IMHO): is better to uninstall only the ingress-sbox node rules and leave the container rules. I think this is more closer/transparent to support some hybrid (DIND and not DIND) scenarios. Although I updated my code to check for the rules on already running containers to avoid duplicating them. Although I consider myself an experienced git user, I'm not much used to GitHub forks/pull requests (for example I don't know how to change this current PR to point to another branch on my fork). So I will reboot my changes on my fork from your indexeds-ids branch an open a new pull request from there.

I made the changes on route-running branched from your indexeds-ids, so please review my proposal on https://github.com/markfqs/docker-ingress-routing-daemon/tree/route-running, if you agree I will merge it on my main to update the pull request on your repository into indexeds-ids.

My code changes:

• Separated ps loop from event loop
• Conatiner routing moved to a function (route_ingress)
• Added a dummy-comment rule to identify and skip containers already routed
• Documentation (README) not updated to reflect this

[struanb]

Thank you @markfqs I hope to consider this soon.

[struanb]

Hi @markfqs thanks for your patience while I got around to reviewing this. It's really nice to modularise the route_ingress process and I see no harm in providing the option to have rules installed on preexisting containers (an option for low-traffic services where DIND can be installed on all nodes close enough together in time; or where service containers are all running on the same node as is used for load balancing).

Regarding the use case(s) for --preexisting, while I agree it could be useful for some users who are applying DIND to running services, in practice (running DIND now for > 1 year) we've found DIND to be extremely stable; and as long as it is being supervised by systemd or s6 such that it is immediately restarted in the unlikely event of failure, it is extremely unlikely that any container launched for a running service will not be correctly configured by the daemon; and as such it should not ever be necessary to redeploy a service (or force service update) to handle the DIND failure.

I've modified your patch slightly to:

• move the check for the preexisting mangle table rules inside route_ingress
• move the job control, lastpipe, and trap above the check for preexisting containers
• add a new option --preexisting to enable the preexisting container check and update usage()
• inside route_ingress, use local variables throughout
• pass and assign variables using double quotes (good practice, in case any value contains whitespace)
• bump version to v4.1.0

I'll push my version of your patch to the indexed-ids branch. Please could you review it? If you think it looks good and can test it behaves, I'll update the README, merge into main and issue a fresh release.

P.S. I think we agree that the uninstall case is more complicated and we will not try and support this for now, although I think it could be feasible to make the install/uninstall process completely seamless if (a) connection tracking were used on the load balancers to apply the TOS byte assignment and disapply the SNAT rule only to newly received connections; and (b) DIND were installed on all service nodes before any load balancer nodes; and (c) in the uninstall case, DIND were uninstalled on all load balancer nodes before any service nodes. Apart from the extra complication this would entail, use of additional connection tracking could incur a performance hit (although it would only be needed temporarily, until all preexisting connections have expired, and then it could be removed).

[markfqs]

Regarding the use case(s) for --preexisting, while I agree it could be useful for some users who are applying DIND to running services, in practice (running DIND now for > 1 year) we've found DIND to be extremely stable; and as long as it is being supervised by systemd or s6 such that it is immediately restarted in the unlikely event of failure, it is extremely unlikely that any container launched for a running service will not be correctly configured by the daemon; and as such it should not ever be necessary to redeploy a service (or force service update) to handle the DIND failure.

I agree, knowing you are running this in production for long term adds a lot of confidence on the solution. By the way I will suggest to document (or even upload to github) systemd unit examples

I've modified your patch slightly to:

* move the check for the preexisting mangle table rules inside `route_ingress`

OK for me, doesn't harm. I just put it out the function as any new service will not have the rule at all so just avoiding the check on each run.

* move the job control, lastpipe, and trap above the check for preexisting containers

OK for me, I think you are more experienced in shell processes than me. ;)

* add a new option `--preexisting` to enable the preexisting container check and update `usage()`

OK for me, I also thought on adding it as an optional feature, just left for further enhancement once the core code is OK.

* inside `route_ingress`, use `local` variables throughout

* pass and assign variables using double quotes (good practice, in case any value contains whitespace)

Totally agree, stick to shell best practices.

I'll push my version of your patch to the indexed-ids branch. Please could you review it? If you think it looks good and can test it behaves, I'll update the README, merge into main and issue a fresh release.

Is totally ok from my point of view, just (as a suggestion) you could add/document a systemd service unit example.

Thanks much for your time reviewing and considering my changes.

[markfqs]

Sorry,

Any news on this? Will you merge this to the main branch?

[struanb]

Apologies, so much to do. Yes it's as good as merged, your work is not wasted, and we'll get that done.

[struanb]

Resolved by https://github.com/newsnowlabs/docker-ingress-routing-daemon/commit/2596ded13c2aa5463d052f71dbe28de9e43bcd66.

Thanks @markfqs I've flagged you as author of this commit, hope that's OK.

[struanb]

Released in https://github.com/newsnowlabs/docker-ingress-routing-daemon/releases/tag/v4.1.0