Skip to content

Commit

Permalink
CLOUDP-296918: fix nil panics in advanced deployment spec (#2073)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@s-urbaniak @josvazg
s-urbaniak and josvazg authored Jan 27, 2025
1 parent 7f2d5b0 commit 85a1f2a
Show file tree
Hide file tree
Showing 5 changed files with 278 additions and 47 deletions.
102 changes: 67 additions & 35 deletions internal/translation/deployment/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,53 +305,79 @@ func normalizeClusterDeployment(cluster *Cluster) {

func normalizeReplicationSpecs(cluster *Cluster, isTenant bool) {
for ix, replicationSpec := range cluster.ReplicationSpecs {
if replicationSpec == nil {
continue
}
if replicationSpec.NumShards == 0 {
replicationSpec.NumShards = 1
}
if replicationSpec.ZoneName == "" {
replicationSpec.ZoneName = fmt.Sprintf("Zone %d", ix+1)
}
cmp.NormalizeSlice(replicationSpec.RegionConfigs, func(a, b *akov2.AdvancedRegionConfig) int {
aPriority := "0"
if a.Priority != nil {
aPriority = strconv.Itoa(*a.Priority)
}
bPriority := "0"
if b.Priority != nil {
bPriority = strconv.Itoa(*b.Priority)
}
return strings.Compare(a.ProviderName+a.RegionName+aPriority, b.ProviderName+b.RegionName+bPriority)
})
for _, regionConfig := range replicationSpec.RegionConfigs {
if regionConfig.ProviderName != string(provider.ProviderTenant) {
regionConfig.BackingProviderName = ""
}
if regionConfig.ElectableSpecs != nil {
if !isTenant && (regionConfig.ElectableSpecs.NodeCount == nil || *regionConfig.ElectableSpecs.NodeCount == 0) {
regionConfig.ElectableSpecs = nil
}
}
if regionConfig.ReadOnlySpecs != nil && (regionConfig.ReadOnlySpecs.NodeCount == nil || *regionConfig.ReadOnlySpecs.NodeCount == 0) {
regionConfig.ReadOnlySpecs = nil
}
if regionConfig.AnalyticsSpecs != nil && (regionConfig.AnalyticsSpecs.NodeCount == nil || *regionConfig.AnalyticsSpecs.NodeCount == 0) {
regionConfig.AnalyticsSpecs = nil
}

computeUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.Compute == nil ||
regionConfig.AutoScaling.Compute.Enabled == nil || !*regionConfig.AutoScaling.Compute.Enabled
diskUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.DiskGB == nil ||
regionConfig.AutoScaling.DiskGB.Enabled == nil || !*regionConfig.AutoScaling.DiskGB.Enabled
if computeUnsetOrDisabled && diskUnsetOrDisabled {
regionConfig.AutoScaling = nil
}
}
normalizeRegionConfigs(replicationSpec.RegionConfigs, isTenant)
}
cmp.NormalizeSlice(cluster.ReplicationSpecs, func(a, b *akov2.AdvancedReplicationSpec) int {
return strings.Compare(a.ZoneName, b.ZoneName)
var zoneA, zoneB string
if a != nil {
zoneA = a.ZoneName
}
if b != nil {
zoneB = b.ZoneName
}
return strings.Compare(zoneA, zoneB)
})
}

func normalizeRegionConfigs(regionConfigs []*akov2.AdvancedRegionConfig, isTenant bool) {
cmp.NormalizeSlice(regionConfigs, func(a, b *akov2.AdvancedRegionConfig) int {
aPriority := "0"
if a != nil && a.Priority != nil {
aPriority = strconv.Itoa(*a.Priority)
}
bPriority := "0"
if b != nil && b.Priority != nil {
bPriority = strconv.Itoa(*b.Priority)
}
var aProviderRegion, bProviderRegion string
if a != nil {
aProviderRegion = a.ProviderName + a.RegionName
}
if b != nil {
bProviderRegion = b.ProviderName + b.RegionName
}
return strings.Compare(aProviderRegion+aPriority, bProviderRegion+bPriority)
})

for _, regionConfig := range regionConfigs {
if regionConfig == nil {
continue
}
if regionConfig.ProviderName != string(provider.ProviderTenant) {
regionConfig.BackingProviderName = ""
}
if regionConfig.ElectableSpecs != nil {
if !isTenant && (regionConfig.ElectableSpecs.NodeCount == nil || *regionConfig.ElectableSpecs.NodeCount == 0) {
regionConfig.ElectableSpecs = nil
}
}
if regionConfig.ReadOnlySpecs != nil && (regionConfig.ReadOnlySpecs.NodeCount == nil || *regionConfig.ReadOnlySpecs.NodeCount == 0) {
regionConfig.ReadOnlySpecs = nil
}
if regionConfig.AnalyticsSpecs != nil && (regionConfig.AnalyticsSpecs.NodeCount == nil || *regionConfig.AnalyticsSpecs.NodeCount == 0) {
regionConfig.AnalyticsSpecs = nil
}

computeUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.Compute == nil ||
regionConfig.AutoScaling.Compute.Enabled == nil || !*regionConfig.AutoScaling.Compute.Enabled
diskUnsetOrDisabled := regionConfig.AutoScaling == nil || regionConfig.AutoScaling.DiskGB == nil ||
regionConfig.AutoScaling.DiskGB.Enabled == nil || !*regionConfig.AutoScaling.DiskGB.Enabled
if computeUnsetOrDisabled && diskUnsetOrDisabled {
regionConfig.AutoScaling = nil
}
}
}

func normalizeProcessArgs(args *akov2.ProcessArgs) {
if args == nil {
return
Expand All @@ -374,7 +400,13 @@ func getAutoscalingOverride(replications []*akov2.AdvancedReplicationSpec) (bool
var instanceSize string
var isTenant bool
for _, replica := range replications {
if replica == nil {
continue
}
for _, region := range replica.RegionConfigs {
if region == nil {
continue
}
if region.ProviderName == string(provider.ProviderTenant) {
isTenant = true
}
Expand Down
204 changes: 204 additions & 0 deletions internal/translation/deployment/conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,210 @@ func TestNormalizeClusterDeployment(t *testing.T) {
deployment *Cluster
expected *Cluster
}{
"nil replication spec": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: nil,
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: nil,
},
},
},
"nil replication spec entries": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
nil, nil, nil,
},
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
nil, nil, nil,
},
},
},
},
"nil region configs": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
RegionConfigs: nil,
},
},
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
NumShards: 1,
ZoneName: "Zone 1",
RegionConfigs: nil,
},
},
},
},
},
"nil region config entries": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
RegionConfigs: []*akov2.AdvancedRegionConfig{
nil, nil, nil,
},
},
},
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
NumShards: 1,
ZoneName: "Zone 1",
RegionConfigs: []*akov2.AdvancedRegionConfig{
nil, nil, nil,
},
},
},
},
},
},
"nil regionconfig specs": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
RegionConfigs: []*akov2.AdvancedRegionConfig{
{
AnalyticsSpecs: nil,
ElectableSpecs: nil,
ReadOnlySpecs: nil,
},
},
},
},
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
NumShards: 1,
ZoneName: "Zone 1",
RegionConfigs: []*akov2.AdvancedRegionConfig{
{
AnalyticsSpecs: nil,
ElectableSpecs: nil,
ReadOnlySpecs: nil,
},
},
},
},
},
},
},
"empty regionconfig specs": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
RegionConfigs: []*akov2.AdvancedRegionConfig{
{
AnalyticsSpecs: &akov2.Specs{},
ElectableSpecs: &akov2.Specs{},
ReadOnlySpecs: &akov2.Specs{},
},
},
},
},
},
},
expected: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
ClusterType: "REPLICASET",
EncryptionAtRestProvider: "NONE",
MongoDBMajorVersion: "7.0",
VersionReleaseSystem: "LTS",
Paused: pointer.MakePtr(false),
PitEnabled: pointer.MakePtr(false),
RootCertType: "ISRGROOTX1",
BackupEnabled: pointer.MakePtr(false),
Tags: []*akov2.TagSpec{},

ReplicationSpecs: []*akov2.AdvancedReplicationSpec{
{
NumShards: 1,
ZoneName: "Zone 1",
RegionConfigs: []*akov2.AdvancedRegionConfig{
{
AnalyticsSpecs: nil,
ElectableSpecs: nil,
ReadOnlySpecs: nil,
},
},
},
},
},
},
},
"normalize deployment configuration": {
deployment: &Cluster{
AdvancedDeploymentSpec: &akov2.AdvancedDeploymentSpec{
Expand Down
3 changes: 3 additions & 0 deletions internal/translation/networkpeering/networkpeering.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ func (np *networkPeeringService) listPeersForProvider(ctx context.Context, proje

func (np *networkPeeringService) DeletePeer(ctx context.Context, projectID, peerID string) error {
_, _, err := np.peeringAPI.DeletePeeringConnection(ctx, projectID, peerID).Execute()
if admin.IsErrorCode(err, "PEER_ALREADY_REQUESTED_DELETION") || admin.IsErrorCode(err, "PEER_NOT_FOUND") {
return nil // if it was already removed or being removed it is also fine
}
if err != nil {
return fmt.Errorf("failed to delete peering connection for peer %s: %w", peerID, err)
}
Expand Down
4 changes: 2 additions & 2 deletions test/contract/audit/audit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestDefaultAuditingGet(t *testing.T) {
ctx := context.Background()
contract.RunGoContractTest(ctx, t, "get default auditing", func(ch contract.ContractHelper) {
projectName := utils.RandomName("default-auditing-project")
require.NoError(t, ch.AddResources(ctx, time.Minute, contract.DefaultAtlasProject(projectName)))
require.NoError(t, ch.AddResources(ctx, 5*time.Minute, contract.DefaultAtlasProject(projectName)))
testProjectID, err := ch.ProjectID(ctx, projectName)
require.NoError(t, err)
as := audit.NewAuditLog(ch.AtlasClient().AuditingApi)
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestSyncs(t *testing.T) {
}
contract.RunGoContractTest(ctx, t, "test syncs", func(ch contract.ContractHelper) {
projectName := utils.RandomName("audit-syncs-project")
require.NoError(t, ch.AddResources(ctx, time.Minute, contract.DefaultAtlasProject(projectName)))
require.NoError(t, ch.AddResources(ctx, 5*time.Minute, contract.DefaultAtlasProject(projectName)))
testProjectID, err := ch.ProjectID(ctx, projectName)
require.NoError(t, err)
as := audit.NewAuditLog(ch.AtlasClient().AuditingApi)
Expand Down
Loading

0 comments on commit 85a1f2a

Please sign in to comment.