From 2641899e5f64b98960cbc460a6d2368f7b40d55c Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Wed, 15 Jan 2025 11:32:25 -0800 Subject: [PATCH 01/17] DB Connected. Signed-off-by: Viraj Kulkarni --- admiral/apis/v1/types.go | 7 +- admiral/cmd/admiral/cmd/root.go | 8 ++ admiral/pkg/clusters/admiralDatabaseClient.go | 79 +++++++++++++++++++ admiral/pkg/clusters/dynamoDB.go | 53 +++++++++++++ admiral/pkg/clusters/types.go | 20 +++++ admiral/pkg/controller/common/config.go | 6 ++ admiral/pkg/controller/common/types.go | 5 ++ 7 files changed, 175 insertions(+), 3 deletions(-) diff --git a/admiral/apis/v1/types.go b/admiral/apis/v1/types.go index b1c40343f..ff9579107 100644 --- a/admiral/apis/v1/types.go +++ b/admiral/apis/v1/types.go @@ -5,9 +5,10 @@ const ( ) type AdmiralConfig struct { - IdpsConfig IdpsConfig `yaml:"idps,omitempty"` - IgnoreIdentityList IgnoreIdentityList `yaml:"ignoreIdentityList,omitempty"` - WorkloadDatabase DynamoDB `yaml:"workloadDynamoDB,omitempty"` + IdpsConfig IdpsConfig `yaml:"idps,omitempty"` + IgnoreIdentityList IgnoreIdentityList `yaml:"ignoreIdentityList,omitempty"` + WorkloadDatabase DynamoDB `yaml:"workloadDynamoDB,omitempty"` + DynamicConfigDatabase DynamoDB `yaml:"dynamicConfigDynamoDB,omitempty"` } type IgnoreIdentityList struct { diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 1255d7f37..0f27a4645 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -78,6 +78,10 @@ func GetRootCmd(args []string) *cobra.Command { log.Fatalf("Error: %v", err) } + if common.IsAdmiralDynamicConfigEnable() { + go clusters.UpdateASyncAdmiralConfig(remoteRegistry.DynamicConfigDatabaseClient, params.DynamicSyncPeriod) + } + // This is required for PERF tests only. // Perf tests requires remote registry object for validations. // There is no way to inject this object @@ -268,6 +272,10 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().StringSliceVar(¶ms.ClientDiscoveryClustersForJobs, "client_discovery_clusters_for_jobs", []string{}, "List of clusters for client discovery for k8s jobs") rootCmd.PersistentFlags().StringSliceVar(¶ms.DiscoveryClustersForNumaflow, "client_discovery_clusters_for_numaflow", []string{}, "List of clusters for client discovery for numaflow types") + rootCmd.PersistentFlags().BoolVar(¶ms.EnableDynamicConfig, "enable_dynamic_config", true, "Enable/Disable Dynamic Configuration") + rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") + rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration after which the dynamic sync get performed") + return rootCmd } diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index d868a2154..5ff57883d 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -3,9 +3,11 @@ package clusters import ( "fmt" "github.com/istio-ecosystem/admiral/admiral/apis/v1" + "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" log "github.com/sirupsen/logrus" "gopkg.in/yaml.v2" "io/ioutil" + "time" ) // TODO: make this more generic to handle new dynamoDB tables @@ -14,6 +16,11 @@ type WorkloadDatabaseClient struct { database *v1.DynamoDB } +type DynamicConfigDatabaseClient struct { + dynamoClient *DynamoClient + database *v1.DynamoDB +} + type DummyDatabaseClient struct{} type AdmiralDatabaseManager interface { @@ -65,6 +72,39 @@ func checkIfDatabaseClientIsInitialize(workloadDatabaseClient *WorkloadDatabaseC return nil } +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Update(data interface{}, ctxLogger *log.Entry) error { + //workloadData := data.(WorkloadData) + // + //err := checkIfDatabaseClientIsInitialize(workloadDatabaseClient) + //if err != nil { + // return err + //} + // + //return workloadDatabaseClient.dynamoClient.updateWorkloadDataItem(&workloadData, workloadDatabaseClient.database.TableName, ctxLogger) + panic("Implement me!") +} + +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) { + + //err := checkIfDatabaseClientIsInitialize(dynamicConfigDatabaseClient) + //if err != nil { + // return nil, err + //} + // + return dynamicConfigDatabaseClient.dynamoClient.getDynamicConfig(dynamicConfigDatabaseClient.database.TableName) +} + +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Delete(data interface{}, ctxLogger *log.Entry) error { + //workloadData := data.(WorkloadData) + // + //err := checkIfDatabaseClientIsInitialize(workloadDatabaseClient) + //if err != nil { + // return err + //} + //return workloadDatabaseClient.dynamoClient.deleteWorkloadDataItem(&workloadData, workloadDatabaseClient.database.TableName) + panic("Implement me!") +} + func (databaseClient *DummyDatabaseClient) Update(data interface{}, logger *log.Entry) error { return nil } @@ -103,3 +143,42 @@ func NewAdmiralDatabaseClient(admiralConfigPath string, dynamoClientInitFunc fun } return workloadDatabaseClient, nil } + +func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role string, region string) (*DynamoClient, error)) (*DynamicConfigDatabaseClient, error) { + var ( + admiralConfig *v1.AdmiralConfig + dynamicConfigClient = &DynamicConfigDatabaseClient{} + ) + + data, err := ioutil.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("error reading admiral config file for DynamicConfig, err: %v", err) + } + + err = yaml.Unmarshal(data, &admiralConfig) + if err != nil { + return nil, fmt.Errorf("error unmarshalling admiral config file for DynamicConfig, err: %v", err) + } + + dynamicConfigClient.database = &admiralConfig.DynamicConfigDatabase + dynamicConfigClient.database.TableName = common.GetAdmiralParams().DynamicConfigDynamoDBTableName + dynamicConfigClient.dynamoClient, err = dynamoClientInitFunc( + admiralConfig.WorkloadDatabase.Role, + admiralConfig.WorkloadDatabase.Region, + ) + if err != nil { + return nil, fmt.Errorf("unable to instantiate dynamo client for DynamicConfig, err: %v", err) + } + return dynamicConfigClient, nil +} + +func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { + + for range time.Tick(time.Minute * time.Duration(syncTime)) { + UpdateSyncAdmiralConfig(dbClient) + } +} + +func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) { + dbClient.Get("", "") +} diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 20f7dc1b9..aa1560d67 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -71,6 +71,11 @@ type WorkloadData struct { FailedClusters []string `json:"failedClusters"` } +type DynamicConfigData struct { + EnableDynamicConfig bool `json:"enableDynamicConfig"` + //NLBEnabledClusters []string `json:"nlbEnabledClusters"` +} + type DynamoClient struct { svc dynamodbiface.DynamoDBAPI } @@ -205,6 +210,54 @@ func (client *DynamoClient) getWorkloadDataItemByIdentityAndEnv(env, identity, t return workloadDataItems, nil } +func (client *DynamoClient) getDynamicConfig(tableName string) (DynamicConfigData, error) { + + configData := DynamicConfigData{} + + //keyCond := expression.KeyEqual(expression.Key("EnableDynamicConfig"), expression.Value(true)) + // + //expr, err := expression.NewBuilder(). + // WithKeyCondition(keyCond). + // Build() + // + //if err != nil { + // return "", err + //} + + dbQuery := dynamodb.ScanInput{ + TableName: aws.String(tableName), + //ExpressionAttributeNames: expr.Names(), + //ExpressionAttributeValues: expr.Values(), + //KeyConditionExpression: expr.KeyCondition(), + //FilterExpression: expr.Filter(), + } + + items, err := client.svc.Scan(&dbQuery) + + if err != nil { + return configData, fmt.Errorf("Failed to fetch dynamic config : %s", err) + } + + if items == nil { + log.Infof("Failed to fetch dynamic config : %s", tableName) + return configData, nil + } + + if *items.Count == 1 { + //err = dynamodbattribute.Unmarshal(items.Items[0], &configData) + //items + err = dynamodbattribute.UnmarshalMap(items.Items[0], &configData) + + if err != nil { + return configData, fmt.Errorf("failed to unmarshal table items, err: %v", err) + } + } else { + return configData, fmt.Errorf("Expected only 1 row but got %s for tableName : %s", items.Count, tableName) + } + + return configData, nil +} + /* Utility function to update workload data item. This will be called by Active admiral instance on every update to serviceentry. diff --git a/admiral/pkg/clusters/types.go b/admiral/pkg/clusters/types.go index f63e5f49d..b62a51151 100644 --- a/admiral/pkg/clusters/types.go +++ b/admiral/pkg/clusters/types.go @@ -81,6 +81,8 @@ type AdmiralCache struct { CnameDependentClusterNamespaceCache *common.MapOfMapOfMaps PartitionIdentityCache *common.Map ClientClusterNamespaceServerCache *common.MapOfMapOfMaps + NLBEnabledClusterCache []string + NLBEnabledServiceCache []string } type RemoteRegistry struct { @@ -93,6 +95,7 @@ type RemoteRegistry struct { StartTime time.Time ServiceEntrySuspender ServiceEntrySuspender AdmiralDatabaseClient AdmiralDatabaseManager + DynamicConfigDatabaseClient AdmiralDatabaseManager DependencyController *admiral.DependencyController ShardController *admiral.ShardController ClientLoader loader.ClientLoader @@ -111,6 +114,7 @@ type ModifySEFunc func(ctx context.Context, event admiral.EventType, env string, func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *RemoteRegistry { var serviceEntrySuspender ServiceEntrySuspender var admiralDatabaseClient AdmiralDatabaseManager + var admiralDynamicConfigDatabaseClient AdmiralDatabaseManager var err error gtpCache := &globalTrafficCache{} @@ -162,6 +166,12 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote serviceEntrySuspender = NewDummyServiceEntrySuspender() } + if common.IsAdmiralDynamicConfigEnable() { + admiralDynamicConfigDatabaseClient, err = NewDynamicConfigDatabaseClient(common.GetAdmiralConfigPath(), NewDynamoClient) + } else { + admiralDynamicConfigDatabaseClient = &DummyDatabaseClient{} + } + if common.GetEnableWorkloadDataStorage() { admiralDatabaseClient, err = NewAdmiralDatabaseClient(common.GetAdmiralConfigPath(), NewDynamoClient) if err != nil { @@ -186,6 +196,7 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote AdmiralCache: admiralCache, ServiceEntrySuspender: serviceEntrySuspender, AdmiralDatabaseClient: admiralDatabaseClient, + DynamicConfigDatabaseClient: admiralDynamicConfigDatabaseClient, ClientLoader: clientLoader, ClusterIdentityStoreHandler: registry.NewClusterIdentityStoreHandler(), ConfigSyncer: registry.NewConfigSync(), @@ -197,6 +208,15 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote rr.AdmiralCache.ClusterLocalityCache = common.NewMapOfMaps() } + /* + Load data from dynamoDB before async process starts. + This is done to avoid any transitive state where component starts with outofsync config. + Later down the process like async processor will take on going config pushes. + */ + if common.IsAdmiralDynamicConfigEnable() { + UpdateSyncAdmiralConfig(rr.DynamicConfigDatabaseClient) + } + return rr } diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index aaac32182..37ac285f6 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -146,6 +146,12 @@ func GetEnableWorkloadDataStorage() bool { return wrapper.params.EnableWorkloadDataStorage } +func IsAdmiralDynamicConfigEnable() bool { + wrapper.RLock() + defer wrapper.RUnlock() + return wrapper.params.EnableDynamicConfig +} + func GetHostnameSuffix() string { wrapper.RLock() defer wrapper.RUnlock() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index 11d3b3a27..679bfd095 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -142,6 +142,11 @@ type AdmiralParams struct { EnableClientDiscovery bool ClientDiscoveryClustersForJobs []string DiscoveryClustersForNumaflow []string + + //DynamicConfig setting + EnableDynamicConfig bool + DynamicConfigDynamoDBTableName string + DynamicSyncPeriod int } func (b AdmiralParams) String() string { From 1beea25c2e0cea28bd8c1453636edab2e3dab992 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Wed, 15 Jan 2025 17:46:47 -0800 Subject: [PATCH 02/17] First parameter overwrite set Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 3 + admiral/pkg/clusters/admiralDatabaseClient.go | 74 ++++++++++--------- admiral/pkg/clusters/dynamoDB.go | 46 ++++++------ admiral/pkg/controller/common/config.go | 6 ++ admiral/pkg/controller/common/types.go | 3 + 5 files changed, 76 insertions(+), 56 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 0f27a4645..018b99b0c 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -275,6 +275,9 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().BoolVar(¶ms.EnableDynamicConfig, "enable_dynamic_config", true, "Enable/Disable Dynamic Configuration") rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration after which the dynamic sync get performed") + rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for NLB") + rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledAssetAliases, "nlb_enabled_assets", "", "Comma seperated list of enabled asset aliases to be enabled for NLB") + rootCmd.PersistentFlags().StringVar(¶ms.CLBEnabledClusters, "clb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for CLB") return rootCmd } diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 5ff57883d..5e49563f0 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -1,12 +1,14 @@ package clusters import ( + "errors" "fmt" "github.com/istio-ecosystem/admiral/admiral/apis/v1" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" log "github.com/sirupsen/logrus" "gopkg.in/yaml.v2" "io/ioutil" + "strings" "time" ) @@ -72,37 +74,19 @@ func checkIfDatabaseClientIsInitialize(workloadDatabaseClient *WorkloadDatabaseC return nil } -func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Update(data interface{}, ctxLogger *log.Entry) error { - //workloadData := data.(WorkloadData) - // - //err := checkIfDatabaseClientIsInitialize(workloadDatabaseClient) - //if err != nil { - // return err - //} - // - //return workloadDatabaseClient.dynamoClient.updateWorkloadDataItem(&workloadData, workloadDatabaseClient.database.TableName, ctxLogger) - panic("Implement me!") +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Update(data interface{}, logger *log.Entry) error { + //TODO implement me + panic("implement me") } -func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) { - - //err := checkIfDatabaseClientIsInitialize(dynamicConfigDatabaseClient) - //if err != nil { - // return nil, err - //} - // - return dynamicConfigDatabaseClient.dynamoClient.getDynamicConfig(dynamicConfigDatabaseClient.database.TableName) +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Delete(data interface{}, logger *log.Entry) error { + //TODO implement me + panic("implement me") } -func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Delete(data interface{}, ctxLogger *log.Entry) error { - //workloadData := data.(WorkloadData) - // - //err := checkIfDatabaseClientIsInitialize(workloadDatabaseClient) - //if err != nil { - // return err - //} - //return workloadDatabaseClient.dynamoClient.deleteWorkloadDataItem(&workloadData, workloadDatabaseClient.database.TableName) - panic("Implement me!") +func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) { + //Variable renaming is done to re-purpose existing interface + return dynamicConfigDatabaseClient.dynamoClient.getDynamicConfig(env, identity, dynamicConfigDatabaseClient.database.TableName) } func (databaseClient *DummyDatabaseClient) Update(data interface{}, logger *log.Entry) error { @@ -147,17 +131,17 @@ func NewAdmiralDatabaseClient(admiralConfigPath string, dynamoClientInitFunc fun func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role string, region string) (*DynamoClient, error)) (*DynamicConfigDatabaseClient, error) { var ( admiralConfig *v1.AdmiralConfig - dynamicConfigClient = &DynamicConfigDatabaseClient{} + dynamicConfigClient = DynamicConfigDatabaseClient{} ) data, err := ioutil.ReadFile(path) if err != nil { - return nil, fmt.Errorf("error reading admiral config file for DynamicConfig, err: %v", err) + return &dynamicConfigClient, fmt.Errorf("error reading admiral config file for DynamicConfig, err: %v", err) } err = yaml.Unmarshal(data, &admiralConfig) if err != nil { - return nil, fmt.Errorf("error unmarshalling admiral config file for DynamicConfig, err: %v", err) + return &dynamicConfigClient, fmt.Errorf("error unmarshalling admiral config file for DynamicConfig, err: %v", err) } dynamicConfigClient.database = &admiralConfig.DynamicConfigDatabase @@ -167,9 +151,9 @@ func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role admiralConfig.WorkloadDatabase.Region, ) if err != nil { - return nil, fmt.Errorf("unable to instantiate dynamo client for DynamicConfig, err: %v", err) + return &dynamicConfigClient, fmt.Errorf("unable to instantiate dynamo client for DynamicConfig, err: %v", err) } - return dynamicConfigClient, nil + return &dynamicConfigClient, nil } func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { @@ -179,6 +163,28 @@ func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { } } -func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) { - dbClient.Get("", "") +func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { + + configData, err := dbClient.Get("EnableDynamicConfig", "admiral") + if err != nil { + //TODO : Log Something Bad Happen but error out + return err + } + + configDataCast, ok := configData.(DynamicConfigData) + if !ok { + return errors.New("Failed parse") + } + + if configDataCast.EnableDynamicConfig == "admiral" { + fmt.Println(configDataCast) + newAdmiralConfig := common.GetAdmiralParams() + newAdmiralConfig.NLBEnabledClusters = strings.Join(configDataCast.NLBEnabledClusters, ",") + + common.UpdateAdmiralParams(newAdmiralConfig) + + fmt.Println(common.GetAdmiralParams().NLBEnabledClusters) + } + + return nil } diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index aa1560d67..2ffa41c21 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -72,8 +72,12 @@ type WorkloadData struct { } type DynamicConfigData struct { - EnableDynamicConfig bool `json:"enableDynamicConfig"` - //NLBEnabledClusters []string `json:"nlbEnabledClusters"` + /* + DynamoDB primary key only support string, binary, int. + Conversation from binary to bool is not straight forward hence choosing string + */ + EnableDynamicConfig string `json:"enableDynamicConfig"` + NLBEnabledClusters []string `json:"nlbEnabledClusters"` } type DynamoClient struct { @@ -210,29 +214,29 @@ func (client *DynamoClient) getWorkloadDataItemByIdentityAndEnv(env, identity, t return workloadDataItems, nil } -func (client *DynamoClient) getDynamicConfig(tableName string) (DynamicConfigData, error) { +func (client *DynamoClient) getDynamicConfig(key string, value string, tableName string) (DynamicConfigData, error) { configData := DynamicConfigData{} - //keyCond := expression.KeyEqual(expression.Key("EnableDynamicConfig"), expression.Value(true)) - // - //expr, err := expression.NewBuilder(). - // WithKeyCondition(keyCond). - // Build() - // - //if err != nil { - // return "", err - //} - - dbQuery := dynamodb.ScanInput{ - TableName: aws.String(tableName), - //ExpressionAttributeNames: expr.Names(), - //ExpressionAttributeValues: expr.Values(), - //KeyConditionExpression: expr.KeyCondition(), - //FilterExpression: expr.Filter(), + keyCond := expression.KeyEqual(expression.Key(key), expression.Value(value)) + + expr, err := expression.NewBuilder(). + WithKeyCondition(keyCond). + Build() + + if err != nil { + return configData, err + } + + dbQuery := dynamodb.QueryInput{ + TableName: aws.String(tableName), + ExpressionAttributeNames: expr.Names(), + ExpressionAttributeValues: expr.Values(), + KeyConditionExpression: expr.KeyCondition(), + FilterExpression: expr.Filter(), } - items, err := client.svc.Scan(&dbQuery) + items, err := client.svc.Query(&dbQuery) if err != nil { return configData, fmt.Errorf("Failed to fetch dynamic config : %s", err) @@ -244,8 +248,6 @@ func (client *DynamoClient) getDynamicConfig(tableName string) (DynamicConfigDat } if *items.Count == 1 { - //err = dynamodbattribute.Unmarshal(items.Items[0], &configData) - //items err = dynamodbattribute.UnmarshalMap(items.Items[0], &configData) if err != nil { diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index 37ac285f6..ab3119a16 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -56,6 +56,12 @@ func GetAdmiralParams() AdmiralParams { return wrapper.params } +func UpdateAdmiralParams(params AdmiralParams) { + //wrapper.Unlock() + //defer wrapper.Lock() + wrapper.params = params +} + func GetAdmiralProfile() string { wrapper.RLock() defer wrapper.RUnlock() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index 679bfd095..75999d435 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -147,6 +147,9 @@ type AdmiralParams struct { EnableDynamicConfig bool DynamicConfigDynamoDBTableName string DynamicSyncPeriod int + NLBEnabledClusters string + NLBEnabledAssetAliases string + CLBEnabledClusters string } func (b AdmiralParams) String() string { From d5fcdcfa385593bd33c8348a6455ba21019faa1b Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Fri, 17 Jan 2025 11:46:53 -0800 Subject: [PATCH 03/17] Some clean up Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 2 +- admiral/pkg/clusters/admiralDatabaseClient.go | 3 ++- admiral/pkg/clusters/dynamoDB.go | 6 ++++-- admiral/pkg/controller/common/types.go | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 018b99b0c..1eb6c42e9 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -276,7 +276,7 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration after which the dynamic sync get performed") rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for NLB") - rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledAssetAliases, "nlb_enabled_assets", "", "Comma seperated list of enabled asset aliases to be enabled for NLB") + rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledIdentityList, "nlb_enabled_identity_list", "", "Comma seperated list of enabled idenity list to be enabled for NLB") rootCmd.PersistentFlags().StringVar(¶ms.CLBEnabledClusters, "clb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for CLB") return rootCmd diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 5e49563f0..c59fe772f 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -180,10 +180,11 @@ func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { fmt.Println(configDataCast) newAdmiralConfig := common.GetAdmiralParams() newAdmiralConfig.NLBEnabledClusters = strings.Join(configDataCast.NLBEnabledClusters, ",") + newAdmiralConfig.CLBEnabledClusters = strings.Join(configDataCast.CLBEnabledClusters, ",") + newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configDataCast.NLBEnabledIdentityList, ",") common.UpdateAdmiralParams(newAdmiralConfig) - fmt.Println(common.GetAdmiralParams().NLBEnabledClusters) } return nil diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 2ffa41c21..49834f48d 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -76,8 +76,10 @@ type DynamicConfigData struct { DynamoDB primary key only support string, binary, int. Conversation from binary to bool is not straight forward hence choosing string */ - EnableDynamicConfig string `json:"enableDynamicConfig"` - NLBEnabledClusters []string `json:"nlbEnabledClusters"` + EnableDynamicConfig string `json:"enableDynamicConfig"` + NLBEnabledClusters []string `json:"nlbEnabledClusters"` + NLBEnabledIdentityList []string `json:"nlbEnabledAssetAlias"` + CLBEnabledClusters []string `json:"clbEnabledClusters"` } type DynamoClient struct { diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index 75999d435..b69aa8e79 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -148,7 +148,7 @@ type AdmiralParams struct { DynamicConfigDynamoDBTableName string DynamicSyncPeriod int NLBEnabledClusters string - NLBEnabledAssetAliases string + NLBEnabledIdentityList string CLBEnabledClusters string } From 88c26ba269ff4159b2edafc1afdbfa0d68c07d08 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Fri, 17 Jan 2025 15:01:43 -0800 Subject: [PATCH 04/17] Refactoring Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 3 ++ admiral/pkg/clusters/admiralDatabaseClient.go | 44 ++++++++++--------- admiral/pkg/controller/common/config.go | 2 - 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 1eb6c42e9..b5120789e 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -272,9 +272,12 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().StringSliceVar(¶ms.ClientDiscoveryClustersForJobs, "client_discovery_clusters_for_jobs", []string{}, "List of clusters for client discovery for k8s jobs") rootCmd.PersistentFlags().StringSliceVar(¶ms.DiscoveryClustersForNumaflow, "client_discovery_clusters_for_numaflow", []string{}, "List of clusters for client discovery for numaflow types") + //Parameter for DynamicConfigPush rootCmd.PersistentFlags().BoolVar(¶ms.EnableDynamicConfig, "enable_dynamic_config", true, "Enable/Disable Dynamic Configuration") rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration after which the dynamic sync get performed") + + //Parameter for NLB releated migration rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for NLB") rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledIdentityList, "nlb_enabled_identity_list", "", "Comma seperated list of enabled idenity list to be enabled for NLB") rootCmd.PersistentFlags().StringVar(¶ms.CLBEnabledClusters, "clb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for CLB") diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index c59fe772f..e56156128 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -76,11 +76,13 @@ func checkIfDatabaseClientIsInitialize(workloadDatabaseClient *WorkloadDatabaseC func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Update(data interface{}, logger *log.Entry) error { //TODO implement me + //At point of release there is no plan to support push config to dyanmic config storage panic("implement me") } func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Delete(data interface{}, logger *log.Entry) error { //TODO implement me + //At point of release there is no plan to support delete config to dyanmic config storage panic("implement me") } @@ -102,17 +104,9 @@ func (databaseClient *DummyDatabaseClient) Get(env, identity string) (interface{ } func NewAdmiralDatabaseClient(admiralConfigPath string, dynamoClientInitFunc func(string, string) (*DynamoClient, error)) (*WorkloadDatabaseClient, error) { - var ( - workloadDatabaseClient = &WorkloadDatabaseClient{} - admiralConfig *v1.AdmiralConfig - ) + var workloadDatabaseClient = &WorkloadDatabaseClient{} - data, err := ioutil.ReadFile(admiralConfigPath) - if err != nil { - return nil, fmt.Errorf("error reading admiral config file, err: %v", err) - } - - err = yaml.Unmarshal(data, &admiralConfig) + admiralConfig, err := ReadDynamoConfigForDynamoDB(admiralConfigPath) if err != nil { return nil, fmt.Errorf("error unmarshalling admiral config file, err: %v", err) } @@ -128,20 +122,28 @@ func NewAdmiralDatabaseClient(admiralConfigPath string, dynamoClientInitFunc fun return workloadDatabaseClient, nil } -func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role string, region string) (*DynamoClient, error)) (*DynamicConfigDatabaseClient, error) { - var ( - admiralConfig *v1.AdmiralConfig - dynamicConfigClient = DynamicConfigDatabaseClient{} - ) +func ReadDynamoConfigForDynamoDB(path string) (*v1.AdmiralConfig, error) { + var admiralConfig *v1.AdmiralConfig data, err := ioutil.ReadFile(path) if err != nil { - return &dynamicConfigClient, fmt.Errorf("error reading admiral config file for DynamicConfig, err: %v", err) + return nil, fmt.Errorf("error reading admiral config file, err: %v", err) } err = yaml.Unmarshal(data, &admiralConfig) if err != nil { - return &dynamicConfigClient, fmt.Errorf("error unmarshalling admiral config file for DynamicConfig, err: %v", err) + return nil, fmt.Errorf("error unmarshalling admiral config file, err: %v", err) + } + + return admiralConfig, nil +} + +func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role string, region string) (*DynamoClient, error)) (*DynamicConfigDatabaseClient, error) { + var dynamicConfigClient = DynamicConfigDatabaseClient{} + + admiralConfig, err := ReadDynamoConfigForDynamoDB(path) + if err != nil { + return nil, fmt.Errorf("error unmarshalling admiral config file, err: %v", err) } dynamicConfigClient.database = &admiralConfig.DynamicConfigDatabase @@ -165,15 +167,15 @@ func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { - configData, err := dbClient.Get("EnableDynamicConfig", "admiral") + dbRawData, err := dbClient.Get("EnableDynamicConfig", "admiral") if err != nil { - //TODO : Log Something Bad Happen but error out + log.Errorf("Error getting EnableDynamicConfig admiral config, err: %v", err) return err } - configDataCast, ok := configData.(DynamicConfigData) + configDataCast, ok := dbRawData.(DynamicConfigData) if !ok { - return errors.New("Failed parse") + return errors.New("Failed to parse DynamicConfigData") } if configDataCast.EnableDynamicConfig == "admiral" { diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index ab3119a16..9e21ecf31 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -57,8 +57,6 @@ func GetAdmiralParams() AdmiralParams { } func UpdateAdmiralParams(params AdmiralParams) { - //wrapper.Unlock() - //defer wrapper.Lock() wrapper.params = params } From 44ed0396693c18da6952114e59b92d6f036404cf Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Fri, 17 Jan 2025 16:25:40 -0800 Subject: [PATCH 05/17] Refactoring and some tests Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 24 +++++++----- admiral/pkg/clusters/types.go | 2 +- admiral/pkg/controller/common/common_test.go | 39 +++++++++++++++++++ 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index e56156128..61bb89156 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -161,11 +161,11 @@ func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { for range time.Tick(time.Minute * time.Duration(syncTime)) { - UpdateSyncAdmiralConfig(dbClient) + ReadAndUpdateSyncAdmiralConfig(dbClient) } } -func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { +func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { dbRawData, err := dbClient.Get("EnableDynamicConfig", "admiral") if err != nil { @@ -173,21 +173,25 @@ func UpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { return err } - configDataCast, ok := dbRawData.(DynamicConfigData) + configData, ok := dbRawData.(DynamicConfigData) if !ok { return errors.New("Failed to parse DynamicConfigData") } - if configDataCast.EnableDynamicConfig == "admiral" { - fmt.Println(configDataCast) + UpdateSyncAdmiralConfig(configData) + + return nil +} + +func UpdateSyncAdmiralConfig(configData DynamicConfigData) { + if configData.EnableDynamicConfig == "admiral" { + //Fetch Existing config and update which are changed. newAdmiralConfig := common.GetAdmiralParams() - newAdmiralConfig.NLBEnabledClusters = strings.Join(configDataCast.NLBEnabledClusters, ",") - newAdmiralConfig.CLBEnabledClusters = strings.Join(configDataCast.CLBEnabledClusters, ",") - newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configDataCast.NLBEnabledIdentityList, ",") + newAdmiralConfig.NLBEnabledClusters = strings.Join(configData.NLBEnabledClusters, ",") + newAdmiralConfig.CLBEnabledClusters = strings.Join(configData.CLBEnabledClusters, ",") + newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configData.NLBEnabledIdentityList, ",") common.UpdateAdmiralParams(newAdmiralConfig) } - - return nil } diff --git a/admiral/pkg/clusters/types.go b/admiral/pkg/clusters/types.go index b62a51151..f2e519728 100644 --- a/admiral/pkg/clusters/types.go +++ b/admiral/pkg/clusters/types.go @@ -214,7 +214,7 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote Later down the process like async processor will take on going config pushes. */ if common.IsAdmiralDynamicConfigEnable() { - UpdateSyncAdmiralConfig(rr.DynamicConfigDatabaseClient) + ReadAndUpdateSyncAdmiralConfig(rr.DynamicConfigDatabaseClient) } return rr diff --git a/admiral/pkg/controller/common/common_test.go b/admiral/pkg/controller/common/common_test.go index 5e2c6efa4..aaf92be5a 100644 --- a/admiral/pkg/controller/common/common_test.go +++ b/admiral/pkg/controller/common/common_test.go @@ -1916,3 +1916,42 @@ func TestSortGtpsByPriorityAndCreationTime(t *testing.T) { }) } } + +func TestIsIstioIngressGatewayService(t *testing.T) { + svc1 := k8sCoreV1.Service{ + TypeMeta: v1.TypeMeta{}, + ObjectMeta: v1.ObjectMeta{}, + Spec: k8sCoreV1.ServiceSpec{}, + Status: k8sCoreV1.ServiceStatus{}, + } + + svc2 := svc1 + svc2.Labels = map[string]string{"app": "istio-ingressgateway"} + svc2.Namespace = NamespaceIstioSystem + + svc3 := svc2 + svc3.Namespace = NamespaceIstioSystem + "_TEST" + + svc4 := svc1 + svc4.Namespace = NamespaceIstioSystem + + type args struct { + svc *k8sCoreV1.Service + key string + } + tests := []struct { + name string + args args + want bool + }{ + {"When Empty K8S Service is present then expect no exception & result is false", args{&svc1, "istio-ingressgateway"}, false}, + {"When correct K8S service is present then expect no exception & result is true", args{&svc2, "istio-ingressgateway"}, true}, + {"When K8S service containts wrong namespace then expect no exception & result is false", args{&svc3, "istio-ingressgateway"}, false}, + {"When K8S service don't have correct label then expect no exception & result is false", args{&svc4, "istio-ingressgateway"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, IsIstioIngressGatewayService(tt.args.svc, tt.args.key), "IsIstioIngressGatewayService(%v, %v)", tt.args.svc, tt.args.key) + }) + } +} From b2db49459461547901c966e0daa12dfb64f07921 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Tue, 21 Jan 2025 15:42:33 -0800 Subject: [PATCH 06/17] Add changes to check if DB config changes. Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 2 +- admiral/pkg/clusters/admiralDatabaseClient.go | 18 +++++++++++++++++- admiral/pkg/clusters/dynamoDB.go | 6 +++++- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index b5120789e..3e2c92bcd 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -275,7 +275,7 @@ func GetRootCmd(args []string) *cobra.Command { //Parameter for DynamicConfigPush rootCmd.PersistentFlags().BoolVar(¶ms.EnableDynamicConfig, "enable_dynamic_config", true, "Enable/Disable Dynamic Configuration") rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") - rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration after which the dynamic sync get performed") + rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration in min after which the dynamic sync get performed") //Parameter for NLB releated migration rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for NLB") diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 61bb89156..398e4cc0b 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -1,6 +1,7 @@ package clusters import ( + "crypto/sha256" "errors" "fmt" "github.com/istio-ecosystem/admiral/admiral/apis/v1" @@ -178,11 +179,26 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { return errors.New("Failed to parse DynamicConfigData") } - UpdateSyncAdmiralConfig(configData) + if IsDBConfigChanged(configData) { + log.Infof("Updating DynamicConfigData with Admiral config") + UpdateSyncAdmiralConfig(configData) + } else { + log.Infof("No need to update DynamicConfigData") + } return nil } +func IsDBConfigChanged(config DynamicConfigData) bool { + + if DynamicConfigCheckSum == sha256.Sum256([]byte(fmt.Sprintf("%v", config))) { + return false + } else { + DynamicConfigCheckSum = sha256.Sum256([]byte(fmt.Sprintf("%v", config))) + return true + } +} + func UpdateSyncAdmiralConfig(configData DynamicConfigData) { if configData.EnableDynamicConfig == "admiral" { //Fetch Existing config and update which are changed. diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 49834f48d..768fd37af 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -35,6 +35,8 @@ type DynamoDBConfigWrapper struct { DynamoDBConfig DynamoDBConfig `yaml:"dynamoDB,omitempty"` } +var DynamicConfigCheckSum [32]byte + /* Reference struct used to unmarshall the DynamoDB config present in the yaml config file */ @@ -83,7 +85,8 @@ type DynamicConfigData struct { } type DynamoClient struct { - svc dynamodbiface.DynamoDBAPI + svc dynamodbiface.DynamoDBAPI + checkSha string } func NewDynamoClient(role, region string) (*DynamoClient, error) { @@ -255,6 +258,7 @@ func (client *DynamoClient) getDynamicConfig(key string, value string, tableName if err != nil { return configData, fmt.Errorf("failed to unmarshal table items, err: %v", err) } + } else { return configData, fmt.Errorf("Expected only 1 row but got %s for tableName : %s", items.Count, tableName) } From 61fa85c058b11a7ad33ac02916f9cdfda94c132f Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Tue, 21 Jan 2025 16:43:47 -0800 Subject: [PATCH 07/17] clean up Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 2 +- admiral/pkg/clusters/admiralDatabaseClient.go | 4 ++-- admiral/pkg/clusters/dynamoDB.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 3e2c92bcd..89924a887 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -274,7 +274,7 @@ func GetRootCmd(args []string) *cobra.Command { //Parameter for DynamicConfigPush rootCmd.PersistentFlags().BoolVar(¶ms.EnableDynamicConfig, "enable_dynamic_config", true, "Enable/Disable Dynamic Configuration") - rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "admiral-dynamic-config-dev", "The name of the dynamic config dynamodb table") + rootCmd.PersistentFlags().StringVar(¶ms.DynamicConfigDynamoDBTableName, "dynamic_config_dynamodb_table_name", "dynamic-config-dev", "The name of the dynamic config dynamodb table") rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration in min after which the dynamic sync get performed") //Parameter for NLB releated migration diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 398e4cc0b..f568881af 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -179,7 +179,7 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { return errors.New("Failed to parse DynamicConfigData") } - if IsDBConfigChanged(configData) { + if IsDynamicConfigChanged(configData) { log.Infof("Updating DynamicConfigData with Admiral config") UpdateSyncAdmiralConfig(configData) } else { @@ -189,7 +189,7 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { return nil } -func IsDBConfigChanged(config DynamicConfigData) bool { +func IsDynamicConfigChanged(config DynamicConfigData) bool { if DynamicConfigCheckSum == sha256.Sum256([]byte(fmt.Sprintf("%v", config))) { return false diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 768fd37af..6afbfa412 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -35,6 +35,7 @@ type DynamoDBConfigWrapper struct { DynamoDBConfig DynamoDBConfig `yaml:"dynamoDB,omitempty"` } +// Store last known CheckSum of DynamoDB config var DynamicConfigCheckSum [32]byte /* @@ -85,8 +86,7 @@ type DynamicConfigData struct { } type DynamoClient struct { - svc dynamodbiface.DynamoDBAPI - checkSha string + svc dynamodbiface.DynamoDBAPI } func NewDynamoClient(role, region string) (*DynamoClient, error) { From 01025e02452c2b28a41bd1ebb5de177c96bd850f Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Tue, 21 Jan 2025 17:42:27 -0800 Subject: [PATCH 08/17] clean up & some unit test Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 20 ++++-- .../clusters/admiralDatabaseClient_test.go | 69 +++++++++++++++++++ admiral/pkg/clusters/dynamoDB.go | 2 +- admiral/pkg/controller/common/common.go | 1 + 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index f568881af..ce29d44e0 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -191,6 +191,10 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { func IsDynamicConfigChanged(config DynamicConfigData) bool { + if config.EnableDynamicConfig != common.Admiral { + return false + } + if DynamicConfigCheckSum == sha256.Sum256([]byte(fmt.Sprintf("%v", config))) { return false } else { @@ -200,12 +204,20 @@ func IsDynamicConfigChanged(config DynamicConfigData) bool { } func UpdateSyncAdmiralConfig(configData DynamicConfigData) { - if configData.EnableDynamicConfig == "admiral" { + if configData.EnableDynamicConfig == common.Admiral { //Fetch Existing config and update which are changed. newAdmiralConfig := common.GetAdmiralParams() - newAdmiralConfig.NLBEnabledClusters = strings.Join(configData.NLBEnabledClusters, ",") - newAdmiralConfig.CLBEnabledClusters = strings.Join(configData.CLBEnabledClusters, ",") - newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configData.NLBEnabledIdentityList, ",") + if len(strings.Join(configData.NLBEnabledClusters, ",")) > 0 { + newAdmiralConfig.NLBEnabledClusters = strings.Join(configData.NLBEnabledClusters, ",") + } + + if len(strings.Join(configData.CLBEnabledClusters, ",")) > 0 { + newAdmiralConfig.CLBEnabledClusters = strings.Join(configData.CLBEnabledClusters, ",") + } + + if len(strings.Join(configData.NLBEnabledIdentityList, ",")) > 0 { + newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configData.NLBEnabledIdentityList, ",") + } common.UpdateAdmiralParams(newAdmiralConfig) diff --git a/admiral/pkg/clusters/admiralDatabaseClient_test.go b/admiral/pkg/clusters/admiralDatabaseClient_test.go index adc034ba1..82e28182a 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient_test.go +++ b/admiral/pkg/clusters/admiralDatabaseClient_test.go @@ -1,5 +1,11 @@ package clusters +import ( + "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" + "github.com/stretchr/testify/assert" + "testing" +) + /* import ( "fmt" @@ -423,3 +429,66 @@ func TestDeleteWorkloadData(t *testing.T) { } } */ + +func TestIsDynamicConfigChanged(t *testing.T) { + type args struct { + config DynamicConfigData + } + + config := DynamicConfigData{} + + config1 := DynamicConfigData{EnableDynamicConfig: common.Admiral} + + config2 := config1 + config2.NLBEnabledIdentityList = []string{"identity1", "identity2"} + + tests := []struct { + name string + args args + want bool + }{ + {"When empty config send then it should return false", args{config}, false}, + {"When admiral is loading/or no previous checksum present then it should return true", args{config1}, true}, + {"When admiral is loaded and no checksum mismatch then it should return false", args{config1}, false}, + {"When admiral is loaded and checksum mismatched then it should return true", args{config2}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, IsDynamicConfigChanged(tt.args.config), "IsDynamicConfigChanged(%v)", tt.args.config) + }) + } +} + +func TestUpdateSyncAdmiralConfig(t *testing.T) { + type args struct { + configData DynamicConfigData + } + + configUpdated := DynamicConfigData{EnableDynamicConfig: common.Admiral} + configUpdated.NLBEnabledClusters = []string{"cluster1", "cluster2"} + configUpdated.NLBEnabledIdentityList = []string{"identity1", "identity2"} + configUpdated.CLBEnabledClusters = []string{"cluster1", "cluster2"} + + expectedAdmiralConfig := common.GetAdmiralParams() + expectedAdmiralConfig.NLBEnabledClusters = "cluster1,cluster2" + expectedAdmiralConfig.CLBEnabledClusters = "cluster1,cluster2" + expectedAdmiralConfig.NLBEnabledIdentityList = "identity1,identity2" + + emptyConfig := DynamicConfigData{} + expectedEmptyConfig := common.GetAdmiralParams() + + tests := []struct { + name string + args args + want common.AdmiralParams + }{ + {"EmptyConfig", args{emptyConfig}, expectedEmptyConfig}, + {"AdmiralConfigUpdate", args{configUpdated}, expectedAdmiralConfig}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + UpdateSyncAdmiralConfig(tt.args.configData) + assert.Equal(t, tt.want, common.GetAdmiralParams()) + }) + } +} diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 6afbfa412..084ad75bb 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -260,7 +260,7 @@ func (client *DynamoClient) getDynamicConfig(key string, value string, tableName } } else { - return configData, fmt.Errorf("Expected only 1 row but got %s for tableName : %s", items.Count, tableName) + return configData, fmt.Errorf("Expected only 1 row but got %d for tableName : %s", items.Count, tableName) } return configData, nil diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index c1db468d3..9cfb0328b 100644 --- a/admiral/pkg/controller/common/common.go +++ b/admiral/pkg/controller/common/common.go @@ -37,6 +37,7 @@ var ( ) const ( + Admiral = "admiral" NamespaceKubeSystem = "kube-system" NamespaceIstioSystem = "istio-system" IstioIngressGatewayLabelValue = "istio-ingressgateway" From ade15b3bacf981044463ff830fdc016897d5c54c Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Tue, 21 Jan 2025 18:07:21 -0800 Subject: [PATCH 09/17] clean up & some unit test Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/types.go | 2 -- admiral/pkg/controller/common/config_test.go | 4 ++++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/admiral/pkg/clusters/types.go b/admiral/pkg/clusters/types.go index f2e519728..e6964c48d 100644 --- a/admiral/pkg/clusters/types.go +++ b/admiral/pkg/clusters/types.go @@ -81,8 +81,6 @@ type AdmiralCache struct { CnameDependentClusterNamespaceCache *common.MapOfMapOfMaps PartitionIdentityCache *common.Map ClientClusterNamespaceServerCache *common.MapOfMapOfMaps - NLBEnabledClusterCache []string - NLBEnabledServiceCache []string } type RemoteRegistry struct { diff --git a/admiral/pkg/controller/common/config_test.go b/admiral/pkg/controller/common/config_test.go index acb5bf439..cf21dc804 100644 --- a/admiral/pkg/controller/common/config_test.go +++ b/admiral/pkg/controller/common/config_test.go @@ -368,6 +368,10 @@ func TestConfigManagement(t *testing.T) { t.Errorf("Enable Traffic Persona mismatch, expected false, got %v", IsPersonaTrafficConfig()) } + if IsAdmiralDynamicConfigEnable() != false { + t.Errorf("Enable Dynamic Config mismatch, expected false, got %v", IsAdmiralDynamicConfigEnable()) + } + if IsDefaultPersona() != true { t.Errorf("Enable Default Persona mismatch, expected false, got %v", IsDefaultPersona()) } From 450de2dd1316f25ea60892915825cf3f172efd8a Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Wed, 22 Jan 2025 13:50:47 -0800 Subject: [PATCH 10/17] dynamo db test Signed-off-by: Viraj Kulkarni --- .../clusters/admiralDatabaseClient_test.go | 52 +++++++++++++++++++ .../admiralDatabaseClientConfig_is_valid.yaml | 19 +++++++ 2 files changed, 71 insertions(+) create mode 100644 admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_is_valid.yaml diff --git a/admiral/pkg/clusters/admiralDatabaseClient_test.go b/admiral/pkg/clusters/admiralDatabaseClient_test.go index 82e28182a..eb16f59b4 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient_test.go +++ b/admiral/pkg/clusters/admiralDatabaseClient_test.go @@ -1,6 +1,8 @@ package clusters import ( + "fmt" + v1 "github.com/istio-ecosystem/admiral/admiral/apis/v1" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" "github.com/stretchr/testify/assert" "testing" @@ -492,3 +494,53 @@ func TestUpdateSyncAdmiralConfig(t *testing.T) { }) } } + +func TestNewDynamicConfigDatabaseClient(t *testing.T) { + var dummyDynamoClientFunc = func(role, region string) (*DynamoClient, error) { + return nil, nil + } + + var dummyDynamoClientFuncWithError = func(role, region string) (*DynamoClient, error) { + return nil, fmt.Errorf("failed to initialize client") + } + + type args struct { + path string + dynamoClientInitFunc func(role string, region string) (*DynamoClient, error) + } + + var dynamicConfigClient = DynamicConfigDatabaseClient{} + + expectedV1AdmiralConfig := v1.AdmiralConfig{} + dynamicConfigClient.database = &expectedV1AdmiralConfig.DynamicConfigDatabase + dynamicConfigClient.database.TableName = common.GetAdmiralParams().DynamicConfigDynamoDBTableName + + testArgsValid := args{ + path: "testdata/admiralDatabaseClientConfig_is_valid.yaml", + dynamoClientInitFunc: dummyDynamoClientFunc, + } + + testArgsError := args{ + path: "testdata/admiralDatabaseClientConfig_is_valid.yaml", + dynamoClientInitFunc: dummyDynamoClientFuncWithError, + } + + tests := []struct { + name string + args args + want *DynamicConfigDatabaseClient + wantErr bool + }{ + {"When valid config is passed then expected client to be initialize with no error", testArgsValid, &dynamicConfigClient, false}, + {"When valid is passed then expected error", testArgsError, &dynamicConfigClient, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := NewDynamicConfigDatabaseClient(tt.args.path, tt.args.dynamoClientInitFunc) + if tt.wantErr { + assert.NotNil(t, err, "NewDynamicConfigDatabaseClient() should have returned an error") + } + assert.Equalf(t, tt.want, got, "NewDynamicConfigDatabaseClient(%v, %v)", tt.args.path, tt.args.dynamoClientInitFunc) + }) + } +} diff --git a/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_is_valid.yaml b/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_is_valid.yaml new file mode 100644 index 000000000..b21b8ee42 --- /dev/null +++ b/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_is_valid.yaml @@ -0,0 +1,19 @@ +ignoreIdentityList: + stateCheckerPeriodInSeconds: 60 + dynamoDB: + region: "us-east-2" + role: "arn:aws:iam::1111111:role/Admiral-IKS-Dynamo-Read-Access" + tableName: "admiral-ignore-identity-state" + clusterEnvironment: "dev" +dynamoDB: + leaseName: qal + podIdentifier: qal-east + waitTimeInSeconds: 15 + failureThreshold: 3 + tableName: admiral-lease + role: arn:aws:iam::11111111:role/Admiral-IKS-Access + region: us-east-2 +database: + region: "us-west-2" + role: "arn:aws:iam::11111111:role/Admiral-IKS-Dynamo-Read-Write-Access" + workloadDataTableName: admiral-workload-data-dev \ No newline at end of file From e95128c0523dfe2acf08b11de064bd71676af6ad Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Wed, 22 Jan 2025 15:39:47 -0800 Subject: [PATCH 11/17] unit test cover update scenario Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 2 +- .../clusters/admiralDatabaseClient_test.go | 72 +++++++++++++++++-- .../admiralDatabaseClientConfig_invalid.yaml | 20 ++++++ 3 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_invalid.yaml diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index ce29d44e0..eb431328c 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -168,7 +168,7 @@ func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) { func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { - dbRawData, err := dbClient.Get("EnableDynamicConfig", "admiral") + dbRawData, err := dbClient.Get("EnableDynamicConfig", common.Admiral) if err != nil { log.Errorf("Error getting EnableDynamicConfig admiral config, err: %v", err) return err diff --git a/admiral/pkg/clusters/admiralDatabaseClient_test.go b/admiral/pkg/clusters/admiralDatabaseClient_test.go index eb16f59b4..8cce7b132 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient_test.go +++ b/admiral/pkg/clusters/admiralDatabaseClient_test.go @@ -1,9 +1,11 @@ package clusters import ( + "errors" "fmt" v1 "github.com/istio-ecosystem/admiral/admiral/apis/v1" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "testing" ) @@ -525,22 +527,82 @@ func TestNewDynamicConfigDatabaseClient(t *testing.T) { dynamoClientInitFunc: dummyDynamoClientFuncWithError, } + testArgsErrorMarshalling := args{ + path: "testdata/admiralDatabaseClientConfig_invalid.yaml", + dynamoClientInitFunc: dummyDynamoClientFunc, + } + tests := []struct { name string args args want *DynamicConfigDatabaseClient - wantErr bool + wantErr error }{ - {"When valid config is passed then expected client to be initialize with no error", testArgsValid, &dynamicConfigClient, false}, - {"When valid is passed then expected error", testArgsError, &dynamicConfigClient, true}, + {"When valid config is passed then expected client to be initialize with no error", testArgsValid, &dynamicConfigClient, nil}, + {"When valid is passed then expected error", testArgsError, &dynamicConfigClient, errors.New("unable to instantiate dynamo client for DynamicConfig")}, + {"When invalid config is passed then expect error", testArgsErrorMarshalling, nil, errors.New("error unmarshalling admiral config file")}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := NewDynamicConfigDatabaseClient(tt.args.path, tt.args.dynamoClientInitFunc) - if tt.wantErr { - assert.NotNil(t, err, "NewDynamicConfigDatabaseClient() should have returned an error") + if tt.wantErr != nil { + assert.Contains(t, err.Error(), tt.wantErr.Error()) } assert.Equalf(t, tt.want, got, "NewDynamicConfigDatabaseClient(%v, %v)", tt.args.path, tt.args.dynamoClientInitFunc) }) } } + +type DummyDynamicConfigDatabaseClient struct { + DynamoClient *DynamoClient + datbaase *v1.DynamoDB +} + +func (d DummyDynamicConfigDatabaseClient) Update(data interface{}, logger *log.Entry) error { + //TODO implement me + panic("implement me") +} + +func (d DummyDynamicConfigDatabaseClient) Delete(data interface{}, logger *log.Entry) error { + //TODO implement me + panic("implement me") +} + +func (d DummyDynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) { + //TODO implement me + dummyDynamicConfigData := DynamicConfigData{ + EnableDynamicConfig: common.Admiral, + NLBEnabledClusters: []string{"cluster1", "cluster2"}, + NLBEnabledIdentityList: []string{"identity1", "identity2"}, + CLBEnabledClusters: []string{"cluster1", "cluster2"}, + } + + return dummyDynamicConfigData, nil +} + +func TestReadAndUpdateSyncAdmiralConfig(t *testing.T) { + + var testData DummyDynamicConfigDatabaseClient + type args struct { + dbClient AdmiralDatabaseManager + } + + var testArgs = args{dbClient: testData} + tests := []struct { + name string + args args + wantErr error + }{ + {"When ReadAndUpdateSyncAdmiralConfig invoked with valid DynamoClient then expect no error", testArgs, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ReadAndUpdateSyncAdmiralConfig(tt.args.dbClient) + if tt.wantErr != nil { + assert.Contains(t, err.Error(), tt.wantErr.Error(), "ReadAndUpdateSyncAdmiralConfig(). Expect error containing %s but got error = %v", tt.wantErr.Error(), err.Error()) + } else { + assert.Nil(t, err, "ReadAndUpdateSyncAdmiralConfig(). Expect no error but got error - %s", err) + } + }) + } +} diff --git a/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_invalid.yaml b/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_invalid.yaml new file mode 100644 index 000000000..60de2613a --- /dev/null +++ b/admiral/pkg/clusters/testdata/admiralDatabaseClientConfig_invalid.yaml @@ -0,0 +1,20 @@ +ignoreIdentityList: + stateCheckerPeriodInSeconds: 60 + dynamoDB: + region: "us-east-2" + role: "arn:aws:iam::1111111:role/Admiral-IKS-Dynamo-Read-Access" + tableName: "admiral-ignore-identity-state" + clusterEnvironment: "dev" +dynamoDB: + leaseName: qal + podIdentifier: qal-east + waitTimeInSeconds: 15 + failureThreshold: 3 + tableName: admiral-lease + role: arn:aws:iam::11111111:role/Admiral-IKS-Access + region: us-east-2 +database: + region: "us-west-2" + role: "arn:aws:iam::11111111:role/Admiral-IKS-Dynamo-Read-Write-Access" + workloadDataTableName: admiral-workload-data-dev + testField: "testValue" \ No newline at end of file From 078e1f63eb2cf3e804e1694e82f03f46ce110bb3 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Wed, 22 Jan 2025 18:15:39 -0800 Subject: [PATCH 12/17] unit test cover DB fetch scenario Signed-off-by: Viraj Kulkarni --- .../clusters/admiralDatabaseClient_test.go | 1 - admiral/pkg/clusters/dynamoDB.go | 3 +- admiral/pkg/clusters/dynamoDB_test.go | 113 ++++++++++++++++++ 3 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 admiral/pkg/clusters/dynamoDB_test.go diff --git a/admiral/pkg/clusters/admiralDatabaseClient_test.go b/admiral/pkg/clusters/admiralDatabaseClient_test.go index 8cce7b132..6229e378e 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient_test.go +++ b/admiral/pkg/clusters/admiralDatabaseClient_test.go @@ -569,7 +569,6 @@ func (d DummyDynamicConfigDatabaseClient) Delete(data interface{}, logger *log.E } func (d DummyDynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) { - //TODO implement me dummyDynamicConfigData := DynamicConfigData{ EnableDynamicConfig: common.Admiral, NLBEnabledClusters: []string{"cluster1", "cluster2"}, diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 084ad75bb..3b2ecd09f 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -252,9 +252,8 @@ func (client *DynamoClient) getDynamicConfig(key string, value string, tableName return configData, nil } - if *items.Count == 1 { + if items.Count != nil && *items.Count == 1 { err = dynamodbattribute.UnmarshalMap(items.Items[0], &configData) - if err != nil { return configData, fmt.Errorf("failed to unmarshal table items, err: %v", err) } diff --git a/admiral/pkg/clusters/dynamoDB_test.go b/admiral/pkg/clusters/dynamoDB_test.go new file mode 100644 index 000000000..3e28e0090 --- /dev/null +++ b/admiral/pkg/clusters/dynamoDB_test.go @@ -0,0 +1,113 @@ +package clusters + +import ( + "errors" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/dynamodb" + "github.com/aws/aws-sdk-go/service/dynamodb/dynamodbattribute" + "github.com/aws/aws-sdk-go/service/dynamodb/dynamodbiface" + "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" + "github.com/stretchr/testify/assert" + "testing" +) + +type mockDynamoDBClient struct { + dynamodbiface.DynamoDBAPI +} + +func (m *mockDynamoDBClient) Query(queryInput *dynamodb.QueryInput) (*dynamodb.QueryOutput, error) { + + if *queryInput.TableName == "validConfig" { + return &dynamodb.QueryOutput{ + Items: []map[string]*dynamodb.AttributeValue{ + { + "EnableDynamicConfig": {S: aws.String(common.Admiral)}, + }, + }, + }, nil + } else if *queryInput.TableName == "nilConfig" { + return nil, nil + } else if *queryInput.TableName == "itemsWithAdmiral" { + configData := DynamicConfigData{ + EnableDynamicConfig: common.Admiral, + NLBEnabledClusters: []string{"cluster1", "cluster2"}, + NLBEnabledIdentityList: nil, + CLBEnabledClusters: nil, + } + testDynamoDBAttribute, _ := dynamodbattribute.MarshalMap(&configData) + + queryOutput := &dynamodb.QueryOutput{ + Items: []map[string]*dynamodb.AttributeValue{}, + } + var queryCount int64 + queryCount = 1 + queryOutput.Items = append(queryOutput.Items, testDynamoDBAttribute) + queryOutput.Count = &queryCount + + return queryOutput, nil + } else if *queryInput.TableName == "throwError" { + return nil, errors.New("test error") + } + return nil, nil + +} + +func TestDynamoClient_getDynamicConfig(t *testing.T) { + + testDynamoDBClient := DynamoClient{} + testDynamoDBClient.svc = &mockDynamoDBClient{} + + type args struct { + key string + value string + tableName string + } + + testArgs := args{ + key: "id", + value: "id", + tableName: "nilConfig", + } + + testArgs1 := args{ + key: "test", + value: "test", + tableName: "validConfig", + } + + testArgs2 := args{ + key: "test", + value: "test", + tableName: "itemsWithAdmiral", + } + + testArgs3 := args{ + key: "test", + value: "test", + tableName: "throwError", + } + + tests := []struct { + name string + fields *DynamoClient + args args + want DynamicConfigData + wantErr error + }{ + {"When dynamodb return empty object then expect no error", &testDynamoDBClient, testArgs, DynamicConfigData{}, nil}, + {"When dynamodb return non object then expect non DynamicConfigData", &testDynamoDBClient, testArgs1, DynamicConfigData{}, errors.New("Expected only 1 row but got 0 for tableName")}, + {"When dynamodb return object with NLBCluster", &testDynamoDBClient, testArgs2, DynamicConfigData{EnableDynamicConfig: common.Admiral, NLBEnabledClusters: []string{"cluster1", "cluster2"}}, nil}, + {"When dynamodb return error then error needs to be bubbled up", &testDynamoDBClient, testArgs3, DynamicConfigData{}, errors.New("Failed to fetch dynamic config")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.fields.getDynamicConfig(tt.args.key, tt.args.value, tt.args.tableName) + if tt.wantErr != nil { + assert.Contains(t, err.Error(), tt.wantErr.Error()) + } else { + assert.Nil(t, err, "GetDynamicConfig() should not return error") + assert.Equalf(t, tt.want, got, "getDynamicConfig(%v, %v, %v)", tt.args.key, tt.args.value, tt.args.tableName) + } + }) + } +} From 9c763719676555cd4706194921bd30b4b379ede2 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Mon, 27 Jan 2025 14:17:29 -0800 Subject: [PATCH 13/17] review part 1 Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 2 +- admiral/pkg/clusters/admiralDatabaseClient.go | 12 ++++++------ admiral/pkg/clusters/dynamoDB.go | 6 +++--- admiral/pkg/clusters/types.go | 4 ++-- admiral/pkg/controller/common/common.go | 2 ++ admiral/pkg/controller/common/config.go | 2 +- admiral/pkg/controller/common/config_test.go | 4 ++-- 7 files changed, 17 insertions(+), 15 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 89924a887..51db01805 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -78,7 +78,7 @@ func GetRootCmd(args []string) *cobra.Command { log.Fatalf("Error: %v", err) } - if common.IsAdmiralDynamicConfigEnable() { + if common.IsAdmiralDynamicConfigEnabled() { go clusters.UpdateASyncAdmiralConfig(remoteRegistry.DynamicConfigDatabaseClient, params.DynamicSyncPeriod) } diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index eb431328c..6df1612a5 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -144,7 +144,7 @@ func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role admiralConfig, err := ReadDynamoConfigForDynamoDB(path) if err != nil { - return nil, fmt.Errorf("error unmarshalling admiral config file, err: %v", err) + return nil, fmt.Errorf("task=%v, error unmarshalling admiral config file, err: %v", common.DynamicConfigUpdate, err) } dynamicConfigClient.database = &admiralConfig.DynamicConfigDatabase @@ -154,7 +154,7 @@ func NewDynamicConfigDatabaseClient(path string, dynamoClientInitFunc func(role admiralConfig.WorkloadDatabase.Region, ) if err != nil { - return &dynamicConfigClient, fmt.Errorf("unable to instantiate dynamo client for DynamicConfig, err: %v", err) + return &dynamicConfigClient, fmt.Errorf("task=%v, unable to instantiate dynamo client for DynamicConfig, err: %v", common.DynamicConfigUpdate, err) } return &dynamicConfigClient, nil } @@ -170,20 +170,20 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { dbRawData, err := dbClient.Get("EnableDynamicConfig", common.Admiral) if err != nil { - log.Errorf("Error getting EnableDynamicConfig admiral config, err: %v", err) + log.Errorf("task=%s, Error getting EnableDynamicConfig admiral config, err: %v", common.DynamicConfigUpdate, err) return err } configData, ok := dbRawData.(DynamicConfigData) if !ok { - return errors.New("Failed to parse DynamicConfigData") + return errors.New(fmt.Sprintf("task=%s, Failed to parse DynamicConfigData", common.DynamicConfigUpdate)) } if IsDynamicConfigChanged(configData) { - log.Infof("Updating DynamicConfigData with Admiral config") + log.Infof(fmt.Sprintf("task=%s, Updating DynamicConfigData with Admiral config", common.DynamicConfigUpdate)) UpdateSyncAdmiralConfig(configData) } else { - log.Infof("No need to update DynamicConfigData") + log.Infof(fmt.Sprintf("task=%s, No need to update DynamicConfigData", common.DynamicConfigUpdate)) } return nil diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index 3b2ecd09f..e0d130615 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -244,7 +244,7 @@ func (client *DynamoClient) getDynamicConfig(key string, value string, tableName items, err := client.svc.Query(&dbQuery) if err != nil { - return configData, fmt.Errorf("Failed to fetch dynamic config : %s", err) + return configData, fmt.Errorf("task=%s, Failed to fetch dynamic config : %s", common.DynamicConfigUpdate, err) } if items == nil { @@ -255,11 +255,11 @@ func (client *DynamoClient) getDynamicConfig(key string, value string, tableName if items.Count != nil && *items.Count == 1 { err = dynamodbattribute.UnmarshalMap(items.Items[0], &configData) if err != nil { - return configData, fmt.Errorf("failed to unmarshal table items, err: %v", err) + return configData, fmt.Errorf("task=%s, failed to unmarshal table items, err: %v", common.DynamicConfigUpdate, err) } } else { - return configData, fmt.Errorf("Expected only 1 row but got %d for tableName : %s", items.Count, tableName) + return configData, fmt.Errorf("task=%s, Expected only 1 row but got %d for tableName : %s", common.DynamicConfigUpdate, items.Count, tableName) } return configData, nil diff --git a/admiral/pkg/clusters/types.go b/admiral/pkg/clusters/types.go index e6964c48d..ef4eb0200 100644 --- a/admiral/pkg/clusters/types.go +++ b/admiral/pkg/clusters/types.go @@ -164,7 +164,7 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote serviceEntrySuspender = NewDummyServiceEntrySuspender() } - if common.IsAdmiralDynamicConfigEnable() { + if common.IsAdmiralDynamicConfigEnabled() { admiralDynamicConfigDatabaseClient, err = NewDynamicConfigDatabaseClient(common.GetAdmiralConfigPath(), NewDynamoClient) } else { admiralDynamicConfigDatabaseClient = &DummyDatabaseClient{} @@ -211,7 +211,7 @@ func NewRemoteRegistry(ctx context.Context, params common.AdmiralParams) *Remote This is done to avoid any transitive state where component starts with outofsync config. Later down the process like async processor will take on going config pushes. */ - if common.IsAdmiralDynamicConfigEnable() { + if common.IsAdmiralDynamicConfigEnabled() { ReadAndUpdateSyncAdmiralConfig(rr.DynamicConfigDatabaseClient) } diff --git a/admiral/pkg/controller/common/common.go b/admiral/pkg/controller/common/common.go index 9cfb0328b..c268aaf33 100644 --- a/admiral/pkg/controller/common/common.go +++ b/admiral/pkg/controller/common/common.go @@ -122,6 +122,8 @@ const ( LastUpdatedAt = "lastUpdatedAt" IntuitTID = "intuit_tid" GTPCtrl = "gtp-ctrl" + + DynamicConfigUpdate = "DynamicConfigUpdate" ) type Event string diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index 9e21ecf31..640e8a0d1 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -150,7 +150,7 @@ func GetEnableWorkloadDataStorage() bool { return wrapper.params.EnableWorkloadDataStorage } -func IsAdmiralDynamicConfigEnable() bool { +func IsAdmiralDynamicConfigEnabled() bool { wrapper.RLock() defer wrapper.RUnlock() return wrapper.params.EnableDynamicConfig diff --git a/admiral/pkg/controller/common/config_test.go b/admiral/pkg/controller/common/config_test.go index cf21dc804..a3ac0cc23 100644 --- a/admiral/pkg/controller/common/config_test.go +++ b/admiral/pkg/controller/common/config_test.go @@ -368,8 +368,8 @@ func TestConfigManagement(t *testing.T) { t.Errorf("Enable Traffic Persona mismatch, expected false, got %v", IsPersonaTrafficConfig()) } - if IsAdmiralDynamicConfigEnable() != false { - t.Errorf("Enable Dynamic Config mismatch, expected false, got %v", IsAdmiralDynamicConfigEnable()) + if IsAdmiralDynamicConfigEnabled() != false { + t.Errorf("Enable Dynamic Config mismatch, expected false, got %v", IsAdmiralDynamicConfigEnabled()) } if IsDefaultPersona() != true { From 8ac12568079b1831ade100f99b337dddfb636ed0 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Mon, 27 Jan 2025 14:28:03 -0800 Subject: [PATCH 14/17] review part 2 Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 8 +++++--- admiral/pkg/clusters/dynamoDB.go | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 6df1612a5..415319c9f 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -1,7 +1,7 @@ package clusters import ( - "crypto/sha256" + "crypto/md5" "errors" "fmt" "github.com/istio-ecosystem/admiral/admiral/apis/v1" @@ -195,10 +195,12 @@ func IsDynamicConfigChanged(config DynamicConfigData) bool { return false } - if DynamicConfigCheckSum == sha256.Sum256([]byte(fmt.Sprintf("%v", config))) { + //md5.Sum() + + if DynamicConfigCheckSum == md5.Sum([]byte(fmt.Sprintf("%v", config))) { return false } else { - DynamicConfigCheckSum = sha256.Sum256([]byte(fmt.Sprintf("%v", config))) + DynamicConfigCheckSum = md5.Sum([]byte(fmt.Sprintf("%v", config))) return true } } diff --git a/admiral/pkg/clusters/dynamoDB.go b/admiral/pkg/clusters/dynamoDB.go index e0d130615..20f69fd23 100644 --- a/admiral/pkg/clusters/dynamoDB.go +++ b/admiral/pkg/clusters/dynamoDB.go @@ -36,7 +36,7 @@ type DynamoDBConfigWrapper struct { } // Store last known CheckSum of DynamoDB config -var DynamicConfigCheckSum [32]byte +var DynamicConfigCheckSum [16]byte /* Reference struct used to unmarshall the DynamoDB config present in the yaml config file From 48d17ce433b4deabf8be203293f586049c692779 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Mon, 27 Jan 2025 15:10:00 -0800 Subject: [PATCH 15/17] review part 3 Signed-off-by: Viraj Kulkarni --- admiral/cmd/admiral/cmd/root.go | 6 +++--- admiral/pkg/clusters/admiralDatabaseClient.go | 13 ++++++------- admiral/pkg/clusters/admiralDatabaseClient_test.go | 6 +++--- admiral/pkg/controller/common/types.go | 6 +++--- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 51db01805..7cfe4733e 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -278,9 +278,9 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().IntVar(¶ms.DynamicSyncPeriod, "dynamic_sync_period", 10, "Duration in min after which the dynamic sync get performed") //Parameter for NLB releated migration - rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for NLB") - rootCmd.PersistentFlags().StringVar(¶ms.NLBEnabledIdentityList, "nlb_enabled_identity_list", "", "Comma seperated list of enabled idenity list to be enabled for NLB") - rootCmd.PersistentFlags().StringVar(¶ms.CLBEnabledClusters, "clb_enabled_clusters", "", "Comma seperated list of enabled clusters to be enabled for CLB") + rootCmd.PersistentFlags().StringSliceVar(¶ms.NLBEnabledClusters, "nlb_enabled_clusters", []string{}, "Comma seperated list of enabled clusters to be enabled for NLB") + rootCmd.PersistentFlags().StringSliceVar(¶ms.NLBEnabledIdentityList, "nlb_enabled_identity_list", []string{}, "Comma seperated list of enabled idenity list to be enabled for NLB") + rootCmd.PersistentFlags().StringSliceVar(¶ms.CLBEnabledClusters, "clb_enabled_clusters", []string{}, "Comma seperated list of enabled clusters to be enabled for CLB") return rootCmd } diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 415319c9f..51e8e20ad 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -9,7 +9,6 @@ import ( log "github.com/sirupsen/logrus" "gopkg.in/yaml.v2" "io/ioutil" - "strings" "time" ) @@ -209,16 +208,16 @@ func UpdateSyncAdmiralConfig(configData DynamicConfigData) { if configData.EnableDynamicConfig == common.Admiral { //Fetch Existing config and update which are changed. newAdmiralConfig := common.GetAdmiralParams() - if len(strings.Join(configData.NLBEnabledClusters, ",")) > 0 { - newAdmiralConfig.NLBEnabledClusters = strings.Join(configData.NLBEnabledClusters, ",") + if len(configData.NLBEnabledClusters) > 0 { + newAdmiralConfig.NLBEnabledClusters = configData.NLBEnabledClusters } - if len(strings.Join(configData.CLBEnabledClusters, ",")) > 0 { - newAdmiralConfig.CLBEnabledClusters = strings.Join(configData.CLBEnabledClusters, ",") + if len(configData.CLBEnabledClusters) > 0 { + newAdmiralConfig.CLBEnabledClusters = configData.CLBEnabledClusters } - if len(strings.Join(configData.NLBEnabledIdentityList, ",")) > 0 { - newAdmiralConfig.NLBEnabledIdentityList = strings.Join(configData.NLBEnabledIdentityList, ",") + if len(configData.NLBEnabledIdentityList) > 0 { + newAdmiralConfig.NLBEnabledIdentityList = configData.NLBEnabledIdentityList } common.UpdateAdmiralParams(newAdmiralConfig) diff --git a/admiral/pkg/clusters/admiralDatabaseClient_test.go b/admiral/pkg/clusters/admiralDatabaseClient_test.go index 6229e378e..d70466eb2 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient_test.go +++ b/admiral/pkg/clusters/admiralDatabaseClient_test.go @@ -474,9 +474,9 @@ func TestUpdateSyncAdmiralConfig(t *testing.T) { configUpdated.CLBEnabledClusters = []string{"cluster1", "cluster2"} expectedAdmiralConfig := common.GetAdmiralParams() - expectedAdmiralConfig.NLBEnabledClusters = "cluster1,cluster2" - expectedAdmiralConfig.CLBEnabledClusters = "cluster1,cluster2" - expectedAdmiralConfig.NLBEnabledIdentityList = "identity1,identity2" + expectedAdmiralConfig.NLBEnabledClusters = []string{"cluster1", "cluster2"} + expectedAdmiralConfig.CLBEnabledClusters = []string{"cluster1", "cluster2"} + expectedAdmiralConfig.NLBEnabledIdentityList = []string{"identity1", "identity2"} emptyConfig := DynamicConfigData{} expectedEmptyConfig := common.GetAdmiralParams() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index b69aa8e79..a0d22612c 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -147,9 +147,9 @@ type AdmiralParams struct { EnableDynamicConfig bool DynamicConfigDynamoDBTableName string DynamicSyncPeriod int - NLBEnabledClusters string - NLBEnabledIdentityList string - CLBEnabledClusters string + NLBEnabledClusters []string + NLBEnabledIdentityList []string + CLBEnabledClusters []string } func (b AdmiralParams) String() string { From a32c8e7775b6860ed5f3a60147289b48e75b9704 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Mon, 27 Jan 2025 16:18:39 -0800 Subject: [PATCH 16/17] review part 4 Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index 51e8e20ad..dd168d177 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -169,20 +169,20 @@ func ReadAndUpdateSyncAdmiralConfig(dbClient AdmiralDatabaseManager) error { dbRawData, err := dbClient.Get("EnableDynamicConfig", common.Admiral) if err != nil { - log.Errorf("task=%s, Error getting EnableDynamicConfig admiral config, err: %v", common.DynamicConfigUpdate, err) + log.Errorf("task=%s, error getting EnableDynamicConfig admiral config, err: %v", common.DynamicConfigUpdate, err) return err } configData, ok := dbRawData.(DynamicConfigData) if !ok { - return errors.New(fmt.Sprintf("task=%s, Failed to parse DynamicConfigData", common.DynamicConfigUpdate)) + return errors.New(fmt.Sprintf("task=%s, failed to parse DynamicConfigData", common.DynamicConfigUpdate)) } if IsDynamicConfigChanged(configData) { - log.Infof(fmt.Sprintf("task=%s, Updating DynamicConfigData with Admiral config", common.DynamicConfigUpdate)) + log.Infof(fmt.Sprintf("task=%s, updating DynamicConfigData with Admiral config", common.DynamicConfigUpdate)) UpdateSyncAdmiralConfig(configData) } else { - log.Infof(fmt.Sprintf("task=%s, No need to update DynamicConfigData", common.DynamicConfigUpdate)) + log.Infof(fmt.Sprintf("task=%s, no need to update DynamicConfigData", common.DynamicConfigUpdate)) } return nil From b5dfcdf04d6f9e2218fe3eb2808f84c468aa5788 Mon Sep 17 00:00:00 2001 From: Viraj Kulkarni Date: Tue, 28 Jan 2025 00:01:50 -0800 Subject: [PATCH 17/17] clean up Signed-off-by: Viraj Kulkarni --- admiral/pkg/clusters/admiralDatabaseClient.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/admiral/pkg/clusters/admiralDatabaseClient.go b/admiral/pkg/clusters/admiralDatabaseClient.go index dd168d177..5af1c1296 100644 --- a/admiral/pkg/clusters/admiralDatabaseClient.go +++ b/admiral/pkg/clusters/admiralDatabaseClient.go @@ -194,8 +194,6 @@ func IsDynamicConfigChanged(config DynamicConfigData) bool { return false } - //md5.Sum() - if DynamicConfigCheckSum == md5.Sum([]byte(fmt.Sprintf("%v", config))) { return false } else {