First pass at enforcing "required" fields in PUT requests

frontend/frontend.go

@@ -213,20 +213,19 @@ func (f *Frontend) ArmResourceCreateOrUpdate(writer http.ResponseWriter, request
 
 	// URL path is already lowercased by middleware.
 	resourceID := request.URL.Path
-	cluster, updating := f.cache.GetCluster(resourceID)
-	if !updating {
-		cluster = api.NewDefaultHCPOpenShiftCluster()
-	}
+	_, updating := f.cache.GetCluster(resourceID)
+	newCluster := api.NewDefaultHCPOpenShiftCluster()
 	body := ctx.Value(ContextKeyBody).([]byte)
-	err := versionedInterface.UnmarshalHCPOpenShiftCluster(body, updating, cluster)
+	err := versionedInterface.UnmarshalHCPOpenShiftCluster(body, newCluster, request.Method, updating)
 	if err != nil {
 		f.logger.Error(err.Error())
 		arm.WriteUnmarshalError(err, writer)
 		return
 	}
-	f.cache.SetCluster(resourceID, cluster)
+	// FIXME Enforce visibility tags on newCluster.
+	f.cache.SetCluster(resourceID, newCluster)
 
-	versionedResource := versionedInterface.NewHCPOpenShiftCluster(cluster)
+	versionedResource := versionedInterface.NewHCPOpenShiftCluster(newCluster)
 	resp, err := json.Marshal(versionedResource)
 	if err != nil {
 		f.logger.Error(err.Error())

internal/api/arm/error.go

@@ -132,15 +132,20 @@ func WriteUnmarshalError(err error, w http.ResponseWriter) {
 		for index, fieldErr := range err {
 			message := fmt.Sprintf("Invalid value '%s' for field '%s'", fieldErr.Value(), fieldErr.Field())
 			// Try to add a corrective suggestion to the message.
-			switch fieldErr.ActualTag() {
-			case "cidrv4":
-				message += " (must be a v4 CIDR address)"
-			case "ipv4":
-				message += " (must be an IPv4 address)"
-			case "oneof":
+			tag := fieldErr.Tag()
+			if strings.HasPrefix(tag, "enum_") {
 				message += fmt.Sprintf(" (must be one of: %s)", fieldErr.Param())
-			case "url":
-				message += " (must be a URL)"
+			} else {
+				switch tag {
+				case "required_for_put": // custom tag
+					message = fmt.Sprintf("Missing required field '%s'", fieldErr.Field())
+				case "cidrv4":
+					message += " (must be a v4 CIDR address)"
+				case "ipv4":
+					message += " (must be an IPv4 address)"
+				case "url":
+					message += " (must be a URL)"
+				}
 			}
 			_, target, _ := strings.Cut(fieldErr.Namespace(), ".")
 			cloudError.Details[index] = CloudErrorBody{

internal/api/hcpopenshiftcluster.go

@@ -12,52 +12,52 @@ import (
 // HCPOpenShiftCluster represents an ARO HCP OpenShift cluster resource.
 type HCPOpenShiftCluster struct {
 	arm.TrackedResource
-	Properties HCPOpenShiftClusterProperties `json:"properties,omitempty"`
+	Properties HCPOpenShiftClusterProperties `json:"properties,omitempty" validate:"required_for_put"`
 }
 
 // HCPOpenShiftClusterProperties represents the property bag of a HCPOpenShiftCluster resource.
 type HCPOpenShiftClusterProperties struct {
 	ProvisioningState arm.ProvisioningState `json:"provisioningState,omitempty" visibility:"read"               validate:"omitempty,enum_provisioningstate"`
-	Spec              ClusterSpec           `json:"spec,omitempty"              visibility:"read,create,update"`
+	Spec              ClusterSpec           `json:"spec,omitempty"              visibility:"read,create,update" validate:"required_for_put"`
 }
 
 // ClusterSpec represents a high level cluster configuration.
 type ClusterSpec struct {
-	Version                       VersionProfile            `json:"version,omitempty"                       visibility:"read,create,update"`
+	Version                       VersionProfile            `json:"version,omitempty"                       visibility:"read,create,update" validate:"required_for_put"`
 	DNS                           DNSProfile                `json:"dns,omitempty"                           visibility:"read,create,update"`
 	Network                       NetworkProfile            `json:"network,omitempty"                       visibility:"read,create"`
 	Console                       ConsoleProfile            `json:"console,omitempty"                       visibility:"read"`
-	API                           APIProfile                `json:"api,omitempty"                           visibility:"read,create"`
+	API                           APIProfile                `json:"api,omitempty"                           visibility:"read,create"        validate:"required_for_put"`
 	FIPS                          bool                      `json:"fips,omitempty"                          visibility:"read,create"`
 	EtcdEncryption                bool                      `json:"etcdEncryption,omitempty"                visibility:"read,create"`
 	DisableUserWorkloadMonitoring bool                      `json:"disableUserWorkloadMonitoring,omitempty" visibility:"read,create,update"`
 	Proxy                         ProxyProfile              `json:"proxy,omitempty"                         visibility:"read,create,update"`
-	Platform                      PlatformProfile           `json:"platform,omitempty"                      visibility:"read,create"`
+	Platform                      PlatformProfile           `json:"platform,omitempty"                      visibility:"read,create"        validate:"required_for_put"`
 	IssuerURL                     string                    `json:"issuerUrl,omitempty"                     visibility:"read"               validate:"omitempty,url"`
 	ExternalAuth                  ExternalAuthConfigProfile `json:"externalAuth,omitempty"                  visibility:"read,create"`
 	Ingress                       []*IngressProfile         `json:"ingressProfile,omitempty"                visibility:"read,create"`
 }
 
 // VersionProfile represents the cluster control plane version.
 type VersionProfile struct {
-	ID                string   `json:"id,omitempty"                visibility:"read,create,update"`
-	ChannelGroup      string   `json:"channelGroup,omitempty"      visibility:"read,create"`
+	ID                string   `json:"id,omitempty"                visibility:"read,create,update" validate:"required_for_put"`
+	ChannelGroup      string   `json:"channelGroup,omitempty"      visibility:"read,create"        validate:"required_for_put"`
 	AvailableUpgrades []string `json:"availableUpgrades,omitempty" visibility:"read"`
 }
 
 // DNSProfile represents the DNS configuration of the cluster.
 type DNSProfile struct {
 	BaseDomain       string `json:"baseDomain,omitempty"       visibility:"read"`
-	BaseDomainPrefix string `json:"baseDomainPrefix,omitempty" visibility:"read,create"`
+	BaseDomainPrefix string `json:"baseDomainPrefix,omitempty" visibility:"read,create" validate:"required_for_put"`
 }
 
 // NetworkProfile represents a cluster network configuration.
 // Visibility for the entire struct is "read,create".
 type NetworkProfile struct {
 	NetworkType NetworkType `json:"networkType,omitempty"`
-	PodCIDR     string      `json:"podCidr,omitempty"     validate:"omitempty,cidrv4"`
-	ServiceCIDR string      `json:"serviceCidr,omitempty" validate:"omitempty,cidrv4"`
-	MachineCIDR string      `json:"machineCidr,omitempty" validate:"omitempty,cidrv4"`
+	PodCIDR     string      `json:"podCidr,omitempty"     validate:"required_for_put,cidrv4"`
+	ServiceCIDR string      `json:"serviceCidr,omitempty" validate:"required_for_put,cidrv4"`
+	MachineCIDR string      `json:"machineCidr,omitempty" validate:"required_for_put,cidrv4"`
 	HostPrefix  int32       `json:"hostPrefix,omitempty"`
 }
 
@@ -71,7 +71,7 @@ type ConsoleProfile struct {
 type APIProfile struct {
 	URL        string     `json:"url,omitempty"        visibility:"read"        validate:"omitempty,url"`
 	IP         string     `json:"ip,omitempty"         visibility:"read"        validate:"omitempty,ipv4"`
-	Visibility Visibility `json:"visibility,omitempty" visibility:"read,create" validate:"omitempty,enum_visibility"`
+	Visibility Visibility `json:"visibility,omitempty" visibility:"read,create" validate:"required_for_put,enum_visibility"`
 }
 
 // ProxyProfile represents the cluster proxy configuration.
@@ -86,10 +86,10 @@ type ProxyProfile struct {
 // PlatformProfile represents the Azure platform configuration.
 // Visibility for the entire struct is "read,create".
 type PlatformProfile struct {
-	ManagedResourceGroup string       `json:"managedResourceGroup,omitempty"`
-	SubnetID             string       `json:"subnetId,omitempty"`
+	ManagedResourceGroup string       `json:"managedResourceGroup,omitempty" validate:"required_for_put"`
+	SubnetID             string       `json:"subnetId,omitempty"             validate:"required_for_put"`
 	OutboundType         OutboundType `json:"outboundType,omitempty"         validate:"omitempty,enum_outboundtype"`
-	PreconfiguredNSGs    bool         `json:"preconfiguredNsgs,omitempty"`
+	PreconfiguredNSGs    bool         `json:"preconfiguredNsgs,omitempty"    validate:"required_for_put"`
 	EtcdEncryptionSetID  string       `json:"etcdEncryptionSetId,omitempty"`
 }
 
@@ -103,7 +103,7 @@ type ExternalAuthConfigProfile struct {
 type IngressProfile struct {
 	IP         string     `json:"ip,omitempty"         visibility:"read"        validate:"omitempty,ipv4"`
 	URL        string     `json:"url,omitempty"        visibility:"read"        validate:"omitempty,url"`
-	Visibility Visibility `json:"visibility,omitempty" visibility:"read,create" validate:"omitempty,enum_visibility"`
+	Visibility Visibility `json:"visibility,omitempty" visibility:"read,create" validate:"required_for_put,enum_visibility"`
 }
 
 // Creates an HCPOpenShiftCluster with any non-zero default values.

internal/api/registry.go

@@ -5,6 +5,7 @@ package api
 
 import (
 	"fmt"
+	"net/http"
 	"reflect"
 	"strings"
 
@@ -36,7 +37,7 @@ type Version interface {
 	// FIXME Disable until we have generated structs for node pools.
 	//NewHCPOpenShiftClusterNodePool(*HCPOpenShiftClusterNodePool) VersionedHCPOpenShiftClusterNodePool
 
-	UnmarshalHCPOpenShiftCluster([]byte, bool, *HCPOpenShiftCluster) error
+	UnmarshalHCPOpenShiftCluster([]byte, *HCPOpenShiftCluster, string, bool) error
 }
 
 // apiRegistry is the map of registered API versions
@@ -65,5 +66,46 @@ func NewValidator() *validator.Validate {
 		return name
 	})
 
+	// Use this for fields required in PUT requests. Do not apply to read-only fields.
+	err := validate.RegisterValidation("required_for_put", func(fl validator.FieldLevel) bool {
+		val := fl.Top().FieldByName("Method")
+		if val.IsZero() {
+			panic("Method field not found for required_for_put")
+		}
+		if val.String() != http.MethodPut {
+			return true
+		}
+
+		// This is replicating the implementation of "required".
+		// See https://github.com/go-playground/validator/issues/492
+		// Sounds like "hasValue" is unlikely to be exported and
+		// "validate.Var" does not seem like a safe alternative.
+		field := fl.Field()
+		_, kind, nullable := fl.ExtractType(field)
+		switch kind {
+		case reflect.Slice, reflect.Map, reflect.Ptr, reflect.Interface, reflect.Chan, reflect.Func:
+			return !field.IsNil()
+		default:
+			if nullable && field.Interface() != nil {
+				return true
+			}
+			return field.IsValid() && !field.IsZero()
+		}
+	})
+	if err != nil {
+		panic(err)
+	}
+
 	return validate
 }
+
+type validateContext struct {
+	// Fields must be exported so valdator can access.
+	Method   string
+	Updating bool
+	Resource any
+}
+
+func ValidateRequest(validate *validator.Validate, method string, updating bool, resource any) error {
+	return validate.Struct(validateContext{Method: method, Updating: updating, Resource: resource})
+}

internal/api/v20240610preview/hcpopenshiftclusters_methods.go

@@ -201,7 +201,7 @@ func (v version) NewHCPOpenShiftCluster(from *api.HCPOpenShiftCluster) api.Versi
 	return out
 }
 
-func (v version) UnmarshalHCPOpenShiftCluster(data []byte, updating bool, out *api.HCPOpenShiftCluster) error {
+func (v version) UnmarshalHCPOpenShiftCluster(data []byte, out *api.HCPOpenShiftCluster, method string, updating bool) error {
 	var resource HcpOpenShiftClusterResource
 
 	err := json.Unmarshal(data, &resource)
@@ -213,7 +213,7 @@ func (v version) UnmarshalHCPOpenShiftCluster(data []byte, updating bool, out *a
 
 	resource.Normalize(out)
 
-	return validate.Struct(out)
+	return api.ValidateRequest(validate, method, updating, out)
 }
 
 func (c *HcpOpenShiftClusterResource) Normalize(out *api.HCPOpenShiftCluster) {