istio / ztunnel

The `ztunnel` component of ambient mesh
Apache License 2.0
308 stars 101 forks source link

Improve ztunnel dashboard UI #638

Open hanxiaop opened 1 year ago

hanxiaop commented 1 year ago

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
howardjohn commented 1 year ago

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.

hanxiaop commented 1 year ago

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.

Stevenjin8 commented 1 year ago

I can do the pretty print

and log level

Stevenjin8 commented 1 year ago

@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.

hanxiaop commented 1 year ago

@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?

Stevenjin8 commented 1 year ago

@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.

hanxiaop commented 1 year ago

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?

howardjohn commented 1 year ago

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: @.***>

GregHanson commented 1 year ago

@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.

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