k8s: use NullableString for preconditions (#383)

lyft · Sep 11, 2020 · 40ecaf1 · 40ecaf1
1 parent f9c0188
commit 40ecaf1
Show file tree

Hide file tree

Showing 7 changed files with 913 additions and 511 deletions.
diff --git a/api/k8s/v1/k8s.proto b/api/k8s/v1/k8s.proto
@@ -5,7 +5,7 @@ package clutch.k8s.v1;
 option go_package = "k8sv1";
 
 import "google/api/annotations.proto";
-import "google/protobuf/wrappers.proto";
+import "google/protobuf/struct.proto";
 import "google/protobuf/timestamp.proto";
 import "validate/validate.proto";
 
@@ -273,12 +273,23 @@ message UpdateDeploymentRequest {
 message UpdateDeploymentResponse {
 }
 
+// This message type is used to add support for nullable strings and is an
+// alternative to the well-known `StringValue` type. We need it, because the
+// grpc-gateway used by Clutch deserializes a null `StringValue` as an empty
+// string.
+message NullableString {
+  oneof kind {
+    google.protobuf.NullValue null = 1;
+    string value = 2;
+  }
+}
+
 // Preconditions to check before updating an object's metadata.
 //
-// Note: An empty StringValue signals that the label/annotation should not be set.
+// Note: A `null` NullableString means that the label/annotation should not be set.
 message ExpectedObjectMetaFields {
-  map<string, google.protobuf.StringValue> labels = 1 [ (validate.rules).map.keys.string = {min_bytes : 1} ];
-  map<string, google.protobuf.StringValue> annotations = 2 [ (validate.rules).map.keys.string = {min_bytes : 1} ];
+  map<string, NullableString> labels = 1 [ (validate.rules).map.keys.string = {min_bytes : 1} ];
+  map<string, NullableString> annotations = 2 [ (validate.rules).map.keys.string = {min_bytes : 1} ];
 }
 
 // Metadata fields to update when updating an object

diff --git a/backend/api/k8s/v1/k8s.pb.go b/backend/api/k8s/v1/k8s.pb.go
diff --git a/backend/api/k8s/v1/k8s.pb.validate.go b/backend/api/k8s/v1/k8s.pb.validate.go
diff --git a/backend/service/k8s/pods.go b/backend/service/k8s/pods.go
@@ -98,7 +98,7 @@ func (s *svc) UpdatePod(ctx context.Context, clientset, cluster, namespace, name
 	// Check that the current state of the pod matches with expectedObjectMetaFields.
 	//
 	// If there is a mismatch, checkExpectedObjectMetaFields() will return an error with the list of mismatches.
-	err = checkExpectedObjectMetaFields(expectedObjectMetaFields, pod.GetObjectMeta())
+	err = s.checkExpectedObjectMetaFields(expectedObjectMetaFields, pod.GetObjectMeta())
 	if err != nil {
 		return err
 	}
@@ -118,7 +118,7 @@ func (s *svc) UpdatePod(ctx context.Context, clientset, cluster, namespace, name
 	return err
 }
 
-func checkExpectedObjectMetaFields(expectedObjectMetaFields *k8sapiv1.ExpectedObjectMetaFields, object metav1.Object) error {
+func (s *svc) checkExpectedObjectMetaFields(expectedObjectMetaFields *k8sapiv1.ExpectedObjectMetaFields, object metav1.Object) error {
 	if len(expectedObjectMetaFields.Labels) > 0 {
 		return errors.New("checking label expectations not implemented")
 	}
@@ -127,35 +127,33 @@ func checkExpectedObjectMetaFields(expectedObjectMetaFields *k8sapiv1.ExpectedOb
 	var mismatchedAnnotations []*mismatchedAnnotation
 
 	for expectedAnnotation, expectedValue := range expectedObjectMetaFields.GetAnnotations() {
-		// "" is a valid annotation value, so nil is used to indicate that the
-		// annotation shouldn't be set
-		annotationShouldBePresent := expectedValue != nil
 		currentValue, annotationIsPresent := podAnnotations[expectedAnnotation]
 
-		// Existance precondition not met
-		if annotationShouldBePresent != annotationIsPresent {
-			mismatchedAnnotations = append(
-				mismatchedAnnotations,
-				&mismatchedAnnotation{
-					Annotation:    expectedAnnotation,
-					ExpectedValue: expectedValue.GetValue(),
-					CurrentValue:  currentValue,
-				},
-			)
-
-			continue
-		}
-
-		// Annotation values mismatched
-		if expectedValue.GetValue() != currentValue {
-			mismatchedAnnotations = append(
-				mismatchedAnnotations,
-				&mismatchedAnnotation{
-					Annotation:    expectedAnnotation,
-					ExpectedValue: expectedValue.GetValue(),
-					CurrentValue:  currentValue,
-				},
-			)
+		switch expectedValue.Kind.(type) {
+		case *k8sapiv1.NullableString_Null:
+			// Existance precondition not met
+			if annotationIsPresent {
+				mismatchedAnnotations = append(
+					mismatchedAnnotations,
+					&mismatchedAnnotation{
+						Annotation:    expectedAnnotation,
+						ExpectedValue: expectedValue.GetValue(),
+						CurrentValue:  currentValue,
+					},
+				)
+			}
+		case *k8sapiv1.NullableString_Value:
+			if !annotationIsPresent || expectedValue.GetValue() != currentValue {
+				// Annotation values mismatched
+				mismatchedAnnotations = append(
+					mismatchedAnnotations,
+					&mismatchedAnnotation{
+						Annotation:    expectedAnnotation,
+						ExpectedValue: expectedValue.GetValue(),
+						CurrentValue:  currentValue,
+					},
+				)
+			}
 		}
 	}
 

diff --git a/backend/service/k8s/pods_test.go b/backend/service/k8s/pods_test.go
@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"google.golang.org/protobuf/types/known/wrapperspb"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -338,7 +337,7 @@ func TestUpdatePod(t *testing.T) {
 		"testing-cluster",
 		"testing-namespace",
 		"testing-pod-name",
-		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*wrapperspb.StringValue{"foo": &wrapperspb.StringValue{Value: "non-matching-value"}}},
+		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*k8sv1.NullableString{"foo": &k8sv1.NullableString{Kind: &k8sv1.NullableString_Value{Value: "non-matching-value"}}}},
 		&k8sv1.ObjectMetaFields{Annotations: map[string]string{"new-annotation": "foo"}},
 		&k8sv1.RemoveObjectMetaFields{},
 	)
@@ -350,7 +349,7 @@ func TestUpdatePod(t *testing.T) {
 		"testing-cluster",
 		"testing-namespace",
 		"testing-pod-name",
-		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*wrapperspb.StringValue{"baz": &wrapperspb.StringValue{Value: "quuz"}}},
+		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*k8sv1.NullableString{"baz": &k8sv1.NullableString{Kind: &k8sv1.NullableString_Value{Value: "quuz"}}}},
 		&k8sv1.ObjectMetaFields{Annotations: map[string]string{"baz": "new-value"}},
 		&k8sv1.RemoveObjectMetaFields{},
 	)
@@ -362,7 +361,7 @@ func TestUpdatePod(t *testing.T) {
 		"testing-cluster",
 		"testing-namespace",
 		"testing-pod-name",
-		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*wrapperspb.StringValue{"baz": &wrapperspb.StringValue{Value: "new-value"}}},
+		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*k8sv1.NullableString{"baz": &k8sv1.NullableString{Kind: &k8sv1.NullableString_Value{Value: "new-value"}}}},
 		&k8sv1.ObjectMetaFields{},
 		&k8sv1.RemoveObjectMetaFields{Annotations: []string{"baz"}},
 	)
@@ -385,7 +384,7 @@ func TestUpdatePod(t *testing.T) {
 		"testing-cluster",
 		"testing-namespace",
 		"testing-pod-name",
-		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*wrapperspb.StringValue{"baz": nil}},
+		&k8sv1.ExpectedObjectMetaFields{Annotations: map[string]*k8sv1.NullableString{"baz": &k8sv1.NullableString{Kind: &k8sv1.NullableString_Null{}}}},
 		&k8sv1.ObjectMetaFields{Annotations: map[string]string{"baz": "new-value"}},
 		&k8sv1.RemoveObjectMetaFields{},
 	)

diff --git a/frontend/api/src/index.d.ts b/frontend/api/src/index.d.ts