Skip to content

Commit 1cdd003

Browse files
joekrjoekr
authored
Fix: validator validateEgressSecurityRuleForNSG (#458)
The API documentation points out both Destination and Protocol are required https://docs.oracle.com/en-us/iaas/api/#/en/iaas/20160918/datatypes/EgressSecurityRule but our validator doesn't catch those.
1 parent c206fa4 commit 1cdd003

File tree

6 files changed

+438
-1
lines changed

6 files changed

+438
-1
lines changed

‎api/v1beta2/ocicluster_webhook_test.go‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,66 @@ func TestOCICluster_ValidateCreate(t *testing.T) {
357357
errorMgsShouldContain: "invalid egressRules CIDR format",
358358
expectErr: true,
359359
},
360+
{
361+
name: "shouldn't allow empty NSG egress destination",
362+
c: &OCICluster{
363+
ObjectMeta: metav1.ObjectMeta{
364+
Name: goodClusterName,
365+
},
366+
Spec: OCIClusterSpec{
367+
CompartmentId: "ocid",
368+
OCIResourceIdentifier: "uuid",
369+
NetworkSpec: NetworkSpec{
370+
Vcn: VCN{
371+
NetworkSecurityGroup: NetworkSecurityGroup{
372+
List: []*NSG{{
373+
Role: Custom,
374+
EgressRules: []EgressSecurityRuleForNSG{{
375+
EgressSecurityRule: EgressSecurityRule{
376+
Destination: nil,
377+
DestinationType: EgressSecurityRuleDestinationTypeCidrBlock,
378+
Protocol: common.String("all"),
379+
},
380+
}},
381+
}},
382+
},
383+
},
384+
},
385+
},
386+
},
387+
errorMgsShouldContain: "invalid egressRules: Destination may not be empty",
388+
expectErr: true,
389+
},
390+
{
391+
name: "shouldn't allow empty NSG egress protocol",
392+
c: &OCICluster{
393+
ObjectMeta: metav1.ObjectMeta{
394+
Name: goodClusterName,
395+
},
396+
Spec: OCIClusterSpec{
397+
CompartmentId: "ocid",
398+
OCIResourceIdentifier: "uuid",
399+
NetworkSpec: NetworkSpec{
400+
Vcn: VCN{
401+
NetworkSecurityGroup: NetworkSecurityGroup{
402+
List: []*NSG{{
403+
Role: Custom,
404+
EgressRules: []EgressSecurityRuleForNSG{{
405+
EgressSecurityRule: EgressSecurityRule{
406+
Destination: common.String("10.0.0.0/15"),
407+
DestinationType: EgressSecurityRuleDestinationTypeCidrBlock,
408+
Protocol: nil,
409+
},
410+
}},
411+
}},
412+
},
413+
},
414+
},
415+
},
416+
},
417+
errorMgsShouldContain: "invalid egressRules: Protocol may not be empty",
418+
expectErr: true,
419+
},
360420
{
361421
name: "shouldn't allow bad NSG ingress cidr",
362422
c: &OCICluster{
@@ -383,6 +443,66 @@ func TestOCICluster_ValidateCreate(t *testing.T) {
383443
errorMgsShouldContain: "invalid ingressRule CIDR format",
384444
expectErr: true,
385445
},
446+
{
447+
name: "shouldn't allow empty NSG ingress protocol",
448+
c: &OCICluster{
449+
ObjectMeta: metav1.ObjectMeta{
450+
Name: goodClusterName,
451+
},
452+
Spec: OCIClusterSpec{
453+
CompartmentId: "ocid",
454+
OCIResourceIdentifier: "uuid",
455+
NetworkSpec: NetworkSpec{
456+
Vcn: VCN{
457+
NetworkSecurityGroup: NetworkSecurityGroup{
458+
List: []*NSG{{
459+
Role: Custom,
460+
IngressRules: []IngressSecurityRuleForNSG{{
461+
IngressSecurityRule: IngressSecurityRule{
462+
Source: common.String("10.0.0.0/15"),
463+
SourceType: IngressSecurityRuleSourceTypeCidrBlock,
464+
Protocol: nil,
465+
},
466+
}},
467+
}},
468+
},
469+
},
470+
},
471+
},
472+
},
473+
errorMgsShouldContain: "invalid ingressRules: Protocol may not be empty",
474+
expectErr: true,
475+
},
476+
{
477+
name: "shouldn't allow empty NSG ingress source",
478+
c: &OCICluster{
479+
ObjectMeta: metav1.ObjectMeta{
480+
Name: goodClusterName,
481+
},
482+
Spec: OCIClusterSpec{
483+
CompartmentId: "ocid",
484+
OCIResourceIdentifier: "uuid",
485+
NetworkSpec: NetworkSpec{
486+
Vcn: VCN{
487+
NetworkSecurityGroup: NetworkSecurityGroup{
488+
List: []*NSG{{
489+
Role: Custom,
490+
IngressRules: []IngressSecurityRuleForNSG{{
491+
IngressSecurityRule: IngressSecurityRule{
492+
Source: nil,
493+
SourceType: IngressSecurityRuleSourceTypeCidrBlock,
494+
Protocol: common.String("all"),
495+
},
496+
}},
497+
}},
498+
},
499+
},
500+
},
501+
},
502+
},
503+
errorMgsShouldContain: "invalid ingressRules: Source may not be empty",
504+
expectErr: true,
505+
},
386506
{
387507
name: "shouldn't allow bad NSG role",
388508
c: &OCICluster{

‎api/v1beta2/validator.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,15 @@ func validateEgressSecurityRuleForNSG(egressRules []EgressSecurityRuleForNSG, fl
167167
for _, r := range egressRules {
168168
rule := r.EgressSecurityRule
169169

170+
// nsg_reconciler will set the service destination if not set for `SERVICE_CIDR_BLOCK` destination type
171+
if rule.DestinationType != EgressSecurityRuleDestinationTypeServiceCidrBlock && rule.Destination == nil {
172+
allErrs = append(allErrs, field.Invalid(fldPath, rule.Destination, "invalid egressRules: Destination may not be empty"))
173+
}
174+
175+
if rule.Protocol == nil {
176+
allErrs = append(allErrs, field.Invalid(fldPath, rule.Protocol, "invalid egressRules: Protocol may not be empty"))
177+
}
178+
170179
if rule.DestinationType == EgressSecurityRuleDestinationTypeCidrBlock && rule.Destination != nil {
171180
if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Destination)); err != nil {
172181
allErrs = append(allErrs, field.Invalid(fldPath, rule.Destination, "invalid egressRules CIDR format"))
@@ -184,6 +193,15 @@ func validateIngressSecurityRuleForNSG(egressRules []IngressSecurityRuleForNSG,
184193
for _, r := range egressRules {
185194
rule := r.IngressSecurityRule
186195

196+
// nsg_reconciler will set the service source if not set for `SERVICE_CIDR_BLOCK` destination type
197+
if rule.SourceType != IngressSecurityRuleSourceTypeServiceCidrBlock && rule.Source == nil {
198+
allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "invalid ingressRules: Source may not be empty"))
199+
}
200+
201+
if rule.Protocol == nil {
202+
allErrs = append(allErrs, field.Invalid(fldPath, rule.Protocol, "invalid ingressRules: Protocol may not be empty"))
203+
}
204+
187205
if rule.SourceType == IngressSecurityRuleSourceTypeCidrBlock && rule.Source != nil {
188206
if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Source)); err != nil {
189207
allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "invalid ingressRule CIDR format"))

‎cloud/scope/machine_pool_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,112 @@ func TestInstanceConfigCreate(t *testing.T) {
309309

310310
},
311311
},
312+
{
313+
name: "instance config create - LaunchInstanceAgentConfig contains nil",
314+
errorExpected: false,
315+
testSpecificSetup: func(ms *MachinePoolScope) {
316+
ms.OCIMachinePool.Spec.InstanceConfiguration = infrav2exp.InstanceConfiguration{
317+
Shape: common.String("test-shape"),
318+
ShapeConfig: &infrav2exp.ShapeConfig{
319+
Ocpus: common.String("2"),
320+
MemoryInGBs: common.String("65"),
321+
BaselineOcpuUtilization: "BASELINE_1_1",
322+
Nvmes: common.Int(5),
323+
},
324+
InstanceVnicConfiguration: &infrastructurev1beta2.NetworkDetails{
325+
AssignPublicIp: true,
326+
SubnetName: "worker-subnet",
327+
SkipSourceDestCheck: common.Bool(true),
328+
NsgNames: []string{"worker-nsg"},
329+
HostnameLabel: common.String("test"),
330+
DisplayName: common.String("test-display"),
331+
AssignPrivateDnsRecord: common.Bool(true),
332+
},
333+
PlatformConfig: &infrastructurev1beta2.PlatformConfig{
334+
PlatformConfigType: infrastructurev1beta2.PlatformConfigTypeAmdvm,
335+
AmdVmPlatformConfig: infrastructurev1beta2.AmdVmPlatformConfig{
336+
IsMeasuredBootEnabled: common.Bool(false),
337+
IsTrustedPlatformModuleEnabled: common.Bool(true),
338+
IsSecureBootEnabled: common.Bool(true),
339+
IsMemoryEncryptionEnabled: common.Bool(true),
340+
},
341+
},
342+
AgentConfig: &infrastructurev1beta2.LaunchInstanceAgentConfig{
343+
IsMonitoringDisabled: nil,
344+
IsManagementDisabled: nil,
345+
AreAllPluginsDisabled: nil,
346+
PluginsConfig: []infrastructurev1beta2.InstanceAgentPluginConfig{
347+
{
348+
Name: nil,
349+
DesiredState: infrastructurev1beta2.InstanceAgentPluginConfigDetailsDesiredStateEnabled,
350+
},
351+
},
352+
},
353+
}
354+
computeManagementClient.EXPECT().ListInstanceConfigurations(gomock.Any(), gomock.Any()).
355+
Return(core.ListInstanceConfigurationsResponse{}, nil)
356+
357+
computeManagementClient.EXPECT().CreateInstanceConfiguration(gomock.Any(), gomock.Eq(core.CreateInstanceConfigurationRequest{
358+
CreateInstanceConfiguration: core.CreateInstanceConfigurationDetails{
359+
DefinedTags: definedTagsInterface,
360+
DisplayName: common.String("test-20"),
361+
FreeformTags: tags,
362+
CompartmentId: common.String("test-compartment"),
363+
InstanceDetails: core.ComputeInstanceDetails{
364+
LaunchDetails: &core.InstanceConfigurationLaunchInstanceDetails{
365+
DefinedTags: definedTagsInterface,
366+
FreeformTags: tags,
367+
DisplayName: common.String("test"),
368+
CompartmentId: common.String("test-compartment"),
369+
CreateVnicDetails: &core.InstanceConfigurationCreateVnicDetails{
370+
DefinedTags: definedTagsInterface,
371+
FreeformTags: tags,
372+
NsgIds: []string{"nsg-id"},
373+
AssignPublicIp: common.Bool(true),
374+
SkipSourceDestCheck: common.Bool(true),
375+
SubnetId: common.String("subnet-id"),
376+
HostnameLabel: common.String("test"),
377+
DisplayName: common.String("test-display"),
378+
AssignPrivateDnsRecord: common.Bool(true),
379+
},
380+
PlatformConfig: core.AmdVmPlatformConfig{
381+
IsMeasuredBootEnabled: common.Bool(false),
382+
IsTrustedPlatformModuleEnabled: common.Bool(true),
383+
IsSecureBootEnabled: common.Bool(true),
384+
IsMemoryEncryptionEnabled: common.Bool(true),
385+
},
386+
Metadata: map[string]string{"user_data": "dGVzdA=="},
387+
Shape: common.String("test-shape"),
388+
ShapeConfig: &core.InstanceConfigurationLaunchInstanceShapeConfigDetails{
389+
Ocpus: common.Float32(2),
390+
MemoryInGBs: common.Float32(65),
391+
BaselineOcpuUtilization: "BASELINE_1_1",
392+
Nvmes: common.Int(5),
393+
},
394+
AgentConfig: &core.InstanceConfigurationLaunchInstanceAgentConfigDetails{
395+
IsMonitoringDisabled: nil,
396+
IsManagementDisabled: nil,
397+
AreAllPluginsDisabled: nil,
398+
PluginsConfig: []core.InstanceAgentPluginConfigDetails{
399+
{
400+
Name: nil,
401+
DesiredState: core.InstanceAgentPluginConfigDetailsDesiredStateEnabled,
402+
},
403+
},
404+
},
405+
SourceDetails: core.InstanceConfigurationInstanceSourceViaImageDetails{},
406+
},
407+
},
408+
},
409+
})).
410+
Return(core.CreateInstanceConfigurationResponse{
411+
InstanceConfiguration: core.InstanceConfiguration{
412+
Id: common.String("id"),
413+
},
414+
}, nil)
415+
416+
},
417+
},
312418
{
313419
name: "instance config update",
314420
errorExpected: false,

0 commit comments

Comments
 (0)