From a63550bdf0a7aae08efdf18d399f6a7867640d97 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 9 Apr 2024 13:00:47 -0400 Subject: [PATCH 1/4] api: Check specifically for "enum_" validation tags In validation error details, check specifically for an "enum_" prefix on the tag name instead of assuming "anyof" is an enum alias. Just to be more precise to avoid potential future bugs. --- internal/api/arm/error.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/api/arm/error.go b/internal/api/arm/error.go index 2f25d5122..95d0145d6 100644 --- a/internal/api/arm/error.go +++ b/internal/api/arm/error.go @@ -132,15 +132,18 @@ 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 "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{ From 3bfbac0ecab752918f34402342dccba9dcca2b3f Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 9 Apr 2024 13:14:50 -0400 Subject: [PATCH 2/4] api: Refactor UnmarshalHCPOpenShiftCluster parameters Reorder and add parameter for HTTP method. --- frontend/frontend.go | 2 +- internal/api/registry.go | 2 +- internal/api/v20240610preview/hcpopenshiftclusters_methods.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/frontend.go b/frontend/frontend.go index 3392a294e..d96d751a6 100644 --- a/frontend/frontend.go +++ b/frontend/frontend.go @@ -218,7 +218,7 @@ func (f *Frontend) ArmResourceCreateOrUpdate(writer http.ResponseWriter, request cluster = api.NewDefaultHCPOpenShiftCluster() } body := ctx.Value(ContextKeyBody).([]byte) - err := versionedInterface.UnmarshalHCPOpenShiftCluster(body, updating, cluster) + err := versionedInterface.UnmarshalHCPOpenShiftCluster(body, cluster, request.Method, updating) if err != nil { f.logger.Error(err.Error()) arm.WriteUnmarshalError(err, writer) diff --git a/internal/api/registry.go b/internal/api/registry.go index c227f8612..d9513529d 100644 --- a/internal/api/registry.go +++ b/internal/api/registry.go @@ -36,7 +36,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 diff --git a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go b/internal/api/v20240610preview/hcpopenshiftclusters_methods.go index 27fdcb2aa..9fec22f83 100644 --- a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go +++ b/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) From a661d5a1f5c88eb5192222b5e7ba552f6f172ec2 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 9 Apr 2024 12:51:51 -0400 Subject: [PATCH 3/4] api: Add custom "required_for_put" tag --- internal/api/arm/error.go | 2 + internal/api/hcpopenshiftcluster.go | 32 +++++++------- internal/api/registry.go | 42 +++++++++++++++++++ .../hcpopenshiftclusters_methods.go | 2 +- 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/internal/api/arm/error.go b/internal/api/arm/error.go index 95d0145d6..59fff2264 100644 --- a/internal/api/arm/error.go +++ b/internal/api/arm/error.go @@ -137,6 +137,8 @@ func WriteUnmarshalError(err error, w http.ResponseWriter) { message += fmt.Sprintf(" (must be one of: %s)", fieldErr.Param()) } 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": diff --git a/internal/api/hcpopenshiftcluster.go b/internal/api/hcpopenshiftcluster.go index f3936e58d..161d485c1 100644 --- a/internal/api/hcpopenshiftcluster.go +++ b/internal/api/hcpopenshiftcluster.go @@ -12,27 +12,27 @@ 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"` @@ -40,24 +40,24 @@ type ClusterSpec struct { // 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. diff --git a/internal/api/registry.go b/internal/api/registry.go index d9513529d..7ccdce499 100644 --- a/internal/api/registry.go +++ b/internal/api/registry.go @@ -5,6 +5,7 @@ package api import ( "fmt" + "net/http" "reflect" "strings" @@ -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}) +} diff --git a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go b/internal/api/v20240610preview/hcpopenshiftclusters_methods.go index 9fec22f83..4f326449e 100644 --- a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go +++ b/internal/api/v20240610preview/hcpopenshiftclusters_methods.go @@ -213,7 +213,7 @@ func (v version) UnmarshalHCPOpenShiftCluster(data []byte, out *api.HCPOpenShift resource.Normalize(out) - return validate.Struct(out) + return api.ValidateRequest(validate, method, updating, out) } func (c *HcpOpenShiftClusterResource) Normalize(out *api.HCPOpenShiftCluster) { From 4f3d40b78731f9ea38a1d18ed816ef6455741259 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 9 Apr 2024 14:26:38 -0400 Subject: [PATCH 4/4] frontend: Fix logic in ArmResourceCreateOrUpdate An updating PUT request must be a complete resource specification. The old version of the resource only comes into play when applying idempotency to read-only or create-only fields in an update request, but this has not yet been implemented. --- frontend/frontend.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/frontend/frontend.go b/frontend/frontend.go index d96d751a6..feec801fa 100644 --- a/frontend/frontend.go +++ b/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, cluster, request.Method, updating) + 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())