Verify udn-allowed-default-services config field

Ensure that the values specified in udn-allowed-default-services are valid namespaced service names. Stop using cli.StringSlice to allow for setting the field through a config file. Signed-off-by: Patryk Diak <[email protected]>
ovn-org · Sep 20, 2024 · c6e6080 · c6e6080
1 parent 312a5cb
commit c6e6080
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 26 deletions.
diff --git a/go-controller/pkg/config/config.go b/go-controller/pkg/config/config.go
@@ -13,14 +13,14 @@ import (
 	"strings"
 	"time"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
-
 	"github.com/urfave/cli/v2"
 	gcfg "gopkg.in/gcfg.v1"
 	lumberjack "gopkg.in/natefinch/lumberjack.v2"
+	"k8s.io/apimachinery/pkg/api/validation"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
-
 	kexec "k8s.io/utils/exec"
 	utilnet "k8s.io/utils/net"
 
@@ -60,20 +60,20 @@ var (
 
 	// Default holds parsed config file parameters and command-line overrides
 	Default = DefaultConfig{
-		MTU:                       1400,
-		ConntrackZone:             64000,
-		EncapType:                 "geneve",
-		EncapIP:                   "",
-		EncapPort:                 DefaultEncapPort,
-		InactivityProbe:           100000, // in Milliseconds
-		OpenFlowProbe:             180,    // in Seconds
-		OfctrlWaitBeforeClear:     0,      // in Milliseconds
-		MonitorAll:                true,
-		OVSDBTxnTimeout:           DefaultDBTxnTimeout,
-		LFlowCacheEnable:          true,
-		RawClusterSubnets:         "10.128.0.0/14/23",
-		Zone:                      types.OvnDefaultZone,
-		UDNAllowedDefaultServices: *cli.NewStringSlice("default/kubernetes", "kube-system/kube-dns"),
+		MTU:                          1400,
+		ConntrackZone:                64000,
+		EncapType:                    "geneve",
+		EncapIP:                      "",
+		EncapPort:                    DefaultEncapPort,
+		InactivityProbe:              100000, // in Milliseconds
+		OpenFlowProbe:                180,    // in Seconds
+		OfctrlWaitBeforeClear:        0,      // in Milliseconds
+		MonitorAll:                   true,
+		OVSDBTxnTimeout:              DefaultDBTxnTimeout,
+		LFlowCacheEnable:             true,
+		RawClusterSubnets:            "10.128.0.0/14/23",
+		Zone:                         types.OvnDefaultZone,
+		UDNAllowedDefaultServicesRaw: "default/kubernetes,kube-system/kube-dns",
 	}
 
 	// Logging holds logging-related parsed config file parameters and command-line overrides
@@ -282,9 +282,13 @@ type DefaultConfig struct {
 	// Zone name to which ovnkube-node/ovnkube-controller belongs to
 	Zone string `gcfg:"zone"`
 
+	// UDNAllowedDefaultServicesRaw holds the unparsed UDNAllowedDefaultServices. Should only be
+	// used inside config module.
+	UDNAllowedDefaultServicesRaw string `gcfg:"udn-allowed-default-services"`
+
 	// UDNAllowedDefaultServices holds a list of namespaced names of
 	// default cluster network services accessible from primary user-defined networks
-	UDNAllowedDefaultServices cli.StringSlice `gcfg:"udn-allowed-default-services"`
+	UDNAllowedDefaultServices []string
 }
 
 // LoggingConfig holds logging-related parsed config file parameters and command-line overrides
@@ -926,13 +930,13 @@ var CommonFlags = []cli.Flag{
 		Value:       Default.Zone,
 		Destination: &cliConfig.Default.Zone,
 	},
-	&cli.StringSliceFlag{
+	&cli.StringFlag{
 		Name: "udn-allowed-default-services",
 		Usage: "a list of namespaced names of default cluster network services accessible from primary" +
 			"user-defined networks. If not specified defaults to [\"default/kubernetes\", \"kube-system/kube-dns\"]." +
 			"Only used when enable-network-segmentation is set",
-		Value:       &Default.UDNAllowedDefaultServices,
-		Destination: &cliConfig.Default.UDNAllowedDefaultServices,
+		Value:       Default.UDNAllowedDefaultServicesRaw,
+		Destination: &cliConfig.Default.UDNAllowedDefaultServicesRaw,
 	},
 }
 
@@ -2124,13 +2128,39 @@ func completeDefaultConfig(allSubnets *ConfigSubnets) error {
 		allSubnets.Append(ConfigSubnetCluster, subnet.CIDR)
 	}
 
+	Default.UDNAllowedDefaultServices, err = parseServicesNamespacedNames(Default.UDNAllowedDefaultServicesRaw)
+	if err != nil {
+		return fmt.Errorf("UDN allowed services field is invalid: %v", err)
+	}
+
 	Default.HostMasqConntrackZone = Default.ConntrackZone + 1
 	Default.OVNMasqConntrackZone = Default.ConntrackZone + 2
 	Default.HostNodePortConntrackZone = Default.ConntrackZone + 3
 	Default.ReassemblyConntrackZone = Default.ConntrackZone + 4
 	return nil
 }
 
+// parseServicesNamespacedNames splits the input string by `,` and returns a slice
+// of keys that were verified to be a valid namespaced service name. It ignores spaces between the elements.
+func parseServicesNamespacedNames(servicesRaw string) ([]string, error) {
+	var services []string
+	for _, udnEnabledSVC := range strings.Split(servicesRaw, ",") {
+		svcKey := strings.TrimSpace(udnEnabledSVC)
+		namespace, name, err := cache.SplitMetaNamespaceKey(strings.TrimSpace(svcKey))
+		if namespace == "" {
+			return nil, fmt.Errorf("UDN enabled service %q no namespace set: %v", svcKey, err)
+		}
+		if errs := validation.ValidateNamespaceName(namespace, false); len(errs) != 0 {
+			return nil, fmt.Errorf("UDN enabled service %q has an invalid namespace: %v", svcKey, err)
+		}
+		if errs := validation.NameIsDNSSubdomain(name, false); len(errs) != 0 {
+			return nil, fmt.Errorf("UDN enabled service %q has an invalid name: %v", svcKey, err)
+		}
+		services = append(services, svcKey)
+	}
+	return services, nil
+}
+
 // getConfigFilePath returns config file path and 'true' if the config file is
 // the fallback path (eg not given by the user), 'false' if given explicitly
 // by the user

diff --git a/go-controller/pkg/config/config_test.go b/go-controller/pkg/config/config_test.go
@@ -9,11 +9,12 @@ import (
 	"testing"
 	"time"
 
-	ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
-	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
 	"github.com/urfave/cli/v2"
 	kexec "k8s.io/utils/exec"
 
+	ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
+
 	. "github.com/onsi/ginkgo"
 	"github.com/onsi/gomega"
 )
@@ -1600,6 +1601,45 @@ foo=bar
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	})
 
+	It("rejects a config with invalid udn allowed services", func() {
+		err := ioutil.WriteFile(cfgFile.Name(), []byte(`[default]
+udn-allowed-default-services=namespace/invalid.name,test
+`), 0o644)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		app.Action = func(ctx *cli.Context) error {
+			_, err = InitConfig(ctx, kexec.New(), nil)
+			gomega.Expect(err).To(gomega.HaveOccurred())
+
+			return nil
+		}
+		cliArgs := []string{
+			app.Name,
+			"-config-file=" + cfgFile.Name(),
+		}
+		err = app.Run(cliArgs)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	})
+
+	It("accepts a config with valid udn allowed services", func() {
+		err := ioutil.WriteFile(cfgFile.Name(), []byte(`[default]
+udn-allowed-default-services= ns/svc, ns1/svc1 
+`), 0o644)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+		app.Action = func(ctx *cli.Context) error {
+			_, err = InitConfig(ctx, kexec.New(), nil)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(Default.UDNAllowedDefaultServices).To(gomega.HaveLen(2))
+			return nil
+		}
+		cliArgs := []string{
+			app.Name,
+			"-config-file=" + cfgFile.Name(),
+		}
+		err = app.Run(cliArgs)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	})
 	Describe("OvnDBAuth operations", func() {
 		var certFile, keyFile, caFile string
 

diff --git a/go-controller/pkg/util/util.go b/go-controller/pkg/util/util.go
@@ -14,9 +14,10 @@ import (
 	"golang.org/x/exp/constraints"
 	"k8s.io/client-go/tools/cache"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"crypto/rand"
 
@@ -582,7 +583,7 @@ func GetServiceEndpointSlices(namespace, svcName, network string, endpointSliceL
 
 // IsUDNEnabledService checks whether the provided namespaced name key is a UDN enabled service specified in config.Default.UDNAllowedDefaultServices
 func IsUDNEnabledService(key string) bool {
-	for _, enabledService := range config.Default.UDNAllowedDefaultServices.Value() {
+	for _, enabledService := range config.Default.UDNAllowedDefaultServices {
 		if enabledService == key {
 			return true
 		}