diff --git a/cni.go b/cni.go index 003f302..70ca382 100644 --- a/cni.go +++ b/cni.go @@ -80,7 +80,10 @@ type libcni struct { cniConfig cnilibrary.CNI networkCount int // minimum network plugin configurations needed to initialize cni networks []*Network - sync.RWMutex + // Mutex contract: + // - lock in public methods: write lock when mutating the state, read lock when reading the state. + // - never lock in private methods. + RWMutex } func defaultCNIConfig() *libcni { @@ -135,11 +138,11 @@ func (c *libcni) Load(opts ...Opt) error { // Status returns the status of CNI initialization. func (c *libcni) Status() error { + c.RLock() + defer c.RUnlock() if err := c.ready(); err != nil { return err } - c.RLock() - defer c.RUnlock() // STATUS is only called for CNI Version 1.1.0 or greater. It is ignored for previous versions. for _, v := range c.networks { err := c.cniConfig.GetStatusNetworkList(context.Background(), v.config) @@ -162,11 +165,11 @@ func (c *libcni) Networks() []*Network { // Setup setups the network in the namespace and returns a Result func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) { + c.RLock() + defer c.RUnlock() if err := c.ready(); err != nil { return nil, err } - c.RLock() - defer c.RUnlock() ns, err := newNamespace(id, path, opts...) if err != nil { return nil, err @@ -180,11 +183,11 @@ func (c *libcni) Setup(ctx context.Context, id string, path string, opts ...Name // SetupSerially setups the network in the namespace and returns a Result func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts ...NamespaceOpts) (*Result, error) { + c.RLock() + defer c.RUnlock() if err := c.ready(); err != nil { return nil, err } - c.RLock() - defer c.RUnlock() ns, err := newNamespace(id, path, opts...) if err != nil { return nil, err @@ -198,7 +201,7 @@ func (c *libcni) SetupSerially(ctx context.Context, id string, path string, opts func (c *libcni) attachNetworksSerially(ctx context.Context, ns *Namespace) ([]*types100.Result, error) { var results []*types100.Result - for _, network := range c.Networks() { + for _, network := range c.networks { r, err := network.Attach(ctx, ns) if err != nil { return nil, err @@ -223,15 +226,15 @@ func asynchAttach(ctx context.Context, index int, n *Network, ns *Namespace, wg func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100.Result, error) { var wg sync.WaitGroup var firstError error - results := make([]*types100.Result, len(c.Networks())) + results := make([]*types100.Result, len(c.networks)) rc := make(chan asynchAttachResult) - for i, network := range c.Networks() { + for i, network := range c.networks { wg.Add(1) go asynchAttach(ctx, i, network, ns, &wg, rc) } - for range c.Networks() { + for range c.networks { rs := <-rc if rs.err != nil && firstError == nil { firstError = rs.err @@ -245,16 +248,16 @@ func (c *libcni) attachNetworks(ctx context.Context, ns *Namespace) ([]*types100 // Remove removes the network config from the namespace func (c *libcni) Remove(ctx context.Context, id string, path string, opts ...NamespaceOpts) error { + c.RLock() + defer c.RUnlock() if err := c.ready(); err != nil { return err } - c.RLock() - defer c.RUnlock() ns, err := newNamespace(id, path, opts...) if err != nil { return err } - for _, network := range c.Networks() { + for _, network := range c.networks { if err := network.Remove(ctx, ns); err != nil { // Based on CNI spec v0.7.0, empty network namespace is allowed to // do best effort cleanup. However, it is not handled consistently @@ -275,16 +278,16 @@ func (c *libcni) Remove(ctx context.Context, id string, path string, opts ...Nam // Check checks if the network is still in desired state func (c *libcni) Check(ctx context.Context, id string, path string, opts ...NamespaceOpts) error { + c.RLock() + defer c.RUnlock() if err := c.ready(); err != nil { return err } - c.RLock() - defer c.RUnlock() ns, err := newNamespace(id, path, opts...) if err != nil { return err } - for _, network := range c.Networks() { + for _, network := range c.networks { err := network.Check(ctx, ns) if err != nil { return err @@ -329,8 +332,6 @@ func (c *libcni) reset() { } func (c *libcni) ready() error { - c.RLock() - defer c.RUnlock() if len(c.networks) < c.networkCount { return ErrCNINotInitialized } diff --git a/cni_test.go b/cni_test.go index 2abdab2..b31c55a 100644 --- a/cni_test.go +++ b/cni_test.go @@ -35,12 +35,13 @@ import ( // TestLibCNIType020 tests the cni version 0.2.0 plugin // config and parses the result into structured data func TestLibCNIType020(t *testing.T) { + t.Parallel() + // Get the default CNI config l := defaultCNIConfig() // Create a fake cni config directory and file - cniDir, confDir := makeFakeCNIConfig(t) - defer tearDownCNIConfig(t, cniDir) + _, confDir := makeFakeCNIConfig(t) l.pluginConfDir = confDir // Set the minimum network count as 2 for this test l.networkCount = 2 @@ -112,11 +113,12 @@ func TestLibCNIType020(t *testing.T) { // TestLibCNIType040 tests the cni version 0.4.0 plugin // config and parses the result into structured data func TestLibCNIType040(t *testing.T) { + t.Parallel() + // Get the default CNI config l := defaultCNIConfig() // Create a fake cni config directory and file - cniDir, confDir := makeFakeCNIConfig(t) - defer tearDownCNIConfig(t, cniDir) + _, confDir := makeFakeCNIConfig(t) l.pluginConfDir = confDir // Set the minimum network count as 2 for this test l.networkCount = 2 @@ -200,11 +202,12 @@ func TestLibCNIType040(t *testing.T) { // TestLibCNIType100 tests the cni version 1.0.0 plugin // config and parses the result into structured data func TestLibCNIType100(t *testing.T) { + t.Parallel() + // Get the default CNI config l := defaultCNIConfig() // Create a fake cni config directory and file - cniDir, confDir := makeFakeCNIConfig(t) - defer tearDownCNIConfig(t, cniDir) + _, confDir := makeFakeCNIConfig(t) l.pluginConfDir = confDir // Set the minimum network count as 2 for this test l.networkCount = 2 @@ -290,11 +293,12 @@ func TestLibCNIType100(t *testing.T) { // TestLibCNIType120 tests the cni version 1.1.0 plugin // config and parses the result into structured data func TestLibCNIType120(t *testing.T) { + t.Parallel() + // Get the default CNI config l := defaultCNIConfig() // Create a fake cni config directory and file - cniDir, confDir := buildFakeConfig(t) - defer tearDownCNIConfig(t, cniDir) + _, confDir := buildFakeConfig(t) l.pluginConfDir = confDir // Set the minimum network count as 2 for this test l.networkCount = 2 @@ -382,11 +386,12 @@ func TestLibCNIType120(t *testing.T) { } func TestLibCNIType120FailStatus(t *testing.T) { + t.Parallel() + // Get the default CNI config l := defaultCNIConfig() // Create a fake cni config directory and file - cniDir, confDir := buildFakeConfig(t) - defer tearDownCNIConfig(t, cniDir) + _, confDir := buildFakeConfig(t) l.pluginConfDir = confDir // Set the minimum network count as 2 for this test l.networkCount = 2 diff --git a/deprecated.go b/deprecated.go index 06afd15..fd8b3eb 100644 --- a/deprecated.go +++ b/deprecated.go @@ -30,5 +30,7 @@ type CNIResult = Result //revive:disable // type name will be used as cni.CNIRes // results fails, or if a network could not be found. // Deprecated: do not use func (c *libcni) GetCNIResultFromResults(results []*types100.Result) (*Result, error) { + c.RLock() + defer c.RUnlock() return c.createResult(results) } diff --git a/go.mod b/go.mod index 1542005..546cd57 100644 --- a/go.mod +++ b/go.mod @@ -4,11 +4,13 @@ go 1.21 require ( github.com/containernetworking/cni v1.2.2 + github.com/sasha-s/go-deadlock v0.3.5 github.com/stretchr/testify v1.8.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect + github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/stretchr/objx v0.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index d3cd8f5..5d66cb1 100644 --- a/go.sum +++ b/go.sum @@ -15,8 +15,12 @@ github.com/onsi/ginkgo/v2 v2.19.0 h1:9Cnnf7UHo57Hy3k6/m5k3dRfGTMXGvxhHFvkDTCTpvA github.com/onsi/ginkgo/v2 v2.19.0/go.mod h1:rlwLi9PilAFJ8jCg9UE1QP6VBpd6/xj3SRC0d6TU0To= github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk= github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16AVYg0= +github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw= +github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU= +github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= diff --git a/mutex.go b/mutex.go new file mode 100644 index 0000000..8d40d62 --- /dev/null +++ b/mutex.go @@ -0,0 +1,23 @@ +//go:build !deadlocks && !race + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package cni + +import "sync" + +type RWMutex = sync.RWMutex diff --git a/mutex_deadlocks.go b/mutex_deadlocks.go new file mode 100644 index 0000000..20e97b2 --- /dev/null +++ b/mutex_deadlocks.go @@ -0,0 +1,25 @@ +//go:build deadlocks || race + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package cni + +import ( + "github.com/sasha-s/go-deadlock" +) + +type RWMutex = deadlock.RWMutex diff --git a/result.go b/result.go index 0dd002e..ea160df 100644 --- a/result.go +++ b/result.go @@ -69,8 +69,6 @@ type Config struct { // interfaces created in the namespace. It returns an error if validation of // results fails, or if a network could not be found. func (c *libcni) createResult(results []*types100.Result) (*Result, error) { - c.RLock() - defer c.RUnlock() r := &Result{ Interfaces: make(map[string]*Config), raw: results, diff --git a/testutils.go b/testutils.go index c270100..d195fea 100644 --- a/testutils.go +++ b/testutils.go @@ -23,22 +23,11 @@ import ( "testing" ) -func makeTmpDir(prefix string) (string, error) { - tmpDir, err := os.MkdirTemp("", prefix) - if err != nil { - return "", err - } - return tmpDir, nil -} - func makeFakeCNIConfig(t *testing.T) (string, string) { - cniDir, err := makeTmpDir("fakecni") - if err != nil { - t.Fatalf("Failed to create plugin config dir: %v", err) - } + cniDir := t.TempDir() cniConfDir := path.Join(cniDir, "net.d") - err = os.MkdirAll(cniConfDir, 0777) + err := os.MkdirAll(cniConfDir, 0777) if err != nil { t.Fatalf("Failed to create network config dir: %v", err) } @@ -69,13 +58,6 @@ func makeFakeCNIConfig(t *testing.T) (string, string) { return cniDir, cniConfDir } -func tearDownCNIConfig(t *testing.T, confDir string) { - err := os.RemoveAll(confDir) - if err != nil { - t.Fatalf("Failed to cleanup CNI configs: %v", err) - } -} - func buildFakeConfig(t *testing.T) (string, string) { conf := ` { @@ -111,13 +93,10 @@ func buildFakeConfig(t *testing.T) (string, string) { ] }` - cniDir, err := makeTmpDir("fakecni") - if err != nil { - t.Fatalf("Failed to create plugin config dir: %v", err) - } + cniDir := t.TempDir() cniConfDir := path.Join(cniDir, "net.d") - err = os.MkdirAll(cniConfDir, 0777) + err := os.MkdirAll(cniConfDir, 0777) if err != nil { t.Fatalf("Failed to create network config dir: %v", err) }