Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

k8s: use NullableString instead of StringValue for preconditions #383

Merged
merged 1 commit into from
Sep 11, 2020
Merged

k8s: use NullableString instead of StringValue for preconditions #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions api/k8s/v1/k8s.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;
}
}
Comment on lines +280 to +285
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear on why this diverges from the way that string value is written i.e.

message StringValue {
  // The string value.
  string value = 1;
}

also would prefer we make whatever solution it is global and copy all of the wrappers over to clutch/api/api/v1/wrappers.proto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for this approach in order to make it very explicit that the value is nullable and how to express a null value.

I don't however have a strong preference for the current version vs copying the wrapper types to Clutch — if you prefer the latter let me know and I'll update the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, though the wrapper types are poorly named (NullableString is a better name for sure), I don't want to introduce a new concept when the core proto ecosystem already has one. Simply copying the wrapper types into our own file to circumvent the pbjson flattening is to me the best fix and clearly communicates our intent.

in documenting that file we can refer to protobufjs/protobuf.js#677 and https://github.com/protocolbuffers/protobuf-go/blob/3f7a61f89bb6813f89d981d1870ed68da0b3c3f1/encoding/protojson/well_known_types.go#L35-L44


// 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
Expand Down
1,027 changes: 568 additions & 459 deletions backend/api/k8s/v1/k8s.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

79 changes: 79 additions & 0 deletions backend/api/k8s/v1/k8s.pb.validate.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 27 additions & 29 deletions backend/service/k8s/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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")
}
Expand All @@ -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,
},
)
}
}
}

Expand Down
9 changes: 4 additions & 5 deletions backend/service/k8s/pods_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{},
)
Expand All @@ -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{},
)
Expand All @@ -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"}},
)
Expand All @@ -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{},
)
Expand Down
65 changes: 61 additions & 4 deletions frontend/api/src/index.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading