From 6292af0f1dd176e5485fe2d47c82c799b199cd2c Mon Sep 17 00:00:00 2001 From: Joe Kratzat Date: Wed, 22 Oct 2025 12:39:46 -0400 Subject: [PATCH] fix: handle nil pointer dereference issues --- cloud/scope/route_table_reconciler.go | 7 + cloud/scope/route_table_reconciler_test.go | 287 +++++++++++++++++++ cloud/scope/security_list_reconciler.go | 5 + cloud/scope/security_list_reconciler_test.go | 102 +++++++ cloud/scope/subnet_reconciler.go | 3 + cloud/scope/subnet_reconciler_test.go | 23 ++ 6 files changed, 427 insertions(+) diff --git a/cloud/scope/route_table_reconciler.go b/cloud/scope/route_table_reconciler.go index b6c18b6fa..3c74c85cc 100644 --- a/cloud/scope/route_table_reconciler.go +++ b/cloud/scope/route_table_reconciler.go @@ -134,6 +134,13 @@ func (s *ClusterScope) CreateRouteTable(ctx context.Context, routeTableType stri } vcnPeering := s.OCIClusterAccessor.GetNetworkSpec().VCNPeering if vcnPeering != nil { + if s.getDRG() == nil { + return nil, errors.New("Create Route Table: DRG has not been specified") + } + if s.getDrgID() == nil { + return nil, errors.New("Create Route Table: DRG ID has not been set") + } + for _, routeRule := range vcnPeering.PeerRouteRules { routeRules = append(routeRules, core.RouteRule{ DestinationType: core.RouteRuleDestinationTypeCidrBlock, diff --git a/cloud/scope/route_table_reconciler_test.go b/cloud/scope/route_table_reconciler_test.go index 7ad3a6eba..1b44b4419 100644 --- a/cloud/scope/route_table_reconciler_test.go +++ b/cloud/scope/route_table_reconciler_test.go @@ -458,6 +458,293 @@ func TestClusterScope_ReconcileRouteTable(t *testing.T) { } } +func TestClusterScope_CreateRouteTable(t *testing.T) { + type args struct { + routeTableType string + spec infrastructurev1beta2.OCIClusterSpec + setupMock func(t *testing.T, vcn *mock_vcn.MockClient) + } + tests := []struct { + name string + args args + wantErr bool + expectedErrorMessage string + }{ + { + name: "private without peering (NAT + Service routes only)", + args: args{ + routeTableType: infrastructurev1beta2.Private, + spec: infrastructurev1beta2.OCIClusterSpec{ + CompartmentId: "comp", + NetworkSpec: infrastructurev1beta2.NetworkSpec{ + Vcn: infrastructurev1beta2.VCN{ + ID: common.String("vcn1"), + NATGateway: infrastructurev1beta2.NATGateway{ + Id: common.String("ngw"), + }, + ServiceGateway: infrastructurev1beta2.ServiceGateway{ + Id: common.String("sgw"), + }, + }, + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + _ = core.CreateRouteTableRequest{ + CreateRouteTableDetails: core.CreateRouteTableDetails{ + VcnId: common.String("vcn1"), + CompartmentId: common.String("comp"), + DisplayName: common.String(PrivateRouteTableName), + FreeformTags: map[string]string{}, + DefinedTags: map[string]map[string]interface{}{}, + RouteRules: []core.RouteRule{ + { + DestinationType: core.RouteRuleDestinationTypeCidrBlock, + Destination: common.String("0.0.0.0/0"), + NetworkEntityId: common.String("ngw"), + Description: common.String("traffic to the internet"), + }, + { + DestinationType: core.RouteRuleDestinationTypeServiceCidrBlock, + Destination: common.String("all-iad-services-in-oracle-services-network"), + NetworkEntityId: common.String("sgw"), + Description: common.String("traffic to OCI services"), + }, + }, + }, + } + vcn.EXPECT(). + CreateRouteTable(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req core.CreateRouteTableRequest) (core.CreateRouteTableResponse, error) { + // Validate core fields; ignore tags differences + if req.CreateRouteTableDetails.VcnId == nil || *req.CreateRouteTableDetails.VcnId != "vcn1" { + t.Errorf("expected VcnId=vcn1, got %v", req.CreateRouteTableDetails.VcnId) + } + if req.CreateRouteTableDetails.CompartmentId == nil || *req.CreateRouteTableDetails.CompartmentId != "comp" { + t.Errorf("expected CompartmentId=comp, got %v", req.CreateRouteTableDetails.CompartmentId) + } + if req.CreateRouteTableDetails.DisplayName == nil || *req.CreateRouteTableDetails.DisplayName != PrivateRouteTableName { + t.Errorf("expected DisplayName=%s, got %v", PrivateRouteTableName, req.CreateRouteTableDetails.DisplayName) + } + rr := req.CreateRouteTableDetails.RouteRules + if len(rr) != 2 { + t.Fatalf("expected 2 route rules, got %d", len(rr)) + } + // NAT route + if rr[0].Destination == nil || *rr[0].Destination != "0.0.0.0/0" { + t.Errorf("expected rule[0] dest 0.0.0.0/0, got %v", rr[0].Destination) + } + if rr[0].NetworkEntityId == nil || *rr[0].NetworkEntityId != "ngw" { + t.Errorf("expected rule[0] NEI=ngw, got %v", rr[0].NetworkEntityId) + } + // Service gateway route + if rr[1].Destination == nil || *rr[1].Destination != "all-iad-services-in-oracle-services-network" { + t.Errorf("expected rule[1] dest all-iad-services-in-oracle-services-network, got %v", rr[1].Destination) + } + if rr[1].NetworkEntityId == nil || *rr[1].NetworkEntityId != "sgw" { + t.Errorf("expected rule[1] NEI=sgw, got %v", rr[1].NetworkEntityId) + } + return core.CreateRouteTableResponse{RouteTable: core.RouteTable{Id: common.String("rt")}}, nil + }) + }, + }, + wantErr: false, + }, + { + name: "private with peering (adds DRG peer routes)", + args: args{ + routeTableType: infrastructurev1beta2.Private, + spec: infrastructurev1beta2.OCIClusterSpec{ + CompartmentId: "comp", + NetworkSpec: infrastructurev1beta2.NetworkSpec{ + VCNPeering: &infrastructurev1beta2.VCNPeering{ + DRG: &infrastructurev1beta2.DRG{ID: common.String("drg-id")}, + PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{ + {VCNCIDRRange: "10.1.0.0/16"}, + }, + }, + Vcn: infrastructurev1beta2.VCN{ + ID: common.String("vcn1"), + NATGateway: infrastructurev1beta2.NATGateway{ + Id: common.String("ngw"), + }, + ServiceGateway: infrastructurev1beta2.ServiceGateway{ + Id: common.String("sgw"), + }, + }, + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + _ = core.CreateRouteTableRequest{ + CreateRouteTableDetails: core.CreateRouteTableDetails{ + VcnId: common.String("vcn1"), + CompartmentId: common.String("comp"), + DisplayName: common.String(PrivateRouteTableName), + FreeformTags: map[string]string{}, + DefinedTags: map[string]map[string]interface{}{}, + RouteRules: []core.RouteRule{ + { + DestinationType: core.RouteRuleDestinationTypeCidrBlock, + Destination: common.String("0.0.0.0/0"), + NetworkEntityId: common.String("ngw"), + Description: common.String("traffic to the internet"), + }, + { + DestinationType: core.RouteRuleDestinationTypeServiceCidrBlock, + Destination: common.String("all-iad-services-in-oracle-services-network"), + NetworkEntityId: common.String("sgw"), + Description: common.String("traffic to OCI services"), + }, + { + DestinationType: core.RouteRuleDestinationTypeCidrBlock, + Destination: common.String("10.1.0.0/16"), + NetworkEntityId: common.String("drg-id"), + Description: common.String("traffic to peer DRG"), + }, + }, + }, + } + vcn.EXPECT(). + CreateRouteTable(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req core.CreateRouteTableRequest) (core.CreateRouteTableResponse, error) { + // Validate core fields; ignore tags differences + if req.CreateRouteTableDetails.VcnId == nil || *req.CreateRouteTableDetails.VcnId != "vcn1" { + t.Errorf("expected VcnId=vcn1, got %v", req.CreateRouteTableDetails.VcnId) + } + if req.CreateRouteTableDetails.CompartmentId == nil || *req.CreateRouteTableDetails.CompartmentId != "comp" { + t.Errorf("expected CompartmentId=comp, got %v", req.CreateRouteTableDetails.CompartmentId) + } + if req.CreateRouteTableDetails.DisplayName == nil || *req.CreateRouteTableDetails.DisplayName != PrivateRouteTableName { + t.Errorf("expected DisplayName=%s, got %v", PrivateRouteTableName, req.CreateRouteTableDetails.DisplayName) + } + rr := req.CreateRouteTableDetails.RouteRules + if len(rr) != 3 { + t.Fatalf("expected 3 route rules, got %d", len(rr)) + } + // NAT route + if rr[0].Destination == nil || *rr[0].Destination != "0.0.0.0/0" { + t.Errorf("expected rule[0] dest 0.0.0.0/0, got %v", rr[0].Destination) + } + if rr[0].NetworkEntityId == nil || *rr[0].NetworkEntityId != "ngw" { + t.Errorf("expected rule[0] NEI=ngw, got %v", rr[0].NetworkEntityId) + } + // Service gateway route + if rr[1].Destination == nil || *rr[1].Destination != "all-iad-services-in-oracle-services-network" { + t.Errorf("expected rule[1] dest all-iad-services-in-oracle-services-network, got %v", rr[1].Destination) + } + if rr[1].NetworkEntityId == nil || *rr[1].NetworkEntityId != "sgw" { + t.Errorf("expected rule[1] NEI=sgw, got %v", rr[1].NetworkEntityId) + } + // DRG peering route + if rr[2].Destination == nil || *rr[2].Destination != "10.1.0.0/16" { + t.Errorf("expected rule[2] dest 10.1.0.0/16, got %v", rr[2].Destination) + } + if rr[2].NetworkEntityId == nil || *rr[2].NetworkEntityId != "drg-id" { + t.Errorf("expected rule[2] NEI=drg-id, got %v", rr[2].NetworkEntityId) + } + return core.CreateRouteTableResponse{RouteTable: core.RouteTable{Id: common.String("rt")}}, nil + }) + }, + }, + wantErr: false, + }, + { + name: "private with peering but DRG is nil", + args: args{ + routeTableType: infrastructurev1beta2.Private, + spec: infrastructurev1beta2.OCIClusterSpec{ + CompartmentId: "comp", + NetworkSpec: infrastructurev1beta2.NetworkSpec{ + VCNPeering: &infrastructurev1beta2.VCNPeering{ + DRG: nil, // Intentionally set to nil + PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{{VCNCIDRRange: "10.1.0.0/16"}}, + RemotePeeringConnections: nil, + }, + Vcn: infrastructurev1beta2.VCN{ + ID: common.String("vcn1"), + NATGateway: infrastructurev1beta2.NATGateway{ + Id: common.String("ngw"), + }, + ServiceGateway: infrastructurev1beta2.ServiceGateway{ + Id: common.String("sgw"), + }, + }, + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + // No CreateRouteTable call expected because we error out before + vcn.EXPECT().CreateRouteTable(gomock.Any(), gomock.Any()).Times(0) + }, + }, + wantErr: true, + expectedErrorMessage: "Create Route Table: DRG has not been specified", + }, + { + name: "private with peering but DRG ID is nil", + args: args{ + routeTableType: infrastructurev1beta2.Private, + spec: infrastructurev1beta2.OCIClusterSpec{ + CompartmentId: "comp", + NetworkSpec: infrastructurev1beta2.NetworkSpec{ + VCNPeering: &infrastructurev1beta2.VCNPeering{ + DRG: &infrastructurev1beta2.DRG{ID: nil}, + PeerRouteRules: []infrastructurev1beta2.PeerRouteRule{{VCNCIDRRange: "10.1.0.0/16"}}, + }, + Vcn: infrastructurev1beta2.VCN{ + ID: common.String("vcn1"), + NATGateway: infrastructurev1beta2.NATGateway{ + Id: common.String("ngw"), + }, + ServiceGateway: infrastructurev1beta2.ServiceGateway{ + Id: common.String("sgw"), + }, + }, + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + // No CreateRouteTable call expected because we error out before + vcn.EXPECT().CreateRouteTable(gomock.Any(), gomock.Any()).Times(0) + }, + }, + wantErr: true, + expectedErrorMessage: "Create Route Table: DRG ID has not been set", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + vcnClient := mock_vcn.NewMockClient(mockCtrl) + if tt.args.setupMock != nil { + tt.args.setupMock(t, vcnClient) + } + + l := log.FromContext(context.Background()) + ociClusterAccessor := OCISelfManagedCluster{ + OCICluster: &infrastructurev1beta2.OCICluster{ + ObjectMeta: metav1.ObjectMeta{UID: "cluster_uid"}, + Spec: tt.args.spec, + }, + } + s := &ClusterScope{ + VCNClient: vcnClient, + OCIClusterAccessor: ociClusterAccessor, + Logger: &l, + RegionKey: "iad", + } + + _, err := s.CreateRouteTable(context.Background(), tt.args.routeTableType) + if (err != nil) != tt.wantErr { + t.Fatalf("CreateRouteTable() error = %v, wantErr %v", err, tt.wantErr) + } + if err != nil && tt.expectedErrorMessage != "" && err.Error() != tt.expectedErrorMessage { + t.Fatalf("CreateRouteTable() expected error = %q, got %q", tt.expectedErrorMessage, err.Error()) + } + }) + } +} + func TestClusterScope_DeleteRouteTables(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() diff --git a/cloud/scope/security_list_reconciler.go b/cloud/scope/security_list_reconciler.go index 369eab52c..236d1a7c6 100644 --- a/cloud/scope/security_list_reconciler.go +++ b/cloud/scope/security_list_reconciler.go @@ -130,6 +130,11 @@ func (s *ClusterScope) IsSecurityListEqual(actual core.SecurityList, desired inf func (s *ClusterScope) UpdateSecurityList(ctx context.Context, securityListSpec infrastructurev1beta2.SecurityList) error { var ingressRules []core.IngressSecurityRule var egressRules []core.EgressSecurityRule + + if securityListSpec.ID == nil { + return errors.New("Update Security List failed: unable to update with a nil ID") + } + for _, rule := range securityListSpec.EgressRules { egressRules = append(egressRules, convertSecurityListEgressRule(rule)) } diff --git a/cloud/scope/security_list_reconciler_test.go b/cloud/scope/security_list_reconciler_test.go index d90a6481c..785f90c43 100644 --- a/cloud/scope/security_list_reconciler_test.go +++ b/cloud/scope/security_list_reconciler_test.go @@ -206,3 +206,105 @@ func TestClusterScope_DeleteSecurityLists(t *testing.T) { }) } } + +func TestClusterScope_UpdateSecurityLists(t *testing.T) { + type fields struct { + // placeholder for future scope fields if needed + } + type args struct { + spec infrastructurev1beta2.SecurityList + } + tests := []struct { + name string + fields fields + args args + setupMock func(t *testing.T, vcn *mock_vcn.MockClient) + wantErr bool + expectedErrorMessage string + }{ + { + name: "successfully updates security list when ID is set", + args: args{ + spec: infrastructurev1beta2.SecurityList{ + ID: common.String("seclist-id"), + Name: "example-seclist", + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + vcn.EXPECT(). + UpdateSecurityList(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req core.UpdateSecurityListRequest) (core.UpdateSecurityListResponse, error) { + if req.SecurityListId == nil || *req.SecurityListId != "seclist-id" { + t.Errorf("expected SecurityListId 'seclist-id', got %v", req.SecurityListId) + } + return core.UpdateSecurityListResponse{ + SecurityList: core.SecurityList{Id: common.String("seclist-id")}, + }, nil + }) + }, + wantErr: false, + }, + { + name: "returns wrapped error when update fails", + args: args{ + spec: infrastructurev1beta2.SecurityList{ + ID: common.String("seclist-id"), + Name: "example-seclist", + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + vcn.EXPECT(). + UpdateSecurityList(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req core.UpdateSecurityListRequest) (core.UpdateSecurityListResponse, error) { + if req.SecurityListId == nil || *req.SecurityListId != "seclist-id" { + t.Errorf("expected SecurityListId 'seclist-id', got %v", req.SecurityListId) + } + return core.UpdateSecurityListResponse{}, errors.New("some update error") + }) + }, + wantErr: true, + expectedErrorMessage: "failed to reconcile the security list, failed to update: some update error", + }, + { + name: "panics when ID is nil", + args: args{ + spec: infrastructurev1beta2.SecurityList{ + // ID intentionally nil + Name: "example-seclist", + }, + }, + setupMock: func(t *testing.T, vcn *mock_vcn.MockClient) { + // no expectation; nil check occurs before client call + }, + wantErr: true, + expectedErrorMessage: "Update Security List failed: unable to update with a nil ID", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + vcnClient := mock_vcn.NewMockClient(mockCtrl) + if tt.setupMock != nil { + tt.setupMock(t, vcnClient) + } + + l := log.FromContext(context.Background()) + s := &ClusterScope{ + VCNClient: vcnClient, + Logger: &l, + } + + err := s.UpdateSecurityList(context.Background(), tt.args.spec) + + if (err != nil) != tt.wantErr { + t.Fatalf("UpdateSecurityList() error = %v, wantErr %v", err, tt.wantErr) + } + if err != nil && tt.expectedErrorMessage != "" && err.Error() != tt.expectedErrorMessage { + t.Fatalf("UpdateSecurityList() expected error = %q, got %q", tt.expectedErrorMessage, err.Error()) + } + }) + } +} diff --git a/cloud/scope/subnet_reconciler.go b/cloud/scope/subnet_reconciler.go index 857e91641..5feb5d0b5 100644 --- a/cloud/scope/subnet_reconciler.go +++ b/cloud/scope/subnet_reconciler.go @@ -34,6 +34,9 @@ import ( func (s *ClusterScope) ReconcileSubnet(ctx context.Context) error { desiredSubnets := s.OCIClusterAccessor.GetNetworkSpec().Vcn.Subnets for _, desiredSubnet := range desiredSubnets { + if desiredSubnet == nil { + return errors.New("Skipping Subnet reconciliation: Subnet can't be nil") + } if desiredSubnet.Skip { s.Logger.Info("Skipping Subnet reconciliation as per spec") continue diff --git a/cloud/scope/subnet_reconciler_test.go b/cloud/scope/subnet_reconciler_test.go index 8da12ea57..e87328c22 100644 --- a/cloud/scope/subnet_reconciler_test.go +++ b/cloud/scope/subnet_reconciler_test.go @@ -511,6 +511,29 @@ func TestClusterScope_ReconcileSubnet(t *testing.T) { core.CreateSubnetResponse{}, errors.New("some error")) }, }, + { + name: "create subnet error - nil subnets", + spec: infrastructurev1beta2.OCIClusterSpec{ + CompartmentId: "foo", + NetworkSpec: infrastructurev1beta2.NetworkSpec{ + Vcn: infrastructurev1beta2.VCN{ + ID: common.String("vcn"), + RouteTable: infrastructurev1beta2.RouteTable{ + PublicRouteTableId: common.String("public"), + }, + Subnets: []*infrastructurev1beta2.Subnet{ + nil, + }, + }, + }, + DefinedTags: definedTags, + }, + wantErr: true, + expectedError: "Skipping Subnet reconciliation: Subnet can't be nil", + testSpecificSetup: func(clusterScope *ClusterScope, nlbClient *mock_vcn.MockClient) { + // nothing should be called + }, + }, { name: "create security list error", spec: infrastructurev1beta2.OCIClusterSpec{