Merge pull request #50620 from juanvallejo/jvallejo/update-kubectl-top-validation

Automatic merge from submit-queue (batch tested with PRs 47896, 50678, 50620, 50631, 51005)

allow heapster option defaults to be set - kubectl top node|pod

**Release note**:
```release-note
NONE
```

Allows downstream / outside consumers of `NewCmdTopNode` and `NewCmdTopPod` commands to set project or case-specific heapster option defaults.

Related downstream PR: https://github.com/openshift/origin/pull/15764

cc @fabianofranz

pkg/kubectl/cmd/top.go

@@ -47,7 +47,7 @@ func NewCmdTop(f cmdutil.Factory, out, errOut io.Writer) *cobra.Command {
 	}
 
 	// create subcommands
-	cmd.AddCommand(NewCmdTopNode(f, out))
+	cmd.AddCommand(NewCmdTopNode(f, nil, out))
-	cmd.AddCommand(NewCmdTopPod(f, out))
+	cmd.AddCommand(NewCmdTopPod(f, nil, out))
 	return cmd
 }

pkg/kubectl/cmd/top_node.go

@@ -50,10 +50,23 @@ type HeapsterTopOptions struct {
 }
 
 func (o *HeapsterTopOptions) Bind(flags *pflag.FlagSet) {
-	flags.StringVar(&o.Namespace, "heapster-namespace", metricsutil.DefaultHeapsterNamespace, "Namespace Heapster service is located in")
+	if len(o.Namespace) == 0 {
-	flags.StringVar(&o.Service, "heapster-service", metricsutil.DefaultHeapsterService, "Name of Heapster service")
+		o.Namespace = metricsutil.DefaultHeapsterNamespace
-	flags.StringVar(&o.Scheme, "heapster-scheme", metricsutil.DefaultHeapsterScheme, "Scheme (http or https) to connect to Heapster as")
+	}
-	flags.StringVar(&o.Port, "heapster-port", metricsutil.DefaultHeapsterPort, "Port name in service to use")
+	if len(o.Service) == 0 {
+		o.Service = metricsutil.DefaultHeapsterService
+	}
+	if len(o.Scheme) == 0 {
+		o.Scheme = metricsutil.DefaultHeapsterScheme
+	}
+	if len(o.Port) == 0 {
+		o.Port = metricsutil.DefaultHeapsterPort
+	}
+
+	flags.StringVar(&o.Namespace, "heapster-namespace", o.Namespace, "Namespace Heapster service is located in")
+	flags.StringVar(&o.Service, "heapster-service", o.Service, "Name of Heapster service")
+	flags.StringVar(&o.Scheme, "heapster-scheme", o.Scheme, "Scheme (http or https) to connect to Heapster as")
+	flags.StringVar(&o.Port, "heapster-port", o.Port, "Port name in service to use")
 }
 
 var (
@@ -70,8 +83,10 @@ var (
 		  kubectl top node NODE_NAME`))
 )
 
-func NewCmdTopNode(f cmdutil.Factory, out io.Writer) *cobra.Command {
+func NewCmdTopNode(f cmdutil.Factory, options *TopNodeOptions, out io.Writer) *cobra.Command {
-	options := &TopNodeOptions{}
+	if options == nil {
+		options = &TopNodeOptions{}
+	}
 
 	cmd := &cobra.Command{
 		Use:     "node [NAME | -l label]",

pkg/kubectl/cmd/top_node_test.go

@@ -67,7 +67,60 @@ func TestTopNodeAllMetrics(t *testing.T) {
 	tf.ClientConfig = defaultClientConfig()
 	buf := bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdTopNode(f, buf)
+	cmd := NewCmdTopNode(f, nil, buf)
+	cmd.Run(cmd, []string{})
+
+	// Check the presence of node names in the output.
+	result := buf.String()
+	for _, m := range metrics.Items {
+		if !strings.Contains(result, m.Name) {
+			t.Errorf("missing metrics for %s: \n%s", m.Name, result)
+		}
+	}
+}
+
+func TestTopNodeAllMetricsCustomDefaults(t *testing.T) {
+	customBaseHeapsterServiceAddress := "/api/v1/namespaces/custom-namespace/services/https:custom-heapster-service:/proxy"
+	customBaseMetricsAddress := customBaseHeapsterServiceAddress + "/apis/metrics"
+
+	initTestErrorHandler(t)
+	metrics, nodes := testNodeMetricsData()
+	expectedMetricsPath := fmt.Sprintf("%s/%s/nodes", customBaseMetricsAddress, metricsApiVersion)
+	expectedNodePath := fmt.Sprintf("/%s/%s/nodes", apiPrefix, apiVersion)
+
+	f, tf, codec, ns := cmdtesting.NewAPIFactory()
+	tf.Printer = &testPrinter{}
+	tf.Client = &fake.RESTClient{
+		APIRegistry:          api.Registry,
+		NegotiatedSerializer: ns,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+			switch p, m := req.URL.Path, req.Method; {
+			case p == expectedMetricsPath && m == "GET":
+				body, err := marshallBody(metrics)
+				if err != nil {
+					t.Errorf("unexpected error: %v", err)
+				}
+				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil
+			case p == expectedNodePath && m == "GET":
+				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, nodes)}, nil
+			default:
+				t.Fatalf("unexpected request: %#v\nGot URL: %#v\nExpected path: %#v", req, req.URL, expectedMetricsPath)
+				return nil, nil
+			}
+		}),
+	}
+	tf.Namespace = "test"
+	tf.ClientConfig = defaultClientConfig()
+	buf := bytes.NewBuffer([]byte{})
+
+	opts := &TopNodeOptions{
+		HeapsterOptions: HeapsterTopOptions{
+			Namespace: "custom-namespace",
+			Scheme:    "https",
+			Service:   "custom-heapster-service",
+		},
+	}
+	cmd := NewCmdTopNode(f, opts, buf)
 	cmd.Run(cmd, []string{})
 
 	// Check the presence of node names in the output.
@@ -116,7 +169,7 @@ func TestTopNodeWithNameMetrics(t *testing.T) {
 	tf.ClientConfig = defaultClientConfig()
 	buf := bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdTopNode(f, buf)
+	cmd := NewCmdTopNode(f, nil, buf)
 	cmd.Run(cmd, []string{expectedMetrics.Name})
 
 	// Check the presence of node names in the output.
@@ -176,7 +229,7 @@ func TestTopNodeWithLabelSelectorMetrics(t *testing.T) {
 	tf.ClientConfig = defaultClientConfig()
 	buf := bytes.NewBuffer([]byte{})
 
-	cmd := NewCmdTopNode(f, buf)
+	cmd := NewCmdTopNode(f, nil, buf)
 	cmd.Flags().Set("selector", label)
 	cmd.Run(cmd, []string{})
 

pkg/kubectl/cmd/top_pod.go

@@ -72,8 +72,10 @@ var (
 		kubectl top pod -l name=myLabel`))
 )
 
-func NewCmdTopPod(f cmdutil.Factory, out io.Writer) *cobra.Command {
+func NewCmdTopPod(f cmdutil.Factory, options *TopPodOptions, out io.Writer) *cobra.Command {
-	options := &TopPodOptions{}
+	if options == nil {
+		options = &TopPodOptions{}
+	}
 
 	cmd := &cobra.Command{
 		Use:     "pod [NAME | -l label]",

pkg/kubectl/cmd/top_pod_test.go

@@ -139,7 +139,150 @@ func TestTopPod(t *testing.T) {
 		tf.ClientConfig = defaultClientConfig()
 		buf := bytes.NewBuffer([]byte{})
 
-		cmd := NewCmdTopPod(f, buf)
+		cmd := NewCmdTopPod(f, nil, buf)
+		for name, value := range testCase.flags {
+			cmd.Flags().Set(name, value)
+		}
+		cmd.Run(cmd, testCase.args)
+
+		// Check the presence of pod names&namespaces/container names in the output.
+		result := buf.String()
+		if testCase.containers {
+			for _, containerName := range expectedContainerNames {
+				if !strings.Contains(result, containerName) {
+					t.Errorf("%s: missing metrics for container %s: \n%s", testCase.name, containerName, result)
+				}
+			}
+		}
+		for _, m := range expectedMetrics {
+			if !strings.Contains(result, m.Name) {
+				t.Errorf("%s: missing metrics for %s: \n%s", testCase.name, m.Name, result)
+			}
+			if testCase.listsNamespaces && !strings.Contains(result, m.Namespace) {
+				t.Errorf("%s: missing metrics for %s/%s: \n%s", testCase.name, m.Namespace, m.Name, result)
+			}
+		}
+		for _, name := range nonExpectedMetricsNames {
+			if strings.Contains(result, name) {
+				t.Errorf("%s: unexpected metrics for %s: \n%s", testCase.name, name, result)
+			}
+		}
+	}
+}
+
+func TestTopPodCustomDefaults(t *testing.T) {
+	customBaseHeapsterServiceAddress := "/api/v1/namespaces/custom-namespace/services/https:custom-heapster-service:/proxy"
+	customBaseMetricsAddress := customBaseHeapsterServiceAddress + "/apis/metrics"
+	customTopPathPrefix := customBaseMetricsAddress + "/" + metricsApiVersion
+
+	testNS := "custom-namespace"
+	testCases := []struct {
+		name            string
+		namespace       string
+		flags           map[string]string
+		args            []string
+		expectedPath    string
+		expectedQuery   string
+		namespaces      []string
+		containers      bool
+		listsNamespaces bool
+	}{
+		{
+			name:            "all namespaces",
+			flags:           map[string]string{"all-namespaces": "true"},
+			expectedPath:    customTopPathPrefix + "/pods",
+			namespaces:      []string{testNS, "secondtestns", "thirdtestns"},
+			listsNamespaces: true,
+		},
+		{
+			name:         "all in namespace",
+			expectedPath: customTopPathPrefix + "/namespaces/" + testNS + "/pods",
+			namespaces:   []string{testNS, testNS},
+		},
+		{
+			name:         "pod with name",
+			args:         []string{"pod1"},
+			expectedPath: customTopPathPrefix + "/namespaces/" + testNS + "/pods/pod1",
+			namespaces:   []string{testNS},
+		},
+		{
+			name:          "pod with label selector",
+			flags:         map[string]string{"selector": "key=value"},
+			expectedPath:  customTopPathPrefix + "/namespaces/" + testNS + "/pods",
+			expectedQuery: "labelSelector=" + url.QueryEscape("key=value"),
+			namespaces:    []string{testNS, testNS},
+		},
+		{
+			name:         "pod with container metrics",
+			flags:        map[string]string{"containers": "true"},
+			args:         []string{"pod1"},
+			expectedPath: customTopPathPrefix + "/namespaces/" + testNS + "/pods/pod1",
+			namespaces:   []string{testNS},
+			containers:   true,
+		},
+	}
+	initTestErrorHandler(t)
+	for _, testCase := range testCases {
+		t.Logf("Running test case: %s", testCase.name)
+		metricsList := testPodMetricsData()
+		var expectedMetrics []metricsapi.PodMetrics
+		var expectedContainerNames, nonExpectedMetricsNames []string
+		for n, m := range metricsList {
+			if n < len(testCase.namespaces) {
+				m.Namespace = testCase.namespaces[n]
+				expectedMetrics = append(expectedMetrics, m)
+				for _, c := range m.Containers {
+					expectedContainerNames = append(expectedContainerNames, c.Name)
+				}
+			} else {
+				nonExpectedMetricsNames = append(nonExpectedMetricsNames, m.Name)
+			}
+		}
+
+		var response interface{}
+		if len(expectedMetrics) == 1 {
+			response = expectedMetrics[0]
+		} else {
+			response = metricsapi.PodMetricsList{
+				ListMeta: metav1.ListMeta{
+					ResourceVersion: "2",
+				},
+				Items: expectedMetrics,
+			}
+		}
+
+		f, tf, _, ns := cmdtesting.NewAPIFactory()
+		tf.Printer = &testPrinter{}
+		tf.Client = &fake.RESTClient{
+			APIRegistry:          api.Registry,
+			NegotiatedSerializer: ns,
+			Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+				switch p, m, q := req.URL.Path, req.Method, req.URL.RawQuery; {
+				case p == testCase.expectedPath && m == "GET" && (testCase.expectedQuery == "" || q == testCase.expectedQuery):
+					body, err := marshallBody(response)
+					if err != nil {
+						t.Errorf("%s: unexpected error: %v", testCase.name, err)
+					}
+					return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil
+				default:
+					t.Fatalf("%s: unexpected request: %#v\nGot URL: %#v\nExpected path: %#v\nExpected query: %#v",
+						testCase.name, req, req.URL, testCase.expectedPath, testCase.expectedQuery)
+					return nil, nil
+				}
+			}),
+		}
+		tf.Namespace = testNS
+		tf.ClientConfig = defaultClientConfig()
+		buf := bytes.NewBuffer([]byte{})
+
+		opts := &TopPodOptions{
+			HeapsterOptions: HeapsterTopOptions{
+				Namespace: "custom-namespace",
+				Scheme:    "https",
+				Service:   "custom-heapster-service",
+			},
+		}
+		cmd := NewCmdTopPod(f, opts, buf)
 		for name, value := range testCase.flags {
 			cmd.Flags().Set(name, value)
 		}