From f8bd170b2afe646833c35801b099e75bd651223e Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 28 Aug 2025 17:28:19 -0400 Subject: [PATCH] daemon: validate args in network.New*Filter 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. Split network.NewFilter into network.NewListFilter and network.NewPruneFilter constructors which validate the filter terms, enforcing the invariant that any network.Filter is a well-formed filter for networks. The network route handlers have been leveraging a hidden 'idOrName' filter term that is not listed in the set of accepted filters and therefore not accepted in API client requests. And it's a good thing that it was never part of the API: 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 either the 'id' or 'name' terms match the Name of the network. Unless the filter contains both 'id' and 'name' terms, the match will evaluate to true for all networks! None of the daemon-internal users of 'idOrName' set either of those terms, therefore it has the same effect as if the filter did not contain the 'idOrName' 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. Fixing up the idOrName filter would necessitate adding it to the list of accepted terms so the filter passes validaton, which would have the side effect of also making the filter available to API clients. Instead, add an exported field to the Filter struct so that daemon code can opt into the internal-only behaviour of having the 'id' term match on either the network Name or ID. Signed-off-by: Cory Snider --- api/types/network/network.go | 17 --- daemon/cluster/networks.go | 18 +-- daemon/network/filter.go | 55 +++++++-- daemon/network/filter_test.go | 109 +++++++++++++++--- daemon/prune.go | 14 +-- .../server/router/network/network_routes.go | 11 +- .../moby/moby/api/types/network/network.go | 17 --- 7 files changed, 156 insertions(+), 85 deletions(-) diff --git a/api/types/network/network.go b/api/types/network/network.go index 7e2f59f387..4d9272d8c5 100644 --- a/api/types/network/network.go +++ b/api/types/network/network.go @@ -2,8 +2,6 @@ package network import ( "time" - - "github.com/moby/moby/api/types/filters" ) const ( @@ -116,21 +114,6 @@ type ConfigReference struct { Network string } -var acceptedFilters = map[string]bool{ - "dangling": true, - "driver": true, - "id": true, - "label": true, - "name": true, - "scope": true, - "type": true, -} - -// ValidateFilters validates the list of filter args with the available filters. -func ValidateFilters(filter filters.Args) error { - return filter.Validate(acceptedFilters) -} - // PruneReport contains the response for Engine API: // POST "/networks/prune" type PruneReport struct { diff --git a/daemon/cluster/networks.go b/daemon/cluster/networks.go index 3f55705a6e..f8c9e01fbb 100644 --- a/daemon/cluster/networks.go +++ b/daemon/cluster/networks.go @@ -16,13 +16,17 @@ import ( // GetNetworks returns all current cluster managed networks. func (c *Cluster) GetNetworks(filter networkSettings.Filter) ([]network.Inspect, error) { - f := &swarmapi.ListNetworksRequest_Filters{ - Names: filter.Get("name"), - NamePrefixes: filter.Get("name"), - IDPrefixes: filter.Get("id"), - } - - list, err := c.listNetworks(context.TODO(), f) + // Swarmkit API's filters are too limited to express the Moby filter + // semantics with much fidelity. It only supports filtering on one of: + // - Names (exact match) + // - NamePrefixes (prefix match) + // - IDPrefixes (prefix match) + // The first of the list that is set is used as the filter predicate. + // The other fields are ignored. However, the Engine API filter + // semantics are to match on any substring of the network name or ID. We + // therefore need to request all networks from Swarmkit and filter them + // ourselves. + list, err := c.listNetworks(context.TODO(), nil) if err != nil { return nil, err } diff --git a/daemon/network/filter.go b/daemon/network/filter.go index 8e381e320e..5e089836ce 100644 --- a/daemon/network/filter.go +++ b/daemon/network/filter.go @@ -9,11 +9,33 @@ import ( "github.com/pkg/errors" ) +var ( + acceptedFilters = map[string]bool{ + "dangling": true, + "driver": true, + "id": true, + "label": true, + "name": true, + "scope": true, + "type": true, + } + + pruneFilters = map[string]bool{ + "label": true, + "label!": true, + "until": true, + } +) + type Filter struct { args filters.Args filterByUse, danglingOnly bool until time.Time + + // IDAlsoMatchesName makes the "id" filter term also match against + // network names. + IDAlsoMatchesName bool } type FilterNetwork interface { @@ -27,11 +49,34 @@ type FilterNetwork interface { HasServiceAttachments() bool } -// NewFilter returns a network filter that filters by the provided args. +// NewFilter returns a network list filter that filters by the provided args. // // An [errdefs.InvalidParameter] error is returned if the filter args are not // well-formed. func NewFilter(args filters.Args) (Filter, error) { + if err := args.Validate(acceptedFilters); err != nil { + return Filter{}, err + } + return newFilter(args) +} + +// NewPruneFilter returns a network prune filter that filters by the provided args. +// +// The filter matches dangling networks which also match args. +func NewPruneFilter(args filters.Args) (Filter, error) { + if err := args.Validate(pruneFilters); err != nil { + return Filter{}, err + } + f, err := newFilter(args) + if err != nil { + return Filter{}, err + } + f.filterByUse = true + f.danglingOnly = true + return f, nil +} + +func newFilter(args filters.Args) (Filter, error) { var filterByUse, danglingOnly bool if values := args.Get("dangling"); len(values) > 0 { if len(values) > 1 { @@ -89,7 +134,8 @@ func (f Filter) Matches(nw FilterNetwork) bool { return false } if f.args.Contains("id") && - !f.args.Match("id", nw.ID()) { + !f.args.Match("id", nw.ID()) && + (!f.IDAlsoMatchesName || !f.args.Match("id", nw.Name())) { return false } if f.args.Contains("label") && @@ -104,11 +150,6 @@ func (f Filter) Matches(nw FilterNetwork) bool { !f.args.ExactMatch("scope", nw.Scope()) { return false } - if f.args.Contains("idOrName") && - !f.args.Match("name", nw.Name()) && - !f.args.Match("id", nw.Name()) { - return false - } if f.filterByUse && !matchesUse(f.danglingOnly, nw) { return false diff --git a/daemon/network/filter_test.go b/daemon/network/filter_test.go index 6cd6169abe..55a89b8348 100644 --- a/daemon/network/filter_test.go +++ b/daemon/network/filter_test.go @@ -129,8 +129,10 @@ func TestFilter(t *testing.T) { } testCases := []struct { - subtest string - filter filters.Args + subtest string + filter filters.Args + pruneFilter bool + idAlsoMatchesName bool err string results []string @@ -234,56 +236,127 @@ func TestFilter(t *testing.T) { subtest: "label=foo=baz", filter: filters.NewArgs(filters.Arg("label", "foo=baz")), }, + { + subtest: "label!=foo", + filter: filters.NewArgs(filters.Arg("label!", "foo")), + err: "invalid filter 'label!'", + }, + { + subtest: "until=1h", + filter: filters.NewArgs(filters.Arg("until", "1h")), + err: "invalid filter 'until'", + }, { subtest: "name=with", filter: filters.NewArgs(filters.Arg("name", "with")), results: []string{"networkwithcontainer", "networkwithservice"}, }, { - subtest: "id=with", + subtest: "id=with/IDAlsoMatchesName=False", filter: filters.NewArgs(filters.Arg("id", "with")), }, { - subtest: "id=jbexjvgu", + subtest: "id=with/IDAlsoMatchesName=True", + filter: filters.NewArgs(filters.Arg("id", "with")), + idAlsoMatchesName: true, + results: []string{"networkwithcontainer", "networkwithservice"}, + }, + { + subtest: "id=jbexjvgu/IDAlsoMatchesName=False", filter: filters.NewArgs(filters.Arg("id", "argjbex")), results: []string{"networkwithcontainer", "networkwithservice"}, }, { - subtest: "id=my", + subtest: "id=jbexjvgu/IDAlsoMatchesName=True", + filter: filters.NewArgs(filters.Arg("id", "argjbex")), + idAlsoMatchesName: true, + results: []string{"networkwithcontainer", "networkwithservice"}, + }, + { + subtest: "id=my/IDAlsoMatchesName=False", filter: filters.NewArgs(filters.Arg("id", "my")), results: []string{"idoverlap"}, }, { - subtest: "label!=foo", - filter: filters.NewArgs(filters.Arg("label!", "foo")), - results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "myoverlay", "mykvnet", "networkwithcontainer", "networkwithservice", "idoverlap"}, + subtest: "id=my/IDAlsoMatchesName=True", + filter: filters.NewArgs(filters.Arg("id", "my")), + idAlsoMatchesName: true, + results: []string{"myoverlay", "mydrivernet", "mykvnet", "idoverlap"}, }, { - subtest: "until=2025-01-01", - filter: filters.NewArgs(filters.Arg("until", "2025-01-01")), - results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + subtest: "Prune/empty", + filter: filters.NewArgs(), + pruneFilter: true, + // Prune filters have an implicit dangling=true + results: []string{"myoverlay", "mydrivernet", "mykvnet", "idoverlap"}, }, { - subtest: "until=2024-12-01T01:00:00", - filter: filters.NewArgs(filters.Arg("until", "2024-12-01T01:00:00")), - results: []string{"myoverlay"}, + subtest: "Prune/label=foo", + filter: filters.NewArgs(filters.Arg("label", "foo")), + pruneFilter: true, + results: []string{"mydrivernet"}, }, { - subtest: "MultipleTerms=until", - filter: filters.NewArgs(filters.Arg("until", "2024-12-01T01:00:00"), filters.Arg("until", "2h")), - err: "more than one until filter specified", + subtest: "Prune/label=foo=bar", + filter: filters.NewArgs(filters.Arg("label", "foo=bar")), + pruneFilter: true, + results: []string{"mydrivernet"}, + }, + { + subtest: "Prune/label=foo=baz", + filter: filters.NewArgs(filters.Arg("label", "foo=baz")), + pruneFilter: true, + }, + { + subtest: "Prune/label!=foo", + filter: filters.NewArgs(filters.Arg("label!", "foo")), + pruneFilter: true, + results: []string{"myoverlay", "mykvnet", "idoverlap"}, + }, + { + subtest: "Prune/until=2025-01-01", + filter: filters.NewArgs(filters.Arg("until", "2025-01-01")), + pruneFilter: true, + results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + }, + { + subtest: "Prune/until=2024-12-01T01:00:00", + filter: filters.NewArgs(filters.Arg("until", "2024-12-01T01:00:00")), + pruneFilter: true, + results: []string{"myoverlay"}, + }, + { + subtest: "Prune/MultipleTerms=until", + filter: filters.NewArgs(filters.Arg("until", "2024-12-01T01:00:00"), filters.Arg("until", "2h")), + pruneFilter: true, + err: "more than one until filter specified", + }, + { + subtest: "Prune/id=invalid", + filter: filters.NewArgs(filters.Arg("id", "invalid")), + pruneFilter: true, + err: "invalid filter 'id'", }, } for _, testCase := range testCases { t.Run(testCase.subtest, func(t *testing.T) { - flt, err := NewFilter(testCase.filter) + var ( + flt Filter + err error + ) + if testCase.pruneFilter { + flt, err = NewPruneFilter(testCase.filter) + } else { + flt, err = NewFilter(testCase.filter) + } if testCase.err != "" { assert.ErrorContains(t, err, testCase.err) return } assert.NilError(t, err) + flt.IDAlsoMatchesName = testCase.idAlsoMatchesName got := map[string]bool{} for _, nw := range networks { if flt.Matches(nw) { diff --git a/daemon/prune.go b/daemon/prune.go index 141fffc336..619f0a14ff 100644 --- a/daemon/prune.go +++ b/daemon/prune.go @@ -29,12 +29,6 @@ var ( "label!": true, "until": true, } - - networksAcceptedFilters = map[string]bool{ - "label": true, - "label!": true, - "until": true, - } ) // ContainersPrune removes unused containers @@ -181,13 +175,7 @@ func (daemon *Daemon) NetworksPrune(ctx context.Context, filterArgs filters.Args } defer daemon.pruneRunning.Store(false) - // make sure that only accepted filters have been received - err := filterArgs.Validate(networksAcceptedFilters) - if err != nil { - return nil, err - } - - pruneFilters, err := dnetwork.NewFilter(filterArgs) + pruneFilters, err := dnetwork.NewPruneFilter(filterArgs) if err != nil { return nil, err } diff --git a/daemon/server/router/network/network_routes.go b/daemon/server/router/network/network_routes.go index 6677f4546c..14e029df78 100644 --- a/daemon/server/router/network/network_routes.go +++ b/daemon/server/router/network/network_routes.go @@ -29,10 +29,6 @@ func (n *networkRouter) getNetworksList(ctx context.Context, w http.ResponseWrit return err } - if err := network.ValidateFilters(filterArgs); err != nil { - return err - } - filter, err := dnetwork.NewFilter(filterArgs) if err != nil { return err @@ -120,7 +116,7 @@ func (n *networkRouter) getNetwork(ctx context.Context, w http.ResponseWriter, r // TODO(@cpuguy83): All this logic for figuring out which network to return does not belong here // Instead there should be a backend function to just get one network. - filterArgs := filters.NewArgs(filters.Arg("idOrName", term)) + filterArgs := filters.NewArgs(filters.Arg("id", term)) if networkScope != "" { filterArgs.Add("scope", networkScope) } @@ -128,6 +124,7 @@ func (n *networkRouter) getNetwork(ctx context.Context, w http.ResponseWriter, r if err != nil { return err } + filter.IDAlsoMatchesName = true networks, _ := n.backend.GetNetworks(filter, backend.NetworkListConfig{Detailed: true, Verbose: verbose}) for _, nw := range networks { @@ -333,10 +330,12 @@ func (n *networkRouter) findUniqueNetwork(term string) (network.Inspect, error) listByFullName := map[string]network.Inspect{} listByPartialID := map[string]network.Inspect{} - filter, err := dnetwork.NewFilter(filters.NewArgs(filters.Arg("idOrName", term))) + filter, err := dnetwork.NewFilter(filters.NewArgs(filters.Arg("id", term))) if err != nil { return network.Inspect{}, err } + filter.IDAlsoMatchesName = true + networks, _ := n.backend.GetNetworks(filter, backend.NetworkListConfig{Detailed: true}) for _, nw := range networks { if nw.ID == term { diff --git a/vendor/github.com/moby/moby/api/types/network/network.go b/vendor/github.com/moby/moby/api/types/network/network.go index 7e2f59f387..4d9272d8c5 100644 --- a/vendor/github.com/moby/moby/api/types/network/network.go +++ b/vendor/github.com/moby/moby/api/types/network/network.go @@ -2,8 +2,6 @@ package network import ( "time" - - "github.com/moby/moby/api/types/filters" ) const ( @@ -116,21 +114,6 @@ type ConfigReference struct { Network string } -var acceptedFilters = map[string]bool{ - "dangling": true, - "driver": true, - "id": true, - "label": true, - "name": true, - "scope": true, - "type": true, -} - -// ValidateFilters validates the list of filter args with the available filters. -func ValidateFilters(filter filters.Args) error { - return filter.Validate(acceptedFilters) -} - // PruneReport contains the response for Engine API: // POST "/networks/prune" type PruneReport struct {