From 27b7309670083e6c25b338e745c63c0bceceddc5 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Fri, 4 Jun 2021 14:17:08 +0200 Subject: [PATCH 1/2] Add optional --cloud-config-file argument to copy cloud-config in bootstrap --- cmd/render/main.go | 18 +++++++++++++++--- pkg/render/render.go | 6 +++++- pkg/render/render_test.go | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/cmd/render/main.go b/cmd/render/main.go index ca7df71d3..d63c76f43 100644 --- a/cmd/render/main.go +++ b/cmd/render/main.go @@ -24,6 +24,7 @@ var ( destinationDir string clusterInfrastructure string imagesFile string + cloudConfigFile string } ) @@ -32,6 +33,7 @@ func init() { renderCmd.PersistentFlags().StringVar(&renderOpts.destinationDir, "dest-dir", "", "The destination dir where CCCMO writes the generated static pods for CCM.") renderCmd.PersistentFlags().StringVar(&renderOpts.clusterInfrastructure, "cluster-infrastructure-file", "", "Input path for the cluster infrastructure file.") renderCmd.PersistentFlags().StringVar(&renderOpts.imagesFile, "images-file", "", "Input path for the images config map file.") + renderCmd.PersistentFlags().StringVar(&renderOpts.cloudConfigFile, "cloud-config-file", "", "Input path for the cloud config config map file generated by cluster-config-operator.") renderCmd.MarkFlagRequired("dest-dir") renderCmd.MarkFlagRequired("cluster-infrastructure-file") renderCmd.MarkFlagRequired("images-file") @@ -44,11 +46,15 @@ func runRenderCmd(cmd *cobra.Command, args []string) error { if err := validate( renderOpts.destinationDir, renderOpts.clusterInfrastructure, - renderOpts.imagesFile); err != nil { + renderOpts.imagesFile, + renderOpts.cloudConfigFile); err != nil { return err } - if err := render.New(renderOpts.clusterInfrastructure, renderOpts.imagesFile).Run(renderOpts.destinationDir); err != nil { + if err := render.New( + renderOpts.clusterInfrastructure, + renderOpts.imagesFile, + renderOpts.cloudConfigFile).Run(renderOpts.destinationDir); err != nil { return err } @@ -56,7 +62,7 @@ func runRenderCmd(cmd *cobra.Command, args []string) error { } // validate verifies all file and dirs exist -func validate(destinationDir, clusterInfrastructure, imagesFile string) error { +func validate(destinationDir, clusterInfrastructure, imagesFile, cloudConfigFile string) error { errs := []error{} if err := isFile(clusterInfrastructure); err != nil { klog.Errorf("error reading --cluster-infrastructure-file=%q: %s", clusterInfrastructure, err) @@ -66,6 +72,12 @@ func validate(destinationDir, clusterInfrastructure, imagesFile string) error { klog.Errorf("error reading --images-file=%q: %s", imagesFile, err) errs = append(errs, fmt.Errorf("error reading --images-file: %s", err)) } + if cloudConfigFile != "" { + if err := isFile(cloudConfigFile); err != nil { + klog.Errorf("error reading --cloud-config-file=%q: %s", imagesFile, err) + errs = append(errs, fmt.Errorf("error reading --cloud-config-file: %s", err)) + } + } if len(errs) > 0 { return utilerrors.NewAggregate(errs) diff --git a/pkg/render/render.go b/pkg/render/render.go index 6d5634684..5182abc50 100644 --- a/pkg/render/render.go +++ b/pkg/render/render.go @@ -32,13 +32,17 @@ type Render struct { infrastructureFile string // path to rendered cloud-controller-manager-images ConfigMap manifest for image references to use imagesFile string + // path to populated cloud-config ConfigMap manifest from cluster-config-operator + // where cloud-config could be extracted for use in CCM static pod + cloudConfigFile string } // New returns controller for render -func New(infrastructureFile, imagesFile string) *Render { +func New(infrastructureFile, imagesFile, cloudConfigFile string) *Render { return &Render{ infrastructureFile: infrastructureFile, imagesFile: imagesFile, + cloudConfigFile: cloudConfigFile, } } diff --git a/pkg/render/render_test.go b/pkg/render/render_test.go index 37d7749ec..73953df22 100644 --- a/pkg/render/render_test.go +++ b/pkg/render/render_test.go @@ -216,7 +216,7 @@ func TestRenderRun(t *testing.T) { infrastructureFile = path } - r := New(infrastructureFile, imagesFile) + r := New(infrastructureFile, imagesFile, "") err = r.Run(destination) if tc.expectError != "" { assert.EqualError(t, err, tc.expectError) From 8ad771f9ee35aca1820108172c4220234b5d2c37 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Fri, 4 Jun 2021 16:37:07 +0200 Subject: [PATCH 2/2] Implement cloud config parser and writer methods --- cmd/render/main.go | 2 +- pkg/render/render.go | 76 ++++++++++++++-- pkg/render/render_test.go | 182 +++++++++++++++++++++++++++++++++++--- 3 files changed, 240 insertions(+), 20 deletions(-) diff --git a/cmd/render/main.go b/cmd/render/main.go index d63c76f43..08e3785a2 100644 --- a/cmd/render/main.go +++ b/cmd/render/main.go @@ -33,7 +33,7 @@ func init() { renderCmd.PersistentFlags().StringVar(&renderOpts.destinationDir, "dest-dir", "", "The destination dir where CCCMO writes the generated static pods for CCM.") renderCmd.PersistentFlags().StringVar(&renderOpts.clusterInfrastructure, "cluster-infrastructure-file", "", "Input path for the cluster infrastructure file.") renderCmd.PersistentFlags().StringVar(&renderOpts.imagesFile, "images-file", "", "Input path for the images config map file.") - renderCmd.PersistentFlags().StringVar(&renderOpts.cloudConfigFile, "cloud-config-file", "", "Input path for the cloud config config map file generated by cluster-config-operator.") + renderCmd.PersistentFlags().StringVar(&renderOpts.cloudConfigFile, "cloud-config-file", "", "Input path for the cloud config configMap manifest generated by cluster-config-operator.") renderCmd.MarkFlagRequired("dest-dir") renderCmd.MarkFlagRequired("cluster-infrastructure-file") renderCmd.MarkFlagRequired("images-file") diff --git a/pkg/render/render.go b/pkg/render/render.go index 5182abc50..29642354b 100644 --- a/pkg/render/render.go +++ b/pkg/render/render.go @@ -20,8 +20,10 @@ import ( ) const ( + configDataKey = "cloud.conf" bootstrapNamespace = "kube-system" bootstrapPrefix = "bootstrap" + configPrefix = "config" // bootstrapFileName is built from bootstrapPrefix, resource name and kind bootstrapFileName = "%s/%s-%s.yaml" ) @@ -49,7 +51,7 @@ func New(infrastructureFile, imagesFile, cloudConfigFile string) *Render { // Run runs boostrap for Machine Config Controller // It writes all the assets to destDir func (r *Render) Run(destinationDir string) error { - infra, imagesMap, err := r.readAssets() + infra, imagesMap, cloudConfig, err := r.readAssets() if err != nil { klog.Errorf("Cannot read assets from provided paths: %v", err) return err @@ -67,36 +69,51 @@ func (r *Render) Run(destinationDir string) error { klog.Infof("Collected resource %s %q successfully", resource.GetObjectKind().GroupVersionKind(), client.ObjectKeyFromObject(resource)) } - return writeAssets(destinationDir, resources) + if err := writeAssets(destinationDir, resources); err != nil { + klog.Errorf("Could not write assets to bootstrap dir: %v", err) + return err + } + + return writeCloudConfig(destinationDir, cloudConfig) } // readAssets collects infrastructure resource and images config map from provided paths -func (r *Render) readAssets() (*configv1.Infrastructure, *corev1.ConfigMap, error) { +func (r *Render) readAssets() (*configv1.Infrastructure, *corev1.ConfigMap, string, error) { infraData, err := ioutil.ReadFile(r.infrastructureFile) if err != nil { klog.Errorf("Unable to read data from %q: %v", r.infrastructureFile, err) - return nil, nil, err + return nil, nil, "", err } infra := &configv1.Infrastructure{} if err := yaml.UnmarshalStrict(infraData, infra); err != nil { klog.Errorf("Cannot decode data into configv1.Infrastructure from %q: %v", r.infrastructureFile, err) - return nil, nil, err + return nil, nil, "", err } imagesData, err := ioutil.ReadFile(r.imagesFile) if err != nil { klog.Errorf("Unable to read data from %q: %v", r.imagesFile, err) - return nil, nil, err + return nil, nil, "", err } imagesConfigMap := &corev1.ConfigMap{} if err := yaml.UnmarshalStrict(imagesData, imagesConfigMap); err != nil { klog.Errorf("Cannot decode data into v1.ConfigMap from %q: %v", r.imagesFile, err) - return nil, nil, err + return nil, nil, "", err + } + + cloudConfig := "" + // if the cloudConfig is set in infra read the cloudConfigFile + if infra.Spec.CloudConfig.Name != "" { + cloudConfig, err = loadBootstrapCloudProviderConfig(infra, r.cloudConfigFile) + if err != nil { + klog.Errorf("failed to load the cloud provider config: %v", err) + return nil, nil, "", err + } } - return infra, imagesConfigMap, nil + return infra, imagesConfigMap, cloudConfig, nil } // writeAssets writes static pods to disk into //-.yaml @@ -122,5 +139,48 @@ func writeAssets(destinationDir string, resources []client.Object) error { return err } } + + return nil +} + +// writeCloudConfig creates config folder and writes resources such as cloud-config file +// for use in bootstrap +func writeCloudConfig(destinationDir string, cloudConfig string) error { + // Create config directory in advance to ensure it is present for any provider + configDir := filepath.Join(destinationDir, configPrefix) + if err := os.MkdirAll(configDir, fs.ModePerm); err != nil { + klog.Errorf("Unable to create destination dir %q: %v", configDir, err) + return err + } + + if cloudConfig != "" { + cloudConfigFile := filepath.Join(configDir, configDataKey) + + klog.Infof("Writing cloud config on disk in %q", cloudConfigFile) + err := os.WriteFile(cloudConfigFile, []byte(cloudConfig), 0666) + if err != nil { + klog.Errorf("Failed to write cloud config to disk in %q: %v", cloudConfigFile, err) + return err + } + } + return nil } + +// loadBootstrapCloudProviderConfig reads the cloud provider config from cloudConfigFile based on infra object. +func loadBootstrapCloudProviderConfig(infra *configv1.Infrastructure, cloudConfigFile string) (string, error) { + data, err := os.ReadFile(cloudConfigFile) + if err != nil { + return "", err + } + cloudConfigMap := &corev1.ConfigMap{} + if err := yaml.UnmarshalStrict(data, cloudConfigMap); err != nil { + return "", err + } + cloudConf, ok := cloudConfigMap.Data[configDataKey] + if !ok { + klog.Infof("falling back to reading cloud provider config from user specified key %s", infra.Spec.CloudConfig.Key) + cloudConf = cloudConfigMap.Data[infra.Spec.CloudConfig.Key] + } + return cloudConf, nil +} diff --git a/pkg/render/render_test.go b/pkg/render/render_test.go index 73953df22..d8389d512 100644 --- a/pkg/render/render_test.go +++ b/pkg/render/render_test.go @@ -11,6 +11,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/aws" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/azure" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,12 +29,28 @@ status: platform: AWS platformStatus: type: AWS +` + infraWithCloudConfig = `apiVersion: config.openshift.io/v1 +kind: Infrastructure +metadata: + name: cluster +spec: + cloudConfig: + name: test + key: test +status: + platform: Azure + platformStatus: + type: Azure ` infraMissingPlatform = `apiVersion: config.openshift.io/v1 kind: Infrastructure metadata: name: cluster -spec: {} +spec: + cloudConfig: + name: test + key: cloud.conf status: {} ` imagesConfigMap = `apiVersion: v1 @@ -47,6 +64,14 @@ data: "cloudControllerManagerAWS": "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", "cloudControllerManagerOpenStack": "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager" } +` + cloudConfigMap = `apiVersion: v1 +data: + test: "{test:test}" +kind: ConfigMap +metadata: + name: kube-cloud-config + namespace: kube-system ` ) @@ -92,6 +117,8 @@ func TestReadAssets(t *testing.T) { infra *configv1.Infrastructure imagesContent string imagesConfigMap *corev1.ConfigMap + cloudConfig string + cloudConfigMap *corev1.ConfigMap expectError string }{{ name: "Unmarshal both infrastructure and images with no issue", @@ -119,12 +146,14 @@ func TestReadAssets(t *testing.T) { infraPath := "infra.yaml" configPath := "imagesConfigMap.yaml" + cloudConfigPath := "cloudConfigMap.yaml" for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { r := Render{ imagesFile: "not_found", infrastructureFile: "not_found", + cloudConfigFile: "not_found", } if tc.imagesContent != "" { file, err := ioutil.TempFile(os.TempDir(), configPath) @@ -148,7 +177,18 @@ func TestReadAssets(t *testing.T) { r.infrastructureFile = path } - infra, config, err := r.readAssets() + if tc.cloudConfig != "" { + file, err := ioutil.TempFile(os.TempDir(), cloudConfigPath) + path := file.Name() + assert.NoError(t, err) + defer file.Close() + + _, err = file.WriteString(tc.cloudConfig) + assert.NoError(t, err) + r.cloudConfigFile = path + } + + infra, config, cloudConfig, err := r.readAssets() if tc.expectError != "" { assert.EqualError(t, err, tc.expectError) } else { @@ -156,44 +196,80 @@ func TestReadAssets(t *testing.T) { } assert.EqualValues(t, config, tc.imagesConfigMap) assert.EqualValues(t, infra, tc.infra) + assert.EqualValues(t, cloudConfig, tc.cloudConfig) }) } } func TestRenderRun(t *testing.T) { tc := []struct { - name string - infraContent string - imagesContent string - expectObjects []client.Object - expectError string + name string + infraContent string + imagesContent string + cloudConfigContent string + expectObjects []client.Object + expectCloudConfig bool + badDestination bool + expectError string }{{ name: "Unmarshal both infrastructure and images with no issue", infraContent: infra, imagesContent: imagesConfigMap, expectObjects: aws.GetBootstrapResources(), }, { - name: "Infrastructure not populated", - infraContent: infraMissingPlatform, + name: "Unmarshal both infrastructure and images with no issue", + infraContent: infraWithCloudConfig, + imagesContent: imagesConfigMap, + cloudConfigContent: cloudConfigMap, + expectObjects: azure.GetBootstrapResources(), + expectCloudConfig: true, + }, { + name: "Infrastructure not populated", + infraContent: infraMissingPlatform, + imagesContent: imagesConfigMap, + cloudConfigContent: cloudConfigMap, + expectError: "platform status is not populated on infrastructure", + }, { + name: "Cloud config missing", + infraContent: infraWithCloudConfig, imagesContent: imagesConfigMap, - expectError: "platform status is not populated on infrastructure", + expectError: "open not_found: no such file or directory", }, { name: "ImagesConfigMap not located", infraContent: infra, imagesContent: "BAD", expectError: "error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1.ConfigMap", + }, { + name: "CloudConfigMap is not populated correctly", + infraContent: infraWithCloudConfig, + imagesContent: imagesConfigMap, + cloudConfigContent: "BAD", + expectError: "error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1.ConfigMap", + }, { + name: "Destination directory is broken", + infraContent: infra, + imagesContent: imagesConfigMap, + expectObjects: aws.GetBootstrapResources(), + badDestination: true, + expectError: "mkdir /dev/null: not a directory", }} infraPath := "infra.yaml" configPath := "imagesConfigMap.yaml" + cloudConfigPath := "cloudConfigMap.yaml" for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { imagesFile := "not_found" infrastructureFile := "not_found" + cloudConfigFile := "not_found" destination, err := ioutil.TempDir("", "test") assert.NoError(t, err) + if tc.badDestination { + destination = "/dev/null" + } + if tc.imagesContent != "" { file, err := ioutil.TempFile(os.TempDir(), configPath) path := file.Name() @@ -205,6 +281,17 @@ func TestRenderRun(t *testing.T) { imagesFile = path } + if tc.cloudConfigContent != "" { + file, err := ioutil.TempFile(os.TempDir(), cloudConfigPath) + path := file.Name() + assert.NoError(t, err) + defer file.Close() + + _, err = file.WriteString(tc.cloudConfigContent) + assert.NoError(t, err) + cloudConfigFile = path + } + if tc.infraContent != "" { file, err := ioutil.TempFile(os.TempDir(), infraPath) path := file.Name() @@ -216,7 +303,7 @@ func TestRenderRun(t *testing.T) { infrastructureFile = path } - r := New(infrastructureFile, imagesFile, "") + r := New(infrastructureFile, imagesFile, cloudConfigFile) err = r.Run(destination) if tc.expectError != "" { assert.EqualError(t, err, tc.expectError) @@ -228,6 +315,15 @@ func TestRenderRun(t *testing.T) { files, err := ioutil.ReadDir(path.Join(destination, bootstrapPrefix)) assert.NoError(t, err) assert.Len(t, files, len(tc.expectObjects)) + + // Assert config dir is present + _, err = ioutil.ReadDir(path.Join(destination, configPrefix)) + assert.NoError(t, err) + + if tc.expectCloudConfig { + _, err := os.ReadFile(path.Join(destination, configPrefix, configDataKey)) + assert.NoError(t, err) + } }) } } @@ -305,3 +401,67 @@ func TestWriteAssets(t *testing.T) { }) } } + +func TestWriteCloudConfig(t *testing.T) { + tc := []struct { + name string + destination string + preCreateMode fs.FileMode + cloudConfig string + expectErr string + }{{ + name: "Writing file finished with success", + destination: "test", + cloudConfig: "some_data", + }, { + name: "No operation for emplty cloud config", + destination: "test", + }, { + name: "Fail to write into /dev/null", + cloudConfig: "some_data", + expectErr: "mkdir /dev/null: not a directory", + }, { + name: "Fail to write into folder with bad permissions", + cloudConfig: "some_data", + destination: "bad_permissions", + preCreateMode: 0444, + expectErr: "permission denied", + }} + + for _, tc := range tc { + t.Run(tc.name, func(t *testing.T) { + destination := "/dev/null" + + if tc.destination != "" { + dirPath, err := ioutil.TempDir("", tc.destination) + assert.NoError(t, err) + destination = dirPath + if tc.preCreateMode != 0 { + os.MkdirAll(path.Join(destination, configPrefix), tc.preCreateMode) + assert.NoError(t, err) + } + } + + err := writeCloudConfig(destination, tc.cloudConfig) + if tc.expectErr != "" { + assert.Contains(t, err.Error(), tc.expectErr) + return + } + assert.NoError(t, err) + + // Assert all files were written to bootstrap dir + files, err := ioutil.ReadDir(path.Join(destination, configPrefix)) + assert.NoError(t, err) + + data, err := os.ReadFile(path.Join(destination, configPrefix, configDataKey)) + if tc.cloudConfig != "" { + assert.NoError(t, err) + assert.Equal(t, string(data), tc.cloudConfig) + assert.Len(t, files, 1) + } else { + assert.Len(t, files, 0) + assert.Error(t, err) + } + }) + } +}