Merge 054d5d7 into 488aba7

pkg/modelregistry/jsonvalues/convertJsonDs1_test.go

@@ -87,7 +87,7 @@ func Test_correctJsonPathRwValuesSubInterfaces(t *testing.T) {
 	sampleTree, err := ioutil.ReadFile("./testdata/sample-openconfig-configuration.json")
 	assert.NilError(t, err)
 
-	pathValues, err := DecomposeJSONWithPaths("", sampleTree, readOnlyPaths, readWritePaths)
+	pathValues, err := DecomposeJSONWithPaths("/interfaces/interface[name=eth1]", sampleTree, readOnlyPaths, readWritePaths)
 	assert.NilError(t, err)
 	assert.Equal(t, len(pathValues), 8)
 
@@ -128,7 +128,7 @@ func Test_correctJsonPathRwValuesSystemLogging(t *testing.T) {
 	// here in the intermediate jsonToValues format
 	sampleTree, err := ioutil.ReadFile("./testdata/sample-openconfig-double-index.json")
 	assert.NilError(t, err)
-	assert.Equal(t, 843, len(sampleTree))
+	assert.Equal(t, 851, len(sampleTree))
 
 	pathValues, err := DecomposeJSONWithPaths("", sampleTree, readOnlyPaths, readWritePaths)
 	assert.NilError(t, err)
@@ -139,12 +139,20 @@ func Test_correctJsonPathRwValuesSystemLogging(t *testing.T) {
 		switch pathValue.Path {
 		case
 			"/system/logging/remote-servers/remote-server[host=h1]/config/source-address",
-			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=1][severity=2]/config/severity",
-			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=1][severity=2]/config/facility",
-			"/system/logging/console/selectors/selector[facility=3][severity=4]/config/facility",
+			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=ALL][severity=2]/config/severity",
 			"/system/logging/console/selectors/selector[facility=3][severity=4]/config/severity":
 			assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_STRING, pathValue.Path)
 			assert.Equal(t, len(pathValue.GetValue().GetTypeOpts()), 0)
+		case
+			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=ALL][severity=2]/config/facility":
+			assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_STRING, pathValue.Path)
+			strValue := (*devicechange.TypedString)(pathValue.Value)
+			assert.Equal(t, "ALL", strValue.String(), "expected IdentityRef to be translated")
+		case
+			"/system/logging/console/selectors/selector[facility=3][severity=4]/config/facility":
+			assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_STRING, pathValue.Path)
+			strValue := (*devicechange.TypedString)(pathValue.Value)
+			assert.Equal(t, "LOCAL4", strValue.String(), "expected IdentityRef to be translated from '3' to LOCAL4")
 		default:
 			t.Fatal("Unexpected path", pathValue.Path)
 		}

pkg/modelregistry/jsonvalues/jsonToValues.go

@@ -49,7 +49,7 @@ func DecomposeJSONWithPaths(prefixPath string, genericJSON []byte, ropaths model
 	if err != nil {
 		return nil, err
 	}
-	values, err := extractValuesWithPaths(f, prefixPath, ropaths, rwpaths)
+	values, err := extractValuesWithPaths(f, removeIndexNames(prefixPath), ropaths, rwpaths)
 	if err != nil {
 		return nil, fmt.Errorf("error decomposing JSON %v", err)
 	}
@@ -246,6 +246,7 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg
 		enum = subPath.Enum
 	} else {
 		modeltype = pathElem.ValueType
+		enum = pathElem.Enum
 	}
 	var typedValue *devicechange.TypedValue
 	switch modeltype {
@@ -254,32 +255,22 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg
 		switch valueTyped := value.(type) {
 		case string:
 			if len(enum) > 0 {
-				// Compare values and convert from digit if necessary
-				for k, v := range enum {
-					if v == valueTyped {
-						stringVal = valueTyped
-						break
-					} else if fmt.Sprintf("%d", k) == valueTyped {
-						stringVal = v
-						break
-					}
-				}
-				if stringVal == "" {
-					enumOpts := make([]string, len(enum)*2)
-					i := 0
-					for k, v := range enum {
-						enumOpts[i*2] = fmt.Sprintf("%d", k)
-						enumOpts[i*2+1] = v
-						i++
-					}
-					return nil, fmt.Errorf("value %s for %s does not match any enumerated value %s",
-						valueTyped, parentPath, strings.Join(enumOpts, ";"))
+				stringVal, err = convertEnumIdx(valueTyped, enum, parentPath)
+				if err != nil {
+					return nil, err
 				}
 			} else {
 				stringVal = valueTyped
 			}
 		case float64:
-			stringVal = fmt.Sprintf("%g", value)
+			if len(enum) > 0 {
+				stringVal, err = convertEnumIdx(fmt.Sprintf("%g", valueTyped), enum, parentPath)
+				if err != nil {
+					return nil, err
+				}
+			} else {
+				stringVal = fmt.Sprintf("%g", valueTyped)
+			}
 		case bool:
 			stringVal = fmt.Sprintf("%v", value)
 		}
@@ -555,3 +546,44 @@ func replaceIndices(path string, ignoreAfter int, indices []indexValue) (string,
 
 	return fmt.Sprintf("%s%s", strings.Join(pathParts, bracketsq), ignored), nil
 }
+
+func convertEnumIdx(valueTyped string, enum map[int]string,
+	parentPath string) (string, error) {
+	var stringVal string
+	for k, v := range enum {
+		if v == valueTyped {
+			stringVal = valueTyped
+			break
+		} else if fmt.Sprintf("%d", k) == valueTyped {
+			stringVal = v
+			break
+		}
+	}
+	if stringVal == "" {
+		enumOpts := make([]string, len(enum)*2)
+		i := 0
+		for k, v := range enum {
+			enumOpts[i*2] = fmt.Sprintf("%d", k)
+			enumOpts[i*2+1] = v
+			i++
+		}
+		return "", fmt.Errorf("value %s for %s does not match any enumerated value %s",
+			valueTyped, parentPath, strings.Join(enumOpts, ";"))
+	}
+	return stringVal, nil
+}
+
+// for a path like
+// "/interfaces/interface[name=eth1]/subinterfaces/subinterface[index=120]/config/description",
+// Remove the "name=" and "index="
+func removeIndexNames(prefixPath string) string {
+	splitPath := strings.Split(prefixPath, equals)
+	for i, pathPart := range splitPath {
+		if i < len(splitPath)-1 {
+			lastBrktPos := strings.LastIndex(pathPart, bracketsq)
+			splitPath[i] = pathPart[:lastBrktPos] + "["
+		}
+	}
+
+	return strings.Join(splitPath, "")
+}

pkg/modelregistry/jsonvalues/jsonToValues_test.go

@@ -105,8 +105,8 @@ func Test_DecomposeTreeConfigOnly(t *testing.T) {
 		case
 			"/system/logging/remote-servers/remote-server[host=h2]/config/host",
 			"/system/logging/remote-servers/remote-server[host=h2]/config/source-address",
-			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=f1][severity=s1]/config/facility",
-			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=f1][severity=s1]/config/severity",
+			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=LOCAL3][severity=s1]/config/facility",
+			"/system/logging/remote-servers/remote-server[host=h1]/selectors/selector[facility=LOCAL3][severity=s1]/config/severity",
 			"/system/logging/remote-servers/remote-server[host=h1]/config/host",
 			"/system/logging/remote-servers/remote-server[host=h1]/config/source-address":
 			assert.Equal(t, devicechange.ValueType_STRING, v.GetValue().GetType(), v.Path)
@@ -224,3 +224,14 @@ func Test_replaceIndices2(t *testing.T) {
 	assert.NilError(t, err, "unexpected error replacing numbers")
 	assert.Equal(t, modelPathExpected, replaced, "unexpected value after replacing numbers")
 }
+
+func Test_removeIndexNames(t *testing.T) {
+	samplePath1 := "/interfaces"
+	samplePath1Remove := "/interfaces"
+	assert.Equal(t, samplePath1Remove, removeIndexNames(samplePath1))
+
+	samplePath2 := "/interfaces/interface[name=eth1]/subinterfaces/subinterface[index=120]/config/description"
+	samplePath2Remove := "/interfaces/interface[eth1]/subinterfaces/subinterface[120]/config/description"
+	assert.Equal(t, samplePath2Remove, removeIndexNames(samplePath2))
+
+}

pkg/modelregistry/jsonvalues/testdata/sample-openconfig-configuration.json

@@ -1,35 +1,28 @@
 {
-  "interfaces": {
-    "interface": [
+  "config": {
+    "description": "new if desc",
+    "mtu": 960
+  },
+  "hold-time": {
+    "config": {
+      "up": 10,
+      "down": 11
+    }
+  },
+  "subinterfaces": {
+    "subinterface": [
       {
-        "name": "eth1",
+        "index": 120,
         "config": {
-          "description": "new if desc",
-          "mtu": 960
-        },
-        "hold-time": {
-          "config": {
-            "up": 10,
-            "down": 11
-          }
-        },
-        "subinterfaces": {
-          "subinterface": [
-            {
-              "index": 120,
-              "config": {
-                "enabled": true,
-                "description": "SI 120"
-              }
-            },
-            {
-              "index": 121,
-              "config": {
-                "enabled": false,
-                "description": "SI 121"
-              }
-            }
-          ]
+          "enabled": true,
+          "description": "SI 120"
+        }
+      },
+      {
+        "index": 121,
+        "config": {
+          "enabled": false,
+          "description": "SI 121"
         }
       }
     ]

pkg/modelregistry/jsonvalues/testdata/sample-openconfig-double-index.json

@@ -11,10 +11,10 @@
             "selectors": {
               "selector": [
                 {
-                  "facility": 1,
+                  "facility": "ALL",
                   "severity": 2,
                   "config": {
-                    "facility": 1,
+                    "facility": "ALL",
                     "severity": 2
                   }
                 }

pkg/modelregistry/jsonvalues/testdata/sample-testdevice2-opstate.json

@@ -10,13 +10,13 @@
         "index1": 101,
         "index2": 102,
         "leaf3c": "mock Value in JSON",
-        "leaf3d": "IDTYPE1"
+        "leaf3d": "1"
       },
       {
         "index1": 101,
         "index2": 103,
         "leaf3c": "Second mock Value",
-        "leaf3d": "2"
+        "leaf3d": 2
       }
     ],
     "leaf2d": 10001,

pkg/modelregistry/jsonvalues/testdata/sample-tree-double-key.json

@@ -13,10 +13,10 @@
       "selectors": {
         "selector": [
           {
-            "facility": "f1",
+            "facility": "LOCAL3",
             "severity": "s1",
             "config": {
-              "facility": "f1",
+              "facility": "LOCAL3",
               "severity": "s1"
             }
           }

pkg/northbound/gnmi/set.go

@@ -343,49 +343,64 @@ func (s *Server) checkForReadOnly(target string, deviceType devicetype.Type, ver
 	modelreg := manager.GetManager().ModelRegistry
 
 	// This ignores versions - if it's RO in one version will be regarded
-	// as RO in all versions - very unlikely that model would change
+	// as RO in all versions - very unlikely that modelRoPaths would change
 	// YANG items from `config false` to `config true` across versions
-	model, ok := modelreg.
+	modelRoPaths, ok := modelreg.
 		ModelReadOnlyPaths[utils.ToModelName(deviceType, version)]
 	if !ok {
 		log.Warnf("Cannot check for Read Only paths for %s %s because "+
 			"Model Plugin not available - continuing", deviceType, version)
 		return nil
 	}
 
+	modelRwPaths, ok := modelreg.
+		ModelReadWritePaths[utils.ToModelName(deviceType, version)]
+	if !ok {
+		log.Warnf("Cannot check for Read Only paths for %s %s because "+
+			"Model Plugin not available - continuing", deviceType, version)
+		return nil
+	}
+
 	// Now iterate through the consolidated set of targets and see if any are read-only paths
 	for path := range targetUpdates { // map - just need the key
-		if err := compareRoPaths(path, model); err != nil {
+		if err := compareRoPaths(path, modelRoPaths, modelRwPaths); err != nil {
 			return fmt.Errorf("update %s", err)
 		}
 	}
 
 	// Now iterate through the consolidated set of targets and see if any are read-only paths
 	for _, path := range targetRemoves { // map - just need the key
-		if err := compareRoPaths(path, model); err != nil {
+		if err := compareRoPaths(path, modelRoPaths, modelRwPaths); err != nil {
 			return fmt.Errorf("remove %s", err)
 		}
 	}
 
 	return nil
 }
 
-func compareRoPaths(path string, model modelregistry.ReadOnlyPathMap) error {
+func compareRoPaths(path string, modelRoPaths modelregistry.ReadOnlyPathMap, modelRwPaths modelregistry.ReadWritePathMap) error {
 	log.Infof("Testing %s for read only", path)
-	for ropath, subpaths := range model {
+	for ropath, subpaths := range modelRoPaths {
 		// Search through for list indices and replace with generic
-		modelPath := modelregistry.RemovePathIndices(path)
+		modelPathNiIdx := modelregistry.RemovePathIndices(path)
 		ropathNoIdx := modelregistry.RemovePathIndices(ropath)
-		if strings.HasPrefix(modelPath, ropathNoIdx) {
+		if strings.HasPrefix(modelPathNiIdx, ropathNoIdx) {
 			for s := range subpaths {
 				fullpath := ropathNoIdx
 				if s != "/" {
 					fullpath = fmt.Sprintf("%s%s", ropathNoIdx, s)
 				}
-				if fullpath == modelPath {
+				if fullpath == modelPathNiIdx {
+					// Check that this is not one of those in both config and state (e.g. index of a list)
+					for rwpath := range modelRwPaths {
+						rwpathNoIdx := modelregistry.RemovePathIndices(rwpath)
+						if rwpathNoIdx == modelPathNiIdx {
+							return nil
+						}
+					}
 					return fmt.Errorf("contains a change to a "+
 						"read only path %s. Rejected. %s, %s, %s, %s, %s",
-						path, modelPath, ropath, ropathNoIdx, s, fullpath)
+						path, modelPathNiIdx, ropath, ropathNoIdx, s, fullpath)
 				}
 			}
 		}

pkg/northbound/gnmi/set_test.go

@@ -521,7 +521,7 @@ func TestSet_checkForReadOnly(t *testing.T) {
 	err := server.checkForReadOnly("device-1", "TestDevice", "1.0.0", updateT1, make([]string, 0))
 	assert.NilError(t, err, "unexpected error on T1")
 	err = server.checkForReadOnly("device-2", "TestDevice", "1.0.0", updateT2, make([]string, 0))
-	assert.Error(t, err, `update contains a change to a read only path /cont1a/cont2a/leaf2c. Rejected. /cont1a/cont2a/leaf2c, /cont1a/cont2a/leaf2c, /cont1a/cont2a/leaf2c, /, /cont1a/cont2a/leaf2c`)
+	assert.NilError(t, err)
 }
 
 // Tests giving a new device without specifying a type