From 0ff1bb4ad8f7755773d90198bd14f5350a8fbe26 Mon Sep 17 00:00:00 2001 From: Risha Mars Date: Wed, 20 Jun 2018 12:59:31 -0700 Subject: [PATCH] Don't allow stat requests for named resources in --all-namespaces (#1163) Don't allow the CLI or Web UI to request named resources if --all-namespaces is used. This follows kubectl, which also does not allow requesting named resources over all namespaces. This PR also updates the Web API's behaviour to be in line with the CLI's. Both will now default to the default namespace if no namespace is specified. --- cli/cmd/stat.go | 13 +++---------- cli/cmd/stat_test.go | 12 ++++++++++++ controller/api/util/api_utils.go | 19 ++++++++++++++++--- web/app/js/components/util/ApiHelpers.jsx | 2 +- web/app/js/components/util/MetricUtils.js | 3 +++ web/app/test/ApiHelpersTest.jsx | 4 ++-- web/srv/api_handlers.go | 5 +++++ 7 files changed, 42 insertions(+), 16 deletions(-) diff --git a/cli/cmd/stat.go b/cli/cmd/stat.go index 61254f069..158abc9f7 100644 --- a/cli/cmd/stat.go +++ b/cli/cmd/stat.go @@ -15,7 +15,6 @@ import ( "github.com/runconduit/conduit/pkg/k8s" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" - "k8s.io/api/core/v1" ) type statOptions struct { @@ -309,14 +308,7 @@ func getNamePrefix(resourceType string) string { } func buildStatSummaryRequest(resource []string, options *statOptions) (*pb.StatSummaryRequest, error) { - targetNamespace := options.namespace - if options.allNamespaces { - targetNamespace = "" - } else if options.namespace == "" { - targetNamespace = v1.NamespaceDefault - } - - target, err := util.BuildResource(targetNamespace, resource...) + target, err := util.BuildResource(options.namespace, resource...) if err != nil { return nil, err } @@ -339,13 +331,14 @@ func buildStatSummaryRequest(resource []string, options *statOptions) (*pb.StatS TimeWindow: options.timeWindow, ResourceName: target.Name, ResourceType: target.Type, - Namespace: targetNamespace, + Namespace: options.namespace, ToName: toRes.Name, ToType: toRes.Type, ToNamespace: options.toNamespace, FromName: fromRes.Name, FromType: fromRes.Type, FromNamespace: options.fromNamespace, + AllNamespaces: options.allNamespaces, } return util.BuildStatSummaryRequest(requestParams) diff --git a/cli/cmd/stat_test.go b/cli/cmd/stat_test.go index ee8e3aed0..ca066fad1 100644 --- a/cli/cmd/stat_test.go +++ b/cli/cmd/stat_test.go @@ -34,4 +34,16 @@ emoji 1/2 100.00% 2.0rps 123ms 123ms 123ms t.Fatalf("Wrong output:\n expected: \n%s\n, got: \n%s", expectedOutput, output) } }) + + t.Run("Returns an error for named resource queries with the --all-namespaces flag", func(t *testing.T) { + options := newStatOptions() + options.allNamespaces = true + args := []string{"po", "web"} + expectedError := "stats for a resource cannot be retrieved by name across all namespaces" + + _, err := buildStatSummaryRequest(args, options) + if err == nil || err.Error() != expectedError { + t.Fatalf("Expected error [%s] instead got [%s]", expectedError, err) + } + }) } diff --git a/controller/api/util/api_utils.go b/controller/api/util/api_utils.go index 1f19a1f4f..7cfc8aa84 100644 --- a/controller/api/util/api_utils.go +++ b/controller/api/util/api_utils.go @@ -9,6 +9,7 @@ import ( "github.com/runconduit/conduit/pkg/k8s" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -54,6 +55,7 @@ type StatSummaryRequestParams struct { FromNamespace string FromType string FromName string + AllNamespaces bool } // GRPCError generates a gRPC error code, as defined in @@ -98,6 +100,17 @@ func BuildStatSummaryRequest(p StatSummaryRequestParams) (*pb.StatSummaryRequest window = p.TimeWindow } + if p.AllNamespaces && p.ResourceName != "" { + return nil, errors.New("stats for a resource cannot be retrieved by name across all namespaces") + } + + targetNamespace := p.Namespace + if p.AllNamespaces { + targetNamespace = "" + } else if p.Namespace == "" { + targetNamespace = v1.NamespaceDefault + } + resourceType, err := k8s.CanonicalKubernetesNameFromFriendlyName(p.ResourceType) if err != nil { return nil, err @@ -106,7 +119,7 @@ func BuildStatSummaryRequest(p StatSummaryRequestParams) (*pb.StatSummaryRequest statRequest := &pb.StatSummaryRequest{ Selector: &pb.ResourceSelection{ Resource: &pb.Resource{ - Namespace: p.Namespace, + Namespace: targetNamespace, Name: p.ResourceName, Type: resourceType, }, @@ -116,7 +129,7 @@ func BuildStatSummaryRequest(p StatSummaryRequestParams) (*pb.StatSummaryRequest if p.ToName != "" || p.ToType != "" || p.ToNamespace != "" { if p.ToNamespace == "" { - p.ToNamespace = p.Namespace + p.ToNamespace = targetNamespace } if p.ToType == "" { p.ToType = resourceType @@ -139,7 +152,7 @@ func BuildStatSummaryRequest(p StatSummaryRequestParams) (*pb.StatSummaryRequest if p.FromName != "" || p.FromType != "" || p.FromNamespace != "" { if p.FromNamespace == "" { - p.FromNamespace = p.Namespace + p.FromNamespace = targetNamespace } if p.FromType == "" { p.FromType = resourceType diff --git a/web/app/js/components/util/ApiHelpers.jsx b/web/app/js/components/util/ApiHelpers.jsx index f67b0732a..259b1b69b 100644 --- a/web/app/js/components/util/ApiHelpers.jsx +++ b/web/app/js/components/util/ApiHelpers.jsx @@ -85,7 +85,7 @@ const ApiHelpers = (pathPrefix, defaultMetricsWindow = '1m') => { } let baseUrl = '/api/tps-reports?resource_type=' + type; - return !namespace ? baseUrl : baseUrl + '&namespace=' + namespace; + return !namespace ? baseUrl + '&all_namespaces=true' : baseUrl + '&namespace=' + namespace; }; // maintain a list of a component's requests, diff --git a/web/app/js/components/util/MetricUtils.js b/web/app/js/components/util/MetricUtils.js index deffbe85e..1f50fb448 100644 --- a/web/app/js/components/util/MetricUtils.js +++ b/web/app/js/components/util/MetricUtils.js @@ -131,6 +131,9 @@ export const processSingleResourceRollup = rawMetrics => { if (_.size(result) > 1) { console.error("Multi metric returned; expected single metric."); } + if (_.isEmpty(result)) { + return []; + } return _.values(result)[0]; }; diff --git a/web/app/test/ApiHelpersTest.jsx b/web/app/test/ApiHelpersTest.jsx index 19dae87a6..7d6748542 100644 --- a/web/app/test/ApiHelpersTest.jsx +++ b/web/app/test/ApiHelpersTest.jsx @@ -285,13 +285,13 @@ describe('ApiHelpers', () => { it('returns the correct rollup url for deployment overviews', () => { api = ApiHelpers('/go/my/own/way'); let deploymentUrl = api.urlsForResource("deployment"); - expect(deploymentUrl).to.equal('/api/tps-reports?resource_type=deployment'); + expect(deploymentUrl).to.equal('/api/tps-reports?resource_type=deployment&all_namespaces=true'); }); it('returns the correct rollup url for pod overviews', () => { api = ApiHelpers('/go/my/own/way'); let deploymentUrls = api.urlsForResource("pod"); - expect(deploymentUrls).to.equal('/api/tps-reports?resource_type=pod'); + expect(deploymentUrls).to.equal('/api/tps-reports?resource_type=pod&all_namespaces=true'); }); it('scopes the query to the provided namespace', () => { diff --git a/web/srv/api_handlers.go b/web/srv/api_handlers.go index 35d735523..a4202b127 100644 --- a/web/srv/api_handlers.go +++ b/web/srv/api_handlers.go @@ -70,6 +70,10 @@ func (h *handler) handleApiPods(w http.ResponseWriter, req *http.Request, p http } func (h *handler) handleApiStat(w http.ResponseWriter, req *http.Request, p httprouter.Params) { + allNs := false + if req.FormValue("all_namespaces") == "true" { + allNs = true + } requestParams := util.StatSummaryRequestParams{ TimeWindow: req.FormValue("window"), ResourceName: req.FormValue("resource_name"), @@ -81,6 +85,7 @@ func (h *handler) handleApiStat(w http.ResponseWriter, req *http.Request, p http FromName: req.FormValue("from_name"), FromType: req.FormValue("from_type"), FromNamespace: req.FormValue("from_namespace"), + AllNamespaces: allNs, } // default to returning deployment stats