Skip to content

Commit

Permalink
Merge pull request #49 from mbarnes/required-for-put
Browse files Browse the repository at this point in the history 
First pass at enforcing "required" fields in PUT requests
  • Loading branch information
@mbarnes
mbarnes authored Apr 10, 2024
2 parents 269405f + 4f3d40b commit 9c6acc0
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 34 deletions.
13 changes: 6 additions & 7 deletions frontend/frontend.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,20 +215,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())
Expand Down
21 changes: 13 additions & 8 deletions internal/api/arm/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
32 changes: 16 additions & 16 deletions internal/api/hcpopenshiftcluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}

Expand All @@ -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.
Expand All @@ -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"`
}

Expand All @@ -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.
Expand Down
44 changes: 43 additions & 1 deletion internal/api/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package api

import (
"fmt"
"net/http"
"reflect"
"strings"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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})
}
4 changes: 2 additions & 2 deletions internal/api/v20240610preview/hcpopenshiftclusters_methods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down

0 comments on commit 9c6acc0

Please sign in to comment.