Finished validating NetworkIds part, and added some checks related to…

… the received patches. It has proven to be nigh impossible to validate the exact content of patches, so I only check the expected number. Could be enhanced in the future. Found one bug, where HostDevices would be overwritten with an empty map in certain empty cases.
nokia · Jul 16, 2019 · c6e1e89 · c6e1e89
1 parent 3e22fbd
commit c6e1e89
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 49 deletions.
diff --git a/pkg/admit/confadmit.go b/pkg/admit/confadmit.go
@@ -43,11 +43,7 @@ func (validator *Validator) ValidateTenantConfig(responseWriter http.ResponseWri
     SendErroneousAdmissionResponse(responseWriter, admissionReview.Request, err)
     return
   }
-  err = mutateConfigManifest(newManifest)
-  if err != nil {
-    SendErroneousAdmissionResponse(responseWriter, admissionReview.Request, err)
-    return
-  }
+  mutateConfigManifest(newManifest)
   responseAdmissionReview := v1beta1.AdmissionReview {
     Response: CreateReviewResponseFromPatches(createPatchListFromConfigChanges(origNewManifest,newManifest)),
   }
@@ -88,7 +84,7 @@ func validateConfig(oldManifest, newManifest *danmtypes.TenantConfig, opType v1b
 func createPatchListFromConfigChanges(origConfig danmtypes.TenantConfig, changedConfig *danmtypes.TenantConfig) []Patch {
   patchList := make([]Patch, 0)
   var hostDevicesPatch string
-  if !reflect.DeepEqual(origConfig.HostDevices, changedConfig.HostDevices) {
+  if !reflect.DeepEqual(origConfig.HostDevices, changedConfig.HostDevices) && len(origConfig.HostDevices) != 0 && len(changedConfig.HostDevices) != 0 {
     hostDevicesPatch = `[`
     for ifaceIndex, ifaceConf := range changedConfig.HostDevices {
       if ifaceIndex != 0 {
@@ -105,17 +101,14 @@ func createPatchListFromConfigChanges(origConfig danmtypes.TenantConfig, changed
   return patchList
 }
 
-func mutateConfigManifest(tconf *danmtypes.TenantConfig) error {
+func mutateConfigManifest(tconf *danmtypes.TenantConfig) {
   for ifaceIndex, ifaceConf := range tconf.HostDevices {
     //We don't want to either re-init existing allocations, or unnecessarily create arrays for non-virtual networks
     if ifaceConf.Alloc != "" || ifaceConf.VniType == "" {
       continue
     }
-    bitArray, err := bitarray.NewBitArray(MaxAllowedVni)
-    if err != nil {
-      return err
-    }
+    bitArray, _ := bitarray.NewBitArray(MaxAllowedVni)
     tconf.HostDevices[ifaceIndex].Alloc = bitArray.Encode()
   }
-  return nil
+  return
 }
diff --git a/pkg/admit/validators.go b/pkg/admit/validators.go
@@ -107,7 +107,7 @@ func validateNetworkId(oldManifest, newManifest *danmtypes.DanmNet, opType admis
     return errors.New("Spec.NetworkID mandatory parameter is missing!")
   }
   if len(newManifest.Spec.NetworkID) > MaxNidLength && IsTypeDynamic(newManifest.Spec.NetworkType) {
-    return errors.New("Spec.NetworkID cannot be longer than 12 characters (otherwise VLAN and VxLAN host interface creation might fail)!")
+    return errors.New("Spec.NetworkID cannot be longer than " + strconv.Itoa(MaxNidLength) + " characters (otherwise VLAN and VxLAN host interface creation might fail)!")
   }
   return nil
 }
@@ -151,7 +151,7 @@ func validateTenantconfig(oldManifest, newManifest *danmtypes.TenantConfig, opTy
       return errors.New("neither NetworkID, nor NetworkType can be empty in a NetworkID mapping!")
     }
     if len(nId) > MaxNidLength && IsTypeDynamic(nType) {
-      return errors.New("NetworkID:" + nId + " cannot be longer than 12 characters (otherwise VLAN and VxLAN host interface creation might fail)!")
+      return errors.New("NetworkID:" + nId + " cannot be longer than " + strconv.Itoa(MaxNidLength) + " characters (otherwise VLAN and VxLAN host interface creation might fail)!")
     }
   }
   return nil

diff --git a/test/stubs/http/responsewriter_stub.go b/test/stubs/http/responsewriter_stub.go
@@ -1,12 +1,9 @@
 package http
 
 import (
-//"log"
-//"fmt"
-"bytes"
+  "bytes"
   "errors"
   "io/ioutil"
-//  "encoding/json"
   "net/http"
   "k8s.io/api/admission/v1beta1"
   "k8s.io/apimachinery/pkg/runtime"
@@ -29,7 +26,6 @@ func (writer *ResponseWriterStub) Header() http.Header {
 }
 
 func (writer *ResponseWriterStub) Write(response []byte) (int, error) {
-//  log.Println("write was here, with the object:" + string(response))
   writer.Response = response
   return 200, nil
 }
@@ -42,9 +38,7 @@ func (writer *ResponseWriterStub) GetAdmissionResponse() (*v1beta1.AdmissionResp
   if writer.Response == nil {
     return nil, errors.New("no response was sent")
   }
-//  log.Println("get was here, the saved object is:" + string(writer.Response))
   review := v1beta1.AdmissionReview{}
-//  err := json.Unmarshal(writer.Response, &response)
   reader := bytes.NewReader(writer.Response)
   readCloser := ioutil.NopCloser(reader)
   payload, err := ioutil.ReadAll(readCloser)
@@ -54,6 +48,5 @@ func (writer *ResponseWriterStub) GetAdmissionResponse() (*v1beta1.AdmissionResp
   codecs := serializer.NewCodecFactory(runtime.NewScheme())
   deserializer := codecs.UniversalDeserializer()
   _, _, err = deserializer.Decode(payload, nil, &review)
-//  fmt.Printf("after unmarshal the saved object is: %+v\n", review)
   return review.Response, err
 }
diff --git a/test/utils/utils.go b/test/utils/utils.go
@@ -75,4 +75,3 @@ func GetTconf(tconfName string, tconfSet []danmtypes.TenantConfig) *danmtypes.Te
   }
   return nil
 }
-
diff --git a/test/uts/admit_tests/confadmit_test.go b/test/uts/admit_tests/confadmit_test.go
@@ -86,21 +86,64 @@ var (
        },
     },
     danmtypes.TenantConfig {
-      ObjectMeta: meta_v1.ObjectMeta {Name: "manual-alloc"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      ObjectMeta: meta_v1.ObjectMeta {Name: "manual-alloc-old"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
       HostDevices: []danmtypes.IfaceProfile {
         danmtypes.IfaceProfile{Name: "ens4", VniType: "vxlan", VniRange: "700-710", Alloc: utils.AllocFor5k},
-        danmtypes.IfaceProfile{Name: "ens5", VniType: "vxlan", VniRange: "700-710"},
        },
     },
     danmtypes.TenantConfig {
-      ObjectMeta: meta_v1.ObjectMeta {Name: "tconf"},
+      ObjectMeta: meta_v1.ObjectMeta {Name: "manual-alloc"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
       HostDevices: []danmtypes.IfaceProfile {
         danmtypes.IfaceProfile{Name: "ens4", VniType: "vxlan", VniRange: "700-710", Alloc: utils.AllocFor5k},
-        danmtypes.IfaceProfile{Name: "ens4", VniType: "vlan", VniRange: "200,500-510", Alloc: utils.AllocFor5k},
-        danmtypes.IfaceProfile{Name: "ens6", VniType: "vxlan", VniRange: "1200-1300", Alloc: utils.AllocFor5k},
-        danmtypes.IfaceProfile{Name: "nokia.k8s.io/sriov_ens1f0", VniType: "vlan", VniRange: "1500-1550", Alloc: utils.AllocFor5k},
-        danmtypes.IfaceProfile{Name: "nokia.k8s.io/sriov_ens1f0", VniType: "vxlan", VniRange: "1600-1650", Alloc: utils.AllocFor5k},
-      },
+        danmtypes.IfaceProfile{Name: "nokia.k8s.io/sriov_ens1f0", VniType: "vlan", VniRange: "700-710"},
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "nonetype"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      NetworkIds: map[string]string {
+        "": "asd",
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "nonid"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      NetworkIds: map[string]string {
+        "flannel": "",
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "longnid"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      NetworkIds: map[string]string {
+        "flannel": "abcdefghijkl",
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "longnid-sriov"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      NetworkIds: map[string]string {
+        "flannel": "abcdefghijkl",
+        "sriov": "abcdefghijkl",
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "shortnid"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      NetworkIds: map[string]string {
+        "flannel": "abcdefghijkl",
+        "sriov": "abcdefghijk",
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "old-iface"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      HostDevices: []danmtypes.IfaceProfile {
+        danmtypes.IfaceProfile{Name: "ens4", VniType: "vxlan", VniRange: "900-4999,5000", Alloc: utils.AllocFor5k},
+       },
+    },
+    danmtypes.TenantConfig {
+      ObjectMeta: meta_v1.ObjectMeta {Name: "new-iface"},TypeMeta: meta_v1.TypeMeta {Kind: "TenantConfig"},
+      HostDevices: []danmtypes.IfaceProfile {
+        danmtypes.IfaceProfile{Name: "ens4", VniType: "vxlan", VniRange: "900-4999,5000", Alloc: utils.AllocFor5k},
+       },
+      NetworkIds: map[string]string {
+        "flannel": "flannel",
+       },
     },
   }
 )
@@ -111,21 +154,28 @@ var validateTconfTcs = []struct {
   newTconfName string
   opType v1beta1.Operation
   isErrorExpected bool
+  expectedPatches string
 }{
-  {"emptyRequest", "", "", "", true},
-  {"malformedOldObject", "malformed", "", "", true},
-  {"malformedNewObject", "", "malformed", "", true},
-  {"objectWithInvalidType", "", "invalid-type", "", true},
-  {"emptyCofig", "", "empty-config", "", true},
-  {"interfaceProfileWithoutName", "", "noname", "", true},
-  {"interfaceProfileWithoutVniRange", "", "norange", "", true},
-  {"interfaceProfileWithoutVniType", "", "notype", "", true},
-  {"interfaceProfileWithInvalidVniType", "", "invalid-vni-type", "", true},
-  {"interfaceProfileWithInvalidVniValue", "", "invalid-vni-value", "", true},
-  {"interfaceProfileWithInvalidVniRange", "", "invalid-vni-range", "", true},
-  {"interfaceProfileWithValidVniRange", "", "valid-vni-range", "", false},
-  {"interfaceProfileWithSetAlloc", "", "manual-alloc", v1beta1.Create, true},
-  {"interfaceProfileChangeWithAlloc", "", "manual-alloc", v1beta1.Update, false},
+  {"emptyRequest", "", "", "", true, "empty"},
+  {"malformedOldObject", "malformed", "", "", true, "empty"},
+  {"malformedNewObject", "", "malformed", "", true, "empty"},
+  {"objectWithInvalidType", "", "invalid-type", "", true, "empty"},
+  {"emptyCofig", "", "empty-config", "", true, "empty"},
+  {"interfaceProfileWithoutName", "", "noname", "", true, "empty"},
+  {"interfaceProfileWithoutVniRange", "", "norange", "", true, "empty"},
+  {"interfaceProfileWithoutVniType", "", "notype", "", true, "empty"},
+  {"interfaceProfileWithInvalidVniType", "", "invalid-vni-type", "", true, "empty"},
+  {"interfaceProfileWithInvalidVniValue", "", "invalid-vni-value", "", true, "empty"},
+  {"interfaceProfileWithInvalidVniRange", "", "invalid-vni-range", "", true, "empty"},
+  {"interfaceProfileWithValidVniRange", "", "valid-vni-range", "", false, "1"},
+  {"interfaceProfileWithSetAlloc", "", "manual-alloc", v1beta1.Create, true, "empty"},
+  {"interfaceProfileChangeWithAlloc", "manual-alloc-old", "manual-alloc", v1beta1.Update, false, "1"},
+  {"networkIdWithoutKey", "", "nonid", "", true, "empty"},
+  {"networkIdWithoutValue", "", "nonetype", "", true, "empty"},
+  {"longNidWithStaticNeType", "", "longnid", "", false, "empty"},
+  {"longNidWithDynamicNeType", "", "longnid-sriov", "", true, "empty"},
+  {"okayNids", "", "shortnid", "", false, "empty"},
+  {"noChangeInIfaces", "old-iface", "new-iface", v1beta1.Update, false, "empty"},
 }
 
 func TestValidateTenantConfig(t *testing.T) {
@@ -141,7 +191,7 @@ func TestValidateTenantConfig(t *testing.T) {
         return
       }
       validator.ValidateTenantConfig(writerStub, request)
-      err = validateHttpResponse(writerStub, tc.isErrorExpected)
+      err = validateHttpResponse(writerStub, tc.isErrorExpected, tc.expectedPatches)
       if err != nil {
         t.Errorf("Received HTTP Response did not match expectation, because:%v", err)
         return
@@ -186,7 +236,7 @@ func canItMalform(config *danmtypes.TenantConfig) []byte {
   return bytes
 }
 
-func validateHttpResponse(writer *httpstub.ResponseWriterStub, isErrorExpected bool) error {
+func validateHttpResponse(writer *httpstub.ResponseWriterStub, isErrorExpected bool, expectedPatches string) error {
   if writer.RespHeader.Get("Content-Type") != "application/json" {
     return errors.New("Content-Type is not set to application/json in the HTTP Header")
   }
@@ -198,7 +248,6 @@ func validateHttpResponse(writer *httpstub.ResponseWriterStub, isErrorExpected b
     if response.Allowed {
       return errors.New("request would have been admitted but we expected an error")
     }
-//    fmt.Printf("lofasz: %+v\n", response)
     if response.Result.Message == "" {
       return errors.New("a faulty response was sent without explanation")
     }
@@ -210,5 +259,26 @@ func validateHttpResponse(writer *httpstub.ResponseWriterStub, isErrorExpected b
       return errors.New("an unnecessary Result message is put into a successful response")
     }
   }
+  if expectedPatches != "" {
+     return validatePatches(response, expectedPatches)
+  }
   return nil
-}
+}
+
+func validatePatches(response *v1beta1.AdmissionResponse, expectedPatches string) error {
+  if expectedPatches == "empty" {
+    if response.Patch != nil {
+      return errors.New("did not expect any patches but some were included in the admission response")
+    }
+    return nil
+  }
+  var patches []admit.Patch
+  err := json.Unmarshal(response.Patch, &patches)
+  if err != nil {
+    return err
+  }
+  if len(patches) != 1 {
+    return errors.New("received number of patches was not the expected 1")
+  }
+  return nil
+}
Original file line number	Diff line number	Diff line change
Expand Up		@@ -75,4 +75,3 @@ func GetTconf(tconfName string, tconfSet []danmtypes.TenantConfig) *danmtypes.Te
		}
		return nil
		}