Skip to content

Commit 85a1f2a

Browse files
s-urbaniakjosvazg
s-urbaniak
and
josvazg
authored
CLOUDP-296918: fix nil panics in advanced deployment spec (#2073)
* add unit test * fix nil panics in advanced deployment spec * fix linter * Fix contract test timeouts * Ensure deletion idempotency for containers and peers --------- Co-authored-by: jose.vazquez <[email protected]>
1 parent 7f2d5b0 commit 85a1f2a

File tree

5 files changed

+278
-47
lines changed

5 files changed

+278
-47
lines changed

‎internal/translation/deployment/conversion.go‎

Lines changed: 67 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -305,53 +305,79 @@ func normalizeClusterDeployment(cluster *Cluster) {
305305

306306
func normalizeReplicationSpecs(cluster *Cluster, isTenant bool) {
307307
for ix, replicationSpec := range cluster.ReplicationSpecs {
308+
if replicationSpec == nil {
309+
continue
310+
}
308311
if replicationSpec.NumShards == 0 {
309312
replicationSpec.NumShards = 1
310313
}
311314
if replicationSpec.ZoneName == "" {
312315
replicationSpec.ZoneName = fmt.Sprintf("Zone %d", ix+1)
313316
}
314-
cmp.NormalizeSlice(replicationSpec.RegionConfigs, func(a, b *akov2.AdvancedRegionConfig) int {
315-
aPriority := "0"
316-
if a.Priority != nil {
317-
aPriority = strconv.Itoa(*a.Priority)
318-
}
319-
bPriority := "0"
320-
if b.Priority != nil {
321-
bPriority = strconv.Itoa(*b.Priority)
322-
}
323-
return strings.Compare(a.ProviderName+a.RegionName+aPriority, b.ProviderName+b.RegionName+bPriority)
324-
})
325-
for _, regionConfig := range replicationSpec.RegionConfigs {
326-
if regionConfig.ProviderName != string(provider.ProviderTenant) {
327-
regionConfig.BackingProviderName = ""
328-
}
329-
if regionConfig.ElectableSpecs != nil {
330-
if !isTenant && (regionConfig.ElectableSpecs.NodeCount == nil || *regionConfig.ElectableSpecs.NodeCount == 0) {
331-
regionConfig.ElectableSpecs = nil
332-
}
333-
}
334-
if regionConfig.ReadOnlySpecs != nil && (regionConfig.ReadOnlySpecs.NodeCount == nil || *regionConfig.ReadOnlySpecs.NodeCount == 0) {
335-
regionConfig.ReadOnlySpecs = nil
336-
}
337-
if regionConfig.AnalyticsSpecs != nil && (regionConfig.AnalyticsSpecs.NodeCount == nil || *regionConfig.AnalyticsSpecs.NodeCount == 0) {
338-
regionConfig.AnalyticsSpecs = nil
339-
}
340317

341-
computeUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.Compute == nil ||
342-
regionConfig.AutoScaling.Compute.Enabled == nil || !*regionConfig.AutoScaling.Compute.Enabled
343-
diskUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.DiskGB == nil ||
344-
regionConfig.AutoScaling.DiskGB.Enabled == nil || !*regionConfig.AutoScaling.DiskGB.Enabled
345-
if computeUnsetOrDisabled && diskUnsetOrDisabled {
346-
regionConfig.AutoScaling = nil
347-
}
348-
}
318+
normalizeRegionConfigs(replicationSpec.RegionConfigs, isTenant)
349319
}
350320
cmp.NormalizeSlice(cluster.ReplicationSpecs, func(a, b *akov2.AdvancedReplicationSpec) int {
351-
return strings.Compare(a.ZoneName, b.ZoneName)
321+
var zoneA, zoneB string
322+
if a != nil {
323+
zoneA = a.ZoneName
324+
}
325+
if b != nil {
326+
zoneB = b.ZoneName
327+
}
328+
return strings.Compare(zoneA, zoneB)
352329
})
353330
}
354331

332+
func normalizeRegionConfigs(regionConfigs []*akov2.AdvancedRegionConfig, isTenant bool) {
333+
cmp.NormalizeSlice(regionConfigs, func(a, b *akov2.AdvancedRegionConfig) int {
334+
aPriority := "0"
335+
if a != nil && a.Priority != nil {
336+
aPriority = strconv.Itoa(*a.Priority)
337+
}
338+
bPriority := "0"
339+
if b != nil && b.Priority != nil {
340+
bPriority = strconv.Itoa(*b.Priority)
341+
}
342+
var aProviderRegion, bProviderRegion string
343+
if a != nil {
344+
aProviderRegion = a.ProviderName + a.RegionName
345+
}
346+
if b != nil {
347+
bProviderRegion = b.ProviderName + b.RegionName
348+
}
349+
return strings.Compare(aProviderRegion+aPriority, bProviderRegion+bPriority)
350+
})
351+
352+
for _, regionConfig := range regionConfigs {
353+
if regionConfig == nil {
354+
continue
355+
}
356+
if regionConfig.ProviderName != string(provider.ProviderTenant) {
357+
regionConfig.BackingProviderName = ""
358+
}
359+
if regionConfig.ElectableSpecs != nil {
360+
if !isTenant && (regionConfig.ElectableSpecs.NodeCount == nil || *regionConfig.ElectableSpecs.NodeCount == 0) {
361+
regionConfig.ElectableSpecs = nil
362+
}
363+
}
364+
if regionConfig.ReadOnlySpecs != nil && (regionConfig.ReadOnlySpecs.NodeCount == nil || *regionConfig.ReadOnlySpecs.NodeCount == 0) {
365+
regionConfig.ReadOnlySpecs = nil
366+
}
367+
if regionConfig.AnalyticsSpecs != nil && (regionConfig.AnalyticsSpecs.NodeCount == nil || *regionConfig.AnalyticsSpecs.NodeCount == 0) {
368+
regionConfig.AnalyticsSpecs = nil
369+
}
370+
371+
computeUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.Compute == nil ||
372+
regionConfig.AutoScaling.Compute.Enabled == nil || !*regionConfig.AutoScaling.Compute.Enabled
373+
diskUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.DiskGB == nil ||
374+
regionConfig.AutoScaling.DiskGB.Enabled == nil || !*regionConfig.AutoScaling.DiskGB.Enabled
375+
if computeUnsetOrDisabled && diskUnsetOrDisabled {
376+
regionConfig.AutoScaling = nil
377+
}
378+
}
379+
}
380+
355381
func normalizeProcessArgs(args *akov2.ProcessArgs) {
356382
if args == nil {
357383
return
@@ -374,7 +400,13 @@ func getAutoscalingOverride(replications []*akov2.AdvancedReplicationSpec) (bool
374400
var instanceSize string
375401
var isTenant bool
376402
for _, replica := range replications {
403+
if replica == nil {
404+
continue
405+
}
377406
for _, region := range replica.RegionConfigs {
407+
if region == nil {
408+
continue
409+
}
378410
if region.ProviderName == string(provider.ProviderTenant) {
379411
isTenant = true
380412
}

‎internal/translation/deployment/conversion_test.go‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,210 @@ func TestNormalizeClusterDeployment(t *testing.T) {
380380
deployment *Cluster
381381
expected *Cluster
382382
}{
383+
"nil replication spec": {
384+
deployment: &Cluster{
385+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
386+
ReplicationSpecs: nil,
387+
},
388+
},
389+
expected: &Cluster{
390+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
391+
ClusterType: "REPLICASET",
392+
EncryptionAtRestProvider: "NONE",
393+
MongoDBMajorVersion: "7.0",
394+
VersionReleaseSystem: "LTS",
395+
Paused: pointer.MakePtr(false),
396+
PitEnabled: pointer.MakePtr(false),
397+
RootCertType: "ISRGROOTX1",
398+
BackupEnabled: pointer.MakePtr(false),
399+
Tags: []*akov2.TagSpec{},
400+
401+
ReplicationSpecs: nil,
402+
},
403+
},
404+
},
405+
"nil replication spec entries": {
406+
deployment: &Cluster{
407+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
408+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
409+
nil, nil, nil,
410+
},
411+
},
412+
},
413+
expected: &Cluster{
414+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
415+
ClusterType: "REPLICASET",
416+
EncryptionAtRestProvider: "NONE",
417+
MongoDBMajorVersion: "7.0",
418+
VersionReleaseSystem: "LTS",
419+
Paused: pointer.MakePtr(false),
420+
PitEnabled: pointer.MakePtr(false),
421+
RootCertType: "ISRGROOTX1",
422+
BackupEnabled: pointer.MakePtr(false),
423+
Tags: []*akov2.TagSpec{},
424+
425+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
426+
nil, nil, nil,
427+
},
428+
},
429+
},
430+
},
431+
"nil region configs": {
432+
deployment: &Cluster{
433+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
434+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
435+
{
436+
RegionConfigs: nil,
437+
},
438+
},
439+
},
440+
},
441+
expected: &Cluster{
442+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
443+
ClusterType: "REPLICASET",
444+
EncryptionAtRestProvider: "NONE",
445+
MongoDBMajorVersion: "7.0",
446+
VersionReleaseSystem: "LTS",
447+
Paused: pointer.MakePtr(false),
448+
PitEnabled: pointer.MakePtr(false),
449+
RootCertType: "ISRGROOTX1",
450+
BackupEnabled: pointer.MakePtr(false),
451+
Tags: []*akov2.TagSpec{},
452+
453+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
454+
{
455+
NumShards: 1,
456+
ZoneName: "Zone 1",
457+
RegionConfigs: nil,
458+
},
459+
},
460+
},
461+
},
462+
},
463+
"nil region config entries": {
464+
deployment: &Cluster{
465+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
466+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
467+
{
468+
RegionConfigs: []*akov2.AdvancedRegionConfig{
469+
nil, nil, nil,
470+
},
471+
},
472+
},
473+
},
474+
},
475+
expected: &Cluster{
476+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
477+
ClusterType: "REPLICASET",
478+
EncryptionAtRestProvider: "NONE",
479+
MongoDBMajorVersion: "7.0",
480+
VersionReleaseSystem: "LTS",
481+
Paused: pointer.MakePtr(false),
482+
PitEnabled: pointer.MakePtr(false),
483+
RootCertType: "ISRGROOTX1",
484+
BackupEnabled: pointer.MakePtr(false),
485+
Tags: []*akov2.TagSpec{},
486+
487+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
488+
{
489+
NumShards: 1,
490+
ZoneName: "Zone 1",
491+
RegionConfigs: []*akov2.AdvancedRegionConfig{
492+
nil, nil, nil,
493+
},
494+
},
495+
},
496+
},
497+
},
498+
},
499+
"nil regionconfig specs": {
500+
deployment: &Cluster{
501+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
502+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
503+
{
504+
RegionConfigs: []*akov2.AdvancedRegionConfig{
505+
{
506+
AnalyticsSpecs: nil,
507+
ElectableSpecs: nil,
508+
ReadOnlySpecs: nil,
509+
},
510+
},
511+
},
512+
},
513+
},
514+
},
515+
expected: &Cluster{
516+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
517+
ClusterType: "REPLICASET",
518+
EncryptionAtRestProvider: "NONE",
519+
MongoDBMajorVersion: "7.0",
520+
VersionReleaseSystem: "LTS",
521+
Paused: pointer.MakePtr(false),
522+
PitEnabled: pointer.MakePtr(false),
523+
RootCertType: "ISRGROOTX1",
524+
BackupEnabled: pointer.MakePtr(false),
525+
Tags: []*akov2.TagSpec{},
526+
527+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
528+
{
529+
NumShards: 1,
530+
ZoneName: "Zone 1",
531+
RegionConfigs: []*akov2.AdvancedRegionConfig{
532+
{
533+
AnalyticsSpecs: nil,
534+
ElectableSpecs: nil,
535+
ReadOnlySpecs: nil,
536+
},
537+
},
538+
},
539+
},
540+
},
541+
},
542+
},
543+
"empty regionconfig specs": {
544+
deployment: &Cluster{
545+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
546+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
547+
{
548+
RegionConfigs: []*akov2.AdvancedRegionConfig{
549+
{
550+
AnalyticsSpecs: &akov2.Specs{},
551+
ElectableSpecs: &akov2.Specs{},
552+
ReadOnlySpecs: &akov2.Specs{},
553+
},
554+
},
555+
},
556+
},
557+
},
558+
},
559+
expected: &Cluster{
560+
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
561+
ClusterType: "REPLICASET",
562+
EncryptionAtRestProvider: "NONE",
563+
MongoDBMajorVersion: "7.0",
564+
VersionReleaseSystem: "LTS",
565+
Paused: pointer.MakePtr(false),
566+
PitEnabled: pointer.MakePtr(false),
567+
RootCertType: "ISRGROOTX1",
568+
BackupEnabled: pointer.MakePtr(false),
569+
Tags: []*akov2.TagSpec{},
570+
571+
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
572+
{
573+
NumShards: 1,
574+
ZoneName: "Zone 1",
575+
RegionConfigs: []*akov2.AdvancedRegionConfig{
576+
{
577+
AnalyticsSpecs: nil,
578+
ElectableSpecs: nil,
579+
ReadOnlySpecs: nil,
580+
},
581+
},
582+
},
583+
},
584+
},
585+
},
586+
},
383587
"normalize deployment configuration": {
384588
deployment: &Cluster{
385589
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{

‎internal/translation/networkpeering/networkpeering.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ func (np *networkPeeringService) listPeersForProvider(ctx context.Context, proje
8484

8585
func (np *networkPeeringService) DeletePeer(ctx context.Context, projectID, peerID string) error {
8686
_, _, err := np.peeringAPI.DeletePeeringConnection(ctx, projectID, peerID).Execute()
87+
if admin.IsErrorCode(err, "PEER_ALREADY_REQUESTED_DELETION") || admin.IsErrorCode(err, "PEER_NOT_FOUND") {
88+
return nil // if it was already removed or being removed it is also fine
89+
}
8790
if err != nil {
8891
return fmt.Errorf("failed to delete peering connection for peer %s: %w", peerID, err)
8992
}

‎test/contract/audit/audit_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func TestDefaultAuditingGet(t *testing.T) {
1919
ctx := context.Background()
2020
contract.RunGoContractTest(ctx, t, "get default auditing", func(ch contract.ContractHelper) {
2121
projectName := utils.RandomName("default-auditing-project")
22-
require.NoError(t, ch.AddResources(ctx, time.Minute, contract.DefaultAtlasProject(projectName)))
22+
require.NoError(t, ch.AddResources(ctx, 5*time.Minute, contract.DefaultAtlasProject(projectName)))
2323
testProjectID, err := ch.ProjectID(ctx, projectName)
2424
require.NoError(t, err)
2525
as := audit.NewAuditLog(ch.AtlasClient().AuditingApi)
@@ -90,7 +90,7 @@ func TestSyncs(t *testing.T) {
9090
}
9191
contract.RunGoContractTest(ctx, t, "test syncs", func(ch contract.ContractHelper) {
9292
projectName := utils.RandomName("audit-syncs-project")
93-
require.NoError(t, ch.AddResources(ctx, time.Minute, contract.DefaultAtlasProject(projectName)))
93+
require.NoError(t, ch.AddResources(ctx, 5*time.Minute, contract.DefaultAtlasProject(projectName)))
9494
testProjectID, err := ch.ProjectID(ctx, projectName)
9595
require.NoError(t, err)
9696
as := audit.NewAuditLog(ch.AtlasClient().AuditingApi)

0 commit comments

Comments
 (0)