Consider making admin-ui hosting via embedded jetty an optional feature

[robertjchristian]

Embedded Jetty may not always be the ideal way to host the admin ui. Consider dependencies on the jetty servlet api, colliding ports, etc.

Feature request: Make Jetty optional, and provide a second option for hosting via juice servlet instead. Would you accept a pull request for something like this?

Thanks,

Rob

[majikthys]

up vote.

[codefromthecrypt]

Up vote

On Tuesday, July 23, 2013, jeremy franklin-ross wrote:

up vote.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/41#issuecomment-21437723
.

[cfregly]

+1.

this would also provide another solution to the the issue where sensitive env and system properties are exposed through the admin ui that runs in the same embedded server as the karyon healthcheck: #39

[izreal]

vote +1

[Jowster]

Up vote

[NiteshKant]

Sounds like a good idea.
Of course I will be delighted to get a pull request for this :)
Should I assign this issue to you?
Thanks!

[cfregly]

of course!

I've offered in the past, but it's hard to know where this stuff falls in the overall plan for karyon.

advice on a suggested design?

On Jul 23, 2013, at 9:44 PM, Nitesh Kant [email protected] wrote:

Sounds like a good idea.
Of course I will be delighted to get a pull request for this :)
Should I assign this issue to you?
Thanks!

—
Reply to this email directly or view it on GitHub.

[robertjchristian]

Hi Chris,

Ideally the design would enable the exclusion of Jetty jars from the
classpath altogether. So it would impact he Gradle scripts and not just
runtime properties.

The web app itself, it seems it would make sense to serve that through
Guice?

Rob

On Wednesday, July 24, 2013, Chris Fregly wrote:

of course!

I've offered in the past, but it's hard to know where this stuff falls in
the overall plan for karyon.

advice on a suggested design?

On Jul 23, 2013, at 9:44 PM, Nitesh Kant <[email protected]<javascript:_e({}, 'cvml', '[email protected]');>>
wrote:

Sounds like a good idea.
Of course I will be delighted to get a pull request for this :)
Should I assign this issue to you?
Thanks!

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/41#issuecomment-21467286
.

[cfregly]

@quidryan - do you have examples of how to conditionally pull in gradle dependencies based on a properties file or some such? is this recommended? while this is a nice feature, i want to make sure we're not making this overly complicated.

also, for the scenario where the user disables the embedded jetty server completely, the developer needs to be aware that healthchecks will no longer work. this is yet another piece of of the puzzle that would need to be documented, etc.

@NiteshKant - looking for your guidance. perhaps we should decouple the various pieces as i've heard rumblings from folks who would prefer to decouple the admin functionality from the healthcheck functionality. has this been considered?

good feature request, but i think we need some more discussion.

@NiteshKant - i can stop by the office when i'm back from OSCON next week.

[quidryan]

It's general recommended to never change dependencies on the fly. The better solution is to create new source sets that exist with new Configurations, or just create new projects to keep things simple. In this case, you could create a new configuration called jetty-enabled that has the jetty dependencies, and you'd have to put the jetty specific code into a new Source set. I'm not familiar enough to say how easy it is to separate out the jetty code, I can say that publishing the resulting work will be hard. A new project that is jetty specific would map to how artifacts are published much better. We also could explore how to do optional dependencies in Gradle.

[robertjchristian]

This is exactly what I had in mind. But we have to be careful about wording. The "non-jetty" version is still perfectly able to run in jetty (ie) via Gradle jettyRun. What we are excluding in this case is "embedded" jetty to host admin. Using the term "embedded" will help keep things clear.

[NiteshKant]

Ok, so let me summarize what I think this change is:

1. Require the admin console to be able to run in the same port/web context path as the main application and not run the embedded container.
2. Remove the build time dependency of jetty altogether.

1 can be done today(I have not tried it though) by disabling the AdminContainer component i.e. specifying the property "netflix.platform.admin.resources.disable" as false. Making the admin UI a part of the main app would then require the module https://github.com/Netflix/karyon/blob/master/karyon-admin/src/main/java/com/netflix/adminresources/AdminResourcesModule.java to be added to the main application as a guice module. This would make the pytheas based admin console available in the main application at the url /admin (of course considering the context path of the app). CC @amit-git to verify that this will work w.r.t pytheas.

For 2) we will have to extract AdminResourceContainer class into a different module. What is left of karyon-admin after removing this will be a set of resources to serve the admin functionality, we can name it karyon-admin-resources. The new module can be named as karyon-admin-embedded which will depend on karyon-admin-resources & have the jetty dependency.

The karyon-admin-web as it stands today will depend on karyon-admin-resources. If an application also include karyon-admin-embedded, the embedded console will be available without any additional configuration, as it works today.

@robertjchristian @cfregly let me know if this sounds good to you guys.

[robertjchristian]

Yes Nitesh, perfect!

On Tuesday, July 30, 2013, Nitesh Kant wrote:

Ok, so let me summarize what I think this change is:

1. Require the admin console to be able to run in the same port/web
   context path as the main application and not run the embedded container.
2. Remove the build time dependency of jetty altogether.

1 can be done today(I have not tried it though) by disabling the
AdminContainer component i.e. specifying the property
"netflix.platform.admin.resources.disable" as false. Making the admin UI a
part of the main app would then require the module
https://github.com/Netflix/karyon/blob/master/karyon-admin/src/main/java/com/netflix/adminresources/AdminResourcesModule.javato be added to the main application as a guice module. This would make the
pytheas based admin console available in the main application at the url
/admin (of course considering the context path of the app). CC @amit-githttps://github.com/amit-gitto verify that this will work w.r.t pytheas.

For 2) we will have to extract AdminResourceContainer class into a
different module. What is left of karyon-admin after removing this will be
a set of resources to serve the admin functionality, we can name it
karyon-admin-resources. The new module can be named as
karyon-admin-embedded which will depend on karyon-admin-resources & have
the jetty dependency.

The karyon-admin-web as it stands today will depend on
karyon-admin-resources. If an application also include
karyon-admin-embedded, the embedded console will be available without any
additional configuration, as it works today.

@robertjchristian https://github.com/robertjchristian @cfreglyhttps://github.com/cfreglylet me know if this sounds good to you guys.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/41#issuecomment-21825030
.

[cfregly]

this sounds great, nitesh. thanks!

-chris

On Tue, Jul 30, 2013 at 9:35 PM, Robert Christian
[email protected]:

Yes Nitesh, perfect!

On Tuesday, July 30, 2013, Nitesh Kant wrote:

Ok, so let me summarize what I think this change is:

1. Require the admin console to be able to run in the same port/web
   context path as the main application and not run the embedded container.
2. Remove the build time dependency of jetty altogether.

1 can be done today(I have not tried it though) by disabling the
AdminContainer component i.e. specifying the property
"netflix.platform.admin.resources.disable" as false. Making the admin UI
a
part of the main app would then require the module

https://github.com/Netflix/karyon/blob/master/karyon-admin/src/main/java/com/netflix/adminresources/AdminResourcesModule.javatobe added to the main application as a guice module. This would make the
pytheas based admin console available in the main application at the url
/admin (of course considering the context path of the app). CC @amit-git<
https://github.com/amit-git>to verify that this will work w.r.t pytheas.

For 2) we will have to extract AdminResourceContainer class into a
different module. What is left of karyon-admin after removing this will
be
a set of resources to serve the admin functionality, we can name it
karyon-admin-resources. The new module can be named as
karyon-admin-embedded which will depend on karyon-admin-resources & have
the jetty dependency.

The karyon-admin-web as it stands today will depend on
karyon-admin-resources. If an application also include
karyon-admin-embedded, the embedded console will be available without
any
additional configuration, as it works today.

@robertjchristian https://github.com/robertjchristian @cfregly<
https://github.com/cfregly>let me know if this sounds good to you guys.

—
Reply to this email directly or view it on GitHub<
https://github.com/Netflix/karyon/issues/41#issuecomment-21825030>
.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/41#issuecomment-21839681
.

[NiteshKant]

@cfregly are you working on this?

[cfregly]

hey Nitesh-

I had to punt on this given other priorities, unfortunately.

perhaps we should hop on a call/chat and discuss alongside the latest karyon roadmap?

setup a time for next week start 11/25, if you'd like. I'm in training all of this week.

thanks!

-Chris

[NiteshKant]

Sounds good to me, we can chat

[robertjchristian]

Hi Nitesh, Chris,

We took the approach of moving admin into the app proper (web.xml). It will be merged into service-nucleus master soon. May be outside of your use case, but you may get a benefit from seeing what it looks like. I will post an URL when it's finished.

Rob

On Nov 18, 2013, at 10:02 PM, Nitesh Kant [email protected] wrote:

Sounds good to me, we can chat

—
Reply to this email directly or view it on GitHub.

[NiteshKant]

@robertjchristian thanks for updating us, will look forward to your work!

[robertjchristian]

Hi Nitesh,

Want to instantiate AdminResources via web.xml instead of Guice:

<servlet>
  <servlet-name>AdminResourcesModule</servlet-name>
  <servlet-class>com.netflix.adminresources.AdminResourcesModule</servlet-class>
</servlet>

<servlet-mapping>
   <servlet-name>AdminResourcesModule</servlet-name>
   <url-pattern>/*</url-pattern>
</servlet-mapping>

... but AdminResourcesModule does not have an empty constructor, and we need a

Provider<HealthCheckInvocationStrategy> healthCheckInvocationStrategyProvider

... can you please advise on the best way to do this?

Thanks,

Rob

[NiteshKant]

@robertjchristian can you elaborate your usecase more? The admin container is more or less tied to guice so you can't really work without guice as such.
The AdminResourceModule is not a servlet so you can't really use it in the way you had specified. My suggestion was to add this as a guice module to your main application if you do not want to use the embedded jetty version.

[robertjchristian]

Hi Nitesh,

Yes, you are right. Sorry for the confusion, it's been a while since I have looked closely at this, and there is another team doing the actual refactor and I spaced on the details.

What you are suggesting sounds ideal. It sounds like you are saying that for:

ServletContextHandler handler = new ServletContextHandler();
handler.setContextPath("/");
handler.setSessionHandler(new SessionHandler());
handler.addFilter(LoggingFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
handler.addFilter(RedirectFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
handler.addFilter(new FilterHolder(adminResourcesFilter), "/*", EnumSet.allOf(DispatcherType.class));
handler.addServlet(new ServletHolder(adminResourcesFilter), "/*");
server.setHandler(handler);
server.start();

... rather than trying to refactor this into a static, old-school Servlet/web.xml style, use the Guice ServletModule?

https://code.google.com/p/google-guice/wiki/ServletModule

Is this correct?

Thanks.

[NiteshKant]

Yes that & add AdminResourceModule as an additional module to guice to get the console functionality.

[pikeas]

Any update on this?

[robertjchristian]

Nothing to report.

On Dec 9, 2014, at 9:48 PM, Aris Pikeas [email protected] wrote:

Any update on this?

—
Reply to this email directly or view it on GitHub.