Send istioctl CRUD requests through Kubernetes apiserver proxy by ayj · Pull Request #669 · istio/old_pilot_repo

ayj · 2017-05-05T04:41:28Z

Use k8s.io/go-client libraries to send Istio API requests through
Kubernetes API server proxy. The advantages of this aproach are the
following:

User does not need to set ISTIO_MANAGER_ADDRESS.
User does not need to run kubectl port-forward in the background
Manager API server is accessed through k8s service instead of a
specifc endpoint instance/pod.

See https://kubernetes.io/docs/concepts/cluster-administration/access-cluster/#discovering-builtin-services for Kubernetes documentation on this method.

Use k8s.io/go-client libraries to send Istio API requests through Kubernetes API server proxy. The advantages of this aproach are the following: * User does not need to set ISTIO_MANAGER_ADDRESS. * User does not need to run `kubectl port-forward` in the background * Manager API server is accessed through k8s service instead of a specifc endpoint instance/pod. See https://kubernetes.io/docs/concepts/cluster-administration/access-cluster/#discovering-builtin-services for Kubernetes documentation on this method.

rshriram

@ayj overall, this is a really cool solution. I like it a lot. However, I am concerned that we are specializing the cli completely for k8s (in which case, it should be called istioctl-k8s), when we just managed to decouple the CLI from k8s almost completely (with manager/mixer API servers).

Having a specialized k8s switch will allow us to keep the tool operational in the magic mode (the one you have) and the old mode (current/explicit mode). The explicit mode will be super useful for troubleshooting.

cc @esnible @liamawhite
(please do keep the curl commands around)

rshriram · 2017-05-05T04:49:20Z

-}
-
-func (m *ManagerClient) toCurl(request *http.Request, body string) string {
-


I think @esnible added this to print the debug output/curl commands, so that users can try the API calls themselves.

From #645 - istio.io/manager/client/proxy/ManagerClient should log HTTP, similar to what the Kubernetes clients do.

This change uses the Kubernetes client so the logging behavior is exactly like what other Kubernetes clients do, e.g.

$ istioctl -v=999 create -f route-rule-delay.yaml I0505 09:35:37.315569 132334 loader.go:354] Config loaded from file /usr/local/google/home/jasonyoung/.kube/config I0505 09:35:37.316597 132334 round_trippers.go:398] curl -k -v -XGET -H "Accept: application/json, */*" -H "User-Agent: istioctl/v0.0.0 (linux/amd64) kubernetes/$Format" -H "Authorization: Basic YWRtaW46WXdRcEVvRzVLV2tzZ1hueQ==" https://35.184.13.238/apis/extensions/v1beta1/thirdpartyresources/istio-config.istio.io I0505 09:35:37.544263 132334 round_trippers.go:417] GET https://35.184.13.238/apis/extensions/v1beta1/thirdpartyresources/istio-config.istio.io 200 OK in 227 milliseconds I0505 09:35:37.544285 132334 round_trippers.go:423] Response Headers: I0505 09:35:37.544298 132334 round_trippers.go:426] Content-Type: application/json I0505 09:35:37.544306 132334 round_trippers.go:426] Content-Length: 369 I0505 09:35:37.544315 132334 round_trippers.go:426] Date: Fri, 05 May 2017 16:35:37 GMT I0505 09:35:37.544345 132334 request.go:991] Response Body: {"kind":"ThirdPartyResource","apiVersion":"extensions/v1beta1","metadata":{"name":"istio-config.istio.io","selfLink":"/apis/extensions/v1beta1/thirdpartyresourcesistio-config.istio.io","uid":"21f2d9e2-2f6c-11e7-a278-42010a80017c","resourceVersion":"1471","creationTimestamp":"2017-05-02T19:18:33Z"},"description":"Istio configuration","versions":[{"name":"v1alpha1"}]} I0505 09:35:37.544539 132334 client.go:173] Resource already exists: "istio-config.istio.io" I0505 09:35:37.544557 132334 client.go:197] Checking for TPR resources I0505 09:35:37.544607 132334 round_trippers.go:398] curl -k -v -XGET -H "Accept: application/json, */*" -H "Authorization: Basic YWRtaW46WXdRcEVvRzVLV2tzZ1hueQ==" https://35.184.13.238/apis/istio.io/v1alpha1/istioconfigs I0505 09:35:37.584012 132334 round_trippers.go:417] GET https://35.184.13.238/apis/istio.io/v1alpha1/istioconfigs 200 OK in 39 milliseconds I0505 09:35:37.584032 132334 round_trippers.go:423] Response Headers: I0505 09:35:37.584039 132334 round_trippers.go:426] Content-Length: 2013 I0505 09:35:37.584044 132334 round_trippers.go:426] Date: Fri, 05 May 2017 16:35:37 GMT I0505 09:35:37.584049 132334 round_trippers.go:426] Content-Type: application/json I0505 09:35:37.584115 132334 request.go:991] Response Body: {"kind":"IstioConfigList","items":[{"apiVersion":"istio.io/v1alpha1","kind":"IstioConfig","metadata":{"name":"route-rule-details-default","namespace":"default","selfLink":"/apis/istio.io/v1alpha1/namespaces/default/istioconfigs/route-rule-details-default","uid":"d80cdbdd-3136-11e7-a278-42010a80017c","resourceVersion":"424970","creationTimestamp":"2017-05-05T02:02:08Z"},"spec":{"destination":"details.default.svc.cluster.local","precedence":1,"route":[{"tags":{"version":"v1"}}]}},{"apiVersion":"istio.io/v1alpha1","kind":"IstioConfig","metadata":{"name":"route-rule-productpage-default","namespace":"default","selfLink":"/apis/istio.io/v1alpha1/namespaces/default/istioconfigs/route-rule-productpage-default","uid":"d7f7929b-3136-11e7-a278-42010a80017c","resourceVersion":"424967","creationTimestamp":"2017-05-05T02:02:07Z"},"spec":{"destination":"productpage.default.svc.cluster.local","precedence":1,"route":[{"tags":{"version":"v1"}}]}},{"apiVersion":"istio.io/v1alpha1","kind":"IstioConfig","metadata":{"name":"route-rule-ratings-default","namespace":"default","selfLink":"/apis/istio.io/v1alpha1/namespaces/default/istioconfigs/route-rule-ratings-default","uid":"d805db3d-3136-11e7-a278-42010a80017c","resourceVersion":"424969","creationTimestamp":"2017-05-05T02:02:08Z"},"spec":{"destination":"ratings.default.svc.cluster.local","precedence":1,"route":[{"tags":{"version":"v1"}}]}},{"apiVersion":"istio.io/v1alpha1","kind":"IstioConfig","metadata":{"name":"route-rule-reviews-default","namespace":"default","selfLink":"/apis/istio.io/v1alpha1/namespaces/default/istioconfigs/route-rule-reviews-default","uid":"d43208f5-313a-11e7-a278-42010a80017c","resourceVersion":"427446","creationTimestamp":"2017-05-05T02:30:39Z"},"spec":{"destination":"reviews.default.svc.cluster.local","precedence":1,"route":[{"tags":{"version":"v2"},"weight":50},{"tags":{"version":"v3"},"weight":50}]}}],"metadata":{"selfLink":"/apis/istio.io/v1alpha1/istioconfigs","resourceVersion":"501380"},"apiVersion":"istio.io/v1alpha1"} I0505 09:35:37.584653 132334 decoder.go:224] decoding stream as YAML I0505 09:35:37.585543 132334 request.go:991] Request Body: {"type":"route-rule","name":"ratings-test-delay","spec":{"destination":"ratings.default.svc.cluster.local","httpFault":{"delay":{"fixedDelay":"7s","percent":100}},"match":{"httpHeaders":{"Cookie":{"regex":"^(.*?;)?(user=test-user)(;.*)?$"}}},"precedence":2,"route":[{"tags":{"version":"v1"}}]}} I0505 09:35:37.585614 132334 round_trippers.go:398] curl -k -v -XPOST -H "Accept: application/json, */*" -H "Content-Type: application/json" -H "Authorization: Basic YWRtaW46WXdRcEVvRzVLV2tzZ1hueQ==" https://35.184.13.238/api/v1/namespaces/default/services/istio-manager:8081/proxy/v1alpha1/config/route-rule/default/ratings-test-delay I0505 09:35:37.635058 132334 round_trippers.go:417] POST https://35.184.13.238/api/v1/namespaces/default/services/istio-manager:8081/proxy/v1alpha1/config/route-rule/default/ratings-test-delay 201 Created in 49 milliseconds I0505 09:35:37.635480 132334 round_trippers.go:423] Response Headers: I0505 09:35:37.635807 132334 round_trippers.go:426] Content-Type: application/json I0505 09:35:37.636078 132334 round_trippers.go:426] Date: Fri, 05 May 2017 16:35:37 GMT I0505 09:35:37.636509 132334 round_trippers.go:426] Content-Length: 436 I0505 09:35:37.636834 132334 request.go:991] Response Body: { "type": "route-rule", "name": "ratings-test-delay", "spec": { "destination": "ratings.default.svc.cluster.local", "httpFault": { "delay": { "fixedDelay": "7s", "percent": 100 } }, "match": { "httpHeaders": { "Cookie": { "regex": "^(.*?;)?(user=test-user)(;.*)?$" } } }, "precedence": 2, "route": [ { "tags": { "version": "v1" } } ] } } Created config: route-rule ratings-test-delay

Oh nice. Then the extra curl kludge is unnecessary for k8s, but needed in generic mode.

rshriram · 2017-05-05T04:55:33Z

+		"Namespace where Istio system resides")
+	rootCmd.PersistentFlags().IntVar(&apiServerPort, "port", 8081,
+		"Manager API server port")



While using the kube api client is a fine idea, i think the istioctl tool is going back and forth between platform agnostic mode and becoming platform specific. With the exception of kubeconfig and the namespace/port stuff, one could technically use istioctl against any manager/mixer deployment (kube/vm/mesos). In that spirit, my suggestion would be to keep the existing infrastructure (or shuffle it), such that one can do
istioctl --kube -n foo --istioNamespace bar --...

The added advantage of this model is that I can always fallback to the current one (directly calling manager/mixer API servers)

I agree. Seems like a sensible idea would be to have a generic client for fallback as well as a kube client. If we go with a straight kube client now then we're going to have to rewrite a generic client at a later date to achieve platform agnosticism anyway.

I agree that we should decouple from k8s before it gets even harder to accomplish down the road. That means no k8s dependencies in base istioctl, and the only functional dependency being authentication via JWT token to kube API server proxy. If this is too hard, I'd prefer creating our own JWT validation and using a load balancer IP for the manager config server.

I don't think there is any disagreement that istioctl should be platform neutral long-term. But I wanted to minimize changes to istioctl and the rest of the Istio install prior to alpha freeze. In particular, accessing kube API server via JWT tokens still requires k8s specific calls to get the token, certs, and API server address vs. using the existing k8s client to do that all for us.

I added the --kube flag to revert to the legacy behavior. Advanced users will need to change the --managerAPIService to localhost: and run kubectl port-forward themselves as before.

iostioctl --kube=false --managerAPIService=http://localhost:8081 create -f route-rule-reviews-v2-v3.yaml

The default behavior is --kube=true.

iostioctl create -f route-rule-reviews-v2-v3.yaml

rshriram · 2017-05-05T04:57:54Z

+// (see https://kubernetes.io/docs/concepts/cluster-administration/access-cluster/#discovering-builtin-services)
+func (r *k8sRESTRequester) absPath(string path) string {
+	return fmt.Sprintf("api/v1/namespaces/%s/services/istio-manager:%d/proxy/%s/%s",
+		r.namespace, r.port, r.version, path)


isn't this hard coding the istio-manager service name? Users certainly can rename this service to something else and our system will still work.

Good point. I generalized apiServerPort to managerAPIService which includes service name and port, e.g. istio-manager:8081

…uests-through-k8s-apiserver-proxy

rshriram

I am fine with this. Will leave it to @kyessenov / @liamawhite / @Enible for code comments

andraxylia · 2017-05-05T23:01:09Z

Will there be any changes on how kube-inject is invoked? If yes, we need to update the documentation.

kyessenov · 2017-05-05T23:06:43Z

+	rootCmd.PersistentFlags().StringVar(&istioNamespace, "istioNamespace", defaultIstioNamespace,
+		"Namespace where Istio system resides")
+	rootCmd.PersistentFlags().StringVar(&istioManagerService, "managerAPIService", "istio-manager:8081",
+		"Name of istio-manager service")


Address, not just name

The meaning of this flag is different based on --kube. Per https://kubernetes.io/docs/concepts/cluster-administration/access-cluster/#discovering-builtin-services it is the service name + port. When --kube=false it is the server address. I updated the flag doc to reflect this.

kyessenov · 2017-05-05T23:08:37Z

+	absPath := fmt.Sprintf("api/v1/namespaces/%s/services/%s/proxy/%s/%s",
+		namespace, service, IstioResourceVersion, path)
+	var status int
+	outBody, err := cl.dyn.Verb(method).


is dyn the right HTTP client? it's configured to use istio TPRs which is not necessary for this client? maybe use cl.client?

dyn/rest.RESTClient has the HTTP verb methods. cl.client/kubernetes.Clientset doesn't. I could have created an additional RESTClient with Group/Version for k8s.io/v1 just for this purpose, but setting the absolute path seemed to work just as well. We should probably refactor the k8s platform client anyway and parameterize Group/Version since we don't need TPR access in istioctl anymore with proper API server. Then we could replace AbsPath() here with Suffix().

…uests-through-k8s-apiserver-proxy

istio-testing · 2017-05-06T06:10:44Z

Jenkins job manager/presubmit passed

…uests-through-k8s-apiserver-proxy

liamawhite

LGTM. Just make sure there is adequate logging in there so we can diagnose easier.

istio-testing · 2017-05-08T00:41:25Z

Jenkins job manager/presubmit passed

istio-testing · 2017-05-08T00:47:38Z

Jenkins job manager/presubmit passed

istio-testing · 2017-05-08T03:26:22Z

Jenkins job manager/presubmit passed

ayj · 2017-05-08T05:04:47Z

jenkins rebuild manager/e2e-smoketest

ayj · 2017-05-08T15:49:58Z

jenkins rebuild manager/e2e-smoketest

istio-testing · 2017-05-08T16:00:13Z

Jenkins job manager/e2e-smoketest passed

* Removes kubectl port-forward dependency for accessing manager/mixer API/config servers. istio/old_pilot_repo#669 adds support to istioctl for accessing manager/mixer API via kubernetes apiserver proxy. kubectl port-forward is no longer needed. * Use --istioNamespace with istioctl commands. istioctl now differentiates the namespace where istio is installed vs. the namespace that is being managed by istio. These are currently the same in the e2e smoke test. * Includes fix for "'panic: shorthand redefinition' for istioctl mixer subcommand"

This is necessary to access the Manager API server via Kubernetes service instead of through pod with `kubectl port-forward` (see istio/old_pilot_repo#669 for corresponding istioctl changes) Former-commit-id: 851e9cb

* Removes `kubectl port-forward` dependency for accessing manager/mixer API/config servers. istio/old_pilot_repo#669 adds support to istioctl for accessing manager/mixer API via kubernetes apiserver proxy. `kubectl port-forward` is no longer needed. * Use --istioNamespace with istioctl commands. istioctl now differentiates the namespace where istio is installed vs. the namespace that is being managed by istio. These are currently the same in the e2e smoke test. * Includes fix for "'panic: shorthand redefinition' for istioctl mixer subcommand" Former-commit-id: 60b4510

* Removes kubectl port-forward dependency for accessing manager/mixer API/config servers. istio/old_pilot_repo#669 adds support to istioctl for accessing manager/mixer API via kubernetes apiserver proxy. kubectl port-forward is no longer needed. * Use --istioNamespace with istioctl commands. istioctl now differentiates the namespace where istio is installed vs. the namespace that is being managed by istio. These are currently the same in the e2e smoke test. * Includes fix for "'panic: shorthand redefinition' for istioctl mixer subcommand" Former-commit-id: 38f44f9

This is necessary to access the Manager API server via Kubernetes service instead of through pod with `kubectl port-forward` (see istio/old_pilot_repo#669 for corresponding istioctl changes) Former-commit-id: 851e9cb

* Removes `kubectl port-forward` dependency for accessing manager/mixer API/config servers. istio/old_pilot_repo#669 adds support to istioctl for accessing manager/mixer API via kubernetes apiserver proxy. `kubectl port-forward` is no longer needed. * Use --istioNamespace with istioctl commands. istioctl now differentiates the namespace where istio is installed vs. the namespace that is being managed by istio. These are currently the same in the e2e smoke test. * Includes fix for "'panic: shorthand redefinition' for istioctl mixer subcommand" Former-commit-id: 60b4510

* Removes kubectl port-forward dependency for accessing manager/mixer API/config servers. istio/old_pilot_repo#669 adds support to istioctl for accessing manager/mixer API via kubernetes apiserver proxy. kubectl port-forward is no longer needed. * Use --istioNamespace with istioctl commands. istioctl now differentiates the namespace where istio is installed vs. the namespace that is being managed by istio. These are currently the same in the e2e smoke test. * Includes fix for "'panic: shorthand redefinition' for istioctl mixer subcommand" Former-commit-id: 38f44f9

rshriram reviewed May 5, 2017

View reviewed changes

ldemailly mentioned this pull request May 5, 2017

istioctl kube-inject does not need ISTIO_MANAGER_ADDRESS to be set #637

Closed

ayj added 4 commits May 5, 2017 13:23

add --kube flag

f8eb77e

Merge branch 'master' of github.com:istio/manager into send-istio-req…

edb1ede

…uests-through-k8s-apiserver-proxy

add toCurl back to BasicHTTPRequester

fa4b515

add comment to exported function

ff40413

rshriram approved these changes May 5, 2017

View reviewed changes

andraxylia assigned ayj May 5, 2017

andraxylia added install UX (User) labels May 5, 2017

andraxylia modified the milestones: manager beta, manager alpha May 5, 2017

kyessenov reviewed May 5, 2017

View reviewed changes

ldemailly mentioned this pull request May 5, 2017

BookInfo Sample docs need update re: ISTIO_MANAGER_ADDRESS istio/istio.io#91

Closed

andraxylia mentioned this pull request May 6, 2017

cannot execute istioctl, 404 from istio manager endpoint istio/istio#199

Closed

ayj added 2 commits May 5, 2017 22:39

fix linter error and clarify --kube flag usage

8926547

Merge branch 'master' of github.com:istio/manager into send-istio-req…

c5aa9a0

…uests-through-k8s-apiserver-proxy

kyessenov approved these changes May 6, 2017

View reviewed changes

ayj mentioned this pull request May 6, 2017

Add http-apiserver port 8081 to istio-manager service istio/istio#202

Merged

ayj added 2 commits May 7, 2017 00:17

make istioctl mixer subcommands use --kube option

984ed3d

Merge branch 'master' of github.com:istio/manager into send-istio-req…

222ae90

…uests-through-k8s-apiserver-proxy

liamawhite approved these changes May 7, 2017

View reviewed changes

ayj added 2 commits May 7, 2017 16:30

fix linter errors

97d9c15

fix //proxy/client:go_default_test

df4b2a2

ayj merged commit dbcc933 into istio:master May 8, 2017

ayj deleted the send-istio-requests-through-k8s-apiserver-proxy branch May 8, 2017 16:01

ayj mentioned this pull request May 8, 2017

Update version and smoke test to use latest istioctl version istio/istio#205

Merged

		}

		func (m ManagerClient) toCurl(request http.Request, body string) string {

Conversation

ayj commented May 5, 2017

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamawhite May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

andraxylia commented May 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented May 6, 2017

Uh oh!

liamawhite left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented May 8, 2017

Uh oh!

istio-testing commented May 8, 2017

Uh oh!

istio-testing commented May 8, 2017

Uh oh!

ayj commented May 8, 2017

Uh oh!

ayj commented May 8, 2017

Uh oh!

istio-testing commented May 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liamawhite May 5, 2017 •

edited

Loading