feat(kuma-cp) validate zone address and global address (#967)

Signed-off-by: Jakub Dyszkiewicz <[email protected]>

app/kumactl/cmd/completion/testdata/zsh.golden

@@ -751,7 +751,7 @@ function _kumactl_install_control-plane {
     '--dataplane-init-image[init image of the Kuma Dataplane component]:' \
     '--image-pull-policy[image pull policy that applies to all components of the Kuma Control Plane]:' \
     '--injector-failure-policy[failue policy of the mutating web hook implemented by the Kuma Injector component]:' \
-    '--kds-global-address[URL of Global Kuma CP]:' \
+    '--kds-global-address[URL of Global Kuma CP (example: grpcs://192.168.0.1:5685)]:' \
     '--kds-tls-cert[TLS certificate for the KDS server]:' \
     '--kds-tls-key[TLS key for the KDS server]:' \
     '--mode[kuma cp modes: one of standalone|remote|global]:' \

app/kumactl/cmd/install/install_control_plane.go

@@ -2,8 +2,7 @@ package install
 
 import (
 	"fmt"
-
-	"github.com/kumahq/kuma/pkg/config/core"
+	"net/url"
 
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -14,6 +13,7 @@ import (
 	controlplane "github.com/kumahq/kuma/app/kumactl/pkg/install/k8s/control-plane"
 	kumacni "github.com/kumahq/kuma/app/kumactl/pkg/install/k8s/kuma-cni"
 	kuma_cmd "github.com/kumahq/kuma/pkg/cmd"
+	"github.com/kumahq/kuma/pkg/config/core"
 	"github.com/kumahq/kuma/pkg/tls"
 	kuma_version "github.com/kumahq/kuma/pkg/version"
 )
@@ -84,64 +84,16 @@ func newInstallControlPlaneCmd(pctx *kumactl_cmd.RootContext) *cobra.Command {
 		Short: "Install Kuma Control Plane on Kubernetes",
 		Long:  `Install Kuma Control Plane on Kubernetes in a 'kuma-system' namespace.`,
 		RunE: func(cmd *cobra.Command, _ []string) error {
-			if err := core.ValidateCpMode(args.KumaCpMode); err != nil {
+			if err := validateArgs(args); err != nil {
 				return err
 			}
-			if args.KumaCpMode == core.Remote && args.Zone == "" {
-				return errors.Errorf("--zone is mandatory with `remote` mode")
-			}
-			if args.KumaCpMode == core.Remote && args.KdsGlobalAddress == "" {
-				return errors.Errorf("--kds-global-address is mandatory with `remote` mode")
-			}
+
 			if useNodePort && args.KumaCpMode != core.Standalone {
 				args.GlobalRemotePortType = "NodePort"
 			}
-			if args.AdmissionServerTlsCert == "" && args.AdmissionServerTlsKey == "" {
-				fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
-				// notice that Kubernetes doesn't requires DNS SAN in a X509 cert of a WebHook
-				admissionCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType)
-				if err != nil {
-					return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
-				}
-				args.AdmissionServerTlsCert = string(admissionCert.CertPEM)
-				args.AdmissionServerTlsKey = string(admissionCert.KeyPEM)
-			} else if args.AdmissionServerTlsCert == "" || args.AdmissionServerTlsKey == "" {
-				return errors.Errorf("Admission Server: both TLS Cert and TLS Key must be provided at the same time")
-			}
 
-			if args.SdsTlsCert == "" && args.SdsTlsKey == "" {
-				fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
-				hosts := []string{
-					fqdn,
-					fmt.Sprintf("%s.%s", args.ControlPlaneServiceName, args.Namespace),
-					args.ControlPlaneServiceName,
-					"localhost",
-				}
-				// notice that Envoy's SDS client (Google gRPC) does require DNS SAN in a X509 cert of an SDS server
-				sdsCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType, hosts...)
-				if err != nil {
-					return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
-				}
-				args.SdsTlsCert = string(sdsCert.CertPEM)
-				args.SdsTlsKey = string(sdsCert.KeyPEM)
-			} else if args.SdsTlsCert == "" || args.SdsTlsKey == "" {
-				return errors.Errorf("SDS: both TLS Cert and TLS Key must be provided at the same time")
-			}
-
-			if args.KdsTlsCert == "" && args.KdsTlsKey == "" {
-				fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
-				hosts := []string{
-					fqdn,
-					"localhost",
-				}
-				kdsCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType, hosts...)
-				if err != nil {
-					return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
-				}
-				args.KdsTlsCert = string(kdsCert.CertPEM)
-				args.KdsTlsKey = string(kdsCert.KeyPEM)
-			} else if args.KdsTlsCert == "" || args.KdsTlsKey == "" {
-				return errors.Errorf("KDS: both TLS Cert and TLS Key must be provided at the same time")
+			if err := autogenerateCerts(&args); err != nil {
+				return err
 			}
 
 			templateFiles, err := InstallCpTemplateFilesFn(args)
@@ -180,7 +132,7 @@ func newInstallControlPlaneCmd(pctx *kumactl_cmd.RootContext) *cobra.Command {
 	cmd.Flags().StringVar(&args.SdsTlsKey, "sds-tls-key", args.SdsTlsKey, "TLS key for the SDS server")
 	cmd.Flags().StringVar(&args.KdsTlsCert, "kds-tls-cert", args.KdsTlsCert, "TLS certificate for the KDS server")
 	cmd.Flags().StringVar(&args.KdsTlsKey, "kds-tls-key", args.KdsTlsKey, "TLS key for the KDS server")
-	cmd.Flags().StringVar(&args.KdsGlobalAddress, "kds-global-address", args.KdsGlobalAddress, "URL of Global Kuma CP")
+	cmd.Flags().StringVar(&args.KdsGlobalAddress, "kds-global-address", args.KdsGlobalAddress, "URL of Global Kuma CP (example: grpcs://192.168.0.1:5685)")
 	cmd.Flags().BoolVar(&args.CNIEnabled, "cni-enabled", args.CNIEnabled, "install Kuma with CNI instead of proxy init container")
 	cmd.Flags().StringVar(&args.CNIImage, "cni-image", args.CNIImage, "image of Kuma CNI component, if CNIEnabled equals true")
 	cmd.Flags().StringVar(&args.CNIVersion, "cni-version", args.CNIVersion, "version of the CNIImage")
@@ -190,6 +142,85 @@ func newInstallControlPlaneCmd(pctx *kumactl_cmd.RootContext) *cobra.Command {
 	return cmd
 }
 
+func validateArgs(args InstallControlPlaneArgs) error {
+	if err := core.ValidateCpMode(args.KumaCpMode); err != nil {
+		return err
+	}
+	if args.KumaCpMode == core.Remote && args.Zone == "" {
+		return errors.Errorf("--zone is mandatory with `remote` mode")
+	}
+	if args.KumaCpMode == core.Remote && args.KdsGlobalAddress == "" {
+		return errors.Errorf("--kds-global-address is mandatory with `remote` mode")
+	}
+	if args.KdsGlobalAddress != "" {
+		if args.KumaCpMode != core.Remote {
+			return errors.Errorf("--kds-global-address can only be used when --mode=remote")
+		}
+		u, err := url.Parse(args.KdsGlobalAddress)
+		if err != nil {
+			return errors.Errorf("--kds-global-address is not valid URL. The allowed format is grpcs://hostname:port")
+		}
+		if u.Scheme != "grpcs" {
+			return errors.Errorf("--kds-global-address should start with grpcs://")
+		}
+	}
+	if (args.AdmissionServerTlsCert == "") != (args.AdmissionServerTlsKey == "") {
+		return errors.Errorf("both --admission-server-tls-cert and --admission-server-tls-key must be provided at the same time")
+	}
+	if (args.SdsTlsCert == "") != (args.SdsTlsKey == "") {
+		return errors.Errorf("both --sds-tls-cert and --sds-tls-key must be provided at the same time")
+	}
+	if (args.KdsTlsCert == "") != (args.KdsTlsKey == "") {
+		return errors.Errorf("both --kds-tls-cert and --kds-tls-key must be provided at the same time")
+	}
+	return nil
+}
+
+func autogenerateCerts(args *InstallControlPlaneArgs) error {
+	if args.AdmissionServerTlsCert == "" && args.AdmissionServerTlsKey == "" {
+		fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
+		// notice that Kubernetes doesn't requires DNS SAN in a X509 cert of a WebHook
+		admissionCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType)
+		if err != nil {
+			return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
+		}
+		args.AdmissionServerTlsCert = string(admissionCert.CertPEM)
+		args.AdmissionServerTlsKey = string(admissionCert.KeyPEM)
+	}
+
+	if args.SdsTlsCert == "" && args.SdsTlsKey == "" {
+		fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
+		hosts := []string{
+			fqdn,
+			fmt.Sprintf("%s.%s", args.ControlPlaneServiceName, args.Namespace),
+			args.ControlPlaneServiceName,
+			"localhost",
+		}
+		// notice that Envoy's SDS client (Google gRPC) does require DNS SAN in a X509 cert of an SDS server
+		sdsCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType, hosts...)
+		if err != nil {
+			return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
+		}
+		args.SdsTlsCert = string(sdsCert.CertPEM)
+		args.SdsTlsKey = string(sdsCert.KeyPEM)
+	}
+
+	if args.KdsTlsCert == "" && args.KdsTlsKey == "" {
+		fqdn := fmt.Sprintf("%s.%s.svc", args.ControlPlaneServiceName, args.Namespace)
+		hosts := []string{
+			fqdn,
+			"localhost",
+		}
+		kdsCert, err := NewSelfSignedCert(fqdn, tls.ServerCertType, hosts...)
+		if err != nil {
+			return errors.Wrapf(err, "Failed to generate TLS certificate for %q", fqdn)
+		}
+		args.KdsTlsCert = string(kdsCert.CertPEM)
+		args.KdsTlsKey = string(kdsCert.KeyPEM)
+	}
+	return nil
+}
+
 func InstallCpTemplateFiles(args InstallControlPlaneArgs) (data.FileList, error) {
 	templateFiles, err := data.ReadFiles(controlplane.Templates)
 	if err != nil {

app/kumactl/cmd/install/install_control_plane_test.go

@@ -151,14 +151,69 @@ var _ = Describe("kumactl install control-plane", func() {
 			goldenFile: "install-control-plane.remote.golden.yaml",
 		}),
 	)
-	It("should fail to install control plane when `kumactl install control-plane run with unknown mode`", func() {
-		// given
-		rootCmd := cmd.DefaultRootCmd()
-		rootCmd.SetArgs([]string{"install", "control-plane", "--mode", "test"})
-		//when
-		err := rootCmd.Execute()
-		// then
-		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(Equal("invalid mode. Available modes: standalone, remote, global"))
-	})
+
+	type errTestCase struct {
+		extraArgs []string
+		errorMsg  string
+	}
+	DescribeTable("should fail to install control plane",
+		func(given errTestCase) {
+			// given
+			rootCmd := cmd.DefaultRootCmd()
+			rootCmd.SetArgs(append([]string{"install", "control-plane"}, given.extraArgs...))
+			rootCmd.SetOut(stdout)
+			rootCmd.SetErr(stderr)
+
+			//when
+			err := rootCmd.Execute()
+
+			// then
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Equal(given.errorMsg))
+		},
+		Entry("--mode is unknown", errTestCase{
+			extraArgs: []string{"--mode", "test"},
+			errorMsg:  "invalid mode. Available modes: standalone, remote, global",
+		}),
+		Entry("--kds-global-address is missing when installing remote", errTestCase{
+			extraArgs: []string{"--mode", "remote", "--zone", "zone-1"},
+			errorMsg:  "--kds-global-address is mandatory with `remote` mode",
+		}),
+		Entry("--kds-global-address is not valid URL", errTestCase{
+			extraArgs: []string{"--kds-global-address", "192.168.0.1:1234", "--mode", "remote", "--zone", "zone-1"},
+			errorMsg:  "--kds-global-address is not valid URL. The allowed format is grpcs://hostname:port",
+		}),
+		Entry("--kds-global-address has no grpcs scheme", errTestCase{
+			extraArgs: []string{"--kds-global-address", "http://192.168.0.1:1234", "--mode", "remote", "--zone", "zone-1"},
+			errorMsg:  "--kds-global-address should start with grpcs://",
+		}),
+		Entry("--kds-global-address is used with standalone", errTestCase{
+			extraArgs: []string{"--kds-global-address", "192.168.0.1:1234", "--mode", "standalone"},
+			errorMsg:  "--kds-global-address can only be used when --mode=remote",
+		}),
+		Entry("--admission-server-tls-cert without --admission-server-tls-key", errTestCase{
+			extraArgs: []string{"--admission-server-tls-cert", "cert.pem"},
+			errorMsg:  "both --admission-server-tls-cert and --admission-server-tls-key must be provided at the same time",
+		}),
+		Entry("--admission-server-tls-key without --admission-server-tls-cert", errTestCase{
+			extraArgs: []string{"--admission-server-tls-key", "key.pem"},
+			errorMsg:  "both --admission-server-tls-cert and --admission-server-tls-key must be provided at the same time",
+		}),
+		Entry("--sds-tls-cert without --sds-tls-key", errTestCase{
+			extraArgs: []string{"--sds-tls-cert", "cert.pem"},
+			errorMsg:  "both --sds-tls-cert and --sds-tls-key must be provided at the same time",
+		}),
+		Entry("--sds-tls-key without --sds-tls-cert", errTestCase{
+			extraArgs: []string{"--sds-tls-key", "key.pem"},
+			errorMsg:  "both --sds-tls-cert and --sds-tls-key must be provided at the same time",
+		}),
+		Entry("--kds-tls-cert without --kds-tls-key", errTestCase{
+			extraArgs: []string{"--kds-tls-cert", "cert.pem"},
+			errorMsg:  "both --kds-tls-cert and --kds-tls-key must be provided at the same time",
+		}),
+		Entry("--sds-tls-key without --kds-tls-cert", errTestCase{
+			extraArgs: []string{"--kds-tls-key", "key.pem"},
+			errorMsg:  "both --kds-tls-cert and --kds-tls-key must be provided at the same time",
+		}),
+	)
 })

docs/cmd/kumactl/HELP.md

@@ -236,7 +236,7 @@ Flags:
   -h, --help                                help for control-plane
       --image-pull-policy string            image pull policy that applies to all components of the Kuma Control Plane (default "IfNotPresent")
       --injector-failure-policy string      failue policy of the mutating web hook implemented by the Kuma Injector component (default "Ignore")
-      --kds-global-address string           URL of Global Kuma CP
+      --kds-global-address string           URL of Global Kuma CP (example: grpcs://192.168.0.1:5685)
       --kds-tls-cert string                 TLS certificate for the KDS server
       --kds-tls-key string                  TLS key for the KDS server
       --mode string                         kuma cp modes: one of standalone|remote|global (default "standalone")

pkg/core/resources/apis/system/zone_validator.go

@@ -2,6 +2,7 @@ package system
 
 import (
 	"net"
+	"net/url"
 
 	"github.com/kumahq/kuma/pkg/core/validators"
 )
@@ -14,15 +15,24 @@ func (c *ZoneResource) Validate() error {
 
 func (c *ZoneResource) validateIngress() validators.ValidationError {
 	var verr validators.ValidationError
-	host, port, err := net.SplitHostPort(c.Spec.GetIngress().GetAddress())
-	if err != nil {
-		verr.AddViolation("address", "invalid address")
+	if c.Spec.GetIngress().GetAddress() == "" {
+		verr.AddViolation("address", "cannot be empty")
 	} else {
-		if host == "" {
-			verr.AddViolation("address", "host has to be explicitly specified")
-		}
-		if port == "" {
-			verr.AddViolation("address", "port has to be explicitly specified")
+		host, port, err := net.SplitHostPort(c.Spec.GetIngress().GetAddress())
+		if err != nil {
+			url, urlErr := url.Parse(c.Spec.GetIngress().GetAddress())
+			if urlErr == nil && url.Scheme != "" {
+				verr.AddViolation("address", "should not be URL. Expected format is hostname:port")
+			} else {
+				verr.AddViolation("address", "invalid address: "+err.Error())
+			}
+		} else {
+			if host == "" {
+				verr.AddViolation("address", "host has to be explicitly specified")
+			}
+			if port == "" {
+				verr.AddViolation("address", "port has to be explicitly specified")
+			}
 		}
 	}
 

pkg/core/resources/apis/system/zone_validator_test.go

@@ -65,15 +65,29 @@ var _ = Describe("Zone", func() {
 				expected: `
                violations:
                  - field: address
-                   message: invalid address`}),
+                   message: cannot be empty`}),
 			Entry("wrong format", testCase{
 				zone: `
                ingress:
                  address: 192.168.0.2`,
 				expected: `
                violations:
                  - field: address
-                   message: invalid address`}),
+                   message: "invalid address: address 192.168.0.2: missing port in address"`}),
+			Entry("spec: empty", testCase{
+				zone: ``,
+				expected: `
+               violations:
+                 - field: address
+                   message: cannot be empty`}),
+			Entry("url instead of address", testCase{
+				zone: `
+               ingress:
+                 address: grpcs://192.168.0.2:1234`,
+				expected: `
+               violations:
+                 - field: address
+                   message: should not be URL. Expected format is hostname:port`}),
 		)
 	})
 })

test/framework/k8s_cluster.go

@@ -475,7 +475,7 @@ func (c *K8sCluster) deleteKumaViaKumactl(opts *deployOptions) error {
 	switch c.controlplane.mode {
 	case core.Remote:
 		// kumactl remote deployment will fail if GlobalAddress is not specified
-		args = append(args, "--kds-global-address", "grpc://0.0.0.0:5685")
+		args = append(args, "--kds-global-address", "grpcs://0.0.0.0:5685")
 	}
 	yaml, err := c.controlplane.InstallCP(args...)
 	if err != nil {