Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proxy command to dashboard #45695

Merged
merged 7 commits into from
Sep 7, 2023

Conversation

hanxiaop
Copy link
Member

@hanxiaop hanxiaop commented Jun 28, 2023

Please provide a description of this PR:
Add a new command to display Ztunnel admin dashboard UI.

@hanxiaop hanxiaop requested a review from a team as a code owner June 28, 2023 05:41
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 28, 2023
@hanxiaop hanxiaop changed the title Add proxy command to dashboard Add ztunnel command to dashboard Jul 21, 2023
@GregHanson
Copy link
Member

GregHanson commented Aug 2, 2023

I agree using istioctl dashboard envoy ds/ztunnel -n istio-system is kind of a poor way to go. And I do think the dashboard command should be made available for ztunnel. @hanxiaop would it make sense to have a single istioctl dashboard proxy command that works for either envoy or ztunnel instance?

Also, we may need to open an item for improving the ztunnel dashboard UI as well. Just comparing it with envoy's:

  • ztunnel /config_dump does not return a pretty printed json object
  • ztunnel /logging is unusable since it expects a POST and a level
  • ztunnel currently doesn't have a /stats API on 15000, but it does have one on :15020/metrics
  • /certs and /server_info is technically in the /config_dump, not sure if they need their own API endpoint

@hanxiaop
Copy link
Member Author

hanxiaop commented Aug 4, 2023

I agree using istioctl dashboard envoy ds/ztunnel -n istio-system is kind of a poor way to go. And I do think the dashboard command should be made available for ztunnel. @hanxiaop would it make sense to have a single istioctl dashboard proxy command that works for either envoy or ztunnel instance?

Also, we may need to open an item for improving the ztunnel dashboard UI as well. Just comparing it with envoy's:

  • ztunnel /config_dump does not return a pretty printed json object
  • ztunnel /logging is unusable since it expects a POST and a level
  • ztunnel currently doesn't have a /stats API on 15000, but it does have one on :15020/metrics
  • /certs and /server_info is technically in the /config_dump, not sure if they need their own API endpoint

@GregHanson Sounds great. The last one seems redundant to me and we may include the API if there are really some needs. Just created an issue istio/ztunnel#638

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 3, 2023
@hanxiaop hanxiaop changed the title Add ztunnel command to dashboard Add proxy command to dashboard Sep 4, 2023
@hanxiaop
Copy link
Member Author

hanxiaop commented Sep 4, 2023

not stale

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 4, 2023
@hanxiaop
Copy link
Member Author

hanxiaop commented Sep 4, 2023

@GregHanson PTAL, thanks

Copy link
Member

@GregHanson GregHanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@istio-testing istio-testing merged commit fb55b2f into istio:master Sep 7, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user experience size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants