From f0d10ae73349ea982536db417002a45701c7d7f5 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 27 Aug 2025 18:17:08 -0400 Subject: [PATCH 1/3] d/network: filter networks individually 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 delete networks from the slice which do not match the filter predicate. 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 to close the hole. Replace FilterNetworks with a function that accepts a single interface-valued network and returns a boolean. Amend libnetwork.Network and write a thin adapter for swarmapi.Network so both implement the aforementioned interface. The daemon functions can thus filter networks before projecting the values into API structs, and can completely skip over non-matching networks, 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. Signed-off-by: Cory Snider --- daemon/cluster/convert/network.go | 39 ++++ daemon/cluster/networks.go | 70 ++++--- daemon/libnetwork/agent.go | 5 + daemon/libnetwork/network.go | 10 + daemon/network.go | 32 ++-- daemon/network/filter.go | 179 ++++++++---------- daemon/network/filter_test.go | 305 +++++++++++++++++++----------- 7 files changed, 376 insertions(+), 264 deletions(-) diff --git a/daemon/cluster/convert/network.go b/daemon/cluster/convert/network.go index 136bb0dd0e..e76cd3221b 100644 --- a/daemon/cluster/convert/network.go +++ b/daemon/cluster/convert/network.go @@ -240,3 +240,42 @@ func IsIngressNetwork(n *swarmapi.Network) bool { _, ok := n.Spec.Annotations.Labels["com.docker.swarm.internal"] return ok && n.Spec.Annotations.Name == "ingress" } + +// FilterNetwork adapts [swarmapi.Network] to the +// [github.com/moby/moby/v2/daemon/network.FilterNetwork] interface. +type FilterNetwork struct { + N *swarmapi.Network +} + +func (nw FilterNetwork) ID() string { + return nw.N.ID +} + +func (nw FilterNetwork) Name() string { + return nw.N.Spec.Annotations.Name +} + +func (nw FilterNetwork) Driver() string { + if nw.N.DriverState != nil { + return nw.N.DriverState.Name + } + return "" +} + +func (nw FilterNetwork) Labels() map[string]string { + return nw.N.Spec.Annotations.Labels +} + +func (nw FilterNetwork) Scope() string { + return scope.Swarm +} + +func (nw FilterNetwork) HasContainerAttachments() bool { + // Not tracked in swarmkit + return false +} + +func (nw FilterNetwork) HasServiceAttachments() bool { + // Not tracked in swarmkit + return false +} diff --git a/daemon/cluster/networks.go b/daemon/cluster/networks.go index 6fdaf067aa..182b1b9d4b 100644 --- a/daemon/cluster/networks.go +++ b/daemon/cluster/networks.go @@ -17,6 +17,11 @@ import ( // GetNetworks returns all current cluster managed networks. func (c *Cluster) GetNetworks(filter filters.Args) ([]network.Inspect, error) { + flt, err := networkSettings.NewFilter(filter) + if err != nil { + return nil, err + } + var f *swarmapi.ListNetworksRequest_Filters if filter.Len() > 0 { @@ -32,49 +37,34 @@ func (c *Cluster) GetNetworks(filter filters.Args) ([]network.Inspect, error) { } } - list, err := c.getNetworks(f) + list, err := c.listNetworks(context.TODO(), f) if err != nil { return nil, err } - filterPredefinedNetworks(&list) - - return networkSettings.FilterNetworks(list, filter) -} - -func filterPredefinedNetworks(networks *[]network.Inspect) { - if networks == nil { - return - } - var idxs []int - for i, nw := range *networks { - if v, ok := nw.Labels["com.docker.swarm.predefined"]; ok && v == "true" { - idxs = append(idxs, i) + var filtered []network.Inspect + for _, n := range list { + if n.Spec.Annotations.Labels["com.docker.swarm.predefined"] == "true" { + continue + } + if flt.Matches(convert.FilterNetwork{N: n}) { + filtered = append(filtered, convert.BasicNetworkFromGRPC(*n)) } } - for i, idx := range idxs { - idx -= i - *networks = append((*networks)[:idx], (*networks)[idx+1:]...) - } + + return filtered, nil } -func (c *Cluster) getNetworks(filters *swarmapi.ListNetworksRequest_Filters) ([]network.Inspect, error) { - var r *swarmapi.ListNetworksResponse - err := c.lockedManagerAction(context.TODO(), func(ctx context.Context, state nodeState) error { - var err error - r, err = state.controlClient.ListNetworks(ctx, &swarmapi.ListNetworksRequest{Filters: filters}) - return err +func (c *Cluster) listNetworks(ctx context.Context, filters *swarmapi.ListNetworksRequest_Filters) ([]*swarmapi.Network, error) { + var list []*swarmapi.Network + err := c.lockedManagerAction(ctx, func(ctx context.Context, state nodeState) error { + l, err := state.controlClient.ListNetworks(ctx, &swarmapi.ListNetworksRequest{Filters: filters}) + if err != nil { + return err + } + list = l.Networks + return nil }) - if err != nil { - return nil, err - } - - networks := make([]network.Inspect, 0, len(r.Networks)) - - for _, nw := range r.Networks { - networks = append(networks, convert.BasicNetworkFromGRPC(*nw)) - } - - return networks, nil + return list, err } // GetNetwork returns a cluster network by an ID. @@ -99,9 +89,17 @@ func (c *Cluster) GetNetwork(input string) (network.Inspect, error) { func (c *Cluster) GetNetworksByName(name string) ([]network.Inspect, error) { // Note that swarmapi.GetNetworkRequest.Name is not functional. // So we cannot just use that with c.GetNetwork. - return c.getNetworks(&swarmapi.ListNetworksRequest_Filters{ + list, err := c.listNetworks(context.TODO(), &swarmapi.ListNetworksRequest_Filters{ Names: []string{name}, }) + if err != nil { + return nil, err + } + nr := make([]network.Inspect, len(list)) + for i, n := range list { + nr[i] = convert.BasicNetworkFromGRPC(*n) + } + return nr, nil } func attacherKey(target, containerID string) string { diff --git a/daemon/libnetwork/agent.go b/daemon/libnetwork/agent.go index 50a4c9e534..3021044eda 100644 --- a/daemon/libnetwork/agent.go +++ b/daemon/libnetwork/agent.go @@ -539,6 +539,11 @@ func (n *Network) Services() map[string]ServiceInfo { return sinfo } +// HasServiceAttachments returns true when len(n.Services()) > 0. +func (n *Network) HasServiceAttachments() bool { + return len(n.Services()) > 0 +} + // clusterAgent returns the cluster agent if the network is a swarm-scoped, // multi-host network. func (n *Network) clusterAgent() (agent *nwAgent, ok bool) { diff --git a/daemon/libnetwork/network.go b/daemon/libnetwork/network.go index 6530c24e32..66c919ec0c 100644 --- a/daemon/libnetwork/network.go +++ b/daemon/libnetwork/network.go @@ -249,6 +249,11 @@ func (n *Network) Type() string { return n.networkType } +// Driver is an alias for [Network.Type]. +func (n *Network) Driver() string { + return n.Type() +} + func (n *Network) Resolvers() []*Resolver { n.mu.Lock() defer n.mu.Unlock() @@ -1283,6 +1288,11 @@ func (n *Network) Endpoints() []*Endpoint { return endpoints } +// HasContainerAttachments returns true when len(n.Endpoints()) > 0. +func (n *Network) HasContainerAttachments() bool { + return len(n.Endpoints()) > 0 +} + // WalkEndpoints uses the provided function to walk the Endpoints. func (n *Network) WalkEndpoints(walker EndpointWalker) { for _, e := range n.Endpoints() { diff --git a/daemon/network.go b/daemon/network.go index 852a72e7c2..ce5fbd82f3 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -592,33 +592,23 @@ func (daemon *Daemon) deleteNetwork(nw *libnetwork.Network, dynamic bool) error // GetNetworks returns a list of all networks func (daemon *Daemon) GetNetworks(filter filters.Args, config backend.NetworkListConfig) ([]networktypes.Inspect, error) { - var idx map[string]*libnetwork.Network - if config.Detailed { - idx = make(map[string]*libnetwork.Network) + flt, err := network.NewFilter(filter) + if err != nil { + return nil, err } allNetworks := daemon.getAllNetworks() networks := make([]networktypes.Inspect, 0, len(allNetworks)) for _, n := range allNetworks { - nr := buildNetworkResource(n) - networks = append(networks, nr) - if config.Detailed { - idx[nr.ID] = n - } - } - - var err error - networks, err = network.FilterNetworks(networks, filter) - if err != nil { - return nil, err - } - - if config.Detailed { - for i, nw := range networks { - networks[i].Containers = buildContainerAttachments(idx[nw.ID]) - if config.Verbose { - networks[i].Services = buildServiceAttachments(idx[nw.ID]) + if flt.Matches(n) { + nr := buildNetworkResource(n) + if config.Detailed { + nr.Containers = buildContainerAttachments(n) + if config.Verbose { + nr.Services = buildServiceAttachments(n) + } } + networks = append(networks, nr) } } diff --git a/daemon/network/filter.go b/daemon/network/filter.go index 6e37a414c6..f6a4ee1b9f 100644 --- a/daemon/network/filter.go +++ b/daemon/network/filter.go @@ -2,128 +2,115 @@ package network import ( "github.com/moby/moby/api/types/filters" - "github.com/moby/moby/api/types/network" "github.com/moby/moby/v2/errdefs" "github.com/pkg/errors" ) -// FilterNetworks filters network list according to user specified filter -// and returns user chosen networks -func FilterNetworks(nws []network.Inspect, filter filters.Args) ([]network.Inspect, error) { - // if filter is empty, return original network list - if filter.Len() == 0 { - return nws, nil - } +type Filter struct { + args filters.Args - displayNet := nws[:0] - for _, nw := range nws { - if filter.Contains("driver") { - if !filter.ExactMatch("driver", nw.Driver) { - continue - } - } - if filter.Contains("name") { - if !filter.Match("name", nw.Name) { - continue - } - } - if filter.Contains("id") { - if !filter.Match("id", nw.ID) { - continue - } - } - if filter.Contains("label") { - if !filter.MatchKVList("label", nw.Labels) { - continue - } - } - if filter.Contains("scope") { - if !filter.ExactMatch("scope", nw.Scope) { - continue - } - } + filterByUse, danglingOnly bool +} - if filter.Contains("idOrName") { - if !filter.Match("name", nw.Name) && !filter.Match("id", nw.Name) { - continue - } - } - displayNet = append(displayNet, nw) - } +type FilterNetwork interface { + Driver() string + Name() string + ID() string + Labels() map[string]string + Scope() string + HasContainerAttachments() bool + HasServiceAttachments() bool +} - if values := filter.Get("dangling"); len(values) > 0 { +// NewFilter returns a network 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) { + var filterByUse, danglingOnly bool + if values := args.Get("dangling"); len(values) > 0 { if len(values) > 1 { - return nil, errdefs.InvalidParameter(errors.New(`got more than one value for filter key "dangling"`)) + return Filter{}, errdefs.InvalidParameter(errors.New(`got more than one value for filter key "dangling"`)) } - var danglingOnly bool + filterByUse = true switch values[0] { case "0", "false": // dangling is false already case "1", "true": danglingOnly = true default: - return nil, errdefs.InvalidParameter(errors.New(`invalid value for filter 'dangling', must be "true" (or "1"), or "false" (or "0")`)) + return Filter{}, errdefs.InvalidParameter(errors.New(`invalid value for filter 'dangling', must be "true" (or "1"), or "false" (or "0")`)) } - - displayNet = filterNetworkByUse(displayNet, danglingOnly) } - - if filter.Contains("type") { - typeNet := []network.Inspect{} - errFilter := filter.WalkValues("type", func(fval string) error { - passList, err := filterNetworkByType(displayNet, fval) - if err != nil { - return err - } - typeNet = append(typeNet, passList...) - return nil - }) - if errFilter != nil { - return nil, errFilter - } - displayNet = typeNet + if err := args.WalkValues("type", validateNetworkTypeFilter); err != nil { + return Filter{}, err } - - return displayNet, nil + return Filter{args: args, filterByUse: filterByUse, danglingOnly: danglingOnly}, nil } -func filterNetworkByUse(nws []network.Inspect, danglingOnly bool) []network.Inspect { - retNws := []network.Inspect{} - - filterFunc := func(nw network.Inspect) bool { - if danglingOnly { - return !IsPredefined(nw.Name) && len(nw.Containers) == 0 && len(nw.Services) == 0 - } - return IsPredefined(nw.Name) || len(nw.Containers) > 0 || len(nw.Services) > 0 +// Matches returns true if nw satisfies the filter criteria. +func (f Filter) Matches(nw FilterNetwork) bool { + if f.args.Contains("driver") && + !f.args.ExactMatch("driver", nw.Driver()) { + return false } - - for _, nw := range nws { - if filterFunc(nw) { - retNws = append(retNws, nw) - } + if f.args.Contains("name") && + !f.args.Match("name", nw.Name()) { + return false } - - return retNws + if f.args.Contains("id") && + !f.args.Match("id", nw.ID()) { + return false + } + if f.args.Contains("label") && + !f.args.MatchKVList("label", nw.Labels()) { + return false + } + if f.args.Contains("scope") && + !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 + } + if netTypes := f.args.Get("type"); len(netTypes) > 0 && + !matchesType(netTypes, nw) { + return false + } + return true } -func filterNetworkByType(nws []network.Inspect, netType string) ([]network.Inspect, error) { - retNws := []network.Inspect{} +func matchesUse(danglingOnly bool, nw FilterNetwork) bool { + if danglingOnly { + return !IsPredefined(nw.Name()) && !nw.HasContainerAttachments() && !nw.HasServiceAttachments() + } + return IsPredefined(nw.Name()) || nw.HasContainerAttachments() || nw.HasServiceAttachments() +} + +func validateNetworkTypeFilter(netType string) error { switch netType { - case "builtin": - for _, nw := range nws { - if IsPredefined(nw.Name) { - retNws = append(retNws, nw) - } - } - case "custom": - for _, nw := range nws { - if !IsPredefined(nw.Name) { - retNws = append(retNws, nw) - } - } + case "builtin", "custom": + return nil default: - return nil, errors.Errorf("invalid filter: 'type'='%s'", netType) + return errors.Errorf("invalid filter: 'type'='%s'", netType) } - return retNws, nil +} + +func matchesType(netTypes []string, nw FilterNetwork) bool { + for _, netType := range netTypes { + switch netType { + case "builtin": + return IsPredefined(nw.Name()) + case "custom": + return !IsPredefined(nw.Name()) + } + } + return false } diff --git a/daemon/network/filter_test.go b/daemon/network/filter_test.go index e3278dae4f..da48870419 100644 --- a/daemon/network/filter_test.go +++ b/daemon/network/filter_test.go @@ -3,170 +3,253 @@ package network import ( - "strings" "testing" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" + "github.com/moby/moby/api/types/filters" "github.com/moby/moby/api/types/network" ) -func TestFilterNetworks(t *testing.T) { - networks := []network.Inspect{ +var _ FilterNetwork = mockFilterNetwork{} + +type mockFilterNetwork struct { + driver string + name string + id string + labels map[string]string + scope string + containers bool + services bool +} + +func (n mockFilterNetwork) Driver() string { + return n.driver +} + +func (n mockFilterNetwork) Name() string { + return n.name +} + +func (n mockFilterNetwork) ID() string { + return n.id +} + +func (n mockFilterNetwork) Labels() map[string]string { + return n.labels +} + +func (n mockFilterNetwork) Scope() string { + return n.scope +} + +func (n mockFilterNetwork) HasContainerAttachments() bool { + return n.containers +} + +func (n mockFilterNetwork) HasServiceAttachments() bool { + return n.services +} + +func TestFilter(t *testing.T) { + networks := []mockFilterNetwork{ { - Name: "host", - Driver: "host", - Scope: "local", + name: network.NetworkHost, + id: "ubfg", // ROT13(name) + driver: "host", + scope: "local", }, { - Name: "bridge", - Driver: "bridge", - Scope: "local", + name: network.NetworkBridge, + id: "oevqtr", + driver: "bridge", + scope: "local", }, { - Name: "none", - Driver: "null", - Scope: "local", + name: network.NetworkNone, + id: "abar", + driver: "null", + scope: "local", }, { - Name: "myoverlay", - Driver: "overlay", - Scope: "swarm", + name: "myoverlay", + id: "zlbireynl", + driver: "overlay", + scope: "swarm", }, { - Name: "mydrivernet", - Driver: "mydriver", - Scope: "local", + name: "mydrivernet", + id: "zlqevirearg", + driver: "mydriver", + scope: "local", + labels: map[string]string{"foo": "bar"}, }, { - Name: "mykvnet", - Driver: "mykvdriver", - Scope: "global", + name: "mykvnet", + id: "zlxiarg", + driver: "mykvdriver", + scope: "global", }, { - Name: "networkwithcontainer", - Driver: "nwc", - Scope: "local", - Containers: map[string]network.EndpointResource{ - "customcontainer": { - Name: "customendpoint", - }, - }, + name: "networkwithcontainer", + id: "argjbexjvgupbagnvare", + driver: "nwc", + scope: "local", + containers: true, + }, + { + name: "networkwithservice", + id: "argjbexjvgufreivpr", + driver: "nwc", + scope: "local", + services: true, }, } testCases := []struct { - filter filters.Args - resultCount int - err string - name string - results []string + subtest string + filter filters.Args + + err string + results []string }{ { - filter: filters.NewArgs(filters.Arg("driver", "bridge")), - resultCount: 1, - err: "", - name: "bridge driver filters", + subtest: "EmptyFilter", + filter: filters.NewArgs(), + results: (func() []string { + var r []string + for _, n := range networks { + r = append(r, n.name) + } + return r + })(), }, { - filter: filters.NewArgs(filters.Arg("driver", "overlay")), - resultCount: 1, - err: "", - name: "overlay driver filters", + subtest: "driver=bridge", + filter: filters.NewArgs(filters.Arg("driver", "bridge")), + results: []string{"bridge"}, }, { - filter: filters.NewArgs(filters.Arg("driver", "noname")), - resultCount: 0, - err: "", - name: "no name driver filters", + subtest: "driver=overlay", + filter: filters.NewArgs(filters.Arg("driver", "overlay")), + results: []string{"myoverlay"}, }, { - filter: filters.NewArgs(filters.Arg("type", "custom")), - resultCount: 4, - err: "", - name: "custom driver filters", + subtest: "driver=noname", + filter: filters.NewArgs(filters.Arg("driver", "noname")), }, { - filter: filters.NewArgs(filters.Arg("type", "builtin")), - resultCount: 3, - err: "", - name: "builtin driver filters", + subtest: "type=custom", + filter: filters.NewArgs(filters.Arg("type", "custom")), + results: []string{"myoverlay", "mydrivernet", "mykvnet", "networkwithcontainer", "networkwithservice"}, }, { - filter: filters.NewArgs(filters.Arg("type", "invalid")), - resultCount: 0, - err: "invalid filter: 'type'='invalid'", - name: "invalid driver filters", + subtest: "type=builtin", + filter: filters.NewArgs(filters.Arg("type", "builtin")), + results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone}, }, { - filter: filters.NewArgs(filters.Arg("scope", "local")), - resultCount: 5, - err: "", - name: "local scope filters", + subtest: "type=invalid", + filter: filters.NewArgs(filters.Arg("type", "invalid")), + err: "invalid filter: 'type'='invalid'", }, { - filter: filters.NewArgs(filters.Arg("scope", "swarm")), - resultCount: 1, - err: "", - name: "swarm scope filters", + subtest: "scope=local", + filter: filters.NewArgs(filters.Arg("scope", "local")), + results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "mydrivernet", "networkwithcontainer", "networkwithservice"}, }, { - filter: filters.NewArgs(filters.Arg("scope", "global")), - resultCount: 1, - err: "", - name: "global scope filters", + subtest: "scope=swarm", + filter: filters.NewArgs(filters.Arg("scope", "swarm")), + results: []string{"myoverlay"}, }, { - filter: filters.NewArgs(filters.Arg("dangling", "true")), - resultCount: 3, - err: "", - name: "dangling filter is 'True'", - results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + subtest: "scope=global", + filter: filters.NewArgs(filters.Arg("scope", "global")), + results: []string{"mykvnet"}, }, { - filter: filters.NewArgs(filters.Arg("dangling", "false")), - resultCount: 4, - err: "", - name: "dangling filter is 'False'", - results: []string{"host", "bridge", "none", "networkwithcontainer"}, + subtest: "dangling=true", + filter: filters.NewArgs(filters.Arg("dangling", "true")), + results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + }, + { + subtest: "dangling=1", + filter: filters.NewArgs(filters.Arg("dangling", "1")), + results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + }, + { + subtest: "dangling=false", + filter: filters.NewArgs(filters.Arg("dangling", "false")), + results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "networkwithcontainer", "networkwithservice"}, + }, + { + subtest: "dangling=0", + filter: filters.NewArgs(filters.Arg("dangling", "0")), + results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "networkwithcontainer", "networkwithservice"}, + }, + { + subtest: "dangling=asdf", + filter: filters.NewArgs(filters.Arg("dangling", "asdf")), + err: "invalid value for filter 'dangling'", + }, + { + subtest: "MultipleTerms=dangling", + filter: filters.NewArgs(filters.Arg("dangling", "1"), filters.Arg("dangling", "true")), + err: `got more than one value for filter key "dangling"`, + }, + { + subtest: "label=foo", + filter: filters.NewArgs(filters.Arg("label", "foo")), + results: []string{"mydrivernet"}, + }, + { + subtest: "label=foo=bar", + filter: filters.NewArgs(filters.Arg("label", "foo=bar")), + results: []string{"mydrivernet"}, + }, + { + subtest: "label=foo=baz", + filter: filters.NewArgs(filters.Arg("label", "foo=baz")), + }, + { + subtest: "name=with", + filter: filters.NewArgs(filters.Arg("name", "with")), + results: []string{"networkwithcontainer", "networkwithservice"}, + }, + { + subtest: "id=with", + filter: filters.NewArgs(filters.Arg("id", "with")), + }, + { + subtest: "id=jbexjvgu", + filter: filters.NewArgs(filters.Arg("id", "argjbex")), + results: []string{"networkwithcontainer", "networkwithservice"}, }, } for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - ls := make([]network.Inspect, 0, len(networks)) - ls = append(ls, networks...) - result, err := FilterNetworks(ls, testCase.filter) + t.Run(testCase.subtest, func(t *testing.T) { + flt, err := NewFilter(testCase.filter) if testCase.err != "" { - if err == nil { - t.Fatalf("expect error '%s', got no error", testCase.err) - } else if !strings.Contains(err.Error(), testCase.err) { - t.Fatalf("expect error '%s', got '%s'", testCase.err, err) - } - } else { - if err != nil { - t.Fatalf("expect no error, got error '%s'", err) - } - // Make sure result is not nil - if result == nil { - t.Fatal("filterNetworks should not return nil") - } + assert.ErrorContains(t, err, testCase.err) + return + } + assert.NilError(t, err) - if len(result) != testCase.resultCount { - t.Fatalf("expect '%d' networks, got '%d' networks", testCase.resultCount, len(result)) - } - - if len(testCase.results) > 0 { - resultMap := make(map[string]bool) - for _, r := range result { - resultMap[r.Name] = true - } - for _, r := range testCase.results { - if _, ok := resultMap[r]; !ok { - t.Fatalf("expected result: '%s' not found", r) - } - } + got := map[string]bool{} + for _, nw := range networks { + if flt.Matches(nw) { + got[nw.Name()] = true } } + + want := map[string]bool{} + for _, r := range testCase.results { + want[r] = true + } + assert.Check(t, is.DeepEqual(got, want)) }) } } From ea1dfbda9e3adf044f9d8663008efdff76abe880 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 29 Aug 2025 19:28:35 -0400 Subject: [PATCH 2/3] daemon: prune networks using network.Filter Construct a network.Filter from the filters.Args only once per API request so we don't waste cycles re-validating an already validated filter. Since (*Daemon).NetworksPrune is implemented in terms of (Cluster).GetNetworks, that method now accepts a network.Filter instead of a filter.Args. Change the signature of (*Daemon).GetNetworks for consistency as both of the GetNetworks methods are used by network API route handlers. Signed-off-by: Cory Snider --- daemon/cluster.go | 4 +- daemon/cluster/convert/network.go | 6 + daemon/cluster/networks.go | 27 +---- daemon/network.go | 10 +- daemon/network/filter.go | 39 ++++++- daemon/network/filter_test.go | 104 +++++++++++++----- daemon/prune.go | 27 ++--- daemon/server/router/network/backend.go | 5 +- .../server/router/network/network_routes.go | 24 +++- 9 files changed, 159 insertions(+), 87 deletions(-) diff --git a/daemon/cluster.go b/daemon/cluster.go index e98e5174a6..6881e78204 100644 --- a/daemon/cluster.go +++ b/daemon/cluster.go @@ -1,9 +1,9 @@ package daemon import ( - "github.com/moby/moby/api/types/filters" "github.com/moby/moby/api/types/network" lncluster "github.com/moby/moby/v2/daemon/libnetwork/cluster" + dnetwork "github.com/moby/moby/v2/daemon/network" ) // Cluster is the interface for [github.com/moby/moby/v2/daemon/cluster.Cluster]. @@ -22,6 +22,6 @@ type ClusterStatus interface { // NetworkManager provides methods to manage networks type NetworkManager interface { GetNetwork(input string) (network.Inspect, error) - GetNetworks(filters.Args) ([]network.Inspect, error) + GetNetworks(dnetwork.Filter) ([]network.Inspect, error) RemoveNetwork(input string) error } diff --git a/daemon/cluster/convert/network.go b/daemon/cluster/convert/network.go index e76cd3221b..b3edf1f696 100644 --- a/daemon/cluster/convert/network.go +++ b/daemon/cluster/convert/network.go @@ -2,6 +2,7 @@ package convert import ( "strings" + "time" gogotypes "github.com/gogo/protobuf/types" "github.com/moby/moby/api/types/network" @@ -270,6 +271,11 @@ func (nw FilterNetwork) Scope() string { return scope.Swarm } +func (nw FilterNetwork) Created() time.Time { + t, _ := gogotypes.TimestampFromProto(nw.N.Meta.CreatedAt) + return t +} + func (nw FilterNetwork) HasContainerAttachments() bool { // Not tracked in swarmkit return false diff --git a/daemon/cluster/networks.go b/daemon/cluster/networks.go index 182b1b9d4b..3f55705a6e 100644 --- a/daemon/cluster/networks.go +++ b/daemon/cluster/networks.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/containerd/log" - "github.com/moby/moby/api/types/filters" "github.com/moby/moby/api/types/network" types "github.com/moby/moby/api/types/swarm" "github.com/moby/moby/v2/daemon/cluster/convert" @@ -16,25 +15,11 @@ import ( ) // GetNetworks returns all current cluster managed networks. -func (c *Cluster) GetNetworks(filter filters.Args) ([]network.Inspect, error) { - flt, err := networkSettings.NewFilter(filter) - if err != nil { - return nil, err - } - - var f *swarmapi.ListNetworksRequest_Filters - - if filter.Len() > 0 { - f = &swarmapi.ListNetworksRequest_Filters{} - - if filter.Contains("name") { - f.Names = filter.Get("name") - f.NamePrefixes = filter.Get("name") - } - - if filter.Contains("id") { - f.IDPrefixes = filter.Get("id") - } +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) @@ -46,7 +31,7 @@ func (c *Cluster) GetNetworks(filter filters.Args) ([]network.Inspect, error) { if n.Spec.Annotations.Labels["com.docker.swarm.predefined"] == "true" { continue } - if flt.Matches(convert.FilterNetwork{N: n}) { + if filter.Matches(convert.FilterNetwork{N: n}) { filtered = append(filtered, convert.BasicNetworkFromGRPC(*n)) } } diff --git a/daemon/network.go b/daemon/network.go index ce5fbd82f3..2a6b82dfae 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -17,7 +17,6 @@ import ( "github.com/docker/go-connections/nat" containertypes "github.com/moby/moby/api/types/container" "github.com/moby/moby/api/types/events" - "github.com/moby/moby/api/types/filters" networktypes "github.com/moby/moby/api/types/network" clustertypes "github.com/moby/moby/v2/daemon/cluster/provider" "github.com/moby/moby/v2/daemon/config" @@ -591,16 +590,11 @@ func (daemon *Daemon) deleteNetwork(nw *libnetwork.Network, dynamic bool) error } // GetNetworks returns a list of all networks -func (daemon *Daemon) GetNetworks(filter filters.Args, config backend.NetworkListConfig) ([]networktypes.Inspect, error) { - flt, err := network.NewFilter(filter) - if err != nil { - return nil, err - } - +func (daemon *Daemon) GetNetworks(filter network.Filter, config backend.NetworkListConfig) ([]networktypes.Inspect, error) { allNetworks := daemon.getAllNetworks() networks := make([]networktypes.Inspect, 0, len(allNetworks)) for _, n := range allNetworks { - if flt.Matches(n) { + if filter.Matches(n) { nr := buildNetworkResource(n) if config.Detailed { nr.Containers = buildContainerAttachments(n) diff --git a/daemon/network/filter.go b/daemon/network/filter.go index f6a4ee1b9f..8e381e320e 100644 --- a/daemon/network/filter.go +++ b/daemon/network/filter.go @@ -1,7 +1,10 @@ package network import ( + "time" + "github.com/moby/moby/api/types/filters" + "github.com/moby/moby/v2/daemon/internal/timestamp" "github.com/moby/moby/v2/errdefs" "github.com/pkg/errors" ) @@ -10,6 +13,7 @@ type Filter struct { args filters.Args filterByUse, danglingOnly bool + until time.Time } type FilterNetwork interface { @@ -18,6 +22,7 @@ type FilterNetwork interface { ID() string Labels() map[string]string Scope() string + Created() time.Time HasContainerAttachments() bool HasServiceAttachments() bool } @@ -46,7 +51,31 @@ func NewFilter(args filters.Args) (Filter, error) { if err := args.WalkValues("type", validateNetworkTypeFilter); err != nil { return Filter{}, err } - return Filter{args: args, filterByUse: filterByUse, danglingOnly: danglingOnly}, nil + until := time.Time{} + if untilFilters := args.Get("until"); len(untilFilters) > 0 { + if len(untilFilters) > 1 { + return Filter{}, errdefs.InvalidParameter(errors.New("more than one until filter specified")) + } + ts, err := timestamp.GetTimestamp(untilFilters[0], time.Now()) + if err != nil { + return Filter{}, errdefs.InvalidParameter(err) + } + seconds, nanoseconds, err := timestamp.ParseTimestamps(ts, 0) + if err != nil { + return Filter{}, errdefs.InvalidParameter(err) + } + until = time.Unix(seconds, nanoseconds) + } + return Filter{ + args: args, + filterByUse: filterByUse, + danglingOnly: danglingOnly, + until: until, + }, nil +} + +func (f Filter) Get(key string) []string { + return f.args.Get(key) } // Matches returns true if nw satisfies the filter criteria. @@ -67,6 +96,10 @@ func (f Filter) Matches(nw FilterNetwork) bool { !f.args.MatchKVList("label", nw.Labels()) { return false } + if f.args.Contains("label!") && + f.args.MatchKVList("label!", nw.Labels()) { + return false + } if f.args.Contains("scope") && !f.args.ExactMatch("scope", nw.Scope()) { return false @@ -84,6 +117,10 @@ func (f Filter) Matches(nw FilterNetwork) bool { !matchesType(netTypes, nw) { return false } + if !f.until.IsZero() && + nw.Created().After(f.until) { + return false + } return true } diff --git a/daemon/network/filter_test.go b/daemon/network/filter_test.go index da48870419..6cd6169abe 100644 --- a/daemon/network/filter_test.go +++ b/daemon/network/filter_test.go @@ -4,6 +4,7 @@ package network import ( "testing" + "time" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -20,6 +21,7 @@ type mockFilterNetwork struct { id string labels map[string]string scope string + created time.Time containers bool services bool } @@ -44,6 +46,10 @@ func (n mockFilterNetwork) Scope() string { return n.scope } +func (n mockFilterNetwork) Created() time.Time { + return n.created +} + func (n mockFilterNetwork) HasContainerAttachments() bool { return n.containers } @@ -55,41 +61,47 @@ func (n mockFilterNetwork) HasServiceAttachments() bool { func TestFilter(t *testing.T) { networks := []mockFilterNetwork{ { - name: network.NetworkHost, - id: "ubfg", // ROT13(name) - driver: "host", - scope: "local", + name: network.NetworkHost, + id: "ubfg", // ROT13(name) + driver: "host", + scope: "local", + created: time.Date(2025, time.June, 1, 0, 0, 0, 0, time.Local), }, { - name: network.NetworkBridge, - id: "oevqtr", - driver: "bridge", - scope: "local", + name: network.NetworkBridge, + id: "oevqtr", + driver: "bridge", + scope: "local", + created: time.Date(2025, time.June, 1, 0, 0, 0, 0, time.Local), }, { - name: network.NetworkNone, - id: "abar", - driver: "null", - scope: "local", + name: network.NetworkNone, + id: "abar", + driver: "null", + scope: "local", + created: time.Date(2025, time.June, 1, 0, 0, 0, 0, time.Local), }, { - name: "myoverlay", - id: "zlbireynl", - driver: "overlay", - scope: "swarm", + name: "myoverlay", + id: "zlbireynl", + driver: "overlay", + scope: "swarm", + created: time.Date(2024, time.June, 1, 12, 0, 0, 0, time.Local), }, { - name: "mydrivernet", - id: "zlqevirearg", - driver: "mydriver", - scope: "local", - labels: map[string]string{"foo": "bar"}, + name: "mydrivernet", + id: "zlqevirearg", + driver: "mydriver", + scope: "local", + labels: map[string]string{"foo": "bar"}, + created: time.Date(2024, time.December, 1, 2, 0, 0, 0, time.Local), }, { - name: "mykvnet", - id: "zlxiarg", - driver: "mykvdriver", - scope: "global", + name: "mykvnet", + id: "zlxiarg", + driver: "mykvdriver", + scope: "global", + created: time.Date(2025, time.January, 1, 0, 0, 0, 0, time.Local), }, { name: "networkwithcontainer", @@ -97,6 +109,7 @@ func TestFilter(t *testing.T) { driver: "nwc", scope: "local", containers: true, + created: time.Date(2025, time.June, 1, 0, 0, 0, 0, time.Local), }, { name: "networkwithservice", @@ -104,6 +117,14 @@ func TestFilter(t *testing.T) { driver: "nwc", scope: "local", services: true, + created: time.Date(2025, time.June, 1, 0, 0, 0, 0, time.Local), + }, + { + name: "idoverlap", + id: "aaaaa0my0bbbbbb", + driver: "nwc", + scope: "local", + created: time.Date(2025, time.February, 1, 0, 0, 0, 0, time.Local), }, } @@ -142,7 +163,7 @@ func TestFilter(t *testing.T) { { subtest: "type=custom", filter: filters.NewArgs(filters.Arg("type", "custom")), - results: []string{"myoverlay", "mydrivernet", "mykvnet", "networkwithcontainer", "networkwithservice"}, + results: []string{"myoverlay", "mydrivernet", "mykvnet", "networkwithcontainer", "networkwithservice", "idoverlap"}, }, { subtest: "type=builtin", @@ -157,7 +178,7 @@ func TestFilter(t *testing.T) { { subtest: "scope=local", filter: filters.NewArgs(filters.Arg("scope", "local")), - results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "mydrivernet", "networkwithcontainer", "networkwithservice"}, + results: []string{network.NetworkHost, network.NetworkBridge, network.NetworkNone, "mydrivernet", "networkwithcontainer", "networkwithservice", "idoverlap"}, }, { subtest: "scope=swarm", @@ -172,12 +193,12 @@ func TestFilter(t *testing.T) { { subtest: "dangling=true", filter: filters.NewArgs(filters.Arg("dangling", "true")), - results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + results: []string{"myoverlay", "mydrivernet", "mykvnet", "idoverlap"}, }, { subtest: "dangling=1", filter: filters.NewArgs(filters.Arg("dangling", "1")), - results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + results: []string{"myoverlay", "mydrivernet", "mykvnet", "idoverlap"}, }, { subtest: "dangling=false", @@ -227,6 +248,31 @@ func TestFilter(t *testing.T) { filter: filters.NewArgs(filters.Arg("id", "argjbex")), results: []string{"networkwithcontainer", "networkwithservice"}, }, + { + subtest: "id=my", + 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: "until=2025-01-01", + filter: filters.NewArgs(filters.Arg("until", "2025-01-01")), + results: []string{"myoverlay", "mydrivernet", "mykvnet"}, + }, + { + subtest: "until=2024-12-01T01:00:00", + filter: filters.NewArgs(filters.Arg("until", "2024-12-01T01:00:00")), + results: []string{"myoverlay"}, + }, + { + 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", + }, } for _, testCase := range testCases { diff --git a/daemon/prune.go b/daemon/prune.go index 3e3c79fb84..141fffc336 100644 --- a/daemon/prune.go +++ b/daemon/prune.go @@ -13,6 +13,7 @@ import ( "github.com/moby/moby/v2/daemon/internal/lazyregexp" "github.com/moby/moby/v2/daemon/internal/timestamp" "github.com/moby/moby/v2/daemon/libnetwork" + dnetwork "github.com/moby/moby/v2/daemon/network" "github.com/moby/moby/v2/daemon/server/backend" "github.com/moby/moby/v2/errdefs" "github.com/pkg/errors" @@ -96,11 +97,9 @@ func (daemon *Daemon) ContainersPrune(ctx context.Context, pruneFilters filters. } // localNetworksPrune removes unused local networks -func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters filters.Args) *network.PruneReport { +func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters dnetwork.Filter) *network.PruneReport { rep := &network.PruneReport{} - until, _ := getUntilFromPruneFilters(pruneFilters) - // When the function returns true, the walk will stop. daemon.netController.WalkNetworks(func(nw *libnetwork.Network) bool { select { @@ -112,10 +111,7 @@ func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters filte if nw.ConfigOnly() { return false } - if !until.IsZero() && nw.Created().After(until) { - return false - } - if !matchLabels(pruneFilters, nw.Labels()) { + if !pruneFilters.Matches(nw) { return false } if !nw.IsPruneable() { @@ -137,11 +133,9 @@ func (daemon *Daemon) localNetworksPrune(ctx context.Context, pruneFilters filte var networkIsInUse = lazyregexp.New(`network ([[:alnum:]]+) is in use`) // clusterNetworksPrune removes unused cluster networks -func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters filters.Args) (*network.PruneReport, error) { +func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters dnetwork.Filter) (*network.PruneReport, error) { rep := &network.PruneReport{} - until, _ := getUntilFromPruneFilters(pruneFilters) - cluster := daemon.GetCluster() if !cluster.IsManager() { @@ -162,12 +156,6 @@ func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters fil // Routing-mesh network removal has to be explicitly invoked by user continue } - if !until.IsZero() && nw.Created.After(until) { - continue - } - if !matchLabels(pruneFilters, nw.Labels) { - continue - } // https://github.com/moby/moby/issues/24186 // `docker network inspect` unfortunately displays ONLY those containers that are local to that node. // So we try to remove it anyway and check the error @@ -187,19 +175,20 @@ func (daemon *Daemon) clusterNetworksPrune(ctx context.Context, pruneFilters fil } // NetworksPrune removes unused networks -func (daemon *Daemon) NetworksPrune(ctx context.Context, pruneFilters filters.Args) (*network.PruneReport, error) { +func (daemon *Daemon) NetworksPrune(ctx context.Context, filterArgs filters.Args) (*network.PruneReport, error) { if !daemon.pruneRunning.CompareAndSwap(false, true) { return nil, errPruneRunning } defer daemon.pruneRunning.Store(false) // make sure that only accepted filters have been received - err := pruneFilters.Validate(networksAcceptedFilters) + err := filterArgs.Validate(networksAcceptedFilters) if err != nil { return nil, err } - if _, err := getUntilFromPruneFilters(pruneFilters); err != nil { + pruneFilters, err := dnetwork.NewFilter(filterArgs) + if err != nil { return nil, err } diff --git a/daemon/server/router/network/backend.go b/daemon/server/router/network/backend.go index 49476f4ede..8c47442523 100644 --- a/daemon/server/router/network/backend.go +++ b/daemon/server/router/network/backend.go @@ -5,13 +5,14 @@ import ( "github.com/moby/moby/api/types/filters" "github.com/moby/moby/api/types/network" + dnetwork "github.com/moby/moby/v2/daemon/network" "github.com/moby/moby/v2/daemon/server/backend" ) // Backend is all the methods that need to be implemented // to provide network specific functionality. type Backend interface { - GetNetworks(filters.Args, backend.NetworkListConfig) ([]network.Inspect, error) + GetNetworks(dnetwork.Filter, backend.NetworkListConfig) ([]network.Inspect, error) CreateNetwork(ctx context.Context, nc network.CreateRequest) (*network.CreateResponse, error) ConnectContainerToNetwork(ctx context.Context, containerName, networkName string, endpointConfig *network.EndpointSettings) error DisconnectContainerFromNetwork(containerName string, networkName string, force bool) error @@ -22,7 +23,7 @@ type Backend interface { // ClusterBackend is all the methods that need to be implemented // to provide cluster network specific functionality. type ClusterBackend interface { - GetNetworks(filters.Args) ([]network.Inspect, error) + GetNetworks(dnetwork.Filter) ([]network.Inspect, error) GetNetwork(name string) (network.Inspect, error) GetNetworksByName(name string) ([]network.Inspect, error) CreateNetwork(nc network.CreateRequest) (string, error) diff --git a/daemon/server/router/network/network_routes.go b/daemon/server/router/network/network_routes.go index a018d98a28..6677f4546c 100644 --- a/daemon/server/router/network/network_routes.go +++ b/daemon/server/router/network/network_routes.go @@ -11,6 +11,7 @@ import ( "github.com/moby/moby/api/types/versions" "github.com/moby/moby/v2/daemon/libnetwork" "github.com/moby/moby/v2/daemon/libnetwork/scope" + dnetwork "github.com/moby/moby/v2/daemon/network" "github.com/moby/moby/v2/daemon/server/backend" "github.com/moby/moby/v2/daemon/server/httputils" "github.com/moby/moby/v2/daemon/server/networkbackend" @@ -23,12 +24,17 @@ func (n *networkRouter) getNetworksList(ctx context.Context, w http.ResponseWrit return err } - filter, err := filters.FromJSON(r.Form.Get("filters")) + filterArgs, err := filters.FromJSON(r.Form.Get("filters")) if err != nil { return err } - if err := network.ValidateFilters(filter); err != nil { + if err := network.ValidateFilters(filterArgs); err != nil { + return err + } + + filter, err := dnetwork.NewFilter(filterArgs) + if err != nil { return err } @@ -114,10 +120,15 @@ 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. - filter := filters.NewArgs(filters.Arg("idOrName", term)) + filterArgs := filters.NewArgs(filters.Arg("idOrName", term)) if networkScope != "" { - filter.Add("scope", networkScope) + filterArgs.Add("scope", networkScope) } + filter, err := dnetwork.NewFilter(filterArgs) + if err != nil { + return err + } + networks, _ := n.backend.GetNetworks(filter, backend.NetworkListConfig{Detailed: true, Verbose: verbose}) for _, nw := range networks { if nw.ID == term { @@ -322,7 +333,10 @@ func (n *networkRouter) findUniqueNetwork(term string) (network.Inspect, error) listByFullName := map[string]network.Inspect{} listByPartialID := map[string]network.Inspect{} - filter := filters.NewArgs(filters.Arg("idOrName", term)) + filter, err := dnetwork.NewFilter(filters.NewArgs(filters.Arg("idOrName", term))) + if err != nil { + return network.Inspect{}, err + } networks, _ := n.backend.GetNetworks(filter, backend.NetworkListConfig{Detailed: true}) for _, nw := range networks { if nw.ID == term { From f8bd170b2afe646833c35801b099e75bd651223e Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 28 Aug 2025 17:28:19 -0400 Subject: [PATCH 3/3] 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 {