Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 67 additions & 0 deletions e2e/machineset_migration_capi_authoritative_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
capiframework "github.com/openshift/cluster-capi-operator/e2e/framework"
awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
"k8s.io/utils/ptr"
)

var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] MachineSet Migration CAPI Authoritative Tests", Ordered, func() {
Expand Down Expand Up @@ -224,6 +225,72 @@ var _ = Describe("[sig-cluster-lifecycle][OCPFeatureGate:MachineAPIMigration] Ma
verifyResourceRemoved(awsMachineTemplate)
})
})

// This context creates a CAPI AWSMachineTemplate with SSHKeyName explicitly set to "",
// matching what the OpenShift installer sets on CAPI-default clusters. The sync controller
// writes the providerSpec from this template into the MAPI MachineSet, and MAPI must be
// able to launch machines from that providerSpec after authority switches to MachineAPI.
// It exercises the full CAPI→MAPI conversion path without the sanitisation that test
// helpers apply when building templates from existing MAPI MachineSets.
Context("switching from ClusterAPI to MachineAPI with empty SSHKeyName in CAPI template", Ordered, func() {
var name string
var emptySSHKeyTemplate *awsv1.AWSMachineTemplate

BeforeAll(func() {
name = generateName("ms-no-ssh-")

// Create a CAPI AWSMachineTemplate with SSHKeyName: ptr("") — the "no SSH key"
// value the installer uses. The CAPI→MAPI sync must normalise this to nil before
// the MAPI actuator calls AWS RunInstances, otherwise the launch fails.
emptySSHKeyTemplate = createAWSMachineTemplate(ctx, cl, name, func(spec *awsv1.AWSMachineSpec) {
spec.SSHKeyName = ptr.To("")
})

// 0 replicas: no CAPI machines needed, only the template drives the CAPI→MAPI sync.
capiMachineSet = capiframework.CreateMachineSet(ctx, cl, capiframework.NewMachineSetParams(
name, clusterName, "", 0,
clusterv1.ContractVersionedObjectReference{
Kind: "AWSMachineTemplate",
APIGroup: infraAPIGroup,
Name: emptySSHKeyTemplate.Name,
},
"worker-user-data",
))
trackResource(capiMachineSet)

mapiMachineSet = createMAPIMachineSetWithAuthoritativeAPI(ctx, cl, 0, name,
mapiv1beta1.MachineAuthorityClusterAPI, mapiv1beta1.MachineAuthorityClusterAPI)

DeferCleanup(func() {
By("Cleaning up 'switching from ClusterAPI to MachineAPI with empty SSHKeyName' resources")
cleanupMachineSetTestResources(
ctx,
cl,
[]*clusterv1.MachineSet{capiMachineSet},
[]*awsv1.AWSMachineTemplate{emptySSHKeyTemplate},
[]*mapiv1beta1.MachineSet{mapiMachineSet},
)
})
})

It("should launch a MAPI machine successfully after switching authority", func() {
By("Verifying CAPI→MAPI sync has completed with the empty-SSHKeyName template")
verifyMAPIMachineSetSynchronizedCondition(mapiMachineSet, mapiv1beta1.MachineAuthorityClusterAPI)

By("Switching authority to MachineAPI")
switchMachineSetAuthoritativeAPI(mapiMachineSet, mapiv1beta1.MachineAuthorityMachineAPI)
switchMachineSetTemplateAuthoritativeAPI(mapiMachineSet, mapiv1beta1.MachineAuthorityMachineAPI)
verifyMachineSetAuthoritative(mapiMachineSet, mapiv1beta1.MachineAuthorityMachineAPI)

By("Scaling MAPI MachineSet to 1 to create a machine from the synced providerSpec")
Expect(mapiframework.ScaleMachineSetWithContext(ctx, name, 1)).To(Succeed(), "should scale up MAPI MachineSet")

By("Verifying the MAPI machine reaches Running — a launch failure indicates a field mistranslation in the CAPI→MAPI sync")
mapiMachine, err := mapiframework.GetLatestMachineFromMachineSet(ctx, cl, mapiMachineSet)
Expect(err).ToNot(HaveOccurred(), "should get MAPI machine from MachineSet")
verifyMachineRunning(cl, mapiMachine)
})
})
Comment on lines +228 to +293

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want an e2e for this thing. I'd say fuzzing/unit is enough. Let's drop this

})

Describe("Delete MachineSets", Ordered, func() {
Expand Down
14 changes: 13 additions & 1 deletion pkg/conversion/capi2mapi/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func (m machineAndAWSMachineAndAWSCluster) toProviderSpec() (*mapiv1beta1.AWSMac
},
// UserDataSecret - Populated below.
// CredentialsSecret - Handled below.
KeyName: m.awsMachine.Spec.SSHKeyName,
KeyName: convertAWSSSHKeyNameToMAPI(m.awsMachine.Spec.SSHKeyName),
// DeviceIndex - OCPCLOUD-2707: Value must always be zero. No other values are valid in MAPA even though the value is configurable.
PublicIP: m.awsMachine.Spec.PublicIP,
SecurityGroups: convertAWSSecurityGroupstoMAPI(m.awsMachine.Spec.AdditionalSecurityGroups), // This is the way we want to convert security groups, as the AdditionalSecurity Groups are what gets added to MAPI SGs.
Expand Down Expand Up @@ -893,6 +893,18 @@ func ConvertAWSLoadBalancerToMAPI(loadBalancer *awsv1.AWSLoadBalancerSpec) (mapi
}
}

// convertAWSSSHKeyNameToMAPI normalizes CAPI's tri-state SSHKeyName for MAPI.
// CAPI: nil=cluster default, ptr("")=no key, ptr("name")=named key.
// MAPI has no tri-state: any non-nil KeyName including "" is passed to AWS RunInstances and rejected.
// ptr("") is normalized to nil so MAPI omits the field, matching the "no SSH key" intent.
func convertAWSSSHKeyNameToMAPI(sshKeyName *string) *string {
if sshKeyName != nil && *sshKeyName == "" {
return nil
}

return sshKeyName
}

// ConvertAWSCPUOptionsToMAPI converts CAPI CPUOptions to MAPI CPUOptions.
func ConvertAWSCPUOptionsToMAPI(cpuOptions awsv1.CPUOptions) *mapiv1beta1.CPUOptions {
mapiCPUOptions := &mapiv1beta1.CPUOptions{}
Expand Down
9 changes: 9 additions & 0 deletions pkg/conversion/capi2mapi/machine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,12 @@ var _ = Describe("capi2mapi Machine Status Conversion", func() {
})
})
})

var _ = DescribeTable("convertAWSSSHKeyNameToMAPI",
func(input *string, expected *string) {
Expect(convertAWSSSHKeyNameToMAPI(input)).To(Equal(expected), "input: %v", input)
},
Entry("should return nil when input is nil", nil, nil),
Entry("should return nil when input is empty string", ptr.To(""), nil),
Entry("should return the key name when input is non-empty", ptr.To("my-key"), ptr.To("my-key")),
)
7 changes: 7 additions & 0 deletions pkg/conversion/mapi2capi/aws_fuzz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,13 @@ func (f *awsProviderFuzzer) fuzzProviderConfig(ps *mapiv1beta1.AWSMachineProvide
ps.PlacementGroupPartition = nil
}

// ptr("") is normalized to nil during CAPI→MAPI conversion because MAPI passes
// KeyName directly to AWS RunInstances, which rejects an empty string.
// Avoid fuzzing KeyName to ptr("") to preserve roundtrip fidelity.
if ps.KeyName != nil && *ps.KeyName == "" {
ps.KeyName = nil
}

// Copy instance-type, region and zone to the struct so they can be set at the machine labels too.
f.InstanceType = ps.InstanceType
f.Region = ps.Placement.Region
Expand Down