fix: tls config validation, follow up on argoproj#1070 (argoproj#1076)

* fix: tls config validation, follow up on argoproj#1070

Signed-off-by: Derek Wang <whynowy@gmail.com>

* test cases

Signed-off-by: Derek Wang <whynowy@gmail.com>

pkg/apis/common/common.go

@@ -17,7 +17,6 @@ limitations under the License.
 package common
 
 import (
-	"errors"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -113,21 +112,6 @@ type TLSConfig struct {
 	DeprecatedClientKeyPath string `json:"clientKeyPath" protobuf:"bytes,6,opt,name=clientKeyPath"`
 }
 
-// ValidateTLSConfig validates a TLS configuration.
-func ValidateTLSConfig(tlsConfig *TLSConfig) error {
-	if tlsConfig == nil {
-		return nil
-	}
-	if tlsConfig.ClientKeySecret != nil && tlsConfig.ClientCertSecret != nil && tlsConfig.CACertSecret != nil {
-		return nil
-	}
-	// DEPRECATED.
-	if tlsConfig.DeprecatedClientCertPath != "" && tlsConfig.DeprecatedClientKeyPath != "" && tlsConfig.DeprecatedCACertPath != "" {
-		return nil
-	}
-	return errors.New("invalid tls config, please configure caCertSecret, clientCertSecret and clientKeySecret")
-}
-
 // Backoff for an operation
 type Backoff struct {
 	// Duration is the duration in nanoseconds

pkg/apis/common/validate.go

@@ -0,0 +1,32 @@
+package common
+
+import "errors"
+
+// ValidateTLSConfig validates a TLS configuration.
+func ValidateTLSConfig(tlsConfig *TLSConfig) error {
+	if tlsConfig == nil {
+		return nil
+	}
+	var caCertSet, clientCertSet, clientKeySet bool
+
+	if tlsConfig.CACertSecret != nil || tlsConfig.DeprecatedCACertPath != "" {
+		caCertSet = true
+	}
+
+	if tlsConfig.ClientCertSecret != nil || tlsConfig.DeprecatedClientCertPath != "" {
+		clientCertSet = true
+	}
+
+	if tlsConfig.ClientKeySecret != nil || tlsConfig.DeprecatedClientKeyPath != "" {
+		clientKeySet = true
+	}
+
+	if !caCertSet && !clientCertSet && !clientKeySet {
+		return errors.New("invalid tls config, please configure either caCertSecret, or clientCertSecret and clientKeySecret, or both")
+	}
+
+	if (clientCertSet || clientKeySet) && (!clientCertSet || !clientKeySet) {
+		return errors.New("invalid tls config, both clientCertSecret and clientKeySecret need to be configured")
+	}
+	return nil
+}

pkg/apis/common/validate_test.go

@@ -0,0 +1,72 @@
+package common
+
+import (
+	strings "strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func fakeTLSConfig(t *testing.T) *TLSConfig {
+	t.Helper()
+	return &TLSConfig{
+		CACertSecret: &corev1.SecretKeySelector{
+			Key: "fake-key1",
+			LocalObjectReference: corev1.LocalObjectReference{
+				Name: "fake-name1",
+			},
+		},
+		ClientCertSecret: &corev1.SecretKeySelector{
+			Key: "fake-key2",
+			LocalObjectReference: corev1.LocalObjectReference{
+				Name: "fake-name2",
+			},
+		},
+		ClientKeySecret: &corev1.SecretKeySelector{
+			Key: "fake-key3",
+			LocalObjectReference: corev1.LocalObjectReference{
+				Name: "fake-name3",
+			},
+		},
+	}
+}
+
+func TestValidateTLSConfig(t *testing.T) {
+	t.Run("test empty", func(t *testing.T) {
+		c := &TLSConfig{}
+		err := ValidateTLSConfig(c)
+		assert.NotNil(t, err)
+		assert.True(t, strings.Contains(err.Error(), "please configure either caCertSecret, or clientCertSecret and clientKeySecret, or both"))
+	})
+
+	t.Run("test clientKeySecret is set, clientCertSecret is empty", func(t *testing.T) {
+		c := fakeTLSConfig(t)
+		c.CACertSecret = nil
+		c.ClientCertSecret = nil
+		err := ValidateTLSConfig(c)
+		assert.NotNil(t, err)
+		assert.True(t, strings.Contains(err.Error(), "both clientCertSecret and clientKeySecret need to be configured"))
+	})
+
+	t.Run("test only caCertSecret is set", func(t *testing.T) {
+		c := fakeTLSConfig(t)
+		c.ClientCertSecret = nil
+		c.ClientKeySecret = nil
+		err := ValidateTLSConfig(c)
+		assert.Nil(t, err)
+	})
+
+	t.Run("test clientCertSecret and clientKeySecret are set", func(t *testing.T) {
+		c := fakeTLSConfig(t)
+		c.CACertSecret = nil
+		err := ValidateTLSConfig(c)
+		assert.Nil(t, err)
+	})
+
+	t.Run("test all of 3 are set", func(t *testing.T) {
+		c := fakeTLSConfig(t)
+		err := ValidateTLSConfig(c)
+		assert.Nil(t, err)
+	})
+}