MGMT-23769: Provide up front validation and handling of template prov…

internal/servers/compute_instances_server_test.go

@@ -172,6 +172,18 @@ var _ = Describe("Compute instances server", func() {
 						Default:     memoryDefault,
 					},
 				},
+				SpecDefaults: privatev1.ComputeInstanceTemplateSpecDefaults_builder{
+					Cores:     proto.Int32(2),
+					MemoryGib: proto.Int32(2),
+					Image: privatev1.ComputeInstanceImage_builder{
+						SourceType: "registry",
+						SourceRef:  "quay.io/containerdisks/fedora:latest",
+					}.Build(),
+					BootDisk: privatev1.ComputeInstanceDisk_builder{
+						SizeGib: 10,
+					}.Build(),
+					RunStrategy: proto.String("Always"),
+				}.Build(),
 			}.Build()
 
 			_, err = templatesDao.Create().SetObject(template).Do(ctx)
@@ -480,5 +492,33 @@ var _ = Describe("Compute instances server", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(response).To(BeNil())
 		})
+
+		It("User-provided values survive public-to-private mapping, missing fields filled from template", func() {
+			createTemplate("mapping-template")
+
+			// Create with some user-provided fields and let template cover the rest for validation:
+			response, err := server.Create(ctx, publicv1.ComputeInstancesCreateRequest_builder{
+				Object: publicv1.ComputeInstance_builder{
+					Spec: publicv1.ComputeInstanceSpec_builder{
+						Template:    "mapping-template",
+						Cores:       proto.Int32(8),
+						MemoryGib:   proto.Int32(16),
+						RunStrategy: proto.String("Halted"),
+					}.Build(),
+				}.Build(),
+			}.Build())
+			Expect(err).ToNot(HaveOccurred())
+			Expect(response).ToNot(BeNil())
+
+			spec := response.GetObject().GetSpec()
+			// User-provided values preserved through mapping:
+			Expect(spec.GetCores()).To(Equal(int32(8)))
+			Expect(spec.GetMemoryGib()).To(Equal(int32(16)))
+			Expect(spec.GetRunStrategy()).To(Equal("Halted"))
+			// Template defaults should be stored:
+			Expect(spec.GetImage().GetSourceType()).To(Equal("registry"))
+			Expect(spec.GetImage().GetSourceRef()).To(Equal("quay.io/containerdisks/fedora:latest"))
+			Expect(spec.GetBootDisk().GetSizeGib()).To(Equal(int32(10)))
+		})
 	})
 })

internal/servers/private_compute_instances_server.go

@@ -19,9 +19,14 @@ import (
 	"log/slog"
 	"strings"
 
+	"maps"
+
 	"github.com/prometheus/client_golang/prometheus"
+
 	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/fieldmaskpb"
 
 	privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1"
@@ -167,8 +172,14 @@ func (s *PrivateComputeInstancesServer) Create(ctx context.Context,
 		return
 	}
 
-	// Validate template:
-	err = s.validateTemplate(ctx, request.GetObject())
+	// Fetch and validate template:
+	template, err := s.fetchAndValidateTemplate(ctx, request.GetObject())
+	if err != nil {
+		return
+	}
+
+	// Apply template spec defaults and validate that all required spec fields are present.
+	err = s.applySpecDefaults(request.GetObject().GetSpec(), template)
 	if err != nil {
 		return
 	}
@@ -188,11 +199,9 @@ func (s *PrivateComputeInstancesServer) Update(ctx context.Context,
 			return
 		}
 	}
-	if hasMaskPrefix(mask, "spec.template", "spec.template_parameters") {
-		err = s.validateTemplate(ctx, request.GetObject())
-		if err != nil {
-			return
-		}
+	err = s.validateTemplateImmutability(ctx, request)
+	if err != nil {
+		return
 	}
 
 	err = s.generic.Update(ctx, request, &response)
@@ -211,61 +220,139 @@ func (s *PrivateComputeInstancesServer) Signal(ctx context.Context,
 	return
 }
 
-// validateTemplate validates the template ID and parameters in the compute instance spec.
-func (s *PrivateComputeInstancesServer) validateTemplate(ctx context.Context, vm *privatev1.ComputeInstance) error {
+// fetchAndValidateTemplate fetches the template, validates parameters in the compute instance spec,
+// applies template parameter defaults, and returns the template.
+func (s *PrivateComputeInstancesServer) fetchAndValidateTemplate(ctx context.Context, vm *privatev1.ComputeInstance) (*privatev1.ComputeInstanceTemplate, error) {
 	if vm == nil {
-		return grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance is mandatory")
+		return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance is mandatory")
 	}
 
 	spec := vm.GetSpec()
 	if spec == nil {
-		return grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance spec is mandatory")
+		return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance spec is mandatory")
+	}
+
+	template, err := s.fetchTemplate(ctx, spec.GetTemplate())
+	if err != nil {
+		return nil, err
 	}
 
-	templateID := spec.GetTemplate()
+	// Validate template parameters:
+	vmParameters := spec.GetTemplateParameters()
+	err = utils.ValidateComputeInstanceTemplateParameters(template, vmParameters)
+	if err != nil {
+		return nil, err
+	}
+
+	// Set default values for template parameters:
+	actualVmParameters := utils.ProcessTemplateParametersWithDefaults(
+		utils.ComputeInstanceTemplateAdapter{ComputeInstanceTemplate: template},
+		vmParameters,
+	)
+	spec.SetTemplateParameters(actualVmParameters)
+
+	return template, nil
+}
+
+// fetchTemplate fetches a compute instance template
+func (s *PrivateComputeInstancesServer) fetchTemplate(ctx context.Context, templateID string) (*privatev1.ComputeInstanceTemplate, error) {
 	if templateID == "" {
-		return grpcstatus.Errorf(grpccodes.InvalidArgument, "template ID is mandatory")
+		return nil, grpcstatus.Errorf(grpccodes.InvalidArgument, "template ID is mandatory")
 	}
 
-	// Get the template:
 	getTemplateResponse, err := s.templatesDao.Get().
 		SetId(templateID).
 		Do(ctx)
 	if err != nil {
+		var notFoundErr *dao.ErrNotFound
+		if errors.As(err, &notFoundErr) {
+			return nil, grpcstatus.Errorf(grpccodes.InvalidArgument,
+				"template '%s' does not exist", templateID)
+		}
 		s.logger.ErrorContext(
 			ctx,
 			"Template retrieval failed",
 			slog.String("template_id", templateID),
 			slog.Any("error", err),
 		)
-		return grpcstatus.Errorf(
+		return nil, grpcstatus.Errorf(
 			grpccodes.Internal,
 			"failed to retrieve template '%s'",
 			templateID,
 		)
 	}
+
 	template := getTemplateResponse.GetObject()
 	if template == nil {
-		return grpcstatus.Errorf(
+		return nil, grpcstatus.Errorf(
 			grpccodes.InvalidArgument,
 			"template '%s' does not exist",
 			templateID,
 		)
 	}
+	return template, nil
+}
 
-	// Validate template parameters:
-	vmParameters := spec.GetTemplateParameters()
-	err = utils.ValidateComputeInstanceTemplateParameters(template, vmParameters)
+// applySpecDefaults applies template spec defaults to the spec in place and validates
+// that all required fields are present. User-provided values are never overridden.
+func (s *PrivateComputeInstancesServer) applySpecDefaults(
+	spec *privatev1.ComputeInstanceSpec,
+	template *privatev1.ComputeInstanceTemplate,
+) error {
+	utils.ApplySpecDefaults(spec, template.GetSpecDefaults())
+	return utils.ValidateRequiredSpecFields(spec)
+}
+
+// validateTemplateImmutability ensures that the template and template_parameters fields
+// cannot be changed after compute instance creation.
+func (s *PrivateComputeInstancesServer) validateTemplateImmutability(ctx context.Context,
+	request *privatev1.ComputeInstancesUpdateRequest) error {
+	updateMask := request.GetUpdateMask()
+	updatingTemplate := hasMaskPrefix(updateMask, "spec.template")
+	updatingTemplateParams := hasMaskPrefix(updateMask, "spec.template_parameters")
+
+	if !updatingTemplate && !updatingTemplateParams {
+		return nil
+	}
+
+	ci := request.GetObject()
+	if ci == nil {
+		return grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance is mandatory")
+	}
+	id := ci.GetId()
+	if id == "" {
+		return grpcstatus.Errorf(grpccodes.InvalidArgument, "compute instance id is mandatory")
+	}
+
+	getResponse, err := s.generic.dao.Get().SetId(id).Do(ctx)
 	if err != nil {
 		return err
 	}
+	existingCI := getResponse.GetObject()
 
-	// Set default values for template parameters:
-	actualVmParameters := utils.ProcessTemplateParametersWithDefaults(
-		utils.ComputeInstanceTemplateAdapter{ComputeInstanceTemplate: template},
-		vmParameters,
-	)
-	spec.SetTemplateParameters(actualVmParameters)
+	existingSpec := existingCI.GetSpec()
+	newSpec := request.GetObject().GetSpec()
+
+	if updatingTemplate && existingSpec.GetTemplate() != newSpec.GetTemplate() {
+		return grpcstatus.Errorf(
+			grpccodes.InvalidArgument,
+			"cannot change spec.template from '%s' to '%s': template is immutable",
+			existingSpec.GetTemplate(),
+			newSpec.GetTemplate(),
+		)
+	}
+
+	if updatingTemplateParams {
+		templateParamsEqual := func(first, second *anypb.Any) bool {
+			return proto.Equal(first, second)
+		}
+		if !maps.EqualFunc(existingSpec.GetTemplateParameters(), newSpec.GetTemplateParameters(), templateParamsEqual) {
+			return grpcstatus.Errorf(
+				grpccodes.InvalidArgument,
+				"cannot change spec.template_parameters: template parameters are immutable",
+			)
+		}
+	}
 
 	return nil
 }