Skip to content

admin: disable admin html#2388

Closed
jpsim wants to merge 1 commit intomainfrom
admin-disable-admin-html
Closed

admin: disable admin html#2388
jpsim wants to merge 1 commit intomainfrom
admin-disable-admin-html

Conversation

@jpsim
Copy link
Contributor

@jpsim jpsim commented Jun 28, 2022

This isn't something we want to expose when running Envoy Mobile.

For more details: envoyproxy/envoy#21814

Risk Level: Low, will impact anyone who's used this feature before.
Testing: Verified that admin_html.cc is not compiled.
Docs Changes: N/A
Release Notes: N/A

This isn't something we want to expose when running Envoy Mobile.

For more details: envoyproxy/envoy#21814

Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim marked this pull request as ready for review June 28, 2022 12:36
@jpsim jpsim requested a review from mattklein123 June 28, 2022 12:36
@Augustyniak
Copy link
Contributor

How does this work with enableAdminInterface method that we currently have on the builder itself? I assume that it will no longer be possible to visit an admin webpage after we call enableAdminInterface on the builder (and build the engine)?

@jpsim
Copy link
Contributor Author

jpsim commented Jun 28, 2022

How does this work with enableAdminInterface method that we currently have on the builder itself?

I was only vaguely aware of this configuration point. I didn't know that the html interface was what we were explicitly exposing. Knowing that this is an explicitly enabled feature changes the discussion on this for sure.

I had started a discussion on Slack here: https://envoyproxy.slack.com/archives/C02F93EEJCE/p1656350561377359

I assume that it will no longer be possible to visit an admin webpage after we call enableAdminInterface on the builder (and build the engine)?

Yes that's my understanding as well based on the description in envoyproxy/envoy#21814.

@Augustyniak
Copy link
Contributor

I was only vaguely aware of this configuration point. I didn't know that the html interface was what we were explicitly exposing. Knowing that this is an explicitly enabled feature changes the discussion on this for sure.

The feature was about exposing curl localhost:9901/stats and not the admin (/) endpoint itself so I think that it should be fine to proceed here. I have a PR that explains how to use EM admin facilities in #2348

@snowp
Copy link
Contributor

snowp commented Jul 13, 2022

This should be good to merge no?

@jpsim
Copy link
Contributor Author

jpsim commented Jul 13, 2022

No, we decided to keep the feature enabled in our last community meeting, but to eventually replace it with safer programmatic APIs to provide the same kind of debugging features.

@jpsim jpsim closed this Jul 13, 2022
@jpsim jpsim deleted the admin-disable-admin-html branch July 13, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants