Repository: kumahq/kuma
Pull request #980 (2020-08-17 09:05:11)

fix(kumacp) API readonly message according to Mode

### Summary

Enabling a more elaborate message when accessing the API, depending on the Kuma CP mode.




Patch:
diff --git a/pkg/api-server/resource_endpoints.go b/pkg/api-server/resource_endpoints.go
index 9362e6336..c847433e7 100644
--- a/pkg/api-server/resource_endpoints.go
+++ b/pkg/api-server/resource_endpoints.go
@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"net/http"
 
+	config_core "github.com/kumahq/kuma/pkg/config/core"
+
 	"github.com/kumahq/kuma/pkg/core/resources/apis/system"
 
 	"github.com/emicklei/go-restful"
@@ -26,10 +28,10 @@ const (
 	k8sReadOnlyMessage = "On Kubernetes you cannot change the state of Kuma resources with 'kumactl apply' or via the HTTP API." +
 		" As a best practice, you should always be using 'kubectl apply' instead." +
 		" You can still use 'kumactl' or the HTTP API to make read-only operations. On Universal this limitation does not apply.\n"
-	//globalReadOnlyMessage = "On global control plane you can not modify dataplane resources with 'kumactl apply' or via the HTTP API." +
-	//	" You can still use 'kumactl' or the HTTP API to modify them on the remote control plane.\n"
-	//remoteReadOnlyMessage = "On remote control plane you can only modify dataplane resources with 'kumactl apply' or via the HTTP API." +
-	//	" You can still use 'kumactl' or the HTTP API to modify the rest of the resource on the global control plane.\n"
+	globalReadOnlyMessage = "On global control plane you can not modify dataplane resources with 'kumactl apply' or via the HTTP API." +
+		" You can still use 'kumactl' or the HTTP API to modify them on the remote control plane.\n"
+	remoteReadOnlyMessage = "On remote control plane you can only modify dataplane resources with 'kumactl apply' or via the HTTP API." +
+		" You can still use 'kumactl' or the HTTP API to modify the rest of the resource on the global control plane.\n"
 )
 
 func meshFromPathParam(param string) meshFromRequestFn {
@@ -39,6 +41,7 @@ func meshFromPathParam(param string) meshFromRequestFn {
 }
 
 type resourceEndpoints struct {
+	mode            config_core.CpMode
 	publicURL       string
 	resManager      manager.ResourceManager
 	meshFromRequest meshFromRequestFn
@@ -167,7 +170,7 @@ func (r *resourceEndpoints) addCreateOrUpdateEndpointReadOnly(ws *restful.WebSer
 }
 
 func (r *resourceEndpoints) createOrUpdateResourceReadOnly(request *restful.Request, response *restful.Response) {
-	err := response.WriteErrorString(http.StatusMethodNotAllowed, k8sReadOnlyMessage)
+	err := response.WriteErrorString(http.StatusMethodNotAllowed, r.readOnlyMessage())
 	if err != nil {
 		core.Log.Error(err, "Could not write the response")
 	}
@@ -197,7 +200,7 @@ func (r *resourceEndpoints) addDeleteEndpointReadOnly(ws *restful.WebService, pa
 }
 
 func (r *resourceEndpoints) deleteResourceReadOnly(request *restful.Request, response *restful.Response) {
-	err := response.WriteErrorString(http.StatusMethodNotAllowed, k8sReadOnlyMessage)
+	err := response.WriteErrorString(http.StatusMethodNotAllowed, r.readOnlyMessage())
 	if err != nil {
 		core.Log.Error(err, "Could not write the response")
 	}
@@ -221,3 +224,14 @@ func (r *resourceEndpoints) validateResourceRequest(request *restful.Request, re
 	err.AddError("", mesh.ValidateMeta(name, meshName))
 	return err.OrNil()
 }
+
+func (r *resourceEndpoints) readOnlyMessage() string {
+	switch r.mode {
+	case config_core.Global:
+		return globalReadOnlyMessage
+	case config_core.Remote:
+		return remoteReadOnlyMessage
+	default:
+		return k8sReadOnlyMessage
+	}
+}
diff --git a/pkg/api-server/server.go b/pkg/api-server/server.go
index 37349fb72..ed22932bd 100644
--- a/pkg/api-server/server.go
+++ b/pkg/api-server/server.go
@@ -20,7 +20,6 @@ import (
 
 	"github.com/kumahq/kuma/app/kuma-ui/pkg/resources"
 	"github.com/kumahq/kuma/pkg/api-server/definitions"
-	api_server_config "github.com/kumahq/kuma/pkg/config/api-server"
 	"github.com/kumahq/kuma/pkg/core"
 	"github.com/kumahq/kuma/pkg/core/resources/apis/mesh"
 	"github.com/kumahq/kuma/pkg/core/resources/manager"
@@ -84,7 +83,7 @@ func NewApiServer(resManager manager.ResourceManager, defs []definitions.Resourc
 		Consumes(restful.MIME_JSON).
 		Produces(restful.MIME_JSON)
 
-	addResourcesEndpoints(ws, defs, resManager, serverConfig)
+	addResourcesEndpoints(ws, defs, resManager, cfg)
 	container.Add(ws)
 
 	if err := addIndexWsEndpoints(ws); err != nil {
@@ -115,7 +114,8 @@ func NewApiServer(resManager manager.ResourceManager, defs []definitions.Resourc
 	return newApiServer, nil
 }
 
-func addResourcesEndpoints(ws *restful.WebService, defs []definitions.ResourceWsDefinition, resManager manager.ResourceManager, config *api_server_config.ApiServerConfig) {
+func addResourcesEndpoints(ws *restful.WebService, defs []definitions.ResourceWsDefinition, resManager manager.ResourceManager, cfg *kuma_cp.Config) {
+	config := cfg.ApiServer
 	endpoints := dataplaneOverviewEndpoints{
 		publicURL:  config.Catalog.ApiServer.Url,
 		resManager: resManager,
@@ -135,6 +135,7 @@ func addResourcesEndpoints(ws *restful.WebService, defs []definitions.ResourceWs
 		switch definition.ResourceFactory().GetType() {
 		case mesh.MeshType:
 			endpoints := resourceEndpoints{
+				mode:                 cfg.Mode,
 				publicURL:            config.Catalog.ApiServer.Url,
 				resManager:           resManager,
 				ResourceWsDefinition: definition,
diff --git a/pkg/plugins/discovery/k8s/controllers/pod_controller.go b/pkg/plugins/discovery/k8s/controllers/pod_controller.go
index 8b5ff1840..56f4146d6 100644
--- a/pkg/plugins/discovery/k8s/controllers/pod_controller.go
+++ b/pkg/plugins/discovery/k8s/controllers/pod_controller.go
@@ -109,7 +109,7 @@ func (r *PodReconciler) findMatchingServices(pod *kube_core.Pod) ([]*kube_core.S
 	}
 
 	// only consider Services that match this Pod
-	matchingServices := util_k8s.FindServices(allServices, util_k8s.MatchServiceThatSelectsPod(pod))
+	matchingServices := util_k8s.FindServices(allServices, util_k8s.AnySelector(), util_k8s.MatchServiceThatSelectsPod(pod))
 
 	return matchingServices, nil
 }
diff --git a/pkg/plugins/discovery/k8s/util/util.go b/pkg/plugins/discovery/k8s/util/util.go
index 084427c48..4dac1c0d9 100644
--- a/pkg/plugins/discovery/k8s/util/util.go
+++ b/pkg/plugins/discovery/k8s/util/util.go
@@ -17,11 +17,28 @@ func MatchServiceThatSelectsPod(pod *kube_core.Pod) ServicePredicate {
 	}
 }
 
-func FindServices(svcs *kube_core.ServiceList, predicate ServicePredicate) []*kube_core.Service {
+// According to K8S docs about Service#selector:
+// Route service traffic to pods with label keys and values matching this selector.
+// If empty or not present, the service is assumed to have an external process managing its endpoints, which Kubernetes will not modify.
+// Only applies to types ClusterIP, NodePort, and LoadBalancer. Ignored if type is ExternalName. More info: https://kubernetes.io/docs/concepts/services-networking/service/
+//
+// When converting Pod to Dataplane, we don't want to take into account Services that has no Selector, otherwise any Pod will match this service
+// and since we just take any int target port in #util.FindPort every Dataplane in the same namespace as this service would get an extra inbound for it.
+func AnySelector() ServicePredicate {
+	return func(svc *kube_core.Service) bool {
+		return len(svc.Spec.Selector) > 0
+	}
+}
+
+func FindServices(svcs *kube_core.ServiceList, predicates ...ServicePredicate) []*kube_core.Service {
 	matching := make([]*kube_core.Service, 0)
 	for i := range svcs.Items {
 		svc := &svcs.Items[i]
-		if predicate(svc) {
+		allMatched := true
+		for _, predicate := range predicates {
+			allMatched = allMatched && predicate(svc)
+		}
+		if allMatched {
 			matching = append(matching, svc)
 		}
 	}
@@ -52,6 +69,11 @@ func FindPort(pod *kube_core.Pod, svcPort *kube_core.ServicePort) (int, error) {
 			}
 		}
 	case kube_intstr.Int:
+		// According to K8S docs about Container#ports:
+		// List of ports to expose from the container. Exposing a port here gives the system additional information about the network connections a container uses, but is primarily informational.
+		// Not specifying a port here DOES NOT prevent that port from being exposed. Any port which is listening on the default "0.0.0.0" address inside a container will be accessible from the network
+		//
+		// Therefore we cannot match service port to the container port.
 		return portName.IntValue(), nil
 	}
 


Test patch:
diff --git a/pkg/plugins/discovery/k8s/controllers/pod_controller_test.go b/pkg/plugins/discovery/k8s/controllers/pod_controller_test.go
index 5c7c7f58d..bdf237725 100644
--- a/pkg/plugins/discovery/k8s/controllers/pod_controller_test.go
+++ b/pkg/plugins/discovery/k8s/controllers/pod_controller_test.go
@@ -49,6 +49,9 @@ var _ = Describe("PodReconciler", func() {
 					Annotations: map[string]string{
 						"kuma.io/sidecar-injected": "true",
 					},
+					Labels: map[string]string{
+						"app": "sample",
+					},
 				},
 			},
 			&kube_core.Pod{
@@ -59,6 +62,9 @@ var _ = Describe("PodReconciler", func() {
 						"kuma.io/mesh":             "poc",
 						"kuma.io/sidecar-injected": "true",
 					},
+					Labels: map[string]string{
+						"app": "sample",
+					},
 				},
 				Spec: kube_core.PodSpec{
 					Containers: []kube_core.Container{
@@ -105,6 +111,9 @@ var _ = Describe("PodReconciler", func() {
 							},
 						},
 					},
+					Selector: map[string]string{
+						"app": "sample",
+					},
 				},
 			})
 
@@ -254,10 +263,12 @@ var _ = Describe("PodReconciler", func() {
             inbound:
             - port: 8080
               tags:
+                app: sample
                 kuma.io/protocol: http
                 kuma.io/service: example_demo_svc_80
             - port: 6060
               tags:
+                app: sample
                 kuma.io/service: example_demo_svc_6061
                 kuma.io/protocol: tcp
 `))
@@ -324,10 +335,12 @@ var _ = Describe("PodReconciler", func() {
             inbound:
             - port: 8080
               tags:
+                app: sample
                 kuma.io/protocol: http
                 kuma.io/service: example_demo_svc_80
             - port: 6060
               tags:
+                app: sample
                 kuma.io/service: example_demo_svc_6061
                 kuma.io/protocol: tcp
 `))
diff --git a/pkg/plugins/discovery/k8s/util/util_test.go b/pkg/plugins/discovery/k8s/util/util_test.go
index a4c8b88d5..d7bfc0682 100644
--- a/pkg/plugins/discovery/k8s/util/util_test.go
+++ b/pkg/plugins/discovery/k8s/util/util_test.go
@@ -100,11 +100,18 @@ var _ = Describe("Util", func() {
 							},
 						},
 					},
+					{
+						ObjectMeta: kube_meta.ObjectMeta{
+							Name:      "kubernetes",
+							Namespace: "default",
+						},
+						Spec: kube_core.ServiceSpec{},
+					},
 				},
 			}
 
 			// when
-			matchingServices := FindServices(svcs, MatchServiceThatSelectsPod(pod))
+			matchingServices := FindServices(svcs, AnySelector(), MatchServiceThatSelectsPod(pod))
 			// then
 			Expect(matchingServices).To(HaveLen(1))
 			Expect(matchingServices).To(ConsistOf(&svcs.Items[0]))
diff --git a/test/framework/interface.go b/test/framework/interface.go
index c126ecbe3..a9acf258c 100644
--- a/test/framework/interface.go
+++ b/test/framework/interface.go
@@ -22,6 +22,7 @@ type deployOptions struct {
 	installationMode InstallationMode
 	helmReleaseName  string
 	helmOpts         map[string]string
+	ctlOpts          map[string]string
 }
 
 type DeployOptionsFunc func(*deployOptions)
@@ -53,6 +54,15 @@ func WithHelmOpt(name, value string) DeployOptionsFunc {
 	}
 }
 
+func WithCtlOpt(name, value string) DeployOptionsFunc {
+	return func(o *deployOptions) {
+		if o.ctlOpts == nil {
+			o.ctlOpts = map[string]string{}
+		}
+		o.ctlOpts[name] = value
+	}
+}
+
 func newDeployOpt(fs ...DeployOptionsFunc) *deployOptions {
 	rv := &deployOptions{
 		installationMode: KumactlInstallationMode,
diff --git a/test/framework/k8s_cluster.go b/test/framework/k8s_cluster.go
index 64b014136..1121696f0 100644
--- a/test/framework/k8s_cluster.go
+++ b/test/framework/k8s_cluster.go
@@ -232,10 +232,22 @@ func (c *K8sCluster) GetPodLogs(pod v1.Pod) (string, error) {
 // deployKumaViaKubectl uses kubectl to install kuma
 // using the resources from the `kumactl install control-plane` command
 func (c *K8sCluster) deployKumaViaKubectl(mode string, opts *deployOptions) error {
-	var args []string
+	argsMap := map[string]string{
+		"--control-plane-image":  kumaCPImage,
+		"--dataplane-image":      kumaDPImage,
+		"--dataplane-init-image": kumaInitImage,
+	}
 	switch mode {
 	case core.Remote:
-		args = append(args, "--kds-global-address", opts.globalAddress)
+		argsMap["--kds-global-address"] = opts.globalAddress
+	}
+	for opt, value := range opts.ctlOpts {
+		argsMap[opt] = value
+	}
+
+	var args []string
+	for k, v := range argsMap {
+		args = append(args, k, v)
 	}
 	yaml, err := c.controlplane.InstallCP(args...)
 	if err != nil {
@@ -471,11 +483,19 @@ func (c *K8sCluster) deleteKumaViaHelm(opts *deployOptions) (errs error) {
 }
 
 func (c *K8sCluster) deleteKumaViaKumactl(opts *deployOptions) error {
-	args := []string{}
+	argsMap := map[string]string{}
 	switch c.controlplane.mode {
 	case core.Remote:
 		// kumactl remote deployment will fail if GlobalAddress is not specified
-		args = append(args, "--kds-global-address", "grpcs://0.0.0.0:5685")
+		argsMap["--kds-global-address"] = "grpcs://0.0.0.0:5685"
+	}
+	for opt, value := range opts.ctlOpts {
+		argsMap[opt] = value
+	}
+
+	var args []string
+	for k, v := range argsMap {
+		args = append(args, k, v)
 	}
 	yaml, err := c.controlplane.InstallCP(args...)
 	if err != nil {
diff --git a/test/framework/kumactl.go b/test/framework/kumactl.go
index a9337503b..0b5916467 100644
--- a/test/framework/kumactl.go
+++ b/test/framework/kumactl.go
@@ -114,9 +114,6 @@ func storeConfigToTempFile(name string, configData string) (string, error) {
 func (o *KumactlOptions) KumactlInstallCP(mode string, args ...string) (string, error) {
 	cmd := []string{
 		"install", "control-plane",
-		"--control-plane-image", kumaCPImage,
-		"--dataplane-image", kumaDPImage,
-		"--dataplane-init-image", kumaInitImage,
 	}
 
 	cmd = append(cmd, "--mode", mode)