From 871bd8c70324dc50df6b45e29db0b1c5c677e1f6 Mon Sep 17 00:00:00 2001 From: Ian Howell Date: Mon, 11 Nov 2019 14:46:43 -0600 Subject: [PATCH] Clean up the reconcileClusters function * This removes unneeded variables and complexity from the reconcileClusters function. * It also adds a unit test for said function, slightly increasing overall unit test coverage * Finally, this also addresses an issue in the rmConfigClusterStragglers function in which the clusters entry was not cleaned up in spite of being empty. Change-Id: I9e7535305840db5f2fb763452d5e4ef46be467c9 --- pkg/config/config.go | 88 ++++++++++++++++++++++++--------------- pkg/config/config_test.go | 54 ++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 34 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 845eb22fe..f8ffe8112 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -105,33 +105,55 @@ func (c *Config) reconcileConfig() error { return nil } -func (c *Config) reconcileClusters() (map[string]*ClusterComplexName, bool) { - updatedClusters := make(map[string]*kubeconfig.Cluster) - updatedClusterNames := make(map[string]*ClusterComplexName) - persistIt := false - for key, cluster := range c.kubeConfig.Clusters { +// reconcileClusters synchronizes the airshipconfig file with the kubeconfig file. +// +// It iterates over the clusters listed in the kubeconfig. If any cluster in +// the kubeconfig does not meet the _ convention, the name is +// first changed to the airship default. +// +// It then updates the airshipconfig's names of those clusters, as well as the +// pointer to the clusters. +// If the cluster wasn't referenced prior to the call, it is created; otherwise +// it is modified. +// +// Finally, any clusters listed in the airshipconfig that are no longer +// referenced in the kubeconfig are deleted +// +// The function returns a mapping of changed names in the kubeconfig, as well +// as a boolean denoting that the config files need to be written to file +func (c *Config) reconcileClusters() (map[string]string, bool) { + // updatedClusterNames is a mapping from OLD cluster names to NEW + // cluster names. This will be used later when we update contexts + updatedClusterNames := map[string]string{} + persistIt := false + for clusterName, cluster := range c.kubeConfig.Clusters { clusterComplexName := NewClusterComplexName() - clusterComplexName.FromName(key) - // Lets check if the cluster from the kubeconfig file complies with the complex naming convention + clusterComplexName.FromName(clusterName) + // Check if the cluster from the kubeconfig file complies with + // the airship naming convention if !clusterComplexName.validName() { clusterComplexName.SetDefaultType() - // Lets update the kubeconfig with proper airship name - updatedClusters[clusterComplexName.Name()] = cluster + // Update the kubeconfig with proper airship name + c.kubeConfig.Clusters[clusterComplexName.Name()] = cluster + delete(c.kubeConfig.Clusters, clusterName) - // Remember name changes since Contexts has to be updated as well for this clusters - updatedClusterNames[key] = clusterComplexName + // We also need to save the mapping from the old name + // so we can update the context in the kubeconfig later + updatedClusterNames[clusterName] = clusterComplexName.Name() + + // Since we've modified the kubeconfig object, we'll + // need to let the caller know that the kubeconfig file + // needs to be updated persistIt = true - if c.kubeConfig.Clusters[key] == nil { - c.kubeConfig.Clusters[key] = updatedClusters[key] - } - // Otherwise this is a cluster that didnt have an airship cluster type, however when you added the cluster type + // Otherwise this is a cluster that didnt have an + // airship cluster type, however when you added the + // cluster type // Probable should just add a number __ -// entries +// Removes Cluster configuration that exist in Airship Config and do not have +// any kubeconfig appropriate _ entries func (c *Config) rmConfigClusterStragglers(persistIt bool) bool { rccs := persistIt // Checking if there is any Cluster reference in airship config that does not match // an actual Cluster struct in kubeconfig - for key := range c.Clusters { - for cType, cluster := range c.Clusters[key].ClusterTypes { - if c.kubeConfig.Clusters[cluster.NameInKubeconf] == nil { + for clusterName := range c.Clusters { + for cType, cluster := range c.Clusters[clusterName].ClusterTypes { + if _, found := c.kubeConfig.Clusters[cluster.NameInKubeconf]; !found { // Instead of removing it , I could add a empty entry in kubeconfig as well // Will see what is more appropriae with use of Modules configuration - delete(c.Clusters[key].ClusterTypes, cType) + delete(c.Clusters[clusterName].ClusterTypes, cType) + + // If that was the last cluster type, then we + // should delete the cluster entry + if len(c.Clusters[clusterName].ClusterTypes) == 0 { + delete(c.Clusters, clusterName) + } rccs = true } } } return rccs } -func (c *Config) reconcileContexts(updatedClusterNames map[string]*ClusterComplexName) { +func (c *Config) reconcileContexts(updatedClusterNames map[string]string) { for key, context := range c.kubeConfig.Contexts { // Check if the Cluster name referred to by the context // was updated during the cluster reconcile - if updatedClusterNames[context.Cluster] != nil { - context.Cluster = updatedClusterNames[context.Cluster].Name() + if newName, ok := updatedClusterNames[context.Cluster]; ok { + context.Cluster = newName } if c.Contexts[key] == nil { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 46ac52e39..c196e62f6 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -24,7 +24,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/client-go/tools/clientcmd" + kubeconfig "k8s.io/client-go/tools/clientcmd/api" "opendev.org/airship/airshipctl/testutil" ) @@ -345,3 +347,55 @@ func TestGetClusters(t *testing.T) { require.NoError(t, err) assert.Len(t, clusters, 4) } + +func TestReconcileClusters(t *testing.T) { + testCluster := &kubeconfig.Cluster{ + Server: "testServer", + InsecureSkipTLSVerify: true, + } + + testKubeConfig := &kubeconfig.Config{ + Clusters: map[string]*kubeconfig.Cluster{ + "invalidName": nil, + "missingFromAirshipConfig_ephemeral": testCluster, + }, + } + + testConfig := &Config{ + Clusters: map[string]*ClusterPurpose{ + "straggler": { + map[string]*Cluster{ + "ephemeral": { + NameInKubeconf: "notThere!", + kCluster: nil, + }, + }, + }, + }, + kubeConfig: testKubeConfig, + } + updatedClusterNames, persistIt := testConfig.reconcileClusters() + + // Check that there are clusters that need to be updated in contexts + expectedUpdatedClusterNames := map[string]string{"invalidName": "invalidName_target"} + assert.Equal(t, expectedUpdatedClusterNames, updatedClusterNames) + + // Check that we need to update the config file + assert.True(t, persistIt) + + // Check that the invalid name was changed to a valid one + assert.NotContains(t, testKubeConfig.Clusters, "invalidName") + assert.Contains(t, testKubeConfig.Clusters, "invalidName_target") + + // Check that the missing cluster was added to the airshipconfig + missingCluster := testConfig.Clusters["missingFromAirshipConfig"] + require.NotNil(t, missingCluster) + require.NotNil(t, missingCluster.ClusterTypes) + require.NotNil(t, missingCluster.ClusterTypes[Ephemeral]) + assert.Equal(t, "missingFromAirshipConfig_ephemeral", + missingCluster.ClusterTypes[Ephemeral].NameInKubeconf) + assert.Equal(t, testCluster, missingCluster.ClusterTypes[Ephemeral].kCluster) + + // Check that the "stragglers" were removed from the airshipconfig + assert.NotContains(t, testConfig.Clusters, "straggler") +}