Skip to content

Commit 897447d

Browse files
joekrjoekr
authored
fix: handle nil pointer dereference issues (#467)
1 parent 18987df commit 897447d

File tree

6 files changed

+427
-0
lines changed

6 files changed

+427
-0
lines changed

‎cloud/scope/route_table_reconciler.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,13 @@ func (s *ClusterScope) CreateRouteTable(ctx context.Context, routeTableType stri
134134
}
135135
vcnPeering := s.OCIClusterAccessor.GetNetworkSpec().VCNPeering
136136
if vcnPeering != nil {
137+
if s.getDRG() == nil {
138+
return nil, errors.New("Create Route Table: DRG has not been specified")
139+
}
140+
if s.getDrgID() == nil {
141+
return nil, errors.New("Create Route Table: DRG ID has not been set")
142+
}
143+
137144
for _, routeRule := range vcnPeering.PeerRouteRules {
138145
routeRules = append(routeRules, core.RouteRule{
139146
DestinationType: core.RouteRuleDestinationTypeCidrBlock,

‎cloud/scope/route_table_reconciler_test.go‎

Lines changed: 287 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,293 @@ func TestClusterScope_ReconcileRouteTable(t *testing.T) {
458458
}
459459
}
460460

461+
func TestClusterScope_CreateRouteTable(t *testing.T) {
462+
type args struct {
463+
routeTableType string
464+
spec infrastructurev1beta2.OCIClusterSpec
465+
setupMock func(t *testing.T, vcn *mock_vcn.MockClient)
466+
}
467+
tests := []struct {
468+
name string
469+
args args
470+
wantErr bool
471+
expectedErrorMessage string
472+
}{
473+
{
474+
name: "private without peering (NAT + Service routes only)",
475+
args: args{
476+
routeTableType: infrastructurev1beta2.Private,
477+
spec: infrastructurev1beta2.OCIClusterSpec{
478+
CompartmentId: "comp",
479+
NetworkSpec: infrastructurev1beta2.NetworkSpec{
480+
Vcn: infrastructurev1beta2.VCN{
481+
ID: common.String("vcn1"),
482+
NATGateway: infrastructurev1beta2.NATGateway{
483+
Id: common.String("ngw"),
484+
},
485+
ServiceGateway: infrastructurev1beta2.ServiceGateway{
486+
Id: common.String("sgw"),
487+
},
488+
},
489+
},
490+
},
491+
setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) {
492+
_ = core.CreateRouteTableRequest{
493+
CreateRouteTableDetails: core.CreateRouteTableDetails{
494+
VcnId: common.String("vcn1"),
495+
CompartmentId: common.String("comp"),
496+
DisplayName: common.String(PrivateRouteTableName),
497+
FreeformTags: map[string]string{},
498+
DefinedTags: map[string]map[string]interface{}{},
499+
RouteRules: []core.RouteRule{
500+
{
501+
DestinationType: core.RouteRuleDestinationTypeCidrBlock,
502+
Destination: common.String("0.0.0.0/0"),
503+
NetworkEntityId: common.String("ngw"),
504+
Description: common.String("traffic to the internet"),
505+
},
506+
{
507+
DestinationType: core.RouteRuleDestinationTypeServiceCidrBlock,
508+
Destination: common.String("all-iad-services-in-oracle-services-network"),
509+
NetworkEntityId: common.String("sgw"),
510+
Description: common.String("traffic to OCI services"),
511+
},
512+
},
513+
},
514+
}
515+
vcn.EXPECT().
516+
CreateRouteTable(gomock.Any(), gomock.Any()).
517+
DoAndReturn(func(_ context.Context, req core.CreateRouteTableRequest) (core.CreateRouteTableResponse, error) {
518+
// Validate core fields; ignore tags differences
519+
if req.CreateRouteTableDetails.VcnId == nil || *req.CreateRouteTableDetails.VcnId != "vcn1" {
520+
t.Errorf("expected VcnId=vcn1, got %v", req.CreateRouteTableDetails.VcnId)
521+
}
522+
if req.CreateRouteTableDetails.CompartmentId == nil || *req.CreateRouteTableDetails.CompartmentId != "comp" {
523+
t.Errorf("expected CompartmentId=comp, got %v", req.CreateRouteTableDetails.CompartmentId)
524+
}
525+
if req.CreateRouteTableDetails.DisplayName == nil || *req.CreateRouteTableDetails.DisplayName != PrivateRouteTableName {
526+
t.Errorf("expected DisplayName=%s, got %v", PrivateRouteTableName, req.CreateRouteTableDetails.DisplayName)
527+
}
528+
rr := req.CreateRouteTableDetails.RouteRules
529+
if len(rr) != 2 {
530+
t.Fatalf("expected 2 route rules, got %d", len(rr))
531+
}
532+
// NAT route
533+
if rr[0].Destination == nil || *rr[0].Destination != "0.0.0.0/0" {
534+
t.Errorf("expected rule[0] dest 0.0.0.0/0, got %v", rr[0].Destination)
535+
}
536+
if rr[0].NetworkEntityId == nil || *rr[0].NetworkEntityId != "ngw" {
537+
t.Errorf("expected rule[0] NEI=ngw, got %v", rr[0].NetworkEntityId)
538+
}
539+
// Service gateway route
540+
if rr[1].Destination == nil || *rr[1].Destination != "all-iad-services-in-oracle-services-network" {
541+
t.Errorf("expected rule[1] dest all-iad-services-in-oracle-services-network, got %v", rr[1].Destination)
542+
}
543+
if rr[1].NetworkEntityId == nil || *rr[1].NetworkEntityId != "sgw" {
544+
t.Errorf("expected rule[1] NEI=sgw, got %v", rr[1].NetworkEntityId)
545+
}
546+
return core.CreateRouteTableResponse{RouteTable: core.RouteTable{Id: common.String("rt")}}, nil
547+
})
548+
},
549+
},
550+
wantErr: false,
551+
},
552+
{
553+
name: "private with peering (adds DRG peer routes)",
554+
args: args{
555+
routeTableType: infrastructurev1beta2.Private,
556+
spec: infrastructurev1beta2.OCIClusterSpec{
557+
CompartmentId: "comp",
558+
NetworkSpec: infrastructurev1beta2.NetworkSpec{
559+
VCNPeering: &infrastructurev1beta2.VCNPeering{
560+
DRG: &infrastructurev1beta2.DRG{ID: common.String("drg-id")},
561+
PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{
562+
{VCNCIDRRange: "10.1.0.0/16"},
563+
},
564+
},
565+
Vcn: infrastructurev1beta2.VCN{
566+
ID: common.String("vcn1"),
567+
NATGateway: infrastructurev1beta2.NATGateway{
568+
Id: common.String("ngw"),
569+
},
570+
ServiceGateway: infrastructurev1beta2.ServiceGateway{
571+
Id: common.String("sgw"),
572+
},
573+
},
574+
},
575+
},
576+
setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) {
577+
_ = core.CreateRouteTableRequest{
578+
CreateRouteTableDetails: core.CreateRouteTableDetails{
579+
VcnId: common.String("vcn1"),
580+
CompartmentId: common.String("comp"),
581+
DisplayName: common.String(PrivateRouteTableName),
582+
FreeformTags: map[string]string{},
583+
DefinedTags: map[string]map[string]interface{}{},
584+
RouteRules: []core.RouteRule{
585+
{
586+
DestinationType: core.RouteRuleDestinationTypeCidrBlock,
587+
Destination: common.String("0.0.0.0/0"),
588+
NetworkEntityId: common.String("ngw"),
589+
Description: common.String("traffic to the internet"),
590+
},
591+
{
592+
DestinationType: core.RouteRuleDestinationTypeServiceCidrBlock,
593+
Destination: common.String("all-iad-services-in-oracle-services-network"),
594+
NetworkEntityId: common.String("sgw"),
595+
Description: common.String("traffic to OCI services"),
596+
},
597+
{
598+
DestinationType: core.RouteRuleDestinationTypeCidrBlock,
599+
Destination: common.String("10.1.0.0/16"),
600+
NetworkEntityId: common.String("drg-id"),
601+
Description: common.String("traffic to peer DRG"),
602+
},
603+
},
604+
},
605+
}
606+
vcn.EXPECT().
607+
CreateRouteTable(gomock.Any(), gomock.Any()).
608+
DoAndReturn(func(_ context.Context, req core.CreateRouteTableRequest) (core.CreateRouteTableResponse, error) {
609+
// Validate core fields; ignore tags differences
610+
if req.CreateRouteTableDetails.VcnId == nil || *req.CreateRouteTableDetails.VcnId != "vcn1" {
611+
t.Errorf("expected VcnId=vcn1, got %v", req.CreateRouteTableDetails.VcnId)
612+
}
613+
if req.CreateRouteTableDetails.CompartmentId == nil || *req.CreateRouteTableDetails.CompartmentId != "comp" {
614+
t.Errorf("expected CompartmentId=comp, got %v", req.CreateRouteTableDetails.CompartmentId)
615+
}
616+
if req.CreateRouteTableDetails.DisplayName == nil || *req.CreateRouteTableDetails.DisplayName != PrivateRouteTableName {
617+
t.Errorf("expected DisplayName=%s, got %v", PrivateRouteTableName, req.CreateRouteTableDetails.DisplayName)
618+
}
619+
rr := req.CreateRouteTableDetails.RouteRules
620+
if len(rr) != 3 {
621+
t.Fatalf("expected 3 route rules, got %d", len(rr))
622+
}
623+
// NAT route
624+
if rr[0].Destination == nil || *rr[0].Destination != "0.0.0.0/0" {
625+
t.Errorf("expected rule[0] dest 0.0.0.0/0, got %v", rr[0].Destination)
626+
}
627+
if rr[0].NetworkEntityId == nil || *rr[0].NetworkEntityId != "ngw" {
628+
t.Errorf("expected rule[0] NEI=ngw, got %v", rr[0].NetworkEntityId)
629+
}
630+
// Service gateway route
631+
if rr[1].Destination == nil || *rr[1].Destination != "all-iad-services-in-oracle-services-network" {
632+
t.Errorf("expected rule[1] dest all-iad-services-in-oracle-services-network, got %v", rr[1].Destination)
633+
}
634+
if rr[1].NetworkEntityId == nil || *rr[1].NetworkEntityId != "sgw" {
635+
t.Errorf("expected rule[1] NEI=sgw, got %v", rr[1].NetworkEntityId)
636+
}
637+
// DRG peering route
638+
if rr[2].Destination == nil || *rr[2].Destination != "10.1.0.0/16" {
639+
t.Errorf("expected rule[2] dest 10.1.0.0/16, got %v", rr[2].Destination)
640+
}
641+
if rr[2].NetworkEntityId == nil || *rr[2].NetworkEntityId != "drg-id" {
642+
t.Errorf("expected rule[2] NEI=drg-id, got %v", rr[2].NetworkEntityId)
643+
}
644+
return core.CreateRouteTableResponse{RouteTable: core.RouteTable{Id: common.String("rt")}}, nil
645+
})
646+
},
647+
},
648+
wantErr: false,
649+
},
650+
{
651+
name: "private with peering but DRG is nil",
652+
args: args{
653+
routeTableType: infrastructurev1beta2.Private,
654+
spec: infrastructurev1beta2.OCIClusterSpec{
655+
CompartmentId: "comp",
656+
NetworkSpec: infrastructurev1beta2.NetworkSpec{
657+
VCNPeering: &infrastructurev1beta2.VCNPeering{
658+
DRG: nil, // Intentionally set to nil
659+
PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{{VCNCIDRRange: "10.1.0.0/16"}},
660+
RemotePeeringConnections: nil,
661+
},
662+
Vcn: infrastructurev1beta2.VCN{
663+
ID: common.String("vcn1"),
664+
NATGateway: infrastructurev1beta2.NATGateway{
665+
Id: common.String("ngw"),
666+
},
667+
ServiceGateway: infrastructurev1beta2.ServiceGateway{
668+
Id: common.String("sgw"),
669+
},
670+
},
671+
},
672+
},
673+
setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) {
674+
// No CreateRouteTable call expected because we error out before
675+
vcn.EXPECT().CreateRouteTable(gomock.Any(), gomock.Any()).Times(0)
676+
},
677+
},
678+
wantErr: true,
679+
expectedErrorMessage: "Create Route Table: DRG has not been specified",
680+
},
681+
{
682+
name: "private with peering but DRG ID is nil",
683+
args: args{
684+
routeTableType: infrastructurev1beta2.Private,
685+
spec: infrastructurev1beta2.OCIClusterSpec{
686+
CompartmentId: "comp",
687+
NetworkSpec: infrastructurev1beta2.NetworkSpec{
688+
VCNPeering: &infrastructurev1beta2.VCNPeering{
689+
DRG: &infrastructurev1beta2.DRG{ID: nil},
690+
PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{{VCNCIDRRange: "10.1.0.0/16"}},
691+
},
692+
Vcn: infrastructurev1beta2.VCN{
693+
ID: common.String("vcn1"),
694+
NATGateway: infrastructurev1beta2.NATGateway{
695+
Id: common.String("ngw"),
696+
},
697+
ServiceGateway: infrastructurev1beta2.ServiceGateway{
698+
Id: common.String("sgw"),
699+
},
700+
},
701+
},
702+
},
703+
setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) {
704+
// No CreateRouteTable call expected because we error out before
705+
vcn.EXPECT().CreateRouteTable(gomock.Any(), gomock.Any()).Times(0)
706+
},
707+
},
708+
wantErr: true,
709+
expectedErrorMessage: "Create Route Table: DRG ID has not been set",
710+
},
711+
}
712+
713+
for _, tt := range tests {
714+
tt := tt
715+
t.Run(tt.name, func(t *testing.T) {
716+
mockCtrl := gomock.NewController(t)
717+
defer mockCtrl.Finish()
718+
vcnClient := mock_vcn.NewMockClient(mockCtrl)
719+
if tt.args.setupMock != nil {
720+
tt.args.setupMock(t, vcnClient)
721+
}
722+
723+
l := log.FromContext(context.Background())
724+
ociClusterAccessor := OCISelfManagedCluster{
725+
OCICluster: &infrastructurev1beta2.OCICluster{
726+
ObjectMeta: metav1.ObjectMeta{UID: "cluster_uid"},
727+
Spec: tt.args.spec,
728+
},
729+
}
730+
s := &ClusterScope{
731+
VCNClient: vcnClient,
732+
OCIClusterAccessor: ociClusterAccessor,
733+
Logger: &l,
734+
RegionKey: "iad",
735+
}
736+
737+
_, err := s.CreateRouteTable(context.Background(), tt.args.routeTableType)
738+
if (err != nil) != tt.wantErr {
739+
t.Fatalf("CreateRouteTable() error = %v, wantErr %v", err, tt.wantErr)
740+
}
741+
if err != nil && tt.expectedErrorMessage != "" && err.Error() != tt.expectedErrorMessage {
742+
t.Fatalf("CreateRouteTable() expected error = %q, got %q", tt.expectedErrorMessage, err.Error())
743+
}
744+
})
745+
}
746+
}
747+
461748
func TestClusterScope_DeleteRouteTables(t *testing.T) {
462749
mockCtrl := gomock.NewController(t)
463750
defer mockCtrl.Finish()

‎cloud/scope/security_list_reconciler.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,11 @@ func (s *ClusterScope) IsSecurityListEqual(actual core.SecurityList, desired inf
130130
func (s *ClusterScope) UpdateSecurityList(ctx context.Context, securityListSpec infrastructurev1beta2.SecurityList) error {
131131
var ingressRules []core.IngressSecurityRule
132132
var egressRules []core.EgressSecurityRule
133+
134+
if securityListSpec.ID == nil {
135+
return errors.New("Update Security List failed: unable to update with a nil ID")
136+
}
137+
133138
for _, rule := range securityListSpec.EgressRules {
134139
egressRules = append(egressRules, convertSecurityListEgressRule(rule))
135140
}

0 commit comments

Comments
 (0)