Removed error logging from config loader

pkg/common/config/config.go

@@ -17,6 +17,7 @@ limitations under the License.
 package config
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"strconv"
@@ -255,23 +256,41 @@ func (cfg *Config) FromEnv() error {
 // ReadConfig parses vSphere cloud config file and stores it into VSphereConfig.
 // Environment variables are also checked
 func ReadConfig(byConfig []byte) (*Config, error) {
+	var cfg *Config
+
 	if len(byConfig) == 0 {
 		return nil, fmt.Errorf("Invalid YAML/INI file")
 	}
 
-	cfg, err := ReadConfigYAML(byConfig)
-	if err != nil {
-		klog.Warningf("ReadConfigYAML failed: %s", err)
+	// Check to see if yaml file before calling parser to prevent unnecessary error messages.
+	yamlErr := isConfigYaml(byConfig)
+	if yamlErr == nil {
+		cfg, yamlErr = ReadConfigYAML(byConfig)
 
-		cfg, err = ReadConfigINI(byConfig)
-		if err != nil {
-			klog.Errorf("ReadConfigINI failed: %s", err)
-			return nil, err
+		// ReadConfigYAML can fail for other config errors not related to YAML.  So it is ok for yamlErr == nil initially
+		// and a new error occurs after read.
+		if yamlErr != nil {
+			klog.Errorf("ReadConfigYAML failed: %s", yamlErr)
+			return nil, yamlErr
 		}
+		klog.V(4).Info("ReadConfig YAML succeeded")
+	} else {
+		klog.V(4).Infof("YAML not detected.  Attempting INI config load.")
+		var iniErr error
+		cfg, iniErr = ReadConfigINI(byConfig)
+		if iniErr != nil {
+			var cfgErr Err
+			if errors.As(iniErr, &cfgErr) {
+				// This error means it was valid INI but something was wrong w/ the config
+				return nil, iniErr
+			}
 
+			// If we are here, YAML and INI failed.  Create an error with both failure messages so user can review to determine cause
+			klog.Errorf("ReadConfigYAML failed: %v", yamlErr)
+			klog.Errorf("ReadConfigINI failed: %v", iniErr)
+			return nil, fmt.Errorf("ReadConfig failed.  YAML=[%v], INI=[%v]", yamlErr, iniErr)
+		}
 		klog.Info("ReadConfig INI succeeded. INI-based cloud-config is deprecated and will be removed in 2.0. Please use YAML based cloud-config.")
-	} else {
-		klog.Info("ReadConfig YAML succeeded")
 	}
 
 	// Env Vars should override config file entries if present

pkg/common/config/config_ini_legacy_test.go

@@ -63,6 +63,27 @@ secret-namespace = "kube-system"
 # port, insecure-flag will be used from Global section.
 `
 
+const missingServerConfigINI = `
+[Global]
+port = 443
+user = user
+password = password
+insecure-flag = true
+datacenters = us-west
+ca-file = /some/path/to/a/ca.pem
+`
+
+const badConfigINI = `
+[Global]]
+server = 0.0.0.0
+port = 443
+user = user
+password = password
+insecure-flag = true
+datacenters = us-west
+ca-file = /some/path/to/a/ca.pem
+`
+
 func TestReadConfigINIGlobal(t *testing.T) {
 	_, err := ReadConfigINI([]byte(""))
 	if err == nil {

pkg/common/config/config_test.go

@@ -0,0 +1,92 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package config
+
+import (
+	"errors"
+	"testing"
+)
+
+const invalidFormat = `
+This is just a string that is neither a yaml or ini file
+`
+
+func TestReadConfig(t *testing.T) {
+	mockConfigTestCases := []struct {
+		name          string
+		configData    string
+		expectedError error
+	}{
+		{
+			name:       "Valid YAML",
+			configData: basicConfigYAML,
+		},
+		{
+			name:          "Invalid YAML",
+			configData:    badConfigYAML,
+			expectedError: errors.New("ReadConfig failed.  YAML=[yaml: line 5: mapping values are not allowed in this context], INI=[2:1: expected section header]"),
+		},
+		{
+			name:       "Valid INI",
+			configData: basicConfigINI,
+		},
+		{
+			name:          "Invalid INI",
+			configData:    badConfigINI,
+			expectedError: errors.New("ReadConfig failed.  YAML=[yaml: unmarshal errors:\n  line 2: cannot unmarshal !!seq into config.CommonConfigYAML], INI=[2:9: expected EOL, EOF, or comment]"),
+		},
+		{
+			name:          "Invalid format",
+			configData:    invalidFormat,
+			expectedError: errors.New("ReadConfig failed.  YAML=[yaml: unmarshal errors:\n  line 2: cannot unmarshal !!str `This is...` into config.CommonConfigYAML], INI=[2:1: expected section header]"),
+		},
+		{
+			name:          "Missing vCenter IP YAML",
+			configData:    missingServerConfigYAML,
+			expectedError: errors.New("No Virtual Center hosts defined"),
+		},
+		{
+			name:          "Missing vCenter IP INI",
+			configData:    missingServerConfigINI,
+			expectedError: errors.New("No Virtual Center hosts defined"),
+		},
+	}
+
+	for _, tc := range mockConfigTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			cfg, err := ReadConfig([]byte(tc.configData))
+
+			if tc.expectedError != nil {
+				if err == nil {
+					t.Fatal("ReadConfig was expected to return error")
+				}
+				if err.Error() != tc.expectedError.Error() {
+					t.Fatalf("Expected: %v, got %v", tc.expectedError, err)
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("ReadConfig was not expected to return error: %v", err)
+				}
+
+				if cfg == nil {
+					t.Fatal("ReadConfig did not return a config object")
+				}
+			}
+		})
+	}
+}

pkg/common/config/config_yaml.go

@@ -227,3 +227,14 @@ func ReadConfigYAML(byConfig []byte) (*Config, error) {
 
 	return cfg.CreateConfig(), nil
 }
+
+func isConfigYaml(byConfig []byte) error {
+	cfg := CommonConfigYAML{
+		Vcenter: make(map[string]*VirtualCenterConfigYAML),
+	}
+
+	if err := yaml.Unmarshal(byConfig, &cfg); err != nil {
+		return err
+	}
+	return nil
+}

pkg/common/config/config_yaml_test.go

@@ -65,6 +65,28 @@ vcenter:
     secretNamespace: kube-system
 `
 
+const missingServerConfigYAML = `
+global:
+  port: 443
+  user: user
+  password: password
+  insecureFlag: true
+  datacenters:
+    - us-west
+  caFile: /some/path/to/a/ca.pem
+`
+
+const badConfigYAML = `
+global:
+  server: 0.0.0.0
+  port: 443
+  user: password: 
+  insecureFlag: true
+  datacenters:
+    - us-west
+  caFile: /some/path/to/a/ca.pem
+`
+
 func TestReadConfigYAMLGlobal(t *testing.T) {
 	_, err := ReadConfigYAML([]byte(""))
 	if err == nil {
@@ -137,3 +159,25 @@ func TestTenantRefsYAML(t *testing.T) {
 		t.Errorf("vcConfig3 SecretRef should be kube-system/eu-secret but actual=%s", vcConfig3.SecretRef)
 	}
 }
+
+func TestIsConfigYAML(t *testing.T) {
+	if err := isConfigYaml([]byte(basicConfigYAML)); err != nil {
+		t.Error("YAML config should be valid")
+	}
+
+	if err := isConfigYaml([]byte(basicConfigINI)); err == nil {
+		t.Error("INI config should be invalid")
+	}
+
+	if err := isConfigYaml([]byte(badConfigYAML)); err == nil {
+		t.Error("Bad config YAML config should be invalid")
+	}
+
+	if err := isConfigYaml([]byte(badConfigINI)); err == nil {
+		t.Error("Bad config YAML config should be invalid")
+	}
+
+	if err := isConfigYaml([]byte(invalidFormat)); err == nil {
+		t.Error("Generic text file should be invalid")
+	}
+}

pkg/common/config/consts_and_errors.go

@@ -16,10 +16,6 @@ limitations under the License.
 
 package config
 
-import (
-	"errors"
-)
-
 const (
 	// DefaultRoundTripperCount is the number of allowed round trips
 	// before an error is returned.
@@ -51,19 +47,34 @@ const (
 
 var (
 	// ErrUsernameMissing is returned when the provided username is empty.
-	ErrUsernameMissing = errors.New("Username is missing")
+	ErrUsernameMissing = getError("Username is missing")
 
 	// ErrPasswordMissing is returned when the provided password is empty.
-	ErrPasswordMissing = errors.New("Password is missing")
+	ErrPasswordMissing = getError("Password is missing")
 
 	// ErrInvalidVCenterIP is returned when the provided vCenter IP address is
 	// missing from the provided configuration.
-	ErrInvalidVCenterIP = errors.New("vsphere.conf does not have the VirtualCenter IP address specified")
+	ErrInvalidVCenterIP = getError("vsphere.conf does not have the VirtualCenter IP address specified")
 
 	// ErrMissingVCenter is returned when the provided configuration does not
 	// define any vCenters.
-	ErrMissingVCenter = errors.New("No Virtual Center hosts defined")
+	ErrMissingVCenter = getError("No Virtual Center hosts defined")
 
 	// ErrInvalidIPFamilyType is returned when an invalid IPFamily type is encountered
-	ErrInvalidIPFamilyType = errors.New("Invalid IP Family type")
+	ErrInvalidIPFamilyType = getError("Invalid IP Family type")
 )
+
+// Err error to be used for any config related errors
+type Err struct {
+	Msg string
+}
+
+func (p Err) Error() string {
+	return p.Msg
+}
+
+func getError(msg string) error {
+	return Err{
+		Msg: msg,
+	}
+}