From 87e1c5be57cf21360324c7f4ce310ecfe6d9ae8c Mon Sep 17 00:00:00 2001 From: Dmitrii Neeman Date: Sun, 2 Feb 2025 13:21:01 +0200 Subject: [PATCH] APPS-1494 Add xdr dc name validation (#216) --- cmd/internal/app/validation.go | 8 + cmd/internal/app/validation_test.go | 293 ++++++++++++++++++++++++++++ cmd/internal/flags/backup_xdr.go | 4 +- config_backup_xdr.go | 19 +- config_backup_xdr_test.go | 55 ++++++ 5 files changed, 374 insertions(+), 5 deletions(-) diff --git a/cmd/internal/app/validation.go b/cmd/internal/app/validation.go index aa23283a..40f7161d 100644 --- a/cmd/internal/app/validation.go +++ b/cmd/internal/app/validation.go @@ -87,6 +87,14 @@ func validateBackupXDRParams(params *models.BackupXDR) error { return fmt.Errorf("backup xdr file limit can't be less than 1") } + if params.InfoRetryIntervalMilliseconds < 0 { + return fmt.Errorf("backup xdr info retry interval can't be negative") + } + + if params.InfoRetriesMultiplier < 0 { + return fmt.Errorf("backup xdr info retries multiplier can't be negative") + } + return nil } diff --git a/cmd/internal/app/validation_test.go b/cmd/internal/app/validation_test.go index f372d972..62174210 100644 --- a/cmd/internal/app/validation_test.go +++ b/cmd/internal/app/validation_test.go @@ -761,6 +761,26 @@ func Test_validateBackupXDRParams(t *testing.T) { }, wantErr: "backup xdr file limit can't be less than 1", }, + { + name: "negative info retry interval", + params: &models.BackupXDR{ + MaxConnections: 1, + ParallelWrite: 1, + FileLimit: 1, + InfoRetryIntervalMilliseconds: -1, + }, + wantErr: "backup xdr info retry interval can't be negative", + }, + { + name: "negative info retries multiplier", + params: &models.BackupXDR{ + MaxConnections: 1, + ParallelWrite: 1, + FileLimit: 1, + InfoRetriesMultiplier: -1, + }, + wantErr: "backup xdr info retries multiplier can't be negative", + }, } for _, tt := range tests { @@ -774,3 +794,276 @@ func Test_validateBackupXDRParams(t *testing.T) { }) } } + +func TestValidateBackup(t *testing.T) { + tests := []struct { + name string + params *ASBackupParams + wantErr bool + errMsg string + }{ + { + name: "Valid backup configuration with output file", + params: &ASBackupParams{ + BackupParams: &models.Backup{ + OutputFile: "backup.asb", + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Valid backup configuration with directory", + params: &ASBackupParams{ + BackupParams: &models.Backup{}, + CommonParams: &models.Common{ + Directory: "backup-dir", + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Valid backup configuration with estimate", + params: &ASBackupParams{ + BackupParams: &models.Backup{ + Estimate: true, + EstimateSamples: 100, + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Valid backup configuration with BackupXDR", + params: &ASBackupParams{ + BackupXDRParams: &models.BackupXDR{ + MaxConnections: 10, + FileLimit: 1000, + }, + }, + wantErr: false, + }, + { + name: "Missing output file and directory", + params: &ASBackupParams{ + BackupParams: &models.Backup{}, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: true, + errMsg: "output file or directory required", + }, + { + name: "Invalid backup params - both output file and directory", + params: &ASBackupParams{ + BackupParams: &models.Backup{ + OutputFile: "backup.asb", + }, + CommonParams: &models.Common{ + Directory: "backup-dir", + Namespace: "test", + }, + }, + wantErr: true, + errMsg: "only one of output-file and directory may be configured at the same time", + }, + { + name: "Invalid common params - missing namespace", + params: &ASBackupParams{ + BackupParams: &models.Backup{ + OutputFile: "backup.asb", + }, + CommonParams: &models.Common{}, + }, + wantErr: true, + errMsg: "namespace is required", + }, + { + name: "Invalid BackupXDR params", + params: &ASBackupParams{ + BackupXDRParams: &models.BackupXDR{ + MaxConnections: 0, // Invalid: must be >= 1 + }, + }, + wantErr: true, + errMsg: "backup xdr max connections can't be less than 1", + }, + { + name: "Multiple cloud storage providers configured", + params: &ASBackupParams{ + BackupParams: &models.Backup{ + OutputFile: "backup.asb", + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + AwsS3: &models.AwsS3{ + Region: "us-west-2", + }, + GcpStorage: &models.GcpStorage{ + BucketName: "my-bucket", + }, + }, + wantErr: true, + errMsg: "only one cloud provider can be configured", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateBackup(tt.params) + if tt.wantErr { + assert.Error(t, err) + if tt.errMsg != "" { + assert.Equal(t, tt.errMsg, err.Error()) + } + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidateRestore(t *testing.T) { + tests := []struct { + name string + params *ASRestoreParams + wantErr bool + errMsg string + }{ + { + name: "Valid restore configuration with input file", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + InputFile: "backup.asb", + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Valid restore configuration with directory", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Directory: "restore-dir", + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Valid restore configuration with directory list", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + DirectoryList: "dir1,dir2", + ParentDirectory: "parent", + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: false, + }, + { + name: "Invalid restore mode", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + InputFile: "backup.asb", + Mode: "invalid-mode", + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: true, + errMsg: "invalid restore mode: invalid-mode", + }, + { + name: "Missing input source", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + }, + wantErr: true, + errMsg: "input file or directory required", + }, + { + name: "Invalid restore params - both input file and directory", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + InputFile: "backup.asb", + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Directory: "restore-dir", + Namespace: "test", + }, + }, + wantErr: true, + errMsg: "only one of directory and input-file may be configured at the same time", + }, + { + name: "Invalid common params - missing namespace", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + InputFile: "backup.asb", + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{}, + }, + wantErr: true, + errMsg: "namespace is required", + }, + { + name: "Multiple cloud storage providers configured", + params: &ASRestoreParams{ + RestoreParams: &models.Restore{ + InputFile: "backup.asb", + Mode: models.RestoreModeASB, + }, + CommonParams: &models.Common{ + Namespace: "test", + }, + AwsS3: &models.AwsS3{ + Region: "us-west-2", + }, + AzureBlob: &models.AzureBlob{ + ContainerName: "my-container", + }, + }, + wantErr: true, + errMsg: "only one cloud provider can be configured", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateRestore(tt.params) + if tt.wantErr { + assert.Error(t, err) + if tt.errMsg != "" { + assert.Equal(t, tt.errMsg, err.Error()) + } + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/cmd/internal/flags/backup_xdr.go b/cmd/internal/flags/backup_xdr.go index be72248a..34771db0 100644 --- a/cmd/internal/flags/backup_xdr.go +++ b/cmd/internal/flags/backup_xdr.go @@ -50,7 +50,9 @@ func (f *BackupXDR) NewFlagSet() *pflag.FlagSet { "If not set the default value is automatically calculated and appears as the number of CPUs on your machine.") flagSet.StringVar(&f.DC, "dc", "dc", - "DC that will be created on source instance for xdr backup.") + "DC that will be created on source instance for xdr backup.\n"+ + "DC name can include only Latin lowercase and uppercase letters with no diacritical marks (a-z, A-Z),\n"+ + "digits 0-9, underscores (_), hyphens (-), and dollar signs ($). Max length is 31 bytes.") flagSet.StringVar(&f.LocalAddress, "local-address", "127.0.0.1", "Local IP address on which XDR server listens on.") diff --git a/config_backup_xdr.go b/config_backup_xdr.go index 6d26d989..d006913d 100644 --- a/config_backup_xdr.go +++ b/config_backup_xdr.go @@ -17,12 +17,15 @@ package backup import ( "crypto/tls" "fmt" + "regexp" "strconv" a "github.com/aerospike/aerospike-client-go/v8" "github.com/aerospike/backup-go/models" ) +var expDCName = regexp.MustCompile(`^[a-zA-Z0-9_\-$]+$`) + // ConfigBackupXDR contains configuration for the xdr backup operation. type ConfigBackupXDR struct { // InfoPolicy applies to Aerospike Info requests made during backup and @@ -92,14 +95,22 @@ func (c *ConfigBackupXDR) validate() error { return fmt.Errorf("filelimit value must not be negative, got %d", c.FileLimit) } - if c.ParallelWrite < MinParallel || c.ParallelWrite > MaxParallel { - return fmt.Errorf("parallel write must be between 1 and 1024, got %d", c.ParallelWrite) - } - if c.DC == "" { return fmt.Errorf("dc name must not be empty") } + if len(c.DC) > 31 { + return fmt.Errorf("dc name must be less than 32 characters") + } + + if !expDCName.MatchString(c.DC) { + return fmt.Errorf("dc name must match ^[a-zA-Z0-9_\\-$]+$") + } + + if c.ParallelWrite < MinParallel || c.ParallelWrite > MaxParallel { + return fmt.Errorf("parallel write must be between 1 and 1024, got %d", c.ParallelWrite) + } + if c.LocalAddress == "" { return fmt.Errorf("local address must not be empty") } diff --git a/config_backup_xdr_test.go b/config_backup_xdr_test.go index e7045349..3b968a17 100644 --- a/config_backup_xdr_test.go +++ b/config_backup_xdr_test.go @@ -238,6 +238,61 @@ func TestConfigBackupXDR_validate(t *testing.T) { wantErr: true, errMsg: "max connections must not be less than 1, got 0", }, + { + name: "parallel write too high", + config: ConfigBackupXDR{ + DC: "dc1", + LocalAddress: "127.0.0.1", + LocalPort: 3000, + Namespace: "test", + Rewind: "all", + ParallelWrite: 1025, + MaxConnections: 1, + }, + wantErr: true, + errMsg: "parallel write must be between 1 and 1024, got 1025", + }, + { + name: "dc name too long (32 chars)", + config: ConfigBackupXDR{ + DC: "abcdefghijklmnopqrstuvwxyz123456", + LocalAddress: "127.0.0.1", + LocalPort: 3000, + Namespace: "test", + Rewind: "all", + MaxConnections: 1, + ParallelWrite: 1, + }, + wantErr: true, + errMsg: "dc name must be less than 32 characters", + }, + { + name: "dc name with invalid characters", + config: ConfigBackupXDR{ + DC: "dc@name", + LocalAddress: "127.0.0.1", + LocalPort: 3000, + Namespace: "test", + Rewind: "all", + MaxConnections: 1, + ParallelWrite: 1, + }, + wantErr: true, + errMsg: "dc name must match ^[a-zA-Z0-9_\\-$]+$", + }, + { + name: "dc name with valid special chars", + config: ConfigBackupXDR{ + DC: "dc-name_$123", + LocalAddress: "127.0.0.1", + LocalPort: 3000, + Namespace: "test", + Rewind: "all", + MaxConnections: 1, + ParallelWrite: 1, + }, + wantErr: false, + }, } for _, tt := range tests {