daemon: refactor network-list filtering code; unify list and prune filter terms#50843
Closed
corhere wants to merge 5 commits intomoby:masterfrom
Closed
daemon: refactor network-list filtering code; unify list and prune filter terms#50843corhere wants to merge 5 commits intomoby:masterfrom
corhere wants to merge 5 commits intomoby:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the network list filtering code by moving filter validation from the API layer to the daemon layer and unifying filter terms between list-networks and prune-networks APIs. It also fixes a bug with the hidden idOrName filter term that was matching all networks regardless of the provided value.
- Replaced the buggy
idOrNamefilter term with a working implementation usingidfilter withIDAlsoMatchesNameflag - Moved network filter validation from
api/types/networkpackage todaemon/networkpackage - Added support for
label!anduntilfilters to the list-networks API anddriver,name,scopefilters to prune-networks API
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| daemon/server/router/network/network_routes.go | Updates network routes to use new daemon-level filter validation and dnetwork.Filter type |
| daemon/server/router/network/backend.go | Changes backend interface signatures to use dnetwork.Filter instead of filters.Args |
| daemon/prune.go | Refactors pruning logic to use the new filter system and removes duplicate filter validation |
| daemon/network/filter_test.go | Updates tests to use mock objects and new filter interface |
| daemon/network/filter.go | Implements new Filter type with unified validation and matching logic |
| daemon/network.go | Refactors GetNetworks to filter before conversion and use new filter interface |
| daemon/libnetwork/network.go | Adds Driver() and ContainerAttachments() methods to support filter interface |
| daemon/libnetwork/agent.go | Adds ServiceAttachments() method to support filter interface |
| daemon/cluster/networks.go | Updates cluster network handling to use new filter system |
| daemon/cluster/convert/network.go | Adds FilterNetwork adapter for swarmapi networks |
| daemon/cluster.go | Updates interface to use dnetwork.Filter |
| api/types/network/network.go | Removes network filter validation (moved to daemon) |
| api/swagger.yaml | Updates API documentation to reflect new supported filters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
edc0241 to
71e7fdf
Compare
Internally a network is represented by either a libnetwork.Network or a swarmapi.Network. The daemon functions backing the API server map these values to the Engine API network.Inspect type on demand. Since they have to convert, the functions to get a list of networks have to loop over the slice of Networks and append them to a slice of network.Inspect values. The function used to filter the list of networks by a user-supplied predicate takes a []network.Inspect and returns a shorter slice. Therefore the daemon functions backing the API server have to loop through the list twice; once to convert, and again inside the FilterNetworks function to prune networks from the slice. Each time an item is deleted from a slice, all items at higher indices need to be copied to lower indices in the backing array. Replace FilterNetworks with a function that accepts a single network summary and returns a boolean. The daemon network-list functions can filter in the conversion loop, which cuts down on a nontrivial amount of copying. Split the validation of the filter predicate from filter evaluation to both make it more ergonomic to use inside loops, and to make invalid states (a filter with an ill-formed predicate) unrepresentable. This change also future-proofs the network filter for when we change network.Inspect to a distinct type that embeds a network.Summary. Callers that need to build a filtered list of network.Inspect values will be able to evaluate the filter on the embedded Summary then append the full Inspect value of matching networks to the output slice. Signed-off-by: Cory Snider <csnider@mirantis.com>
The network filter has an hidden 'idOrName' term. API clients cannot use this term as it will not pass ValidateFilters: it is not listed in the acceptedFilters set. And it's a good thing, too: it is completely broken and not fit for purpose. When a filter contains an idOrName term, the term values are ignored and instead the filter tests whether the 'id' or 'name' terms match the Name field of the network. Unless the filter contains both 'id' and 'name' terms, the match will evaluate to true! None of the daemon-internal users of 'idOrName' set either term, therefore it has the same effect as if the filter did not contain the term in the first place. Filtering networks by id-or-name is a quirky thing that the daemon needs to do to uphold its end of the Engine API contract, not something that would be of use to clients. Instead of fixing up the idOrName filter, add an exported field to the Filter struct to enable the internal-only behaviour of having the 'id' term match on either the network Name or ID. Signed-off-by: Cory Snider <csnider@mirantis.com>
Modify network.Filter to evaluate an interface-valued parameter which both libnetwork.Network and swarmapi.Network (with a small adapter) implement. Filter the objects before converting them to API network.Inspect struct values. Signed-off-by: Cory Snider <csnider@mirantis.com>
Filter-term validation does not belong in the API module. Clients should not be making any assumptions about which terms the daemon understands. Users should not need to upgrade their clients to use filter terms introduced in a newer daemon. Move the network filter validation from the api module into the daemon. Have network.NewFilter validate the filter terms, enforcing the invariant that any network.Filter is a well-formed filter for networks. Lift the conversion from filters.Args to network.Filter into the API endpoint handlers so that validation errors continue to be returned separately from errors that occur when listing networks. As a side benefit, this also affords the API handlers configuration of the filter options (IDAlsoMatchesName) without additional plumbing. Unify the filter terms available when listing networks and pruning networks. Support filtering on the absence of a label or network creation time when listing networks. And support all the network listing filter terms when pruning networks. Signed-off-by: Cory Snider <csnider@mirantis.com>
The semantics of Swarmkit filters doesn't seem to align with the semantics of Moby filters very well. Revert to listing all Swarm networks and filtering in the daemon, as we were (unintentionally) doing before the idOrName fix. Signed-off-by: Cory Snider <csnider@mirantis.com>
9d55e3a to
baaf9de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
idOrNameterm (which matched all networks irrespective of value, as if the term wasn't present) with an implementation that actually works as intended- How I did it
See individual commit messages.
- How to verify it
Existing tests.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)