From c5b72ab02efcce91122f95b288d4e1bf27d4b78c Mon Sep 17 00:00:00 2001 From: Juan Hernandez Date: Fri, 4 Jul 2025 18:09:33 +0200 Subject: [PATCH 1/2] Validate and set default values of template parameters This patch changes the clusters server so that it validates that the template parameters specified in the cluster are valid, and sets the default values for those that aren't specified. Signed-off-by: Juan Hernandez --- internal/servers/clusters_server.go | 88 +++++++++++++ internal/servers/clusters_server_test.go | 149 +++++++++++++++++++++++ 2 files changed, 237 insertions(+) diff --git a/internal/servers/clusters_server.go b/internal/servers/clusters_server.go index 27697d22c..e3fb8375d 100644 --- a/internal/servers/clusters_server.go +++ b/internal/servers/clusters_server.go @@ -25,6 +25,7 @@ import ( "google.golang.org/genproto/googleapis/api/httpbody" grpccodes "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/anypb" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -265,6 +266,93 @@ func (s *ClustersServer) Create(ctx context.Context, } cluster.GetSpec().SetNodeSets(actualNodeSets) + // Check that all the specified template parameters are in the template: + templateParameters := template.GetParameters() + clusterParameters := cluster.GetSpec().GetTemplateParameters() + for clusterParameterName := range clusterParameters { + clusterParameterValid := false + for _, templateParameter := range templateParameters { + if templateParameter.GetName() == clusterParameterName { + clusterParameterValid = true + break + } + } + if !clusterParameterValid { + templateParameterNames := make([]string, len(templateParameters)) + for i, templateParameter := range templateParameters { + templateParameterNames[i] = templateParameter.GetName() + } + sort.Strings(templateParameterNames) + for i, templateParameterName := range templateParameterNames { + templateParameterNames[i] = fmt.Sprintf("'%s'", templateParameterName) + } + err = grpcstatus.Errorf( + grpccodes.InvalidArgument, + "template parameter '%s' doesn't exist, valid values for template '%s' are %s", + clusterParameterName, templateId, english.WordSeries(templateParameterNames, "and"), + ) + return + } + } + + // Check that all the mandatory parameters have a value: + for _, templateParameter := range templateParameters { + if !templateParameter.GetRequired() { + continue + } + templateParameterName := templateParameter.GetName() + clusterParameter := clusterParameters[templateParameterName] + if clusterParameter == nil { + err = grpcstatus.Errorf( + grpccodes.InvalidArgument, + "parameter '%s' of template '%s' is mandatory", + templateParameterName, templateId, + ) + return + } + } + + // Check that the parameter values are compatible with the template: + for clusterParameterName, clusterParameter := range clusterParameters { + for _, templateParameter := range templateParameters { + templateParameterName := templateParameter.GetName() + if clusterParameterName != templateParameterName { + continue + } + clusterParameterType := clusterParameter.GetTypeUrl() + templateParameterType := templateParameter.GetType() + if clusterParameterType != templateParameterType { + err = grpcstatus.Errorf( + grpccodes.InvalidArgument, + "type of parameter '%s' of template '%s' should be '%s', "+ + "but it is '%s'", + clusterParameterName, + templateId, + templateParameterType, + clusterParameterType, + ) + return + } + } + } + + // Set default values for template parameters: + actualClusterParameters := make(map[string]*anypb.Any) + for _, templateParameter := range templateParameters { + templateParameterName := templateParameter.GetName() + clusterParameter := clusterParameters[templateParameterName] + actualClusterParameter := &anypb.Any{ + TypeUrl: templateParameter.GetType(), + } + if clusterParameter != nil { + actualClusterParameter.Value = clusterParameter.Value + } else { + actualClusterParameter.Value = templateParameter.GetDefault().GetValue() + } + actualClusterParameters[templateParameterName] = actualClusterParameter + } + cluster.GetSpec().SetTemplateParameters(actualClusterParameters) + err = s.generic.Create(ctx, request, &response) return } diff --git a/internal/servers/clusters_server_test.go b/internal/servers/clusters_server_test.go index a47329728..ea3c3a711 100644 --- a/internal/servers/clusters_server_test.go +++ b/internal/servers/clusters_server_test.go @@ -23,6 +23,8 @@ import ( grpccodes "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/wrapperspb" ffv1 "github.com/innabox/fulfillment-service/internal/api/fulfillment/v1" privatev1 "github.com/innabox/fulfillment-service/internal/api/private/v1" @@ -125,6 +127,12 @@ var _ = Describe("Clusters server", func() { Describe("Behaviour", func() { var server *ClustersServer + makeAny := func(m proto.Message) *anypb.Any { + a, err := anypb.New(m) + Expect(err).ToNot(HaveOccurred()) + return a + } + BeforeEach(func() { var err error @@ -173,6 +181,31 @@ var _ = Describe("Clusters server", func() { Expect(err).ToNot(HaveOccurred()) err = templatesDao.Delete(ctx, "my_deleted_template") Expect(err).ToNot(HaveOccurred()) + + // Create a template with parameters: + _, err = templatesDao.Create(ctx, privatev1.ClusterTemplate_builder{ + Id: "my_with_parameters", + Title: "My with parameters", + Description: "My with parameters.", + Parameters: []*privatev1.ClusterTemplateParameterDefinition{ + privatev1.ClusterTemplateParameterDefinition_builder{ + Name: "my_required_bool", + Title: "My required bool", + Description: "My required bool.", + Required: true, + Type: "type.googleapis.com/google.protobuf.BoolValue", + }.Build(), + privatev1.ClusterTemplateParameterDefinition_builder{ + Name: "my_optional_string", + Title: "My optional string", + Description: "My optional string.", + Required: false, + Type: "type.googleapis.com/google.protobuf.StringValue", + Default: makeAny(wrapperspb.String("my value")), + }.Build(), + }, + }.Build()) + Expect(err).ToNot(HaveOccurred()) }) It("Creates object", func() { @@ -409,6 +442,122 @@ var _ = Describe("Clusters server", func() { )) }) + It("Doesn't create object if there are missing required template parameters", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + }.Build(), + }.Build(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(Equal( + "parameter 'my_required_bool' of template 'my_with_parameters' is mandatory", + )) + }) + + It("Doesn't create object if parameter doesn't exist in the template", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + TemplateParameters: map[string]*anypb.Any{ + "junk": makeAny(wrapperspb.Int32(123)), + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(Equal( + "template parameter 'junk' doesn't exist, valid values for template " + + "'my_with_parameters' are 'my_optional_string' and 'my_required_bool'", + )) + }) + + It("Doesn't create object if parameter type doesn't match the template", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + TemplateParameters: map[string]*anypb.Any{ + "my_required_bool": makeAny(wrapperspb.Int32(123)), + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(Equal( + "type of parameter 'my_required_bool' of template 'my_with_parameters' should be " + + "'type.googleapis.com/google.protobuf.BoolValue', but it is " + + "'type.googleapis.com/google.protobuf.Int32Value'", + )) + }) + + It("Takes default values of parameters from the template", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + TemplateParameters: map[string]*anypb.Any{ + "my_required_bool": makeAny(wrapperspb.Bool(true)), + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + object := response.GetObject() + templateParameters := object.GetSpec().GetTemplateParameters() + + parameterValue := templateParameters["my_required_bool"] + Expect(parameterValue).ToNot(BeNil()) + boolValue := &wrapperspb.BoolValue{} + err = anypb.UnmarshalTo(parameterValue, boolValue, proto.UnmarshalOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(boolValue.GetValue()).To(BeTrue()) + + parameterValue = templateParameters["my_optional_string"] + Expect(parameterValue).ToNot(BeNil()) + stringValue := &wrapperspb.StringValue{} + err = anypb.UnmarshalTo(parameterValue, stringValue, proto.UnmarshalOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(stringValue.GetValue()).To(Equal("my value")) + }) + + It("Allows overriding of default values of template parameters", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + TemplateParameters: map[string]*anypb.Any{ + "my_required_bool": makeAny(wrapperspb.Bool(false)), + "my_optional_string": makeAny(wrapperspb.String("your value")), + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + object := response.GetObject() + templateParameters := object.GetSpec().GetTemplateParameters() + parameterValue := templateParameters["my_optional_string"] + Expect(parameterValue).ToNot(BeNil()) + stringValue := &wrapperspb.StringValue{} + err = anypb.UnmarshalTo(parameterValue, stringValue, proto.UnmarshalOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(stringValue.GetValue()).To(Equal("your value")) + }) + It("List objects", func() { // Create a few objects: const count = 10 From 116c64ac8bfec7ff5fb682dd267ba0ee61374f88 Mon Sep 17 00:00:00 2001 From: Juan Hernandez Date: Wed, 16 Jul 2025 18:21:33 +0200 Subject: [PATCH 2/2] Return a single error message for all incorrect template parameters This patch changes the clusters server so that it returns a single error message with the names of all the template parameters that don't exist. For example, if the user tries to use parameters `junk1` and `junk2` that don't exist in the template the error message returned will be like this: ``` template parameters 'junk1' and 'junk2' don't exist, valid values for template 'my_template' are 'my_param' and 'your_param'", ``` Signed-off-by: Juan Hernandez --- internal/servers/clusters_server.go | 42 +++++++++++++++++------- internal/servers/clusters_server_test.go | 25 +++++++++++++- 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/internal/servers/clusters_server.go b/internal/servers/clusters_server.go index e3fb8375d..bece99ebf 100644 --- a/internal/servers/clusters_server.go +++ b/internal/servers/clusters_server.go @@ -269,6 +269,7 @@ func (s *ClustersServer) Create(ctx context.Context, // Check that all the specified template parameters are in the template: templateParameters := template.GetParameters() clusterParameters := cluster.GetSpec().GetTemplateParameters() + var invalidParameterNames []string for clusterParameterName := range clusterParameters { clusterParameterValid := false for _, templateParameter := range templateParameters { @@ -278,21 +279,40 @@ func (s *ClustersServer) Create(ctx context.Context, } } if !clusterParameterValid { - templateParameterNames := make([]string, len(templateParameters)) - for i, templateParameter := range templateParameters { - templateParameterNames[i] = templateParameter.GetName() - } - sort.Strings(templateParameterNames) - for i, templateParameterName := range templateParameterNames { - templateParameterNames[i] = fmt.Sprintf("'%s'", templateParameterName) - } + invalidParameterNames = append(invalidParameterNames, clusterParameterName) + } + } + if len(invalidParameterNames) > 0 { + templateParameterNames := make([]string, len(templateParameters)) + for i, templateParameter := range templateParameters { + templateParameterNames[i] = templateParameter.GetName() + } + sort.Strings(templateParameterNames) + for i, templateParameterName := range templateParameterNames { + templateParameterNames[i] = fmt.Sprintf("'%s'", templateParameterName) + } + sort.Strings(invalidParameterNames) + for i, invalidParameterName := range invalidParameterNames { + invalidParameterNames[i] = fmt.Sprintf("'%s'", invalidParameterName) + } + if len(invalidParameterNames) == 1 { err = grpcstatus.Errorf( grpccodes.InvalidArgument, - "template parameter '%s' doesn't exist, valid values for template '%s' are %s", - clusterParameterName, templateId, english.WordSeries(templateParameterNames, "and"), + "template parameter %s doesn't exist, valid values for template '%s' are %s", + invalidParameterNames[0], + templateId, + english.WordSeries(templateParameterNames, "and"), + ) + } else { + err = grpcstatus.Errorf( + grpccodes.InvalidArgument, + "template parameters %s don't exist, valid values for template '%s' are %s", + english.WordSeries(invalidParameterNames, "and"), + templateId, + english.WordSeries(templateParameterNames, "and"), ) - return } + return } // Check that all the mandatory parameters have a value: diff --git a/internal/servers/clusters_server_test.go b/internal/servers/clusters_server_test.go index ea3c3a711..6f2f8b334 100644 --- a/internal/servers/clusters_server_test.go +++ b/internal/servers/clusters_server_test.go @@ -460,7 +460,7 @@ var _ = Describe("Clusters server", func() { )) }) - It("Doesn't create object if parameter doesn't exist in the template", func() { + It("Doesn't create object if one parameter doesn't exist in the template", func() { response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ Object: ffv1.Cluster_builder{ Spec: ffv1.ClusterSpec_builder{ @@ -482,6 +482,29 @@ var _ = Describe("Clusters server", func() { )) }) + It("Doesn't create object if two parameters don't exist in the template", func() { + response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ + Object: ffv1.Cluster_builder{ + Spec: ffv1.ClusterSpec_builder{ + Template: "my_with_parameters", + TemplateParameters: map[string]*anypb.Any{ + "junk1": makeAny(wrapperspb.Int32(123)), + "junk2": makeAny(wrapperspb.Int32(123)), + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).To(HaveOccurred()) + Expect(response).To(BeNil()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.InvalidArgument)) + Expect(status.Message()).To(Equal( + "template parameters 'junk1' and 'junk2' don't exist, valid values for template " + + "'my_with_parameters' are 'my_optional_string' and 'my_required_bool'", + )) + }) + It("Doesn't create object if parameter type doesn't match the template", func() { response, err := server.Create(ctx, ffv1.ClustersCreateRequest_builder{ Object: ffv1.Cluster_builder{