Skip to content

SPLAT-2612: Changed AWS dedicated host webhook to allow host info with other tenancies #1452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vr4manta wants to merge 1 commit into from
Closed

SPLAT-2612: Changed AWS dedicated host webhook to allow host info with other tenancies #1452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 36 additions & 37 deletions pkg/webhooks/machine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,47 +928,14 @@ func validateAWS(m *machinev1beta1.Machine, config *admissionConfig) (bool, []st
return true, warnings, nil
}

// processAWSPlacement analyzes the Placement field in relation to Tenancy and host placement. These are analyzed
// together based based on their relations to one another.
// processAWSPlacement analyzes the Placement field in relation to Tenancy and host placement.
func processAWSPlacementTenancy(placement machinev1beta1.Placement) field.ErrorList {
var errs field.ErrorList

// Validate tenancy values
switch placement.Tenancy {
case "", machinev1beta1.DefaultTenancy, machinev1beta1.DedicatedTenancy:
// Host is not supported for these cases
if placement.Host != nil {
errs = append(errs, field.Forbidden(field.NewPath("spec.placement.host"), "host may only be specified when tenancy is 'host'"))
}
case machinev1beta1.HostTenancy:
if placement.Host != nil {
klog.V(4).Infof("Validating AWS Host Placement")

if placement.Host.Affinity == nil {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.affinity"), "affinity is required and must be set to either AnyAvailable or DedicatedHost"))
} else {
switch *placement.Host.Affinity {
case machinev1beta1.HostAffinityAnyAvailable:
// DedicatedHost is optional. If it is set, make sure it follows conventions
if placement.Host.DedicatedHost != nil && !awsDedicatedHostNamePattern.MatchString(placement.Host.DedicatedHost.ID) {
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.dedicatedHost.id"), placement.Host.DedicatedHost.ID, "id must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
}
case machinev1beta1.HostAffinityDedicatedHost:
// We need to make sure DedicatedHost is set with an ID
if placement.Host.DedicatedHost == nil {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.dedicatedHost"), "dedicatedHost is required when hostAffinity is DedicatedHost, and optional otherwise"))
} else {
// If not set, return required error. If it does not match pattern, return pattern failure message.
if placement.Host.DedicatedHost.ID == "" {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.dedicatedHost.id"), "id is required and must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
} else if !awsDedicatedHostNamePattern.MatchString(placement.Host.DedicatedHost.ID) {
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.dedicatedHost.id"), placement.Host.DedicatedHost.ID, "id must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
}
}
default:
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.affinity"), placement.Host.Affinity, "hostAffinity must be either AnyAvailable or DedicatedHost"))
}
}
}
case "", machinev1beta1.DefaultTenancy, machinev1beta1.DedicatedTenancy, machinev1beta1.HostTenancy:
// Do nothing, valid values
default:
errs = append(
errs,
Expand All @@ -980,6 +947,38 @@ func processAWSPlacementTenancy(placement machinev1beta1.Placement) field.ErrorL
)
}

// Validate host settings. Upstream CAPI does not enforce host placement requiring tenancy=host, so we are just validating values and allowing MAPI to follow suit.
// Any deviation will result in CAPI2MAPI / MAPI2CAPI converison issues.
if placement.Host != nil {
klog.V(4).Infof("Validating AWS Host Placement")

if placement.Host.Affinity == nil {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.affinity"), "affinity is required and must be set to either AnyAvailable or DedicatedHost"))
} else {
switch *placement.Host.Affinity {
case machinev1beta1.HostAffinityAnyAvailable:
// DedicatedHost is optional. If it is set, make sure it follows conventions
if placement.Host.DedicatedHost != nil && !awsDedicatedHostNamePattern.MatchString(placement.Host.DedicatedHost.ID) {
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.dedicatedHost.id"), placement.Host.DedicatedHost.ID, "id must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
}
case machinev1beta1.HostAffinityDedicatedHost:
// We need to make sure DedicatedHost is set with an ID
if placement.Host.DedicatedHost == nil {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.dedicatedHost"), "dedicatedHost is required when hostAffinity is DedicatedHost, and optional otherwise"))
} else {
// If not set, return required error. If it does not match pattern, return pattern failure message.
if placement.Host.DedicatedHost.ID == "" {
errs = append(errs, field.Required(field.NewPath("spec.placement.host.dedicatedHost.id"), "id is required and must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
} else if !awsDedicatedHostNamePattern.MatchString(placement.Host.DedicatedHost.ID) {
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.dedicatedHost.id"), placement.Host.DedicatedHost.ID, "id must start with 'h-' followed by 8 or 17 lowercase hexadecimal characters (0-9 and a-f)"))
}
}
default:
errs = append(errs, field.Invalid(field.NewPath("spec.placement.host.affinity"), placement.Host.Affinity, "hostAffinity must be either AnyAvailable or DedicatedHost"))
}
}
}

return errs
}

Expand Down
8 changes: 6 additions & 2 deletions pkg/webhooks/machine_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,9 @@ func TestMachineCreation(t *testing.T) {
},
},
},
expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: spec.placement.host: Forbidden: host may only be specified when tenancy is 'host'",
// We used to error on the following, but due to CAPI allowing it even though AWS will ignore the host info
// and just honor the tenancy, we are changing our logic to behave the same
//expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: spec.placement.host: Forbidden: host may only be specified when tenancy is 'host'",
},
{
name: "configure default tenancy with host placement",
Expand All @@ -677,7 +679,9 @@ func TestMachineCreation(t *testing.T) {
},
},
},
expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: spec.placement.host: Forbidden: host may only be specified when tenancy is 'host'",
// We used to error on the following, but due to CAPI allowing it even though AWS will ignore the host info
// and just honor the tenancy, we are changing our logic to behave the same
//expectedError: "admission webhook \"validation.machine.machine.openshift.io\" denied the request: spec.placement.host: Forbidden: host may only be specified when tenancy is 'host'",
},
{
name: "with VolumeType set to gp3 and Throughput set under minium value",
Expand Down