diff --git a/api/v1beta2/ocicluster_webhook_test.go b/api/v1beta2/ocicluster_webhook_test.go index 034be37c..825befa2 100644 --- a/api/v1beta2/ocicluster_webhook_test.go +++ b/api/v1beta2/ocicluster_webhook_test.go @@ -357,6 +357,66 @@ func TestOCICluster_ValidateCreate(t *testing.T) { errorMgsShouldContain: "invalid egressRules CIDR format", expectErr: true, }, + { + name: "shouldn't allow empty NSG egress destination", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: goodClusterName, + }, + Spec: OCIClusterSpec{ + CompartmentId: "ocid", + OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroup: NetworkSecurityGroup{ + List: []*NSG{{ + Role: Custom, + EgressRules: []EgressSecurityRuleForNSG{{ + EgressSecurityRule: EgressSecurityRule{ + Destination: nil, + DestinationType: EgressSecurityRuleDestinationTypeCidrBlock, + Protocol: common.String("all"), + }, + }}, + }}, + }, + }, + }, + }, + }, + errorMgsShouldContain: "invalid egressRules: Destination may not be empty", + expectErr: true, + }, + { + name: "shouldn't allow empty NSG egress protocol", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: goodClusterName, + }, + Spec: OCIClusterSpec{ + CompartmentId: "ocid", + OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroup: NetworkSecurityGroup{ + List: []*NSG{{ + Role: Custom, + EgressRules: []EgressSecurityRuleForNSG{{ + EgressSecurityRule: EgressSecurityRule{ + Destination: common.String("10.0.0.0/15"), + DestinationType: EgressSecurityRuleDestinationTypeCidrBlock, + Protocol: nil, + }, + }}, + }}, + }, + }, + }, + }, + }, + errorMgsShouldContain: "invalid egressRules: Protocol may not be empty", + expectErr: true, + }, { name: "shouldn't allow bad NSG ingress cidr", c: &OCICluster{ @@ -383,6 +443,66 @@ func TestOCICluster_ValidateCreate(t *testing.T) { errorMgsShouldContain: "invalid ingressRule CIDR format", expectErr: true, }, + { + name: "shouldn't allow empty NSG ingress protocol", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: goodClusterName, + }, + Spec: OCIClusterSpec{ + CompartmentId: "ocid", + OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroup: NetworkSecurityGroup{ + List: []*NSG{{ + Role: Custom, + IngressRules: []IngressSecurityRuleForNSG{{ + IngressSecurityRule: IngressSecurityRule{ + Source: common.String("10.0.0.0/15"), + SourceType: IngressSecurityRuleSourceTypeCidrBlock, + Protocol: nil, + }, + }}, + }}, + }, + }, + }, + }, + }, + errorMgsShouldContain: "invalid ingressRules: Protocol may not be empty", + expectErr: true, + }, + { + name: "shouldn't allow empty NSG ingress source", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: goodClusterName, + }, + Spec: OCIClusterSpec{ + CompartmentId: "ocid", + OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroup: NetworkSecurityGroup{ + List: []*NSG{{ + Role: Custom, + IngressRules: []IngressSecurityRuleForNSG{{ + IngressSecurityRule: IngressSecurityRule{ + Source: nil, + SourceType: IngressSecurityRuleSourceTypeCidrBlock, + Protocol: common.String("all"), + }, + }}, + }}, + }, + }, + }, + }, + }, + errorMgsShouldContain: "invalid ingressRules: Source may not be empty", + expectErr: true, + }, { name: "shouldn't allow bad NSG role", c: &OCICluster{ diff --git a/api/v1beta2/validator.go b/api/v1beta2/validator.go index 86c990da..b29ca683 100644 --- a/api/v1beta2/validator.go +++ b/api/v1beta2/validator.go @@ -167,6 +167,15 @@ func validateEgressSecurityRuleForNSG(egressRules []EgressSecurityRuleForNSG, fl for _, r := range egressRules { rule := r.EgressSecurityRule + // nsg_reconciler will set the service destination if not set for `SERVICE_CIDR_BLOCK` destination type + if rule.DestinationType != EgressSecurityRuleDestinationTypeServiceCidrBlock && rule.Destination == nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Destination, "invalid egressRules: Destination may not be empty")) + } + + if rule.Protocol == nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Protocol, "invalid egressRules: Protocol may not be empty")) + } + if rule.DestinationType == EgressSecurityRuleDestinationTypeCidrBlock && rule.Destination != nil { if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Destination)); err != nil { allErrs = append(allErrs, field.Invalid(fldPath, rule.Destination, "invalid egressRules CIDR format")) @@ -184,6 +193,15 @@ func validateIngressSecurityRuleForNSG(egressRules []IngressSecurityRuleForNSG, for _, r := range egressRules { rule := r.IngressSecurityRule + // nsg_reconciler will set the service source if not set for `SERVICE_CIDR_BLOCK` destination type + if rule.SourceType != IngressSecurityRuleSourceTypeServiceCidrBlock && rule.Source == nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "invalid ingressRules: Source may not be empty")) + } + + if rule.Protocol == nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Protocol, "invalid ingressRules: Protocol may not be empty")) + } + if rule.SourceType == IngressSecurityRuleSourceTypeCidrBlock && rule.Source != nil { if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Source)); err != nil { allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "invalid ingressRule CIDR format")) diff --git a/cloud/scope/machine_pool_test.go b/cloud/scope/machine_pool_test.go index 23487192..2e3b3d50 100644 --- a/cloud/scope/machine_pool_test.go +++ b/cloud/scope/machine_pool_test.go @@ -309,6 +309,112 @@ func TestInstanceConfigCreate(t *testing.T) { }, }, + { + name: "instance config create - LaunchInstanceAgentConfig contains nil", + errorExpected: false, + testSpecificSetup: func(ms *MachinePoolScope) { + ms.OCIMachinePool.Spec.InstanceConfiguration = infrav2exp.InstanceConfiguration{ + Shape: common.String("test-shape"), + ShapeConfig: &infrav2exp.ShapeConfig{ + Ocpus: common.String("2"), + MemoryInGBs: common.String("65"), + BaselineOcpuUtilization: "BASELINE_1_1", + Nvmes: common.Int(5), + }, + InstanceVnicConfiguration: &infrastructurev1beta2.NetworkDetails{ + AssignPublicIp: true, + SubnetName: "worker-subnet", + SkipSourceDestCheck: common.Bool(true), + NsgNames: []string{"worker-nsg"}, + HostnameLabel: common.String("test"), + DisplayName: common.String("test-display"), + AssignPrivateDnsRecord: common.Bool(true), + }, + PlatformConfig: &infrastructurev1beta2.PlatformConfig{ + PlatformConfigType: infrastructurev1beta2.PlatformConfigTypeAmdvm, + AmdVmPlatformConfig: infrastructurev1beta2.AmdVmPlatformConfig{ + IsMeasuredBootEnabled: common.Bool(false), + IsTrustedPlatformModuleEnabled: common.Bool(true), + IsSecureBootEnabled: common.Bool(true), + IsMemoryEncryptionEnabled: common.Bool(true), + }, + }, + AgentConfig: &infrastructurev1beta2.LaunchInstanceAgentConfig{ + IsMonitoringDisabled: nil, + IsManagementDisabled: nil, + AreAllPluginsDisabled: nil, + PluginsConfig: []infrastructurev1beta2.InstanceAgentPluginConfig{ + { + Name: nil, + DesiredState: infrastructurev1beta2.InstanceAgentPluginConfigDetailsDesiredStateEnabled, + }, + }, + }, + } + computeManagementClient.EXPECT().ListInstanceConfigurations(gomock.Any(), gomock.Any()). + Return(core.ListInstanceConfigurationsResponse{}, nil) + + computeManagementClient.EXPECT().CreateInstanceConfiguration(gomock.Any(), gomock.Eq(core.CreateInstanceConfigurationRequest{ + CreateInstanceConfiguration: core.CreateInstanceConfigurationDetails{ + DefinedTags: definedTagsInterface, + DisplayName: common.String("test-20"), + FreeformTags: tags, + CompartmentId: common.String("test-compartment"), + InstanceDetails: core.ComputeInstanceDetails{ + LaunchDetails: &core.InstanceConfigurationLaunchInstanceDetails{ + DefinedTags: definedTagsInterface, + FreeformTags: tags, + DisplayName: common.String("test"), + CompartmentId: common.String("test-compartment"), + CreateVnicDetails: &core.InstanceConfigurationCreateVnicDetails{ + DefinedTags: definedTagsInterface, + FreeformTags: tags, + NsgIds: []string{"nsg-id"}, + AssignPublicIp: common.Bool(true), + SkipSourceDestCheck: common.Bool(true), + SubnetId: common.String("subnet-id"), + HostnameLabel: common.String("test"), + DisplayName: common.String("test-display"), + AssignPrivateDnsRecord: common.Bool(true), + }, + PlatformConfig: core.AmdVmPlatformConfig{ + IsMeasuredBootEnabled: common.Bool(false), + IsTrustedPlatformModuleEnabled: common.Bool(true), + IsSecureBootEnabled: common.Bool(true), + IsMemoryEncryptionEnabled: common.Bool(true), + }, + Metadata: map[string]string{"user_data": "dGVzdA=="}, + Shape: common.String("test-shape"), + ShapeConfig: &core.InstanceConfigurationLaunchInstanceShapeConfigDetails{ + Ocpus: common.Float32(2), + MemoryInGBs: common.Float32(65), + BaselineOcpuUtilization: "BASELINE_1_1", + Nvmes: common.Int(5), + }, + AgentConfig: &core.InstanceConfigurationLaunchInstanceAgentConfigDetails{ + IsMonitoringDisabled: nil, + IsManagementDisabled: nil, + AreAllPluginsDisabled: nil, + PluginsConfig: []core.InstanceAgentPluginConfigDetails{ + { + Name: nil, + DesiredState: core.InstanceAgentPluginConfigDetailsDesiredStateEnabled, + }, + }, + }, + SourceDetails: core.InstanceConfigurationInstanceSourceViaImageDetails{}, + }, + }, + }, + })). + Return(core.CreateInstanceConfigurationResponse{ + InstanceConfiguration: core.InstanceConfiguration{ + Id: common.String("id"), + }, + }, nil) + + }, + }, { name: "instance config update", errorExpected: false, diff --git a/cloud/scope/machine_test.go b/cloud/scope/machine_test.go index 63be9dee..c7086394 100644 --- a/cloud/scope/machine_test.go +++ b/cloud/scope/machine_test.go @@ -671,6 +671,7 @@ func TestInstanceReconciliation(t *testing.T) { SourceDetails: core.InstanceSourceViaImageDetails{ ImageId: common.String("image"), BootVolumeSizeInGBs: common.Int64(120), + KmsKeyId: nil, }, CreateVnicDetails: &core.CreateVnicDetails{ SubnetId: common.String("nodesubnet"), @@ -682,6 +683,11 @@ func TestInstanceReconciliation(t *testing.T) { ociutil.ClusterResourceIdentifier: "resource_uid", }, NsgIds: make([]string, 0), + // Explicitly calling out nil checks here + HostnameLabel: nil, + SkipSourceDestCheck: nil, + DisplayName: nil, + AssignPrivateDnsRecord: nil, }, Metadata: map[string]string{ "user_data": base64.StdEncoding.EncodeToString([]byte("test")), @@ -1203,6 +1209,41 @@ func TestInstanceReconciliation(t *testing.T) { })).Return(core.LaunchInstanceResponse{}, nil) }, }, + { + name: "agent config - LaunchInstanceAgentConfigDetails has nil properties", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.AgentConfig = &infrastructurev1beta2.LaunchInstanceAgentConfig{ + IsMonitoringDisabled: nil, + IsManagementDisabled: nil, + AreAllPluginsDisabled: nil, + PluginsConfig: []infrastructurev1beta2.InstanceAgentPluginConfig{ + { + Name: nil, + DesiredState: infrastructurev1beta2.InstanceAgentPluginConfigDetailsDesiredStateEnabled, + }, + }, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return agentConfigMatcher(request, &core.LaunchInstanceAgentConfigDetails{ + IsMonitoringDisabled: nil, + IsManagementDisabled: nil, + AreAllPluginsDisabled: nil, + PluginsConfig: []core.InstanceAgentPluginConfigDetails{ + { + Name: nil, + DesiredState: core.InstanceAgentPluginConfigDetailsDesiredStateEnabled, + }, + }, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, { name: "launch options", errorExpected: false, @@ -1230,6 +1271,32 @@ func TestInstanceReconciliation(t *testing.T) { })).Return(core.LaunchInstanceResponse{}, nil) }, }, + { + name: "launch options - without IsConsistentVolumeNamingEnabled", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.LaunchOptions = &infrastructurev1beta2.LaunchOptions{ + BootVolumeType: infrastructurev1beta2.LaunchOptionsBootVolumeTypeIde, + Firmware: infrastructurev1beta2.LaunchOptionsFirmwareUefi64, + NetworkType: infrastructurev1beta2.LaunchOptionsNetworkTypeVfio, + RemoteDataVolumeType: infrastructurev1beta2.LaunchOptionsRemoteDataVolumeTypeIde, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return launchOptionsMatcher(request, &core.LaunchOptions{ + BootVolumeType: core.LaunchOptionsBootVolumeTypeIde, + Firmware: core.LaunchOptionsFirmwareUefi64, + NetworkType: core.LaunchOptionsNetworkTypeVfio, + RemoteDataVolumeType: core.LaunchOptionsRemoteDataVolumeTypeIde, + IsConsistentVolumeNamingEnabled: nil, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, { name: "instance options", errorExpected: false, @@ -1249,6 +1316,25 @@ func TestInstanceReconciliation(t *testing.T) { })).Return(core.LaunchInstanceResponse{}, nil) }, }, + { + name: "instance options - AreLegacyImdsEndpointsDisabled is nil", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.InstanceOptions = &infrastructurev1beta2.InstanceOptions{ + AreLegacyImdsEndpointsDisabled: nil, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return instanceOptionsMatcher(request, &core.InstanceOptions{ + AreLegacyImdsEndpointsDisabled: nil, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, { name: "availability config", errorExpected: false, @@ -1270,6 +1356,27 @@ func TestInstanceReconciliation(t *testing.T) { })).Return(core.LaunchInstanceResponse{}, nil) }, }, + { + name: "availability config - IsLiveMigrationPreferred nil", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.AvailabilityConfig = &infrastructurev1beta2.LaunchInstanceAvailabilityConfig{ + IsLiveMigrationPreferred: nil, + RecoveryAction: infrastructurev1beta2.LaunchInstanceAvailabilityConfigDetailsRecoveryActionRestoreInstance, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return avalabilityConfigMatcher(request, &core.LaunchInstanceAvailabilityConfigDetails{ + IsLiveMigrationPreferred: nil, + RecoveryAction: core.LaunchInstanceAvailabilityConfigDetailsRecoveryActionRestoreInstance, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, { name: "preemtible config", errorExpected: false, @@ -1293,6 +1400,48 @@ func TestInstanceReconciliation(t *testing.T) { })).Return(core.LaunchInstanceResponse{}, nil) }, }, + { + name: "preemtible config - TerminatePreemptionAction nil", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.PreemptibleInstanceConfig = &infrastructurev1beta2.PreemptibleInstanceConfig{ + TerminatePreemptionAction: nil, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return preemtibleConfigMatcher(request, &core.PreemptibleInstanceConfigDetails{ + PreemptionAction: nil, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, + { + name: "preemtible config - TerminatePreemptionAction.PreserveBootVolume nil", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + setupAllParams(ms) + ms.OCIMachine.Spec.PreemptibleInstanceConfig = &infrastructurev1beta2.PreemptibleInstanceConfig{ + TerminatePreemptionAction: &infrastructurev1beta2.TerminatePreemptionAction{ + PreserveBootVolume: nil, + }, + } + computeClient.EXPECT().ListInstances(gomock.Any(), gomock.Eq(core.ListInstancesRequest{ + DisplayName: common.String("name"), + CompartmentId: common.String("test"), + })).Return(core.ListInstancesResponse{}, nil) + computeClient.EXPECT().LaunchInstance(gomock.Any(), Eq(func(request interface{}) error { + return preemtibleConfigMatcher(request, &core.PreemptibleInstanceConfigDetails{ + PreemptionAction: core.TerminatePreemptionAction{ + PreserveBootVolume: nil, + }, + }) + })).Return(core.LaunchInstanceResponse{}, nil) + }, + }, } for _, tc := range tests { diff --git a/cloud/scope/vnic_reconciler.go b/cloud/scope/vnic_reconciler.go index c3371411..728aa25c 100644 --- a/cloud/scope/vnic_reconciler.go +++ b/cloud/scope/vnic_reconciler.go @@ -19,6 +19,7 @@ package scope import ( "context" "fmt" + "github.com/oracle/cluster-api-provider-oci/cloud/ociutil" infrastructurev1beta2 "github.com/oracle/cluster-api-provider-oci/api/v1beta2" @@ -41,7 +42,7 @@ func (m *MachineScope) ReconcileVnicAttachments(ctx context.Context) error { vnicId, err := m.createVnicAttachment(ctx, vnicAttachment) if err != nil { msg := fmt.Sprintf("Error creating VnicAttachment %s for cluster %s", - *vnicAttachment.DisplayName, m.Cluster.Name) + ociutil.DerefString(vnicAttachment.DisplayName), m.Cluster.Name) m.Logger.Error(err, msg) return err } diff --git a/cloud/scope/vnic_reconciler_test.go b/cloud/scope/vnic_reconciler_test.go index aa652302..ecd70b15 100644 --- a/cloud/scope/vnic_reconciler_test.go +++ b/cloud/scope/vnic_reconciler_test.go @@ -158,6 +158,49 @@ func TestReconcileVnicAttachment(t *testing.T) { }, nil) }, }, + { + name: "Crete vnic attachment - no display name", + errorExpected: false, + testSpecificSetup: func(machineScope *MachineScope, computeClient *mock_compute.MockComputeClient) { + ms.OCIMachine.Spec.InstanceId = common.String("test") + + // Make sure the DisplayName is in fact nil + ms.OCIMachine.Spec.VnicAttachments[0].DisplayName = nil + + computeClient.EXPECT().ListVnicAttachments(gomock.Any(), gomock.Eq(core.ListVnicAttachmentsRequest{ + InstanceId: common.String("test"), + CompartmentId: common.String("testCompartment"), + })). + Return(core.ListVnicAttachmentsResponse{ + Items: []core.VnicAttachment{ + { + InstanceId: common.String("test"), + DisplayName: common.String("existing-vnic"), + }, + }, + }, nil) + + computeClient.EXPECT().AttachVnic(gomock.Any(), gomock.Eq(core.AttachVnicRequest{ + AttachVnicDetails: core.AttachVnicDetails{ + NicIndex: common.Int(0), + InstanceId: common.String("test"), + CreateVnicDetails: &core.CreateVnicDetails{ + // explicitly check DisplayName is nil + DisplayName: nil, + AssignPublicIp: common.Bool(false), + DefinedTags: map[string]map[string]interface{}{}, + FreeformTags: map[string]string{ + ociutil.CreatedBy: ociutil.OCIClusterAPIProvider, + ociutil.ClusterResourceIdentifier: "resource_uid", + }, + NsgIds: make([]string, 0), + }, + }})). + Return(core.AttachVnicResponse{ + VnicAttachment: core.VnicAttachment{Id: common.String("vnic.id")}, + }, nil) + }, + }, { name: "Crete vnic attachment error", errorExpected: true,