feat: filter containers seen by docker-gen

[buchdag]

The PR add the -container-filter option to filter the containers that docker-gen will see.

The option work pretty much like -notify-filter and can be used multiple times to combine filters with AND.

This:

docker-gen \
    -container-filter status=running \
    -container-filter label=com.github.nginx-proxy.nginx \
    ./some/template.tmpl ./result.ext

will result in docker-gen only seeing running container that possess the label com.github.nginx-proxy.nginx.

Closes #276

Unfortunately Docker filter options for containers does not provide negative filtering, so #117 and #252 won't be entirely fixed by this.

Thanks @tarasov65536 for the inspiration.

[Copilot]

Pull Request Overview

This PR adds a new -container-filter option to docker-gen, allowing users to filter containers in a more flexible and composable way. Key changes include the removal of legacy filtering logic from the template package, updates to generator and configuration modules to support the new filter option, and corresponding updates in command-line options and documentation.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/template/template.go	Removed legacy filterRunning logic and updated logging to use unfiltered containers, assuming filtering now occurs upstream.
internal/generator/generator.go	Updated getContainers signature and internal filtering logic to use config.ContainerFilter; removed the All flag.
internal/context/context.go & context_test.go	Removed the PublishedAddresses method and its test, as published/exposed filtering is now handled via container filters.
internal/config/config.go	Replaced OnlyExposed, OnlyPublished, and IncludeStopped flags with ContainerFilter.
cmd/docker-gen/main.go	Added container-filter flag and updated default filter logic, along with documentation changes.
README.md	Updated command option descriptions to reflect the new filtering option.

Comments suppressed due to low confidence (3)

internal/context/context.go:107

• The PublishedAddresses method and its associated tests have been removed. Verify that no other part of the codebase relies on this functionality and that its removal is fully covered by the new container filtering approach.

func (r *RuntimeContainer) PublishedAddresses() []Address {

internal/template/template.go:153

• The removal of the filterRunning function means that filtering is now assumed to happen upstream. Ensure that the logging statement (which now logs the count of 'containers') still accurately reflects the intended filtered list; otherwise, update the log to avoid potential confusion.

contents := executeTemplate(config.Template, containers)

internal/generator/generator.go:400

• The removal of the All flag and the decision to always pass 'true' for All in the Docker client's ListContainersOptions should be carefully validated. Confirm that this new approach interacts correctly with the provided container filters under all expected scenarios.

All:     true,

[eorea]

The reordering and formatting done in README.md and main.go files introduced a little bit of noise in the review, but I understand it has to be done.

Other than the fact that the generateAtInterval and generateFromEvents functions in generate.go declare and uses cfg as the config's variable name instead of config like all others, seems a bit off, concerning logic all LGTM.

[p12tic]

LGTM to me as well.

[buchdag]

Thanks @eorea and @p12tic

outdated proposition

I think I might change the `getContainers` method signature to

func (g *generator) getContainers(filters map[string][]string) ([]*context.RuntimeContainer, error) {

because we're actually just interested by the container filters here, not the whole config.

edit : nah let's stay consistent with the existing code.

Other than the fact that the generateAtInterval and generateFromEvents functions in generate.go declare and uses cfg as the config's variable name instead of config like all others, seems a bit off

Yeah agreed, but I did not want to add even more noise to this PR.

[buchdag]

Other than the fact that the generateAtInterval and generateFromEvents functions in generate.go declare and uses cfg as the config's variable name instead of config like all others, seems a bit off

Well there's actually an explanation, both method also declare this goroutine go func(cfg config.Config).

We can't declare a config variable in those methods because that would shadow the imported config package used in the goroutine signature.

[eorea]

That makes sense, thanks for the clarification

[buchdag]

@p12tic @eorea while we're here and before I merge this, this is how I see it implemented on nginx-proxy and acme-companion : an additional environment variable on either container (let's say DOCKER_CONTAINER_FILTERS, name not final), that take a comma separated list of key=value pairs, which will then be split and fed as -container-filter flag to docker-gen, most probably by the entrypoint script. What do you think ?

[eorea]

@buchdag I like the general approach as it allows the use of all available filter, but instead of modifying nginx_proxy entrypoint script which in turns uses forego to launch docker_gen (in https://github.com/nginx-proxy/nginx-proxy/blob/main/app/Procfile#L1) I think could be better if docker_gen supports the use of DOCKER_CONTAINER_FILTERS env variable, so if the env variable exists docker_gen gets all the filters from it, maybe the argument could be used with higher precedence and could overwrite any filter from the env variable if both are provided, but besides that, this approach would mean also that there should be no need to modify nginx-proxy but to update the documentation to indicate the use of DOCKER_CONTAINER_FILTERS env variable.

I was already proposing to add label filtering in nginx_proxy template with nginx-proxy/nginx-proxy#2659, and @p12tic was proposing a similar approach to filter containers by using slightly different prefix'd VIRTUAL_HOST environment variable name with nginx-proxy/nginx-proxy#2660

[buchdag]

@eorea excellent suggestion 👌🏻

And yes, I saw both your PRs, hence the renewed work on this 👍🏻