Merge pull request cert-manager#7014 from inteon/improve_config_valid…

…ation

Improve config validation

cmd/cainjector/app/cainjector.go

@@ -23,7 +23,6 @@ import (
 	"path/filepath"
 
 	"github.com/spf13/cobra"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	"github.com/cert-manager/cert-manager/cainjector-binary/app/options"
 	config "github.com/cert-manager/cert-manager/internal/apis/config/cainjector"
@@ -90,11 +89,15 @@ servers and webhook servers.`,
 				return err
 			}
 
-			if err := validation.ValidateCAInjectorConfiguration(cainjectorConfig); err != nil {
-				return fmt.Errorf("error validating flags: %w", err)
+			if err := validation.ValidateCAInjectorConfiguration(cainjectorConfig, nil); len(err) > 0 {
+				return fmt.Errorf("error validating flags: %w", err.ToAggregate())
 			}
 
-			if err := logf.ValidateAndApplyAsField(&cainjectorConfig.Logging, field.NewPath("logging")); err != nil {
+			// ValidateCAInjectorConfiguration should already have validated the
+			// logging flags, the logging API does not have a Apply-only function
+			// so we validate again here. This should not catch any validation errors
+			// anymore.
+			if err := logf.ValidateAndApply(&cainjectorConfig.Logging); err != nil {
 				return fmt.Errorf("failed to validate cainjector logging flags: %w", err)
 			}
 

cmd/controller/app/options/options_test.go

@@ -17,14 +17,12 @@ limitations under the License.
 package options
 
 import (
-	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	config "github.com/cert-manager/cert-manager/internal/apis/config/controller"
 	defaults "github.com/cert-manager/cert-manager/internal/apis/config/controller/v1alpha1"
-	"github.com/cert-manager/cert-manager/internal/apis/config/controller/validation"
 )
 
 func TestEnabledControllers(t *testing.T) {
@@ -38,19 +36,19 @@ func TestEnabledControllers(t *testing.T) {
 		},
 		"if some controllers enabled, return list": {
 			controllers: []string{"foo", "bar"},
-			expEnabled:  sets.New[string]("foo", "bar"),
+			expEnabled:  sets.New("foo", "bar"),
 		},
 		"if some controllers enabled, one then disabled, return list without disabled": {
 			controllers: []string{"foo", "bar", "-foo"},
-			expEnabled:  sets.New[string]("bar"),
+			expEnabled:  sets.New("bar"),
 		},
 		"if all default controllers enabled, return all default controllers": {
 			controllers: []string{"*"},
-			expEnabled:  sets.New[string](defaults.DefaultEnabledControllers...),
+			expEnabled:  sets.New(defaults.DefaultEnabledControllers...),
 		},
 		"if all controllers enabled, some diabled, return all controllers with disabled": {
 			controllers: []string{"*", "-clusterissuers", "-issuers"},
-			expEnabled:  sets.New[string](defaults.DefaultEnabledControllers...).Delete("clusterissuers", "issuers"),
+			expEnabled:  sets.New(defaults.DefaultEnabledControllers...).Delete("clusterissuers", "issuers"),
 		},
 	}
 
@@ -68,43 +66,3 @@ func TestEnabledControllers(t *testing.T) {
 		})
 	}
 }
-
-func TestValidate(t *testing.T) {
-	tests := map[string]struct {
-		DNS01RecursiveServers []string
-		expError              string
-	}{
-		"if valid dns servers with ip address and port, return no errors": {
-			DNS01RecursiveServers: []string{"192.168.0.1:53", "10.0.0.1:5353"},
-			expError:              "",
-		},
-		"if valid DNS servers with DoH server addresses including https prefix, return no errors": {
-			DNS01RecursiveServers: []string{"https://dns.example.com", "https://doh.server"},
-			expError:              "",
-		},
-		"if invalid DNS server format due to missing https prefix, return 'invalid DNS server' error": {
-			DNS01RecursiveServers: []string{"dns.example.com"},
-			expError:              "invalid DNS server",
-		},
-		"if invalid DNS server format due to invalid IP address length and no port, return 'invalid DNS server' error": {
-			DNS01RecursiveServers: []string{"192.168.0.1.53"},
-			expError:              "invalid DNS server",
-		},
-	}
-
-	for name, test := range tests {
-		t.Run(name, func(t *testing.T) {
-			o, _ := NewControllerConfiguration()
-			o.ACMEDNS01Config.RecursiveNameservers = test.DNS01RecursiveServers
-
-			err := validation.ValidateControllerConfiguration(o)
-			if test.expError != "" {
-				if err == nil || !strings.Contains(err.Error(), test.expError) {
-					t.Errorf("expected error containing '%s', but got: %v", test.expError, err)
-				}
-			} else if err != nil {
-				t.Errorf("unexpected error: %v", err)
-			}
-		})
-	}
-}

cmd/controller/app/start.go

@@ -23,7 +23,6 @@ import (
 	"path/filepath"
 
 	"github.com/spf13/cobra"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	"github.com/cert-manager/cert-manager/controller-binary/app/options"
 	config "github.com/cert-manager/cert-manager/internal/apis/config/controller"
@@ -100,11 +99,15 @@ to renew certificates at an appropriate time before expiry.`,
 				return err
 			}
 
-			if err := validation.ValidateControllerConfiguration(controllerConfig); err != nil {
-				return fmt.Errorf("error validating flags: %w", err)
+			if err := validation.ValidateControllerConfiguration(controllerConfig, nil); len(err) > 0 {
+				return fmt.Errorf("error validating flags: %w", err.ToAggregate())
 			}
 
-			if err := logf.ValidateAndApplyAsField(&controllerConfig.Logging, field.NewPath("logging")); err != nil {
+			// ValidateControllerConfiguration should already have validated the
+			// logging flags, the logging API does not have a Apply-only function
+			// so we validate again here. This should not catch any validation errors
+			// anymore.
+			if err := logf.ValidateAndApply(&controllerConfig.Logging); err != nil {
 				return fmt.Errorf("failed to validate controller logging flags: %w", err)
 			}
 

cmd/webhook/app/webhook.go

@@ -23,7 +23,6 @@ import (
 	"path/filepath"
 
 	"github.com/spf13/cobra"
-	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	config "github.com/cert-manager/cert-manager/internal/apis/config/webhook"
 	"github.com/cert-manager/cert-manager/internal/apis/config/webhook/validation"
@@ -97,11 +96,15 @@ functionality for cert-manager.`,
 				return err
 			}
 
-			if err := validation.ValidateWebhookConfiguration(webhookConfig); err != nil {
-				return fmt.Errorf("error validating flags: %w", err)
+			if err := validation.ValidateWebhookConfiguration(webhookConfig, nil); len(err) > 0 {
+				return fmt.Errorf("error validating flags: %w", err.ToAggregate())
 			}
 
-			if err := logf.ValidateAndApplyAsField(&webhookConfig.Logging, field.NewPath("logging")); err != nil {
+			// ValidateWebhookConfiguration should already have validated the
+			// logging flags, the logging API does not have a Apply-only function
+			// so we validate again here. This should not catch any validation errors
+			// anymore.
+			if err := logf.ValidateAndApply(&webhookConfig.Logging); err != nil {
 				return fmt.Errorf("failed to validate webhook logging flags: %w", err)
 			}
 

internal/apis/certmanager/validation/issuer_test.go

@@ -349,7 +349,7 @@ func TestValidateVaultIssuerAuth(t *testing.T) {
 }
 
 func TestValidateACMEIssuerConfig(t *testing.T) {
-	fldPath := field.NewPath("")
+	fldPath := (*field.Path)(nil)
 
 	caBundle := unitcrypto.MustCreateCryptoBundle(t,
 		&pubcmapi.Certificate{Spec: pubcmapi.CertificateSpec{CommonName: "test"}},
@@ -694,7 +694,8 @@ func TestValidateACMEIssuerConfig(t *testing.T) {
 }
 
 func TestValidateIssuerSpec(t *testing.T) {
-	fldPath := field.NewPath("")
+	fldPath := (*field.Path)(nil)
+
 	scenarios := map[string]struct {
 		spec     *cmapi.IssuerSpec
 		errs     field.ErrorList
@@ -822,7 +823,8 @@ func TestValidateIssuerSpec(t *testing.T) {
 }
 
 func TestValidateACMEIssuerHTTP01Config(t *testing.T) {
-	fldPath := field.NewPath("")
+	fldPath := (*field.Path)(nil)
+
 	scenarios := map[string]struct {
 		isExpectedFailure bool
 		cfg               *cmacme.ACMEChallengeSolverHTTP01
@@ -1519,7 +1521,8 @@ func TestValidateSecretKeySelector(t *testing.T) {
 	validKey := "key"
 	// invalidName := cmmeta.LocalObjectReference{"-name-"}
 	// invalidKey := "-key-"
-	fldPath := field.NewPath("")
+	fldPath := (*field.Path)(nil)
+
 	scenarios := map[string]struct {
 		isExpectedFailure bool
 		selector          *cmmeta.SecretKeySelector

internal/apis/config/cainjector/validation/validation.go

@@ -17,9 +17,18 @@ limitations under the License.
 package validation
 
 import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	logsapi "k8s.io/component-base/logs/api/v1"
+
 	config "github.com/cert-manager/cert-manager/internal/apis/config/cainjector"
+	sharedvalidation "github.com/cert-manager/cert-manager/internal/apis/config/shared/validation"
 )
 
-func ValidateCAInjectorConfiguration(cfg *config.CAInjectorConfiguration) error {
-	return nil
+func ValidateCAInjectorConfiguration(cfg *config.CAInjectorConfiguration, fldPath *field.Path) field.ErrorList {
+	var allErrors field.ErrorList
+
+	allErrors = append(allErrors, logsapi.Validate(&cfg.Logging, nil, fldPath.Child("logging"))...)
+	allErrors = append(allErrors, sharedvalidation.ValidateLeaderElectionConfig(&cfg.LeaderElectionConfig, fldPath.Child("leaderElectionConfig"))...)
+
+	return allErrors
 }

internal/apis/config/cainjector/validation/validation_test.go

@@ -19,22 +19,69 @@ package validation
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	logsapi "k8s.io/component-base/logs/api/v1"
+
 	config "github.com/cert-manager/cert-manager/internal/apis/config/cainjector"
+	"github.com/cert-manager/cert-manager/internal/apis/config/shared"
 )
 
 func TestValidateCAInjectorConfiguration(t *testing.T) {
 	tests := []struct {
-		name    string
-		config  *config.CAInjectorConfiguration
-		wantErr bool
+		name   string
+		config *config.CAInjectorConfiguration
+		errs   func(*config.CAInjectorConfiguration) field.ErrorList
 	}{
-		// TODO: Add test cases once validation function padded out.
+		{
+			"with valid config",
+			&config.CAInjectorConfiguration{
+				Logging: logsapi.LoggingConfiguration{
+					Format: "text",
+				},
+			},
+			nil,
+		},
+		{
+			"with invalid logging config",
+			&config.CAInjectorConfiguration{
+				Logging: logsapi.LoggingConfiguration{
+					Format: "unknown",
+				},
+			},
+			func(wc *config.CAInjectorConfiguration) field.ErrorList {
+				return field.ErrorList{
+					field.Invalid(field.NewPath("logging.format"), wc.Logging.Format, "Unsupported log format"),
+				}
+			},
+		},
+		{
+			"with invalid leader election config",
+			&config.CAInjectorConfiguration{
+				Logging: logsapi.LoggingConfiguration{
+					Format: "text",
+				},
+				LeaderElectionConfig: shared.LeaderElectionConfig{
+					Enabled: true,
+				},
+			},
+			func(cc *config.CAInjectorConfiguration) field.ErrorList {
+				return field.ErrorList{
+					field.Invalid(field.NewPath("leaderElectionConfig.leaseDuration"), cc.LeaderElectionConfig.LeaseDuration, "must be greater than 0"),
+					field.Invalid(field.NewPath("leaderElectionConfig.renewDeadline"), cc.LeaderElectionConfig.RenewDeadline, "must be greater than 0"),
+					field.Invalid(field.NewPath("leaderElectionConfig.retryPeriod"), cc.LeaderElectionConfig.RetryPeriod, "must be greater than 0"),
+				}
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := ValidateCAInjectorConfiguration(tt.config); (err != nil) != tt.wantErr {
-				t.Errorf("ValidateCAInjectorConfiguration() error = %v, wantErr %v", err, tt.wantErr)
+			errList := ValidateCAInjectorConfiguration(tt.config, nil)
+			var expErrs field.ErrorList
+			if tt.errs != nil {
+				expErrs = tt.errs(tt.config)
 			}
+			assert.ElementsMatch(t, expErrs, errList)
 		})
 	}
 }