Detect duplicate key values when diffing keyed arrays (#3320)

The Azure spec has "keyed arrays" which are essentially sets. This PR
resolves #3311 which showed that some keyed arrays actually have
duplicate key values, which defeats the purpose and is invalid (filed
Azure/azure-rest-api-specs#29288 upstream).

This PR adds detection of this degenerate case, upon which we abort the
keyed array diff and continue with a regular array diff.


Tested locally with the exact repro of #3311.

provider/pkg/provider/diff.go

@@ -598,6 +598,10 @@ func diffKeyedArrays(properties map[string]resources.AzureAPIProperty,
 		if !ok {
 			return nil, false
 		}
+		if _, ok := newSeen[hash]; ok {
+			logging.V(9).Infof("WARNING: diffKeyedArray: duplicate key values, treating as normal array\n")
+			return nil, false
+		}
 		newSeen[hash] = struct{}{}
 
 		oldItem, ok := oldIdValues[hash]
@@ -609,8 +613,6 @@ func diffKeyedArrays(properties map[string]resources.AzureAPIProperty,
 				sames[i] = newItem
 			} else if diff.Object != nil || diff.Array != nil {
 				updates[i] = *diff
-			} else { // diffs in primitives only
-
 			}
 		}
 	}

provider/pkg/provider/diff_test.go

@@ -789,25 +789,25 @@ func TestDiffKeyedArrays(t *testing.T) {
 	})
 
 	t.Run("recognizes invalid keyed array", func(t *testing.T) {
-		olds := [][]resource.PropertyValue{
-			{
-				// ok
+		olds := map[string][]resource.PropertyValue{
+			"not an object": {
 				{V: resource.PropertyMap{"p1": {V: "oldvalue"}}},
-				// not ok - primitive
 				{V: "oldvalue"},
 			},
-			{
-				// ok
+			"missing keys": {
 				{V: resource.PropertyMap{"p1": {V: "oldvalue"}}},
-				// not ok - missing key
 				{V: resource.PropertyMap{"foo": {V: "bar"}}},
 			},
+			"duplicate key value": {
+				{V: resource.PropertyMap{"p1": {V: "oldvalue"}}},
+				{V: resource.PropertyMap{"p1": {V: "oldvalue"}}},
+			},
 		}
 
-		for _, old := range olds {
+		for name, old := range olds {
 			diff, ok := diffKeyedArrays(map[string]resources.AzureAPIProperty{}, []string{"p1"}, old, old, "")
-			require.False(t, ok)
-			require.Nil(t, diff)
+			require.False(t, ok, name)
+			require.Nil(t, diff, name)
 		}
 	})
 }