istioctl describe: fixes, test case, remove deprecated code (#20169)

* istioctl describe: fixes, test case, remove deprecated code * Remove unneeded subtest structure * Remove unnecessary change in describe * Match describe output by regexp, not string * Additional output to test the tests * Keep constructor signature * Fix compilation problem * Look for Envoy config metadata in Istio 1.5.x format * 'make format' * Ran 'gen'
rh-bjeon · Jan 19, 2020 · 926a1bf · 926a1bf
1 parent ca0b966
commit 926a1bf
Show file tree

Hide file tree

Showing 8 changed files with 154 additions and 1,116 deletions.
diff --git a/istioctl/cmd/deprecated_cmd.go b/istioctl/cmd/deprecated_cmd.go
diff --git a/istioctl/cmd/describe.go b/istioctl/cmd/describe.go
@@ -767,12 +767,13 @@ func getIstioVirtualServiceNameForSvc(cd *configdump.Wrapper, svc v1.Service, po
 		}
 	}
 
-	re := regexp.MustCompile("/apis/networking/v1alpha3/namespaces/(?P<namespace>[^/]+)/virtual-service/(?P<name>[^/]+)")
+	// Starting with recent 1.5.0 builds, the path will include .istio.io.  Handle both.
+	re := regexp.MustCompile("/apis/networking(\\.istio\\.io)?/v1alpha3/namespaces/(?P<namespace>[^/]+)/virtual-service/(?P<name>[^/]+)")
 	ss := re.FindStringSubmatch(path)
 	if ss == nil {
 		return "", "", fmt.Errorf("not a VS path: %s", path)
 	}
-	return ss[2], ss[1], nil
+	return ss[3], ss[2], nil
 }
 
 // getIstioVirtualServicePathForSvcFromRoute returns something like "/apis/networking/v1alpha3/namespaces/default/virtual-service/reviews"
@@ -914,12 +915,13 @@ func getIstioDestinationRuleNameForSvc(cd *configdump.Wrapper, svc v1.Service, p
 		return "", "", err
 	}
 
-	re := regexp.MustCompile("/apis/networking/v1alpha3/namespaces/(?P<namespace>[^/]+)/destination-rule/(?P<name>[^/]+)")
+	// Starting with recent 1.5.0 builds, the path will include .istio.io.  Handle both.
+	re := regexp.MustCompile("/apis/networking(\\.istio\\.io)?/v1alpha3/namespaces/(?P<namespace>[^/]+)/destination-rule/(?P<name>[^/]+)")
 	ss := re.FindStringSubmatch(path)
 	if ss == nil {
 		return "", "", fmt.Errorf("not a DR path: %s", path)
 	}
-	return ss[2], ss[1], nil
+	return ss[3], ss[2], nil
 }
 
 // getIstioDestinationRulePathForSvc returns something like "/apis/networking/v1alpha3/namespaces/default/destination-rule/reviews"

diff --git a/istioctl/cmd/istioctl_test.go b/istioctl/cmd/istioctl_test.go
@@ -16,17 +16,13 @@ package cmd
 
 import (
 	"bytes"
-	"fmt"
 	"regexp"
 	"sort"
 	"strings"
 	"testing"
 
-	networking "istio.io/api/networking/v1alpha3"
-	"istio.io/istio/galley/pkg/config/schema/resource"
-
 	"istio.io/istio/galley/pkg/config/schema/collection"
-	"istio.io/istio/galley/pkg/config/schema/collections"
+	"istio.io/istio/galley/pkg/config/schema/resource"
 	"istio.io/istio/pilot/pkg/config/memory"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pilot/test/util"
@@ -50,235 +46,6 @@ type testCase struct {
 	wantException bool
 }
 
-var (
-	testGateways = []model.Config{
-		{
-			ConfigMeta: model.ConfigMeta{
-				Name:      "bookinfo-gateway",
-				Namespace: "default",
-				Type:      collections.IstioNetworkingV1Alpha3Gateways.Resource().Kind(),
-				Group:     collections.IstioNetworkingV1Alpha3Gateways.Resource().Group(),
-				Version:   collections.IstioNetworkingV1Alpha3Gateways.Resource().Version(),
-			},
-			Spec: &networking.Gateway{
-				Selector: map[string]string{"istio": "ingressgateway"},
-				Servers: []*networking.Server{
-					{
-						Port: &networking.Port{
-							Number:   80,
-							Name:     "http",
-							Protocol: "HTTP",
-						},
-						Hosts: []string{"*"},
-					},
-				},
-			},
-		},
-	}
-
-	testVirtualServices = []model.Config{
-		{
-			ConfigMeta: model.ConfigMeta{
-				Name:      "bookinfo",
-				Namespace: "default",
-				Type:      collections.IstioNetworkingV1Alpha3Virtualservices.Resource().Kind(),
-				Group:     collections.IstioNetworkingV1Alpha3Virtualservices.Resource().Group(),
-				Version:   collections.IstioNetworkingV1Alpha3Virtualservices.Resource().Version(),
-			},
-			Spec: &networking.VirtualService{
-				Hosts:    []string{"*"},
-				Gateways: []string{"bookinfo-gateway"},
-				Http: []*networking.HTTPRoute{
-					{
-						Match: []*networking.HTTPMatchRequest{
-							{
-								Uri: &networking.StringMatch{
-									MatchType: &networking.StringMatch_Exact{Exact: "/productpage"},
-								},
-							},
-							{
-								Uri: &networking.StringMatch{
-									MatchType: &networking.StringMatch_Exact{Exact: "/login"},
-								},
-							},
-							{
-								Uri: &networking.StringMatch{
-									MatchType: &networking.StringMatch_Exact{Exact: "/logout"},
-								},
-							},
-							{
-								Uri: &networking.StringMatch{
-									MatchType: &networking.StringMatch_Prefix{Prefix: "/api/v1/products"},
-								},
-							},
-						},
-						Route: []*networking.HTTPRouteDestination{
-							{
-								Destination: &networking.Destination{
-									Host: "productpage",
-									Port: &networking.PortSelector{
-										Number: 80,
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-
-	testDestinationRules = []model.Config{
-		{
-			ConfigMeta: model.ConfigMeta{
-				Name:      "googleapis",
-				Namespace: "default",
-				Type:      collections.IstioNetworkingV1Alpha3Destinationrules.Resource().Kind(),
-				Group:     collections.IstioNetworkingV1Alpha3Destinationrules.Resource().Group(),
-				Version:   collections.IstioNetworkingV1Alpha3Destinationrules.Resource().Version(),
-			},
-			Spec: &networking.DestinationRule{
-				Host: "*.googleapis.com",
-				TrafficPolicy: &networking.TrafficPolicy{
-					Tls: &networking.TLSSettings{
-						Mode: networking.TLSSettings_SIMPLE,
-					},
-				},
-			},
-		},
-	}
-
-	testServiceEntries = []model.Config{
-		{
-			ConfigMeta: model.ConfigMeta{
-				Name:      "googleapis",
-				Namespace: "default",
-				Type:      collections.IstioNetworkingV1Alpha3Serviceentries.Resource().Kind(),
-				Group:     collections.IstioNetworkingV1Alpha3Serviceentries.Resource().Group(),
-				Version:   collections.IstioNetworkingV1Alpha3Serviceentries.Resource().Version(),
-			},
-			Spec: &networking.ServiceEntry{
-				Hosts: []string{"*.googleapis.com"},
-				Ports: []*networking.Port{
-					{
-						Name:     "https",
-						Number:   443,
-						Protocol: "HTTP",
-					},
-				},
-			},
-		},
-	}
-)
-
-func TestGet(t *testing.T) {
-	cases := []testCase{
-		{
-			configs: []model.Config{},
-			args:    strings.Split("get destinationrules", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-No resources found.
-`,
-		},
-		{
-			configs: testGateways,
-			args:    strings.Split("get gateways -n default", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-GATEWAY NAME       HOSTS     NAMESPACE   AGE
-bookinfo-gateway   *         default     0s
-`,
-		},
-		{
-			configs: testVirtualServices,
-			args:    strings.Split("get virtualservices -n default", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-VIRTUAL-SERVICE NAME   GATEWAYS           HOSTS     #HTTP     #TCP      NAMESPACE   AGE
-bookinfo               bookinfo-gateway   *             1        0      default     0s
-`,
-		},
-		{
-			configs: []model.Config{},
-			args:    strings.Split("get all", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-No resources found.
-`,
-		},
-		{
-			configs: testDestinationRules,
-			args:    strings.Split("get destinationrules -n default", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-DESTINATION-RULE NAME   HOST               SUBSETS   NAMESPACE   AGE
-googleapis              *.googleapis.com             default     0s
-`,
-		},
-		{
-			configs: testServiceEntries,
-			args:    strings.Split("get serviceentries -n default", " "),
-			expectedOutput: `Command "get" is deprecated, Use ` + "`kubectl get`" + ` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)
-SERVICE-ENTRY NAME   HOSTS              PORTS      NAMESPACE   AGE
-googleapis           *.googleapis.com   HTTP/443   default     0s
-`,
-		},
-	}
-
-	for i, c := range cases {
-		t.Run(fmt.Sprintf("case %d %s", i, strings.Join(c.args, " ")), func(t *testing.T) {
-			verifyOutput(t, c)
-		})
-	}
-}
-
-func TestCreate(t *testing.T) {
-	cases := []testCase{
-		{ // invalid doesn't provide -f filename
-			configs:        []model.Config{},
-			args:           strings.Split("create virtualservice", " "),
-			expectedRegexp: regexp.MustCompile("^Command \"create\" is deprecated, Use `kubectl create` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)*"), // nolint: lll
-			wantException:  true,
-		},
-	}
-
-	for i, c := range cases {
-		t.Run(fmt.Sprintf("case %d %s", i, strings.Join(c.args, " ")), func(t *testing.T) {
-			verifyOutput(t, c)
-		})
-	}
-}
-
-func TestReplace(t *testing.T) {
-	cases := []testCase{
-		{ // invalid doesn't provide -f
-			configs:        []model.Config{},
-			args:           strings.Split("replace virtualservice", " "),
-			expectedRegexp: regexp.MustCompile("^Command \"replace\" is deprecated, Use `kubectl apply` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)*"), // nolint: lll
-			wantException:  true,
-		},
-	}
-
-	for i, c := range cases {
-		t.Run(fmt.Sprintf("case %d %s", i, strings.Join(c.args, " ")), func(t *testing.T) {
-			verifyOutput(t, c)
-		})
-	}
-}
-
-func TestDelete(t *testing.T) {
-	cases := []testCase{
-		{
-			configs:        []model.Config{},
-			args:           strings.Split("delete all foo", " "),
-			expectedRegexp: regexp.MustCompile("^Command \"delete\" is deprecated, Use `kubectl delete` instead (see https://kubernetes.io/docs/tasks/tools/install-kubectl)*"), // nolint: lll
-			wantException:  true,
-		},
-	}
-
-	for i, c := range cases {
-		t.Run(fmt.Sprintf("case %d %s", i, strings.Join(c.args, " ")), func(t *testing.T) {
-			verifyOutput(t, c)
-		})
-	}
-}
-
 func TestBadParse(t *testing.T) {
 	// unknown flags should be a command parse
 	rootCmd := GetRootCmd([]string{"--unknown-flag"})

diff --git a/istioctl/cmd/root.go b/istioctl/cmd/root.go
@@ -176,13 +176,6 @@ debug and diagnose their Istio mesh.
 		Manual:  "Istio Control",
 	}))
 
-	// Deprecated commands
-	rootCmd.AddCommand(postCmd)
-	rootCmd.AddCommand(putCmd)
-	rootCmd.AddCommand(getCmd)
-	rootCmd.AddCommand(deleteCmd)
-	rootCmd.AddCommand(contextCmd)
-
 	rootCmd.AddCommand(validate.NewValidateCommand(&istioNamespace))
 
 	// BFS apply the flag error function to all subcommands

diff --git a/pkg/test/framework/components/bookinfo/bookinfo.go b/pkg/test/framework/components/bookinfo/bookinfo.go
@@ -27,7 +27,7 @@ type Config struct {
 	Cfg       bookInfoConfig
 }
 
-// DeployOrFail returns a new instance of deployed BookInfo or fails test
+// Deploy returns a new instance of deployed BookInfo
 func Deploy(ctx resource.Context, cfg Config) (i deployment.Instance, err error) {
 	err = resource.UnsupportedEnvironment(ctx.Environment())
 

diff --git a/pkg/test/framework/components/istioctl/istioctl.go b/pkg/test/framework/components/istioctl/istioctl.go
@@ -17,10 +17,12 @@ package istioctl
 import (
 	"testing"
 
+	"istio.io/istio/pkg/test"
 	"istio.io/istio/pkg/test/framework/components/environment"
 	"istio.io/istio/pkg/test/framework/resource"
 )
 
+// Instance represents "istioctl"
 type Instance interface {
 	// Invoke invokes an istioctl command and returns the output and exception.
 	// Cobra commands don't make it easy to separate stdout and stderr and the string parameter
@@ -31,7 +33,7 @@ type Instance interface {
 	InvokeOrFail(t *testing.T, args []string) string
 }
 
-// Structured config for the istioctl component
+// Config is structured config for the istioctl component
 type Config struct {
 	// currently nothing, we might add stuff like OS env settings later
 }
@@ -52,10 +54,16 @@ func New(ctx resource.Context, cfg Config) (i Instance, err error) {
 }
 
 // NewOrFail returns a new instance of "istioctl".
-func NewOrFail(t *testing.T, c resource.Context, config Config) Instance {
+func NewOrFail(_ *testing.T, c resource.Context, config Config) Instance {
+	failer, ok := c.(test.Failer)
+	if !ok {
+		panic("context must be a Failer (typically a framework.TestContext)")
+	}
+
 	i, err := New(c, config)
 	if err != nil {
-		t.Fatalf("istioctl.NewOrFail:: %v", err)
+		failer.Fatalf("istioctl.NewOrFail:: %v", err)
 	}
+
 	return i
 }
diff --git a/tests/integration/istioctl/testdata/a.yaml b/tests/integration/istioctl/testdata/a.yaml
@@ -0,0 +1,33 @@
+apiVersion: networking.istio.io/v1alpha3
+kind: VirtualService
+metadata:
+  name: a
+spec:
+  hosts:
+  - a
+  http:
+  # (This 'match' clause started as a work-around, because WaitUntilCallable() times out if
+  # all destinations have a subset.  We are now using it for testing.)
+  - match:
+    - headers:
+        end-user:
+          exact: jason
+    route:
+    - destination:
+        host: a
+  # Fallthrough
+  - route:
+    - destination:
+        host: a
+        subset: v1
+---
+apiVersion: networking.istio.io/v1alpha3
+kind: DestinationRule
+metadata:
+  name: a
+spec:
+  host: a
+  subsets:
+  - name: v1
+    labels:
+      version: v1