ztunnel icon indicating copy to clipboard operation
ztunnel copied to clipboard

Improve ztunnel dashboard UI

Open hanxiaop opened this issue 1 year ago • 9 comments

Ref: https://github.com/istio/istio/pull/45695#issuecomment-1662312782

  1. ztunnel /config_dump does not return a pretty printed json object
  2. ztunnel /logging is unusable since it expects a POST and a level, this should be similar to envoy logging API.
  3. ztunnel currently doesn't have a /stats API on 15000, but it does have one on :15020/metrics

hanxiaop avatar Aug 04 '23 04:08 hanxiaop

Doesn't logging in envoy use a POST as well? Modifications should require a POST? Or are we just talking about viewing the current level?

For stats, we don't need to copy envoy IMO.

howardjohn avatar Aug 04 '23 15:08 howardjohn

Doesn't logging in envoy use a POST as well? Modifications should require a POST? Or are we just talking about viewing the current level?

We are talking about viewing the current level, currently it's not viewable directly like envoy.

For stats, we don't need to copy envoy IMO.

+1, I think 15020/metrics is good.

hanxiaop avatar Aug 07 '23 01:08 hanxiaop

I can do the pretty print

and log level

Stevenjin8 avatar Aug 10 '23 22:08 Stevenjin8

@hanxiaop

ztunnel /logging is unusable since it expects a POST and a level, this should be similar to envoy logging API.

I don't think this is completely true. I can get the logging level of ztunnel through an empty post:

root@ztunnel-4v22b:/# curl -d "" localhost:15000/logging
current log level is info

Though, it is much less descriptive and formatted differently than envoy:

istio-proxy@my-pod:/$ curl -d "" localhost:15000/logging
active loggers:
  admin: warning
  alternate_protocols_cache: warning
...

Neither ztunnel nor enovy support GET requests for /logging.

So I wouldn't say that " /logging is unusable since it expects a POST and a level," but the ztunnel api is definitely different than the envoy API.

Stevenjin8 avatar Aug 11 '23 19:08 Stevenjin8

@Stevenjin8 Thanks for testing this. We are talking about the /logging API in the dashboard. Now, you can try to see the ztunnel dashboard using a command like istioctl d envoy ztunnel-bg4b7.istio-system. Once you click on the /logging API, it shows

Invalid HTTP method
 
usage: POST /logging						(To list current level)
usage: POST /logging?level=<level>				(To change global levels)
usage: POST /logging?level={mod1}:{level1},{mod2}:{level2}	(To change specific mods' logging level)

hint: loglevel:	error|warn|info|debug|trace|off
hint: mod_name:	the module name, i.e. ztunnel::proxy

I'm not sure, but I think we may need to modify the dashboard page, and perhaps the API a little, to make it work?

hanxiaop avatar Aug 14 '23 03:08 hanxiaop

@hanxiaop Thank you for that. I was able to reproduce.

From a brief look at the HTML, the difference lies in the dashboard page rather than the API.

The issue with the current ztunnel dashboard page is that it uses a hyperlink to reference the logging endpoint.

<a href="logging">logging</a>

This means that when you click on the endpoint, the browser will send a GET request to the logging endpoint rather than an empty POST request (I couldn't find a way to change this behavior without javascript)

The envoy logging dashboard uses buttons instead of hyperlinks which allows it to send post requests. This is kinda hacky thought because if you go to the logging endpoint and then refresh, the page, you'll get an error. It also looks kinda janky because there are a mix of buttons and hyperlinks

I think the better solution is to have envoy and ztunnel accept GET requests. If we just want to mirror behavior, we can replace the hyperlink with a button.

Stevenjin8 avatar Aug 14 '23 16:08 Stevenjin8

I think the better solution is to have envoy and ztunnel accept GET requests. If we just want to mirror behavior, we can replace the hyperlink with a button.

@Stevenjin8 I think adding a GET request to Envoy may not be acceptable since POST has been used for a while and may cause confusion. However, since we have full control of ztunnel, it may be fine to have a different behavior than Envoy? @GregHanson Do you have any suggestions?

hanxiaop avatar Aug 15 '23 07:08 hanxiaop

I wouldn't spend so much time on this...

On Tue, Aug 15, 2023 at 12:10 AM Xiaopeng Han @.***> wrote:

I think the better solution is to have envoy and ztunnel accept GET requests. If we just want to mirror behavior, we can replace the hyperlink with a button.

@Stevenjin8 https://github.com/Stevenjin8 I think adding a GET request to Envoy may not be acceptable since POST has been used for a while and may cause confusion. However, since we have full control of ztunnel, it may be fine to have a different behavior than Envoy? @GregHanson https://github.com/GregHanson Do you have any suggestions?

— Reply to this email directly, view it on GitHub https://github.com/istio/ztunnel/issues/638#issuecomment-1678513498, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXKPEMDZNKXNHR2PEQ3XVMOHZANCNFSM6AAAAAA3DVXV5Y . You are receiving this because you commented.Message ID: @.***>

howardjohn avatar Aug 15 '23 14:08 howardjohn

@hanxiaop @Stevenjin8 I don't like the idea of modifying the API to accept GET requests. I envisioned this one would require some html magic to get a page more similar to what envoy has (i.e. buttons for POST etc).

This issue encompassed a few items of varying degrees of importance IMO.

  • pretty printing the json to something human-readable: HIGH
  • including the stats API: design/implementation question
  • fixing the html for the dashboard: post-beta

Agreeing with John that the remaining item is lower priority. This issue could be used for future tracking - or we can mark it as community/help-wanted/community/good-first-issue for any savvy html devs that want to take a stab at it

GregHanson avatar Aug 18 '23 13:08 GregHanson