fpga_webhook: never reject already mutated CRs

rojkov · rojkov · commit 2cb914225c45 · 2022-03-29T11:01:49.000+03:00
For some reason the API server may want to pass an already mutated CR
through the webhook once again. The webhook must accept such CR with
no additional transformations.

This patch adds support for such idempotence by maintaining
a set of identity mappings which effectively resolve to themselves. No
patching is applied to them.
diff --git a/pkg/fpgacontroller/patcher/patcher.go b/pkg/fpgacontroller/patcher/patcher.go
@@ -80,6 +80,11 @@ type Patcher struct {
 	afMap           map[string]*fpgav2.AcceleratorFunction
 	resourceMap     map[string]string
 	resourceModeMap map[string]string
+
+	// This set is needed to maintain the webhook's idempotence: it must be possible to
+	// resolve actual resources (AFs and regions) to themselves. For this we build
+	// a set of identities which must be accepted by the webhook without any transformation.
+	identitySet map[string]int
 }
 
 func newPatcher(log logr.Logger) *Patcher {
@@ -88,6 +93,21 @@ func newPatcher(log logr.Logger) *Patcher {
 		afMap:           make(map[string]*fpgav2.AcceleratorFunction),
 		resourceMap:     make(map[string]string),
 		resourceModeMap: make(map[string]string),
+		identitySet:     make(map[string]int),
+	}
+}
+
+func (p *Patcher) incIdentity(id string) {
+	// Initialize to 1 or increment by 1.
+	p.identitySet[id] = p.identitySet[id] + 1
+}
+
+func (p *Patcher) decIdentity(id string) {
+	counter := p.identitySet[id]
+	if counter > 1 {
+		p.identitySet[id] = counter - 1
+	} else {
+		delete(p.identitySet, id)
 	}
 }
 
@@ -103,9 +123,13 @@ func (p *Patcher) AddAf(accfunc *fpgav2.AcceleratorFunction) error {
 			return err
 		}
 
-		p.resourceMap[namespace+"/"+accfunc.Name] = rfc6901Escaper.Replace(namespace + "/" + devtype)
+		mapping := rfc6901Escaper.Replace(namespace + "/" + devtype)
+		p.resourceMap[namespace+"/"+accfunc.Name] = mapping
+		p.incIdentity(mapping)
 	} else {
-		p.resourceMap[namespace+"/"+accfunc.Name] = rfc6901Escaper.Replace(namespace + "/region-" + accfunc.Spec.InterfaceID)
+		mapping := rfc6901Escaper.Replace(namespace + "/region-" + accfunc.Spec.InterfaceID)
+		p.resourceMap[namespace+"/"+accfunc.Name] = mapping
+		p.incIdentity(mapping)
 	}
 
 	p.resourceModeMap[namespace+"/"+accfunc.Name] = accfunc.Spec.Mode
@@ -118,24 +142,32 @@ func (p *Patcher) AddRegion(region *fpgav2.FpgaRegion) {
 	p.Lock()
 
 	p.resourceModeMap[namespace+"/"+region.Name] = regiondevel
-	p.resourceMap[namespace+"/"+region.Name] = rfc6901Escaper.Replace(namespace + "/region-" + region.Spec.InterfaceID)
+	mapping := rfc6901Escaper.Replace(namespace + "/region-" + region.Spec.InterfaceID)
+	p.resourceMap[namespace+"/"+region.Name] = mapping
+	p.incIdentity(mapping)
 }
 
 func (p *Patcher) RemoveAf(name string) {
 	defer p.Unlock()
 	p.Lock()
 
-	delete(p.afMap, namespace+"/"+name)
-	delete(p.resourceMap, namespace+"/"+name)
-	delete(p.resourceModeMap, namespace+"/"+name)
+	nname := namespace + "/" + name
+
+	p.decIdentity(p.resourceMap[nname])
+	delete(p.afMap, nname)
+	delete(p.resourceMap, nname)
+	delete(p.resourceModeMap, nname)
 }
 
 func (p *Patcher) RemoveRegion(name string) {
 	defer p.Unlock()
 	p.Lock()
 
-	delete(p.resourceMap, namespace+"/"+name)
-	delete(p.resourceModeMap, namespace+"/"+name)
+	nname := namespace + "/" + name
+
+	p.decIdentity(p.resourceMap[nname])
+	delete(p.resourceMap, nname)
+	delete(p.resourceModeMap, nname)
 }
 
 // sanitizeContainer filters out env variables reserved for CRI hook.
@@ -160,6 +192,16 @@ func sanitizeContainer(container corev1.Container) corev1.Container {
 	return container
 }
 
+func (p *Patcher) getNoopsOrError(name string) ([]string, error) {
+	if _, isVirtual := p.identitySet[rfc6901Escaper.Replace(name)]; isVirtual {
+		// `name` is not a real mapping, but a virtual one for an actual resource which
+		// needs to be resolved to itself with no transformations.
+		return []string{}, nil
+	}
+
+	return nil, errors.Errorf("no such resource: %q", name)
+}
+
 func (p *Patcher) getPatchOps(containerIdx int, container corev1.Container) ([]string, error) {
 	container = sanitizeContainer(container)
 
@@ -180,7 +222,7 @@ func (p *Patcher) getPatchOps(containerIdx int, container corev1.Container) ([]s
 	for rname, quantity := range requestedResources {
 		mode, found := p.resourceModeMap[rname]
 		if !found {
-			return nil, errors.Errorf("no such resource: %q", rname)
+			return p.getNoopsOrError(rname)
 		}
 
 		switch mode {
diff --git a/pkg/fpgacontroller/patcher/patcher_test.go b/pkg/fpgacontroller/patcher/patcher_test.go
@@ -31,13 +31,27 @@ func init() {
 	_ = flag.Set("v", "4")
 }
 
+func checkExpectedError(t *testing.T, expectedErr bool, err error, testName string) {
+	t.Helper()
+
+	if expectedErr && err == nil {
+		t.Errorf("Test case '%s': no error returned", testName)
+	}
+
+	if !expectedErr && err != nil {
+		t.Errorf("Test case '%s': unexpected error: %+v", testName, err)
+	}
+}
+
 func TestPatcherStorageFunctions(t *testing.T) {
 	goodAf := &fpgav2.AcceleratorFunction{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "arria10-nlb0",
 		},
 		Spec: fpgav2.AcceleratorFunctionSpec{
-			AfuID: "d8424dc4a4a3c413f89e433683f9040b",
+			AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+			InterfaceID: "ce48969398f05f33946d560708be108a",
+			Mode:        af,
 		},
 	}
 	brokenAf := &fpgav2.AcceleratorFunction{
@@ -58,37 +72,103 @@ func TestPatcherStorageFunctions(t *testing.T) {
 			InterfaceID: "ce48969398f05f33946d560708be108a",
 		},
 	}
-
-	p := newPatcher(ctrl.Log.WithName("test"))
-
-	if err := p.AddAf(goodAf); err != nil {
-		t.Error("unexpected error")
-	}
-
-	if len(p.resourceModeMap) != 1 || len(p.afMap) != 1 || len(p.resourceMap) != 1 {
-		t.Error("Failed to add AF to patcher")
-	}
-
-	if err := p.AddAf(brokenAf); err == nil {
-		t.Error("AddAf() must fail")
-	}
-
-	p.RemoveAf(goodAf.Name)
-
-	if len(p.resourceModeMap) != 0 || len(p.afMap) != 0 || len(p.resourceMap) != 0 {
-		t.Error("Failed to remove AF from patcher")
+	regionAlias := &fpgav2.FpgaRegion{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "arria10-alias",
+		},
+		Spec: fpgav2.FpgaRegionSpec{
+			InterfaceID: "ce48969398f05f33946d560708be108a",
+		},
 	}
 
-	p.AddRegion(region)
-
-	if len(p.resourceModeMap) != 1 || len(p.resourceMap) != 1 {
-		t.Error("Failed to add fpga region to patcher")
+	tcases := []struct {
+		name                        string
+		afsToAdd                    []*fpgav2.AcceleratorFunction
+		afsToRemove                 []*fpgav2.AcceleratorFunction
+		regionsToAdd                []*fpgav2.FpgaRegion
+		regionsToRemove             []*fpgav2.FpgaRegion
+		expectedResourceModeMapSize int
+		expectedResourceMapSize     int
+		expectedIdentitySetSize     int
+		expectedErr                 bool
+	}{
+		{
+			name:                        "Add one good af",
+			afsToAdd:                    []*fpgav2.AcceleratorFunction{goodAf},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:        "Add one broken af",
+			afsToAdd:    []*fpgav2.AcceleratorFunction{brokenAf},
+			expectedErr: true,
+		},
+		{
+			name:        "Add one and remove one good af",
+			afsToAdd:    []*fpgav2.AcceleratorFunction{goodAf},
+			afsToRemove: []*fpgav2.AcceleratorFunction{goodAf},
+		},
+		{
+			name:                        "Add one good region",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:            "Add one and remove one good region",
+			regionsToAdd:    []*fpgav2.FpgaRegion{region},
+			regionsToRemove: []*fpgav2.FpgaRegion{region},
+		},
+		{
+			name:                        "Add one region and one alias then remove one alias",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region, regionAlias},
+			regionsToRemove:             []*fpgav2.FpgaRegion{region},
+			expectedResourceModeMapSize: 1,
+			expectedResourceMapSize:     1,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:                        "Add one region and one alias",
+			regionsToAdd:                []*fpgav2.FpgaRegion{region, regionAlias},
+			expectedResourceModeMapSize: 2,
+			expectedResourceMapSize:     2,
+			expectedIdentitySetSize:     1,
+		},
+		{
+			name:            "Add one region and one alias then remove all",
+			regionsToAdd:    []*fpgav2.FpgaRegion{region, regionAlias},
+			regionsToRemove: []*fpgav2.FpgaRegion{region, regionAlias},
+		},
 	}
 
-	p.RemoveRegion(region.Name)
-
-	if len(p.resourceModeMap) != 0 || len(p.resourceMap) != 0 {
-		t.Error("Failed to remove fpga region from patcher")
+	for _, tt := range tcases {
+		t.Run(tt.name, func(t *testing.T) {
+			p := newPatcher(ctrl.Log.WithName("test"))
+			for _, af := range tt.afsToAdd {
+				err := p.AddAf(af)
+				checkExpectedError(t, tt.expectedErr, err, tt.name)
+			}
+			for _, reg := range tt.regionsToAdd {
+				p.AddRegion(reg)
+			}
+			for _, af := range tt.afsToRemove {
+				p.RemoveAf(af.Name)
+			}
+			for _, reg := range tt.regionsToRemove {
+				p.RemoveRegion(reg.Name)
+			}
+			if tt.expectedResourceModeMapSize != len(p.resourceModeMap) {
+				t.Errorf("wrong size of resourceModeMap. Expected %d, but got %d", tt.expectedResourceModeMapSize, len(p.resourceModeMap))
+			}
+			if tt.expectedResourceMapSize != len(p.resourceMap) {
+				t.Errorf("wrong size of resourceMap. Expected %d, but got %d", tt.expectedResourceMapSize, len(p.resourceMap))
+			}
+			if tt.expectedIdentitySetSize != len(p.identitySet) {
+				t.Errorf("wrong size of identitySet. Expected %d, but got %d", tt.expectedIdentitySetSize, len(p.identitySet))
+			}
+		})
 	}
 }
 
@@ -245,6 +325,41 @@ func TestGetPatchOps(t *testing.T) {
 			},
 			expectedOps: 4,
 		},
+		{
+			name: "Skip handling for an identity mapping",
+			container: corev1.Container{
+				Resources: corev1.ResourceRequirements{
+					Limits: corev1.ResourceList{
+						"fpga.intel.com/af-ce4.d84.zkiWk5jwXzOUbVYHCL4QithCTcSko8QT-J5DNoP5BAs": resource.MustParse("1"),
+					},
+					Requests: corev1.ResourceList{
+						"fpga.intel.com/af-ce4.d84.zkiWk5jwXzOUbVYHCL4QithCTcSko8QT-J5DNoP5BAs": resource.MustParse("1"),
+					},
+				},
+			},
+			afs: []*fpgav2.AcceleratorFunction{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "arria10-nlb0",
+					},
+					Spec: fpgav2.AcceleratorFunctionSpec{
+						AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+						InterfaceID: "ce48969398f05f33946d560708be108a",
+						Mode:        af,
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "arria10-nlb0-alias",
+					},
+					Spec: fpgav2.AcceleratorFunctionSpec{
+						AfuID:       "d8424dc4a4a3c413f89e433683f9040b",
+						InterfaceID: "ce48969398f05f33946d560708be108a",
+						Mode:        af,
+					},
+				},
+			},
+		},
 		{
 			name: "Successful handling for regiondevel mode",
 			container: corev1.Container{
@@ -399,12 +514,7 @@ func TestGetPatchOps(t *testing.T) {
 				p.AddRegion(region)
 			}
 			ops, err := p.getPatchOps(0, tt.container)
-			if tt.expectedErr && err == nil {
-				t.Errorf("Test case '%s': no error returned", tt.name)
-			}
-			if !tt.expectedErr && err != nil {
-				t.Errorf("Test case '%s': unexpected error: %+v", tt.name, err)
-			}
+			checkExpectedError(t, tt.expectedErr, err, tt.name)
 			if len(ops) != tt.expectedOps {
 				t.Errorf("test case '%s': expected %d ops, but got %d\n%v", tt.name, tt.expectedOps, len(ops), ops)
 			}
diff --git a/pkg/internal/containers/containers.go b/pkg/internal/containers/containers.go
@@ -27,7 +27,7 @@ func GetRequestedResources(container corev1.Container, ns string) (map[string]in
 	// Container may happen to have Requests, but not Limits. Check Requests first,
 	// then in the next loop iterate over Limits.
 	for resourceName, resourceQuantity := range container.Resources.Requests {
-		rname := strings.ToLower(string(resourceName))
+		rname := string(resourceName)
 		if !strings.HasPrefix(rname, ns) {
 			continue
 		}
@@ -42,7 +42,7 @@ func GetRequestedResources(container corev1.Container, ns string) (map[string]in
 	resources := make(map[string]int64)
 
 	for resourceName, resourceQuantity := range container.Resources.Limits {
-		rname := strings.ToLower(string(resourceName))
+		rname := string(resourceName)
 		if !strings.HasPrefix(rname, ns) {
 			continue
 		}
