From 97991e8bca78aefdafadf464eb60fa115435e158 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 30 Jun 2021 21:01:09 +0000 Subject: [PATCH 1/5] handle empty state file --- cni/client/client_integration_test.go | 5 ++--- network/manager.go | 10 ++++++++-- store/json.go | 12 +++++++++++- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/cni/client/client_integration_test.go b/cni/client/client_integration_test.go index ecab0bc9d9..89afd6714f 100644 --- a/cni/client/client_integration_test.go +++ b/cni/client/client_integration_test.go @@ -59,8 +59,7 @@ func TestMain(m *testing.M) { // todo: enable this test in CI, requires built azure vnet func TestGetStateFromAzureCNI(t *testing.T) { - - c := AzureCNIClient{exec: exec.New()} + c := New(exec.New()) state, err := c.GetEndpointState() require.NoError(t, err) @@ -75,7 +74,7 @@ func TestGetStateFromAzureCNI(t *testing.T) { } func TestGetVersion(t *testing.T) { - c := &AzureCNIClient{exec: exec.New()} + c := New(exec.New()) version, err := c.GetVersion() require.NoError(t, err) diff --git a/network/manager.go b/network/manager.go index 221953d430..39ed47fea7 100644 --- a/network/manager.go +++ b/network/manager.go @@ -384,13 +384,19 @@ func (nm *networkManager) GetAllEndpoints(networkId string) (map[string]*Endpoin nm.Lock() defer nm.Unlock() + eps := make(map[string]*EndpointInfo) + + // Special case when CNS invokes CNI, but there is no state, but return gracefully + if len(nm.ExternalInterfaces) == 0 { + log.Printf("Network manager has no external interfaces, is the state file populated?") + return eps, nil + } + nw, err := nm.getNetwork(networkId) if err != nil { return nil, err } - eps := make(map[string]*EndpointInfo) - for epid, ep := range nw.Endpoints { eps[epid] = ep.getInfo() } diff --git a/store/json.go b/store/json.go index ca1a9f65d4..00f92ad60b 100644 --- a/store/json.go +++ b/store/json.go @@ -71,8 +71,18 @@ func (kvs *jsonFileStore) Read(key string, value interface{}) error { } defer file.Close() + b, err := ioutil.ReadAll(file) + if err != nil { + return err + } + + if len(b) == 0 { + log.Printf("Unable to read file %s, was empty", kvs.fileName) + return nil + } + // Decode to raw JSON messages. - if err := json.NewDecoder(file).Decode(&kvs.data); err != nil { + if err := json.Unmarshal(b, &kvs.data); err != nil { return err } From 6a4989cb632a9c377c204517677314b0c4b8aba6 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 30 Jun 2021 22:37:17 +0000 Subject: [PATCH 2/5] update tests --- cni/client/client_integration_test.go | 71 ++++++++++++++++++--------- cni/network/network.go | 5 +- network/manager.go | 2 +- store/json.go | 2 +- store/store.go | 1 + 5 files changed, 54 insertions(+), 27 deletions(-) diff --git a/cni/client/client_integration_test.go b/cni/client/client_integration_test.go index 89afd6714f..5472b7e7e6 100644 --- a/cni/client/client_integration_test.go +++ b/cni/client/client_integration_test.go @@ -16,45 +16,47 @@ import ( "k8s.io/utils/exec" ) -func TestMain(m *testing.M) { - testutils.RequireRootforTestMain() +const ( + stateFilePath = "/var/run/azure-vnet.json" +) + +func setTestFile() error { var err error // copy test state file to /var/run/azure-vnet.json in, err := os.Open("./testresources/azure-vnet-test.json") if err != nil { - return + return err } - out, err := os.Create("/var/run/azure-vnet.json") + out, err := os.Create(stateFilePath) if err != nil { - return + return err } - exit := 0 - defer func() { - if in != nil { - in.Close() - } - - if out != nil { - out.Close() - } + _, err = io.Copy(out, in) + if err != nil { + return err + } - err := os.Remove("/var/run/azure-vnet.json") - if err != nil { - log.Print(err) - os.Exit(1) - } + return nil +} - os.Exit(exit) - }() +func cleanupTestFile() { + err := os.Remove(stateFilePath) + if err != nil { + log.Print(err) + } +} - _, err = io.Copy(out, in) +func TestMain(m *testing.M) { + testutils.RequireRootforTestMain() + err := setTestFile() if err != nil { - return + log.Panic(err) } - exit = m.Run() + os.Exit(m.Run()) + cleanupTestFile() } // todo: enable this test in CI, requires built azure vnet @@ -83,3 +85,24 @@ func TestGetVersion(t *testing.T) { require.Equal(t, expectedVersion, version) } + +func TestGetStateWithEmptyStateFile(t *testing.T) { + defer func() { + cleanupTestFile() + setTestFile() + }() + + c := New(exec.New()) + + err := os.Remove(stateFilePath) + require.NoError(t, err) + + out, err := os.Create(stateFilePath) + require.NoError(t, err) + out.Close() + + state, err := c.GetEndpointState() + require.NoError(t, err) + require.Exactly(t, 0, len(state.ContainerInterfaces)) + +} diff --git a/cni/network/network.go b/cni/network/network.go index 241deafaac..d117026076 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -27,6 +27,7 @@ import ( "github.com/Azure/azure-container-networking/network/policy" "github.com/Azure/azure-container-networking/platform" nnscontracts "github.com/Azure/azure-container-networking/proto/nodenetworkservice/3.302.0.744" + "github.com/Azure/azure-container-networking/store" "github.com/Azure/azure-container-networking/telemetry" cniSkel "github.com/containernetworking/cni/pkg/skel" cniTypes "github.com/containernetworking/cni/pkg/types" @@ -161,7 +162,9 @@ func (plugin *netPlugin) GetAllEndpointState(networkid string) (*api.AzureCNISta } eps, err := plugin.nm.GetAllEndpoints(networkid) - if err != nil { + if err == store.ErrStoreEmpty { + log.Printf("failed to retrieve endpoint state with err %v", err) + } else if err != nil { return nil, err } diff --git a/network/manager.go b/network/manager.go index 39ed47fea7..0d777b1598 100644 --- a/network/manager.go +++ b/network/manager.go @@ -389,7 +389,7 @@ func (nm *networkManager) GetAllEndpoints(networkId string) (map[string]*Endpoin // Special case when CNS invokes CNI, but there is no state, but return gracefully if len(nm.ExternalInterfaces) == 0 { log.Printf("Network manager has no external interfaces, is the state file populated?") - return eps, nil + return eps, store.ErrStoreEmpty } nw, err := nm.getNetwork(networkId) diff --git a/store/json.go b/store/json.go index 00f92ad60b..87d867c29d 100644 --- a/store/json.go +++ b/store/json.go @@ -78,7 +78,7 @@ func (kvs *jsonFileStore) Read(key string, value interface{}) error { if len(b) == 0 { log.Printf("Unable to read file %s, was empty", kvs.fileName) - return nil + return ErrStoreEmpty } // Decode to raw JSON messages. diff --git a/store/store.go b/store/store.go index 019f51245b..eb2e44263f 100644 --- a/store/store.go +++ b/store/store.go @@ -26,6 +26,7 @@ var ( ErrKeyNotFound = fmt.Errorf("key not found") ErrStoreLocked = fmt.Errorf("store is already locked") ErrStoreNotLocked = fmt.Errorf("store is not locked") + ErrStoreEmpty = fmt.Errorf("store is empty") ErrTimeoutLockingStore = fmt.Errorf("timed out locking store") ErrNonBlockingLockIsAlreadyLocked = fmt.Errorf("attempted to perform non-blocking lock on an already locked store") ) From 9fff031bab5ffbb01bd187bb3b7d0ccfc96cb2c1 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 30 Jun 2021 22:56:11 +0000 Subject: [PATCH 3/5] restore --- cni/network/network_test.go | 8 ++++++++ ipam/manager.go | 3 +++ network/manager.go | 3 +++ 3 files changed, 14 insertions(+) diff --git a/cni/network/network_test.go b/cni/network/network_test.go index 8e09233baf..96054e7b58 100644 --- a/cni/network/network_test.go +++ b/cni/network/network_test.go @@ -130,3 +130,11 @@ func TestGetAllEndpointState(t *testing.T) { require.Exactly(t, res, state) } + +func TestEndpointsWithEmptyState(t *testing.T) { + plugin, _ := getTestResources() + networkid := "azure" + state, err := plugin.GetAllEndpointState(networkid) + require.NoError(t, err) + require.Equal(t, 0, len(state.ContainerInterfaces)) +} diff --git a/ipam/manager.go b/ipam/manager.go index 3fc932abaa..a823b30059 100644 --- a/ipam/manager.go +++ b/ipam/manager.go @@ -120,6 +120,9 @@ func (am *addressManager) restore() error { if err == store.ErrKeyNotFound { log.Printf("[ipam] store key not found") return nil + } else if err == store.ErrStoreEmpty { + log.Printf("[ipam] store empty") + return nil } else { log.Printf("[ipam] Failed to restore state, err:%v\n", err) return err diff --git a/network/manager.go b/network/manager.go index 0d777b1598..023f9c1b94 100644 --- a/network/manager.go +++ b/network/manager.go @@ -123,6 +123,9 @@ func (nm *networkManager) restore(isRehydrationRequired bool) error { log.Printf("[net] network store key not found") // Considered successful. return nil + } else if err == store.ErrStoreEmpty { + log.Printf("[net] network store empty") + return nil } else { log.Printf("[net] Failed to restore state, err:%v\n", err) return err From 80f5df7d742924000fc65036b0443dd95c2f3029 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 30 Jun 2021 18:28:46 -0500 Subject: [PATCH 4/5] fix: add custom unmarshaller for struct with embedded custom interface type --- cns/api.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/cns/api.go b/cns/api.go index 355784d502..6101690806 100644 --- a/cns/api.go +++ b/cns/api.go @@ -62,6 +62,45 @@ func (i IPConfigurationStatus) String() string { i.ID, i.NCID, i.IPAddress, i.State, i.PodInfo) } +// UnmarshalJSON is a custom unmarshaller for IPConfigurationStatus that +// is capable of unmarshalling to interface type `PodInfo` contained in the +// struct. Without this custom unmarshaller, the default unmarshaller can't +// deserialize the json data in to that interface type. +func (i *IPConfigurationStatus) UnmarshalJSON(b []byte) error { + m := map[string]json.RawMessage{} + if err := json.Unmarshal(b, &m); err != nil { + return err + } + if s, ok := m["NCID"]; ok { + if err := json.Unmarshal(s, &(i.NCID)); err != nil { + return err + } + } + if s, ok := m["ID"]; ok { + if err := json.Unmarshal(s, &(i.ID)); err != nil { + return err + } + } + if s, ok := m["IPAddress"]; ok { + if err := json.Unmarshal(s, &(i.IPAddress)); err != nil { + return err + } + } + if s, ok := m["State"]; ok { + if err := json.Unmarshal(s, &(i.State)); err != nil { + return err + } + } + if s, ok := m["PodInfo"]; ok { + pi, err := UnmarshalPodInfo(s) + if err != nil { + return err + } + i.PodInfo = pi + } + return nil +} + // SetEnvironmentRequest describes the Request to set the environment in CNS. type SetEnvironmentRequest struct { Location string From f524ddb59509573674c3380ebf2b32e47990e798 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 30 Jun 2021 23:41:20 +0000 Subject: [PATCH 5/5] mkdir images --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 481c98c410..e06b22a65a 100644 --- a/Makefile +++ b/Makefile @@ -279,6 +279,7 @@ tools: acncli .PHONY: tools-images tools-images: + mkdir -p $(IMAGE_DIR) docker build --no-cache -f ./tools/acncli/Dockerfile --build-arg VERSION=$(VERSION) -t $(AZURE_CNI_IMAGE):$(VERSION) . docker save $(AZURE_CNI_IMAGE):$(VERSION) | gzip -c > $(IMAGE_DIR)/$(CNI_IMAGE_ARCHIVE_NAME)