Ability to tap into istio-agent to route debug messages to istiod #31749

esnible · 2021-03-28T16:53:50Z

This is for #22274

This PR causes the sidecar agent to listen on port 15009. (Currently plaintext and open, but that isn't the wish for the final version.)

When a request to :15009/debug/, for example :15009/debug/syncz, sends an XDS request with that TypeURL to Istiod and returns the response from Istiod. I did it this way so I could test with the browser, we may want to switch to require a POST of the actual XDS request.

kubectl exec deployment/httpbin -c istio-proxy -- curl -s localhost:8080/debug/syncz
{
  "versionInfo": "2021-03-31T16:39:37Z/107",
  "resources": [
    {
      "@type": "type.googleapis.com/envoy.service.status.v3.ClientConfig",
      "node": {
        "id": "httpbin-86446ddffb-qz4fb.default"
      },
      "xdsConfig": [
        {
          "status": "SYNCED",
          "listenerConfig": {

          }
...

@costinm @howardjohn @therealmitchconnors Does this match your expectations in terms of the direction?

istio-testing · 2021-03-28T16:54:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

esnible · 2021-03-28T17:09:29Z

/test all

esnible · 2021-03-29T10:28:10Z

/retest

howardjohn

At a high level this looks like the right approach. Nice work!

howardjohn · 2021-03-29T18:25:35Z

pkg/istio-agent/xds_proxy.go

@@ -464,7 +473,11 @@ func (p *XdsProxy) handleUpstreamResponse(con *ProxyConnection) {
 					forwardToEnvoy(con, resp)
 				}
 			default:
-				forwardToEnvoy(con, resp)
+				if strings.HasPrefix(resp.TypeUrl, "istio.io/debug/") {


I wonder if there is a way to tag the response in some way. hypothetically the tap may want to support all types, including envoy types?

Would it make sense to use DiscoveryRequest.ResponseNonce, and have the DiscoveryResponse.Nonce contain a matching value? I realize this is backwards from how it is used, but it would let the requests and replies be matched precisely.

It would be great if this change involved no changes to pilot's code at all. If that is impossible we can compromise, but ideally its all client side. Maybe resourceName could encode it? I will think on this

With the latest, the only change to Pilot is to use Event Generator instead of API Generator.

howardjohn · 2021-03-29T20:04:44Z

pkg/istio-agent/xds_proxy.go

+	if !ok {
+		requestsQueue = make([]chan *discovery.DiscoveryResponse, 0, 1)
+	}
+	requestsQueue = append(requestsQueue, responseChannel)


do we really need multiple concurrently queued requests? or just a single one per-typeurl?

actually I am not sure its even possible to send 2 requests in a row without a response? Istiod will block the second request

I imagine multiple 3rd party tools pulling at the same time. I can re-factor the code so that if multiple tools hit the debug tap at once that block on the agent rather than going up together to Istiod.

Yeah I think that would work better, and agree its useful to be able to have multiple requests

howardjohn · 2021-03-29T20:06:36Z

pkg/istio-agent/xds_proxy.go

+
+func (p *XdsProxy) makeTapHandler() func(w http.ResponseWriter, req *http.Request) {
+	return func(w http.ResponseWriter, req *http.Request) {
+		serviceAccount := env.RegisterStringVar("SERVICE_ACCOUNT", "default", "").Get()


long term we should use the entire same metadata we use for real XDS

Which shouldn't be too bad, just a lot of plumbing data around

howardjohn · 2021-03-29T20:09:16Z

pkg/istio-agent/xds_proxy.go

+
+	p.connected.requestsChan <- req
+
+	return responseChannel, nil


if we do just have one enque-able, I think this function can also just block until it gets a response as well?

howardjohn · 2021-03-29T20:10:17Z

pkg/istio-agent/xds_proxy.go

+	httpMux := http.NewServeMux()
+	httpMux.HandleFunc("/debug/", p.makeTapHandler())
+
+	HTTPAddr := ":15009"


I know this was for demo, but we cannot listen on 0.0.0.0. Either UDS or localhost.

howardjohn · 2021-03-29T22:02:22Z

pilot/pkg/xds/ads.go

@@ -242,6 +245,10 @@ func (s *DiscoveryServer) processRequest(req *discovery.DiscoveryRequest, con *C
 	return s.pushXds(con, push, versionInfo(), con.Watched(req.TypeUrl), request)
 }

+func isXdsEvent(req *discovery.DiscoveryRequest) bool {


I would prefer not to have this logic if possible. I think I know why we do - to avoid treating the REQ as an ACK and being ignored? I think we could also set empty responceNonce as an option?

howardjohn · 2021-03-29T22:04:59Z

pkg/istio-agent/xds_proxy.go

@@ -464,7 +473,11 @@ func (p *XdsProxy) handleUpstreamResponse(con *ProxyConnection) {
 					forwardToEnvoy(con, resp)
 				}
 			default:
-				forwardToEnvoy(con, resp)
+				if strings.HasPrefix(resp.TypeUrl, "istio.io/debug/") {


It would be great if this change involved no changes to pilot's code at all. If that is impossible we can compromise, but ideally its all client side. Maybe resourceName could encode it? I will think on this

irisdingbj · 2021-03-30T03:08:07Z

@esnible I may lack of context for the PR's purpose. So please correct me if I miss something. We now have an experimental command istioctl x internal-debug and it can send debug request to our secured debug interface. ( It will dispatch the xds request to our internal http debug server under the hood). If you provide a pod for this command, it will run using this pod's identify. (eg: istioctl x internal-debug productpage-xxx.default) . Wondering can we reuse it ?

Basically it supports our current /debug/xxxz command.

therealmitchconnors · 2021-03-30T16:54:17Z

pkg/istio-agent/xds_proxy.go

+	p.tapMutex.Lock()
+	requestsQueue, ok := p.tapRequests[resp.TypeUrl]
+	if ok {
+		requestChan := requestsQueue[0]


it looks like multiple concurrent requests are supported. How can we be sure the first request is the one that matches this response?

As discussed at the UX meeting, multiple concurrent requests are not needed. The latest change eliminates that.
With the design you commented on, the only parameter is the TypeUrl, and we check that it matches. The intention was that although the performance might be unfair, the results would be correct -- everyone sees the TypeUrl they asked for.

esnible · 2021-03-31T08:14:45Z

/test all

esnible · 2021-03-31T08:39:33Z

/test all

esnible · 2021-03-31T09:42:56Z

/test integ-pilot-multicluster-tests_istio

howardjohn

Looks great

cc @hzxuzhonghu @ramaraochavali

howardjohn · 2021-03-31T15:31:08Z

pkg/istio-agent/xds_proxy.go

+		return nil, fmt.Errorf("proxy not connected to Istiod")
+	}
+
+	defer p.tapMutex.Unlock()


nit: I suppose this works too but it seems more common to add this immediately after the .Lock()

howardjohn · 2021-03-31T15:32:49Z

pkg/istio-agent/xds_proxy.go

@@ -464,7 +471,11 @@ func (p *XdsProxy) handleUpstreamResponse(con *ProxyConnection) {
 					forwardToEnvoy(con, resp)
 				}
 			default:
-				forwardToEnvoy(con, resp)
+				if strings.HasPrefix(resp.TypeUrl, "istio.io/debug/") {
+					p.tapResponseChannel <- resp


I think if pilot pushes a few responses and some times out this may deadlock? since there is no receiver trying to read from the channel. This would impact the real XDS traffic

howardjohn · 2021-03-31T15:33:32Z

pkg/istio-agent/xds_proxy.go

+		typeURL := fmt.Sprintf("istio.io%s", req.URL.Path)
+		response, err := p.tapRequest(&discovery.DiscoveryRequest{
+			Node: &envoy_v3.Node{
+				Id: "sidecar~1.1.1.1~debug~cluster.local", // TODO listen for and use Envoy's ID?


do we need it at all?

howardjohn · 2021-03-31T15:34:23Z

pkg/istio-agent/xds_proxy.go

+		}
+
+		// Send Istiod's response as HTTP body
+		j, err := json.Marshal(response)


for xds marshal we probably need jsonpb?

esnible · 2021-03-31T17:41:17Z

/test all

lei-tang · 2021-03-31T20:26:18Z

pkg/istio-agent/xds_proxy.go

 	"os"
 	"path"
 	"strings"
 	"sync"
 	"time"

 	discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
+	// _ "github.com/envoyproxy/go-control-plane/envoy/service/status/v3" // needed for piggyback marshal


This line is commented out. Is it still needed?

lei-tang · 2021-03-31T20:28:44Z

pkg/istio-agent/xds_proxy.go

+		// Send Istiod's response as HTTP body
+		jsonm := &jsonpb.Marshaler{Indent: "  "}
+		w.Header().Add("Content-Type", "application/json")
+		_ = jsonm.Marshal(w, response)


nit: Can the line "_ = jsonm.Marshal(w, response)" be simplified to "jsonm.Marshal(w, response)"?

That's there for lint. If I don't do it, Istio's build process says Error return value of jsonm.Marshal is not checked.

It would be possible to Marshal to a buffer, check the result, and then send that result to w. It seemed like a lot of work, given that it isn't really possible for this code to fail.

…A_AGENT=false

esnible · 2021-04-05T15:59:50Z

/retest

howardjohn · 2021-04-05T21:07:17Z

pilot/pkg/xds/gen.go

-		// TODO move this to just directly using the resource TypeUrl
-		g = s.Generators["api"] // default to "MCP" generators - any type supported by store
+		if strings.HasPrefix(typeURL, "istio.io/debug/") {
+			g = s.Generators["event"]


One issue - when we send a debug request, we register a watch for that type. This watch is never removed. As a result, all config changes will re-push a new debug response. Example logs:

2021-04-05T21:06:24.639273Z info ads Push debounce stable[10] 1: 101.013956ms since last change, 101.013815ms since last push, full=true 2021-04-05T21:06:24.639415Z info ads XDS: Pushing:2021-04-05T21:06:24Z/8 Services:5 ConnectedEndpoints:2 Version:2021-04-05T21:06:24Z/8 2021-04-05T21:06:24.640212Z info ads CDS: PUSH for node:echo-cb96f8d94-ssbm8.default resources:24 size:17.1kB 2021-04-05T21:06:24.640247Z info ads EDS: PUSH for node:echo-cb96f8d94-ssbm8.default resources:6 size:1.2kB empty:0 cached:6/6 2021-04-05T21:06:24.641634Z info ads LDS: PUSH for node:echo-cb96f8d94-ssbm8.default resources:16 size:64.8kB 2021-04-05T21:06:24.641921Z info ads RDS: PUSH for node:echo-cb96f8d94-ssbm8.default resources:6 size:4.3kB 2021-04-05T21:06:24.641971Z info ads istio.io/debug/edsz: PUSH for node:echo-cb96f8d94-ssbm8.default resources:0 size:0B 2021-04-05T21:06:24.642000Z info ads istio.io/debug/configz: PUSH for node:echo-cb96f8d94-ssbm8.default resources:0 size:0B 2021-04-05T21:06:24.642010Z info ads istio.io/debug/config_dump with 0 ResourceNames 2021-04-05T21:06:24.642028Z info ads istio.io/debug/config_dump: PUSH for node:echo-cb96f8d94-ssbm8.default resources:0 size:0B 2021-04-05T21:06:24.642075Z info ads istio.io/debug/syncz: PUSH for node:echo-cb96f8d94-ssbm8.default resources:2 size:134B 2021-04-05T21:06:24.656383Z info ads EDS: PUSH request for node:echo-cb96f8d94-ssbm8.default resources:15 size:3.0kB empty:0 cached:15/15

howardjohn · 2021-04-05T21:09:22Z

pilot/pkg/xds/gen.go

-		// TODO move this to just directly using the resource TypeUrl
-		g = s.Generators["api"] // default to "MCP" generators - any type supported by store
+		if strings.HasPrefix(typeURL, "istio.io/debug/") {
+			g = s.Generators["event"]


shouldn't this be s.Generators[TypeDebug]?

We don't need Generators[TypeDebug] if the typeURL is istio.io/debug (no trailing slash). If the type URL is istio.io/debug/syncz, we do need a generator, but it is the event generator, not the debug one.

howardjohn · 2021-04-05T21:10:35Z

tests/integration/pilot/piggyback_test.go

+			if err != nil {
+				t.Fatalf("couldn't curl sidecar: %v", err)
+			}
+			if !strings.Contains(out, "\"typeUrl\": \"istio.io/debug/syncz\"") {


nit: maybe we should attempt unmarshal it to make sure its valid, then we can do better checks as well?

Good catch. I am now trying to unmarshal using jsonpb. If that fails I try again as json. Created #31997 to explain the problem.

esnible · 2021-04-07T21:15:41Z

/retest

howardjohn · 2021-04-09T20:47:29Z

pilot/cmd/pilot-agent/options/options.go

@@ -81,6 +81,8 @@ var (
 	proxyXDSViaAgent = env.RegisterBoolVar("PROXY_XDS_VIA_AGENT", true,
 		"If set to true, envoy will proxy XDS calls via the agent instead of directly connecting to istiod. This option "+
 			"will be removed once the feature is stabilized.").Get()
+	proxyXDSDebugViaAgent = env.RegisterBoolVar("PROXY_XDS_DEBUG_VIA_AGENT", true,


if we backport please make it false. This is good for master though

…tio#31749) * Proof-of-concept * Use port 15009 * Send nonce with debug request * Use event generator for debug types * Remove debug printf * Refactor * Cleanup on close * test case, improved JSON serialization, simplify xDS request * Test case fix * Remove incorrect comment * Release note * Feature was added, not updated * Use port 15014, allow disabling with proxyMetadata.PROXY_XDS_DEBUG_VIA_AGENT=false * Move agent debug back to 15009 * Support for debug on single resources, and don't watch debug queries * typo * Fix test

esnible added the area/user experience label Mar 28, 2021

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 28, 2021

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 28, 2021

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2021

howardjohn reviewed Mar 29, 2021

View reviewed changes

therealmitchconnors reviewed Mar 30, 2021

View reviewed changes

howardjohn reviewed Mar 31, 2021

View reviewed changes

esnible marked this pull request as ready for review March 31, 2021 19:52

esnible requested review from a team as code owners March 31, 2021 19:52

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 31, 2021

lei-tang reviewed Mar 31, 2021

View reviewed changes

esnible requested review from a team as code owners March 31, 2021 21:38

esnible added 7 commits April 1, 2021 14:18

Proof-of-concept

a65633b

Use port 15009

c6a3cfb

Send nonce with debug request

492b0f3

Use event generator for debug types

2b8e4d4

Remove debug printf

d607c4d

Refactor

9a3c5c2

Cleanup on close

bef6c77

esnible added 5 commits April 1, 2021 14:18

test case, improved JSON serialization, simplify xDS request

a45a2e0

Test case fix

2e4080a

Remove incorrect comment

ac65752

Release note

d563959

Feature was added, not updated

d52ce0f

esnible force-pushed the agent-piggyback-v2 branch from d11b87b to d52ce0f Compare April 1, 2021 18:19

esnible added the do-not-merge/hold Block automatic merging of a PR. label Apr 5, 2021

Use port 15014, allow disabling with proxyMetadata.PROXY_XDS_DEBUG_VI…

6e06f51

…A_AGENT=false

esnible removed the do-not-merge/hold Block automatic merging of a PR. label Apr 5, 2021

Move agent debug back to 15009

ef1fbe7

howardjohn reviewed Apr 5, 2021

View reviewed changes

esnible added the do-not-merge/hold Block automatic merging of a PR. label Apr 6, 2021

esnible mentioned this pull request Apr 6, 2021

Log discovery requests when logging at debug level #31933

Merged

esnible added 3 commits April 7, 2021 13:57

Support for debug on single resources, and don't watch debug queries

9a8ecd6

typo

c47a80e

Fix test

3540d55

esnible removed the do-not-merge/hold Block automatic merging of a PR. label Apr 8, 2021

howardjohn approved these changes Apr 9, 2021

View reviewed changes

istio-testing merged commit f464909 into istio:master Apr 9, 2021

ingwonsong mentioned this pull request Aug 20, 2022

Add Tap gRPC handler to Istio Agent. #40566

Merged

Ability to tap into istio-agent to route debug messages to istiod #31749

Ability to tap into istio-agent to route debug messages to istiod #31749

Uh oh!

Conversation

esnible commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Mar 28, 2021

Uh oh!

esnible commented Mar 28, 2021

Uh oh!

esnible commented Mar 29, 2021

Uh oh!

howardjohn 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!

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!

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!

irisdingbj commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esnible commented Mar 31, 2021

Uh oh!

esnible commented Mar 31, 2021

Uh oh!

esnible commented Mar 31, 2021

Uh oh!

howardjohn 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!

esnible commented Mar 31, 2021

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!

esnible commented Apr 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esnible commented Mar 28, 2021 •

edited

Loading

irisdingbj commented Mar 30, 2021 •

edited

Loading