From 92e54f81224189dd02abc813e49343c0c8c123d2 Mon Sep 17 00:00:00 2001 From: Matthew Long <61910737+thatmattlong@users.noreply.github.com> Date: Thu, 11 Aug 2022 13:24:58 -0700 Subject: [PATCH 1/2] feat: compare node IP with NNCs node IP --- cns/configuration/env.go | 7 +++++ .../nodenetworkconfig/reconciler.go | 27 ++++++++++++++++++- cns/service/main.go | 13 ++++----- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/cns/configuration/env.go b/cns/configuration/env.go index 63f3b028a6..31eb1b4568 100644 --- a/cns/configuration/env.go +++ b/cns/configuration/env.go @@ -9,6 +9,8 @@ import ( const ( // EnvNodeName is the NODENAME env var string key. EnvNodeName = "NODENAME" + // EnvNodeIP is the IP of the node running this CNS binary + EnvNodeIP = "NODE_IP" ) // ErrNodeNameUnset indicates the the $EnvNodeName variable is unset in the environment. @@ -22,3 +24,8 @@ func NodeName() (string, error) { } return nodeName, nil } + +// NodeIP returns the value of the NODE_IP environment variable, or empty string if unset. +func NodeIP() string { + return os.Getenv(EnvNodeIP) +} diff --git a/cns/kubecontroller/nodenetworkconfig/reconciler.go b/cns/kubecontroller/nodenetworkconfig/reconciler.go index 575be479db..f6515d57a3 100644 --- a/cns/kubecontroller/nodenetworkconfig/reconciler.go +++ b/cns/kubecontroller/nodenetworkconfig/reconciler.go @@ -40,18 +40,20 @@ type Reconciler struct { nnccli nncGetter once sync.Once started chan interface{} + nodeIP string } // NewReconciler creates a NodeNetworkConfig Reconciler which will get updates from the Kubernetes // apiserver for NNC events. // Provided nncListeners are passed the NNC after the Reconcile preprocesses it. Note: order matters! The // passed Listeners are notified in the order provided. -func NewReconciler(cnscli cnsClient, nnccli nncGetter, ipampoolmonitorcli nodeNetworkConfigListener) *Reconciler { +func NewReconciler(cnscli cnsClient, nnccli nncGetter, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string) *Reconciler { return &Reconciler{ cnscli: cnscli, ipampoolmonitorcli: ipampoolmonitorcli, nnccli: nnccli, started: make(chan interface{}), + nodeIP: nodeIP, } } @@ -71,8 +73,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco logger.Printf("[cns-rc] CRD Spec: %+v", nnc.Spec) ipAssignments := 0 + + // keep a count of NCs we find that were created for this Node + foundNCForNodeCount := 0 + // for each NC, parse it in to a CreateNCRequest and forward it to the appropriate Listener for i := range nnc.Status.NetworkContainers { + // check if this NC matches the Node IP if we have one to check against + if r.nodeIP != "" { + if r.nodeIP != nnc.Status.NetworkContainers[i].NodeIP { + // skip this NC since it was created for a different node + logger.Debugf("[cns-rc] skipping network container %s found in NNC because node IP doesn't match, got %s, expected %s", + nnc.Status.NetworkContainers[i].ID, nnc.Status.NetworkContainers[i].NodeIP, r.nodeIP) + continue + } + } + + foundNCForNodeCount++ + var req *cns.CreateNetworkContainerRequest var err error switch nnc.Status.NetworkContainers[i].AssignmentMode { //nolint:exhaustive // skipping dynamic case @@ -98,6 +116,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } ipAssignments += len(req.SecondaryIPConfigs) } + + if foundNCForNodeCount == 0 { + logger.Debugf("[cns-rc] reconciler is not yet considered to be started since the NCs in the NNC were not created for this Node") + // return nil error since requeuing won't solve anything + return reconcile.Result{}, nil + } + // record assigned IPs metric allocatedIPs.Set(float64(ipAssignments)) diff --git a/cns/service/main.go b/cns/service/main.go index de596be5a3..2f5fb76a96 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -1080,8 +1080,11 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn return errors.Wrapf(err, "failed to get node %s", nodeName) } + // get CNS Node IP to compare NC Node IP with this Node IP to ensure NCs were created for this node + nodeIP := configuration.NodeIP() + // NodeNetworkConfig reconciler - nncReconciler := nncctrl.NewReconciler(httpRestServiceImplementation, nnccli, poolMonitor) + nncReconciler := nncctrl.NewReconciler(httpRestServiceImplementation, nnccli, poolMonitor, nodeIP) // pass Node to the Reconciler for Controller xref if err := nncReconciler.SetupWithManager(manager, node); err != nil { //nolint:govet // intentional shadow return errors.Wrapf(err, "failed to setup nnc reconciler with manager") @@ -1141,11 +1144,9 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn } }() logger.Printf("initialized NodeNetworkConfig reconciler") - // wait for up to 10m for the Reconciler to run once. - timedCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) //nolint:gomnd // default 10m - defer cancel() - if started := nncReconciler.Started(timedCtx); !started { - return errors.Errorf("timed out waiting for reconciler start") + // wait for the Reconciler to run once on a NNC that was made for this Node + if started := nncReconciler.Started(ctx); !started { + return errors.Errorf("context cancelled while waiting for reconciler start") } logger.Printf("started NodeNetworkConfig reconciler") From 07c766470b94a1aee5fa21b0bb1059e7d70e2311 Mon Sep 17 00:00:00 2001 From: Matthew Long <61910737+thatmattlong@users.noreply.github.com> Date: Tue, 16 Aug 2022 09:35:16 -0700 Subject: [PATCH 2/2] fix: don't block reconciler from starting if nnc doesn't have status --- .../nodenetworkconfig/conversion_test.go | 7 ++++- .../nodenetworkconfig/reconciler.go | 11 -------- .../nodenetworkconfig/reconciler_test.go | 27 ++++++++++++++++++- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cns/kubecontroller/nodenetworkconfig/conversion_test.go b/cns/kubecontroller/nodenetworkconfig/conversion_test.go index a21d5d7f5b..e64abf78f7 100644 --- a/cns/kubecontroller/nodenetworkconfig/conversion_test.go +++ b/cns/kubecontroller/nodenetworkconfig/conversion_test.go @@ -22,6 +22,7 @@ const ( subnetPrefixLen = 24 testSecIP = "10.0.0.2" version = 1 + nodeIP = "10.1.0.5" ) var invalidStatusMultiNC = v1alpha.NodeNetworkConfigStatus{ @@ -46,6 +47,7 @@ var validSwiftNC = v1alpha.NetworkContainer{ DefaultGateway: defaultGateway, SubnetAddressSpace: subnetAddressSpace, Version: version, + NodeIP: nodeIP, } var validSwiftStatus = v1alpha.NodeNetworkConfigStatus{ @@ -55,7 +57,8 @@ var validSwiftStatus = v1alpha.NodeNetworkConfigStatus{ } var validSwiftRequest = &cns.CreateNetworkContainerRequest{ - Version: strconv.FormatInt(version, 10), + HostPrimaryIP: nodeIP, + Version: strconv.FormatInt(version, 10), IPConfiguration: cns.IPConfiguration{ GatewayIPAddress: defaultGateway, IPSubnet: cns.IPSubnet{ @@ -78,6 +81,7 @@ var validOverlayNC = v1alpha.NetworkContainer{ AssignmentMode: v1alpha.Static, Type: v1alpha.Overlay, PrimaryIP: overlayPrimaryIP, + NodeIP: nodeIP, SubnetName: subnetName, SubnetAddressSpace: subnetAddressSpace, Version: version, @@ -162,6 +166,7 @@ func TestCreateNCRequestFromDynamicNC(t *testing.T) { input: v1alpha.NetworkContainer{ PrimaryIP: ipIsCIDR, ID: ncID, + NodeIP: nodeIP, IPAssignments: []v1alpha.IPAssignment{ { Name: uuid, diff --git a/cns/kubecontroller/nodenetworkconfig/reconciler.go b/cns/kubecontroller/nodenetworkconfig/reconciler.go index f6515d57a3..ec10694a2d 100644 --- a/cns/kubecontroller/nodenetworkconfig/reconciler.go +++ b/cns/kubecontroller/nodenetworkconfig/reconciler.go @@ -74,9 +74,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco ipAssignments := 0 - // keep a count of NCs we find that were created for this Node - foundNCForNodeCount := 0 - // for each NC, parse it in to a CreateNCRequest and forward it to the appropriate Listener for i := range nnc.Status.NetworkContainers { // check if this NC matches the Node IP if we have one to check against @@ -89,8 +86,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } } - foundNCForNodeCount++ - var req *cns.CreateNetworkContainerRequest var err error switch nnc.Status.NetworkContainers[i].AssignmentMode { //nolint:exhaustive // skipping dynamic case @@ -117,12 +112,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco ipAssignments += len(req.SecondaryIPConfigs) } - if foundNCForNodeCount == 0 { - logger.Debugf("[cns-rc] reconciler is not yet considered to be started since the NCs in the NNC were not created for this Node") - // return nil error since requeuing won't solve anything - return reconcile.Result{}, nil - } - // record assigned IPs metric allocatedIPs.Set(float64(ipAssignments)) diff --git a/cns/kubecontroller/nodenetworkconfig/reconciler_test.go b/cns/kubecontroller/nodenetworkconfig/reconciler_test.go index 82a9a76e4b..9c13a02c56 100644 --- a/cns/kubecontroller/nodenetworkconfig/reconciler_test.go +++ b/cns/kubecontroller/nodenetworkconfig/reconciler_test.go @@ -53,6 +53,7 @@ func TestReconcile(t *testing.T) { in reconcile.Request ncGetter mockNCGetter cnsClient mockCNSClient + nodeIP string want reconcile.Result wantCNSClientState cnsClientState wantErr bool @@ -145,11 +146,35 @@ func TestReconcile(t *testing.T) { }, }, }, + { + name: "node IP mismatch", + ncGetter: mockNCGetter{ + get: func(context.Context, types.NamespacedName) (*v1alpha.NodeNetworkConfig, error) { + return &v1alpha.NodeNetworkConfig{ + Status: validSwiftStatus, + Spec: v1alpha.NodeNetworkConfigSpec{ + RequestedIPCount: 1, + }, + }, nil + }, + }, + cnsClient: mockCNSClient{ + createOrUpdateNC: func(*cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { + return cnstypes.Success + }, + update: func(*v1alpha.NodeNetworkConfig) error { + return nil + }, + }, + nodeIP: "192.168.1.5", // nodeIP in above NNC status is 10.1.0.5 + wantErr: false, + wantCNSClientState: cnsClientState{}, // state should be empty since we should skip this NC + }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - r := NewReconciler(&tt.cnsClient, &tt.ncGetter, &tt.cnsClient) + r := NewReconciler(&tt.cnsClient, &tt.ncGetter, &tt.cnsClient, tt.nodeIP) got, err := r.Reconcile(context.Background(), tt.in) if tt.wantErr { require.Error(t, err)