add the ability to set aliases per network - #27
Conversation
obeone
left a comment
There was a problem hiding this comment.
Hi @egolus, thanks for this PR — the use case is clearly valid and the implementation is clean overall. A few things to address before merging:
Bug: aliases applied to all networks
The aliases are currently applied to every network the container is connected to. They should ideally be scoped to the relevant network(s) only, otherwise Traefik may receive the same aliases on unrelated networks, which could cause conflicts.
Code consistency
The rest of the codebase uses container.labels (the idiomatic Docker SDK API). Please replace container.attrs["Config"]["Labels"] with it:
alias_label = container.labels.get("traefik.aliases", "")
aliases = [a.strip() for a in alias_label.split(",") if a.strip()] if alias_label else []Tests
The project has a tests/ directory — please add unit tests covering at least: no alias, single alias, multiple comma-separated aliases, and aliases with extra whitespace.
Documentation
Please document the new traefik.aliases label in the README (alongside the existing label documentation).
Optionally, if you want to go further: making the label name configurable via config.traefik.aliasLabel would follow the existing pattern — but that's a nice-to-have, not a blocker.
|
Sorry for waiting so long. Can you take a look if the fixes are ok? |
obeone
left a comment
There was a problem hiding this comment.
Hey, thanks for this! The per network alias support is genuinely handy and the tests are nice and readable. I checked out the branch, ran it, and the whole suite is green.
I did run into one small edge case that I think is worth fixing before merge.
The aliases only get applied when the network is explicitly listed in the allowed networks label. But if a container does not set that label at all, allowed_networks ends up as [''], so the outer guard still connects Traefik (which is what we want), yet the inner net in allowed_networks check is False and the aliases get dropped silently. So someone who just sets traefik.aliases without also listing allowed networks gets no alias and no error, which is a bit surprising.
Since that block already lives inside the if allowed_networks == [''] or net in allowed_networks: guard, the extra check is redundant and we can drop it. I left a one line suggestion on the relevant line.
I would also add a small regression test for the no label case so it does not creep back in later:
def test_alias_set_without_network_label(self, mock_docker_client, mock_config, mock_logger):
"""
Regression: when traefik.aliases is set but no allowed-networks label is
provided (all networks allowed by default), the aliases must still be
applied to the network connect call.
"""
container = MagicMock()
container.name = "web-app"
container.attrs = {
"NetworkSettings": {"Networks": {"app_net": {}}}
}
container.labels = {
"traefik.aliases": "app",
}
traefik = MagicMock()
traefik.attrs = {"NetworkSettings": {"Networks": {"bridge": {}}}}
mock_docker_client.containers.get.return_value = traefik
network = MagicMock()
network.attrs = {"Name": "app_net", "Labels": {}}
mock_docker_client.networks.get.return_value = network
main.connect_traefik_to_network(container)
network.connect.assert_called_once_with(traefik, aliases=["app"])With both changes the full suite stays green (62 passing on my side). Thanks again for the contribution!
The alias block lived inside the allowed-networks guard but added a redundant "net in allowed_networks" check. In the default case where no allowed-networks label is provided (allowed_networks == ['']), that check was False, so aliases were silently dropped even though Traefik was still connected. Drop the redundant check so aliases apply whenever they are set. Add a regression test covering the no-label case.
Align the workflow with the base branch so the Claude review can access secrets and an OIDC token on pull requests opened from forks, where the plain pull_request trigger runs without them.
|
@claude please review the two latest commits on this PR (the alias fix and its regression test). Flag anything blocking before I merge. |
|
Claude finished @obeone's task in 1m 21s —— View job Review: alias fix + regression test
No blockers. These two commits are clean and correct. Here's the breakdown:
|
|
Hey @egolus, thanks a lot for this. The per-network alias support is a genuinely nice addition, and your tests were a pleasure to read. Quick apology for the noise on this PR. Most of the red CI runs and the open/close dance you saw came from my side: I was wiring up the Claude code review GitHub app on the repo at the same time, and figuring out how it behaves on pull requests opened from forks (turns out a fork PR cannot reach the secrets it needs unless the workflow runs as pull_request_target). None of that was about your code. While I was in there, I pushed two small commits onto your branch: the alias fix from my review (apply the aliases even when no allowed-networks label is set, plus a regression test for that case), and a tweak to the review workflow so it stops failing on fork PRs. The suite stays green, so I went ahead and merged. Thanks again for the contribution, and sorry for making you sit through a CI light show in the meantime. |
|
no worries. Thanks for the last fixes and merging 👍 |
We have the problem that we want to isolate our backend services but they sometimes still have to talk to each other. (i.e. a webserver may need to send e-mails through a mail server)
Normally a backend container can't reach the reverse proxy by the public domain names. So with this patch we give the reverse proxy those needed public domain names as alias so one container can reach one in another stack through the reverse proxy.
an example compose file would look something like this:
traefik.aliasescan be a comma-separated string. If there are multiple containers in a stack,traefik.aliasesshould only appear once as it may be overwritten