diff --git a/pkg/providers/aws/config.go b/pkg/providers/aws/config.go index 809155cff..7cbd2f5e1 100644 --- a/pkg/providers/aws/config.go +++ b/pkg/providers/aws/config.go @@ -27,7 +27,7 @@ const ( DefaultFinalizer = "finalizers.cloud-resources-operator.integreatly.org" - defaultReconcileTime = time.Second * 300 + defaultReconcileTime = time.Second * 30 regionUSEast1 = "us-east-1" regionUSWest2 = "us-west-2" diff --git a/pkg/providers/aws/credentials.go b/pkg/providers/aws/credentials.go index 6eed1e3a3..a9104aaec 100644 --- a/pkg/providers/aws/credentials.go +++ b/pkg/providers/aws/credentials.go @@ -61,6 +61,7 @@ var ( "elasticache:DescribeCacheClusters", "elasticache:DescribeCacheSubnetGroups", "elasticache:CreateCacheSubnetGroup", + "elasticache:ModifyReplicationGroup", "rds:DescribeDBInstances", "rds:CreateDBInstance", "rds:DeleteDBInstance", diff --git a/pkg/providers/aws/provider_postgres.go b/pkg/providers/aws/provider_postgres.go index a0f6aa92d..8e99d1b29 100644 --- a/pkg/providers/aws/provider_postgres.go +++ b/pkg/providers/aws/provider_postgres.go @@ -208,18 +208,24 @@ func (p *PostgresProvider) createRDSInstance(ctx context.Context, cr *v1alpha1.P // check rds instance phase if *foundInstance.DBInstanceStatus != "available" { + logrus.Infof("found instance %s current status %s", *foundInstance.DBInstanceIdentifier, *foundInstance.DBInstanceStatus) return nil, croType.StatusMessage(fmt.Sprintf("createRDSInstance() in progress, current aws rds resource status is %s", *foundInstance.DBInstanceStatus)), nil } // check if found instance and user strategy differs, and modify instance - logrus.Info("found existing rds instance") + logrus.Infof("found existing rds instance: %s", *foundInstance.DBInstanceIdentifier) mi := buildRDSUpdateStrategy(rdsCfg, foundInstance) + if mi == nil { + logrus.Infof("rds instance %s is as expected", *foundInstance.DBInstanceIdentifier) + } if mi != nil { if _, err = rdsSvc.ModifyDBInstance(mi); err != nil { - return nil, "failed to modify instance", err + errMsg := fmt.Sprintf("error experienced trying to modify db instance: %s", *foundInstance.DBInstanceIdentifier) + return nil, croType.StatusMessage(errMsg), errorUtil.Wrap(err, errMsg) } - return nil, croType.StatusMessage(fmt.Sprintf("changes detected, modifyDBInstance() in progress, current aws rds resource status is %s", *foundInstance.DBInstanceStatus)), nil + logrus.Infof("set pending modifications for rds instance: %s", *foundInstance.DBInstanceIdentifier) } + // Add Tags to Aws Postgres resources msg, err := p.TagRDSPostgres(ctx, cr, rdsSvc, foundInstance) if err != nil { @@ -456,8 +462,9 @@ func (p *PostgresProvider) getRDSConfig(ctx context.Context, r *v1alpha1.Postgre return rdsCreateConfig, rdsDeleteConfig, stratCfg, nil } -// verifies if there is a change between a found instance and the configuration from the instance strat +// verifies if there is a change between a found instance and the configuration from the instance strat and verified the changes are not pending func buildRDSUpdateStrategy(rdsConfig *rds.CreateDBInstanceInput, foundConfig *rds.DBInstance) *rds.ModifyDBInstanceInput { + logrus.Infof("verifying that %s configuration is as expected", *foundConfig.DBInstanceIdentifier) updateFound := false mi := &rds.ModifyDBInstanceInput{} @@ -495,12 +502,51 @@ func buildRDSUpdateStrategy(rdsConfig *rds.CreateDBInstanceInput, foundConfig *r mi.MultiAZ = rdsConfig.MultiAZ updateFound = true } - if !updateFound { + if !updateFound || !verifyPendingModification(mi, foundConfig.PendingModifiedValues) { return nil } return mi } +// returns true if modify input is not pending +func verifyPendingModification(mi *rds.ModifyDBInstanceInput, pm *rds.PendingModifiedValues) bool { + pendingModifications := true + if pm == nil { + return pendingModifications + } + if mi.DBPortNumber != nil && pm.Port != nil { + if *mi.DBPortNumber == *pm.Port { + pendingModifications = false + } + } + if mi.BackupRetentionPeriod != nil && pm.BackupRetentionPeriod != nil { + if *mi.BackupRetentionPeriod == *pm.BackupRetentionPeriod { + pendingModifications = false + } + } + if mi.DBInstanceClass != nil && pm.DBInstanceClass != nil { + if *mi.DBInstanceClass == *pm.DBInstanceClass { + pendingModifications = false + } + } + if mi.AllocatedStorage != nil && pm.AllocatedStorage != nil { + if *mi.AllocatedStorage == *pm.AllocatedStorage { + pendingModifications = false + } + } + if mi.EngineVersion != nil && pm.EngineVersion != nil { + if *mi.EngineVersion == *pm.EngineVersion { + pendingModifications = false + } + } + if mi.MultiAZ != nil && pm.MultiAZ != nil { + if *mi.MultiAZ == *pm.MultiAZ { + pendingModifications = false + } + } + return pendingModifications +} + // verify postgres create config func (p *PostgresProvider) buildRDSCreateStrategy(ctx context.Context, pg *v1alpha1.Postgres, ec2Svc ec2iface.EC2API, rdsCreateConfig *rds.CreateDBInstanceInput, postgresPassword string) error { if rdsCreateConfig.DeletionProtection == nil { @@ -653,7 +699,7 @@ func (p *PostgresProvider) configureRDSVpc(ctx context.Context, rdsSvc rdsiface. } } if foundSubnet != nil { - logrus.Info(fmt.Sprintf("subnet group %s found", *foundSubnet.DBSubnetGroupName)) + logrus.Infof("subnet group %s found", *foundSubnet.DBSubnetGroupName) return nil } @@ -748,7 +794,7 @@ func (p *PostgresProvider) setPostgresServiceMaintenanceMetric(ctx context.Conte return } - logrus.Info(fmt.Sprintf("rds serviceupdates: %d available", len(output.PendingMaintenanceActions))) + logrus.Infof("rds serviceupdates: %d available", len(output.PendingMaintenanceActions)) for _, su := range output.PendingMaintenanceActions { metricLabels := map[string]string{} @@ -775,7 +821,7 @@ func (p *PostgresProvider) createRDSConnectionMetric(ctx context.Context, cr *v1 logrus.Infof("testing and exposing postgres connection metric for: %s", *instance.DBInstanceIdentifier) clusterID, err := resources.GetClusterID(ctx, p.Client) if err != nil { - logrus.Error(fmt.Sprintf("failed to get cluster id while exposing connection metric for %s", *instance.DBInstanceIdentifier)) + logrus.Errorf("failed to get cluster id while exposing connection metric for %s", *instance.DBInstanceIdentifier) } diff --git a/pkg/providers/aws/provider_postgres_test.go b/pkg/providers/aws/provider_postgres_test.go index c896d7aa8..136e8f6e3 100644 --- a/pkg/providers/aws/provider_postgres_test.go +++ b/pkg/providers/aws/provider_postgres_test.go @@ -211,7 +211,7 @@ func buildDbInstanceDeletionProtection() []*rds.DBInstance { } } -func buildDBInstance(testID string) []*rds.DBInstance { +func buildAvailableDBInstance(testID string) []*rds.DBInstance { return []*rds.DBInstance{ { DBInstanceIdentifier: aws.String(testID), @@ -237,6 +237,86 @@ func buildDBInstance(testID string) []*rds.DBInstance { } } +func buildPendingDBInstance(testID string) []*rds.DBInstance { + return []*rds.DBInstance{ + { + DBInstanceIdentifier: aws.String(testID), + DBInstanceStatus: aws.String("pending"), + }, + } +} + +func buildAvailableCreateInput(testID string) *rds.CreateDBInstanceInput { + return &rds.CreateDBInstanceInput{ + DBInstanceIdentifier: aws.String(testID), + DeletionProtection: aws.Bool(defaultAwsPostgresDeletionProtection), + Port: aws.Int64(defaultAwsPostgresPort), + BackupRetentionPeriod: aws.Int64(defaultAwsBackupRetentionPeriod), + DBInstanceClass: aws.String(defaultAwsDBInstanceClass), + PubliclyAccessible: aws.Bool(defaultAwsPubliclyAccessible), + AllocatedStorage: aws.Int64(defaultAwsAllocatedStorage), + EngineVersion: aws.String(defaultAwsEngineVersion), + MultiAZ: aws.Bool(true), + } +} + +func buildRequiresModificationsCreateInput(testID string) *rds.CreateDBInstanceInput { + return &rds.CreateDBInstanceInput{ + DBInstanceIdentifier: aws.String(testID), + DeletionProtection: aws.Bool(defaultAwsPostgresDeletionProtection), + Port: aws.Int64(123), + BackupRetentionPeriod: aws.Int64(defaultAwsBackupRetentionPeriod), + DBInstanceClass: aws.String(defaultAwsDBInstanceClass), + PubliclyAccessible: aws.Bool(defaultAwsPubliclyAccessible), + AllocatedStorage: aws.Int64(defaultAwsAllocatedStorage), + EngineVersion: aws.String(defaultAwsEngineVersion), + MultiAZ: aws.Bool(true), + } +} + +func buildNewRequiresModificationsCreateInput(testID string) *rds.CreateDBInstanceInput { + return &rds.CreateDBInstanceInput{ + DBInstanceIdentifier: aws.String(testID), + DeletionProtection: aws.Bool(defaultAwsPostgresDeletionProtection), + Port: aws.Int64(123), + BackupRetentionPeriod: aws.Int64(123), + DBInstanceClass: aws.String(defaultAwsDBInstanceClass), + PubliclyAccessible: aws.Bool(defaultAwsPubliclyAccessible), + AllocatedStorage: aws.Int64(defaultAwsAllocatedStorage), + EngineVersion: aws.String(defaultAwsEngineVersion), + MultiAZ: aws.Bool(true), + } +} + +func buildPendingModifiedDBInstance(testID string) []*rds.DBInstance { + return []*rds.DBInstance{ + { + DBInstanceIdentifier: aws.String(testID), + DBInstanceStatus: aws.String("available"), + AvailabilityZone: aws.String("test-availabilityZone"), + DBInstanceArn: aws.String("arn-test"), + DeletionProtection: aws.Bool(defaultAwsPostgresDeletionProtection), + MasterUsername: aws.String(defaultAwsPostgresUser), + DBName: aws.String(defaultAwsPostgresDatabase), + BackupRetentionPeriod: aws.Int64(defaultAwsBackupRetentionPeriod), + DBInstanceClass: aws.String(defaultAwsDBInstanceClass), + PubliclyAccessible: aws.Bool(defaultAwsPubliclyAccessible), + AllocatedStorage: aws.Int64(defaultAwsAllocatedStorage), + EngineVersion: aws.String(defaultAwsEngineVersion), + Engine: aws.String(defaultAwsEngine), + MultiAZ: aws.Bool(true), + Endpoint: &rds.Endpoint{ + Address: aws.String("blob"), + HostedZoneId: aws.String("blog"), + Port: aws.Int64(defaultAwsPostgresPort), + }, + PendingModifiedValues: &rds.PendingModifiedValues{ + Port: aws.Int64(123), + }, + }, + } +} + func buildVpcs() []*ec2.Vpc { return []*ec2.Vpc{ { @@ -317,7 +397,7 @@ func TestAWSPostgresProvider_createPostgresInstance(t *testing.T) { wantErr bool }{ { - name: "test rds is created", + name: "test rds CreateReplicationGroup is called", args: args{ rdsSvc: &mockRdsClient{dbInstances: []*rds.DBInstance{}}, ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, @@ -338,7 +418,7 @@ func TestAWSPostgresProvider_createPostgresInstance(t *testing.T) { { name: "test rds is exists and is available", args: args{ - rdsSvc: &mockRdsClient{dbInstances: buildDBInstance(testIdentifier)}, + rdsSvc: &mockRdsClient{dbInstances: buildAvailableDBInstance(testIdentifier)}, ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, ctx: context.TODO(), cr: buildTestPostgresCR(), @@ -363,9 +443,9 @@ func TestAWSPostgresProvider_createPostgresInstance(t *testing.T) { wantErr: false, }, { - name: "test rds needs to be modified", + name: "test rds is exists and is not available", args: args{ - rdsSvc: &mockRdsClient{dbInstances: buildDBInstance(testIdentifier)}, + rdsSvc: &mockRdsClient{dbInstances: buildPendingDBInstance(testIdentifier)}, ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, ctx: context.TODO(), cr: buildTestPostgresCR(), @@ -383,6 +463,82 @@ func TestAWSPostgresProvider_createPostgresInstance(t *testing.T) { want: nil, wantErr: false, }, + { + name: "test rds exists and status is available and needs to be modified", + args: args{ + rdsSvc: &mockRdsClient{dbInstances: buildAvailableDBInstance(testIdentifier)}, + ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, + ctx: context.TODO(), + cr: buildTestPostgresCR(), + postgresCfg: buildRequiresModificationsCreateInput(testIdentifier), + }, + fields: fields{ + Client: fake.NewFakeClientWithScheme(scheme, buildTestPostgresCR(), builtTestCredSecret(), buildTestInfra()), + Logger: testLogger, + CredentialManager: nil, + ConfigManager: nil, + TCPPinger: buildMockConnectionTester(), + }, + want: nil, + wantErr: false, + }, + { + name: "test rds exists and status is available and does not need to be modified", + args: args{ + rdsSvc: &mockRdsClient{dbInstances: buildAvailableDBInstance(testIdentifier)}, + ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, + ctx: context.TODO(), + cr: buildTestPostgresCR(), + postgresCfg: buildAvailableCreateInput(testIdentifier), + }, + fields: fields{ + Client: fake.NewFakeClientWithScheme(scheme, buildTestPostgresCR(), builtTestCredSecret(), buildTestInfra()), + Logger: testLogger, + CredentialManager: nil, + ConfigManager: nil, + TCPPinger: buildMockConnectionTester(), + }, + want: nil, + wantErr: false, + }, + { + name: "test rds exists and status is available and needs to be modified but maintenance is pending", + args: args{ + rdsSvc: &mockRdsClient{dbInstances: buildPendingModifiedDBInstance(testIdentifier)}, + ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, + ctx: context.TODO(), + cr: buildTestPostgresCR(), + postgresCfg: buildRequiresModificationsCreateInput(testIdentifier), + }, + fields: fields{ + Client: fake.NewFakeClientWithScheme(scheme, buildTestPostgresCR(), builtTestCredSecret(), buildTestInfra()), + Logger: testLogger, + CredentialManager: nil, + ConfigManager: nil, + TCPPinger: buildMockConnectionTester(), + }, + want: nil, + wantErr: false, + }, + { + name: "test rds exists and status is available and needs to update pending maintenance", + args: args{ + rdsSvc: &mockRdsClient{dbInstances: buildPendingModifiedDBInstance(testIdentifier)}, + ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName), azs: buildAZ()}, + ctx: context.TODO(), + cr: buildTestPostgresCR(), + postgresCfg: buildNewRequiresModificationsCreateInput(testIdentifier), + }, + fields: fields{ + Client: fake.NewFakeClientWithScheme(scheme, buildTestPostgresCR(), builtTestCredSecret(), buildTestInfra()), + Logger: testLogger, + CredentialManager: nil, + ConfigManager: nil, + TCPPinger: buildMockConnectionTester(), + }, + want: nil, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/providers/aws/provider_redis.go b/pkg/providers/aws/provider_redis.go index 333d2af3e..e71303982 100644 --- a/pkg/providers/aws/provider_redis.go +++ b/pkg/providers/aws/provider_redis.go @@ -174,23 +174,29 @@ func (p *RedisProvider) createElasticacheCluster(ctx context.Context, r *v1alpha // check elasticache phase if *foundCache.Status != "available" { + logrus.Infof("found instance %s current status %s", *foundCache.ReplicationGroupId, *foundCache.Status) return nil, croType.StatusMessage(fmt.Sprintf("createReplicationGroup() in progress, current aws elasticache status is %s", *foundCache.Status)), nil } // check if found cluster and user strategy differs, and modify instance - logrus.Info("found existing elasticache instance") + logrus.Infof("found existing elasticache instance %s", *foundCache.ReplicationGroupId) ec := buildElasticacheUpdateStrategy(elasticacheConfig, foundCache) + if ec == nil { + logrus.Infof("elasticache replication group %s is as expected", *foundCache.ReplicationGroupId) + } if ec != nil { - if _, err = cacheSvc.ModifyReplicationGroup(ec); err != nil { + logrus.Infof("%s differs from expected strategy, applying pending modifications :\n%s", *foundCache.ReplicationGroupId, ec) + if _, err := cacheSvc.ModifyReplicationGroup(ec); err != nil { errMsg := "failed to modify elasticache cluster" return nil, croType.StatusMessage(errMsg), errorUtil.Wrap(err, errMsg) } - return nil, croType.StatusMessage(fmt.Sprintf("changes detected, modifyDBInstance() in progress, current aws elasticache status is %s", *foundCache.Status)), nil + logrus.Infof("set pending modifications to elasticache replication group %s", *foundCache.ReplicationGroupId) } // add tags to cache nodes cacheInstance := *foundCache.NodeGroups[0] if *cacheInstance.Status != "available" { + logrus.Infof("elasticache node %s current status is %s", *cacheInstance.NodeGroupId, *cacheInstance.Status) return nil, croType.StatusMessage(fmt.Sprintf("cache node status not available, current status: %s", *foundCache.Status)), nil } @@ -417,7 +423,6 @@ func (p *RedisProvider) getElasticacheConfig(ctx context.Context, r *v1alpha1.Re if err != nil { return nil, nil, nil, errorUtil.Wrap(err, "failed to read aws strategy config") } - defRegion, err := GetRegionFromStrategyOrDefault(ctx, p.Client, stratCfg) if err != nil { return nil, nil, nil, errorUtil.Wrap(err, "failed to get default region") @@ -442,8 +447,8 @@ func (p *RedisProvider) getElasticacheConfig(ctx context.Context, r *v1alpha1.Re // checks found config vs user strategy for changes, if found returns a modify replication group func buildElasticacheUpdateStrategy(elasticacheConfig *elasticache.CreateReplicationGroupInput, foundConfig *elasticache.ReplicationGroup) *elasticache.ModifyReplicationGroupInput { + logrus.Infof("verifying that %s configuration is as expected", *foundConfig.ReplicationGroupId) updateFound := false - ec := &elasticache.ModifyReplicationGroupInput{} ec.ReplicationGroupId = foundConfig.ReplicationGroupId @@ -571,7 +576,7 @@ func (p *RedisProvider) configureElasticacheVpc(ctx context.Context, cacheSvc el } } if foundSubnet != nil { - logrus.Info(fmt.Sprintf("%s resource subnet group found", *foundSubnet.CacheSubnetGroupName)) + logrus.Infof("%s resource subnet group found", *foundSubnet.CacheSubnetGroupName) return nil } @@ -657,7 +662,7 @@ func (p *RedisProvider) setRedisServiceMaintenanceMetric(ctx context.Context, cr return } - logrus.Info(fmt.Sprintf("there are elasticache service updates: %d available", len(output.ServiceUpdates))) + logrus.Infof("there are elasticache service updates: %d available", len(output.ServiceUpdates)) for _, su := range output.ServiceUpdates { metricLabels := map[string]string{} metricLabels["clusterID"] = clusterID @@ -685,7 +690,7 @@ func (p *RedisProvider) createElasticacheConnectionMetric(ctx context.Context, c logrus.Infof("testing and exposing redis connection metric for: %s", *cache.ReplicationGroupId) clusterID, err := resources.GetClusterID(ctx, p.Client) if err != nil { - logrus.Error(fmt.Sprintf("failed to get cluster id while exposing connection metric for %s", *cache.ReplicationGroupId)) + logrus.Errorf("failed to get cluster id while exposing connection metric for %s", *cache.ReplicationGroupId) } // build generic labels to be added to metric diff --git a/pkg/providers/aws/provider_redis_test.go b/pkg/providers/aws/provider_redis_test.go index 54180b035..91c6bde10 100644 --- a/pkg/providers/aws/provider_redis_test.go +++ b/pkg/providers/aws/provider_redis_test.go @@ -243,6 +243,27 @@ func Test_createRedisCluster(t *testing.T) { want: nil, wantErr: false, }, + { + name: "test elasticache already exists and status is available", + args: args{ + ctx: context.TODO(), + cacheSvc: &mockElasticacheClient{replicationGroups: buildReplicationGroupReady()}, + ec2Svc: &mockEc2Client{vpcs: buildVpcs(), subnets: buildSubnets(), secGroups: buildSecurityGroups(secName)}, + r: buildTestRedisCR(), + stsSvc: &mockStsClient{}, + redisConfig: &elasticache.CreateReplicationGroupInput{ReplicationGroupId: aws.String("test-id")}, + stratCfg: &StrategyConfig{Region: "test"}, + }, + fields: fields{ + ConfigManager: nil, + CredentialManager: nil, + Logger: testLogger, + TCPPinger: buildMockConnectionTester(), + Client: fake.NewFakeClientWithScheme(scheme, buildTestRedisCR(), builtTestCredSecret(), buildTestInfra(), buildTestPrometheusRule()), + }, + want: buildTestRedisCluster(), + wantErr: false, + }, { name: "test elasticache already exists and status is not available", args: args{ @@ -282,7 +303,7 @@ func Test_createRedisCluster(t *testing.T) { TCPPinger: buildMockConnectionTester(), Client: fake.NewFakeClientWithScheme(scheme, buildTestRedisCR(), builtTestCredSecret(), buildTestInfra(), buildTestPrometheusRule()), }, - want: nil, + want: buildTestRedisCluster(), wantErr: false, }, { diff --git a/pkg/resources/configmaps.go b/pkg/resources/configmaps.go index 6973bfd20..263cda5d7 100644 --- a/pkg/resources/configmaps.go +++ b/pkg/resources/configmaps.go @@ -2,6 +2,8 @@ package resources import ( "context" + "fmt" + "github.com/sirupsen/logrus" errorUtil "github.com/pkg/errors" v1 "k8s.io/api/core/v1" @@ -14,6 +16,7 @@ func GetConfigMapOrDefault(ctx context.Context, c client.Client, name types.Name cm := &v1.ConfigMap{} if err := c.Get(ctx, name, cm); err != nil { if errors.IsNotFound(err) { + logrus.Debug(fmt.Sprintf("%s config not found in ns ( %s ) falling back to default strategy", name.Name, name.Namespace)) return def, nil } return nil, errorUtil.Wrap(err, "failed to get config map, not returning default")