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

Deprecate @ServletEndpoint, @ControllerEndpoint and @RestControllerEndpoint #31768

Closed
mhalbritter opened this issue Jul 15, 2022 · 39 comments
Closed
Assignees
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Milestone

Comments

@mhalbritter
Copy link
Contributor

mhalbritter commented Jul 15, 2022

We should decide if we want to deprecate and later remove @ControllerEndpoint and @RestControllerEndpoint from the actuator. Using them ties the user to WebMVC or WebFlux and they were meant to ease the upgrade path to the weblayer-abstracting @Endpoint with @ReadOperation, etc.

Getting rid of those would pave the way for #20290.

If you're seeing this ticket and object to this idea, please comment, your feedback is very valuable. Please also explain your use case and why this use case can't be solved with @Endpoint or @WebEndpoint.

@mhalbritter mhalbritter added type: task A general task status: pending-design-work Needs design work before any code can be developed labels Jul 15, 2022
@mhalbritter mhalbritter added this to the 3.x milestone Jul 15, 2022
@jeffbswope
Copy link

We have some endpoints where we wanted to be able to include app and/or actuator links in the response. The low friction way we found was to move to @RestControllerEndpoint and get the HttpServletRequest and use getRequestURL() to build them. It was a while ago so I don't know if we missed some way to do that with a @WebEndpoint then or added since then...

Or if the idea of those links is an anathema to the endpoint abstraction entirely... they are strange endpoints I won't defend in general so no objections to deprecation, just a data point...

@mmoayyed
Copy link
Contributor

We have quite a few @RestControllerEndpoint annotations used here.

Main use cases that I think would be useful and helpful to support going forward would be:

  • Support the convenience of @GetMapping, @PostMapping, @ResponseBody, etc.

I think most of these could likely be converted and written to conform to the new way of doing this.

  • Ability to read the servlet request body in raw form without mapping the request body it to an object, using HttpServletRequest#getInputStream()

It might be doable to do this today, but I wasn't able to find an alternative.

@wilkinsona
Copy link
Member

wilkinsona commented Jul 21, 2022

Thank you, @mmoayyed.

Support the convenience of @GetMapping, @PostMapping, @ResponseBody

In Actuator endpoint terms, @GetMapping is a @ReadOperation and @PostMapping is a @WriteOperation. There's also @DeleteOperation which is the equivalent of @DeleteMapping.

Can you expand a bit on the need for @ResponseBody? The value returned from an endpoint operation is already written to the body of the response.

Ability to read the servlet request body in raw form … It might be doable to do this today

We don't support this at the moment. Any body is read up front as a source of arguments for the endpoint operation's method parameters. There's an implicit assumption here that the body can be converted into key-value pairs.

@mmoayyed
Copy link
Contributor

Thank you Andy.

The main example that I found for @ResponseBody is dealing with exporting data out in form of a zip file. The endpoint creates a zip file Resource, and returns it in the form of new ResponseEntity<>(resource, headers, HttpStatus.OK) where the header contains content-disposition entries, etc. The endpoint itself is tagged as

@GetMapping(path = "/export", produces = MediaType.APPLICATION_OCTET_STREAM_VALUE)
@ResponseBody

Likewise, the reverse of this operation is handled as an import endpoint in the same component that reads the request.getInputStream(), and stores the processed data somewhere, returning some http status endpoint. The body could contain on object or a collection of objects (as would be the classical import use case)

@wilkinsona
Copy link
Member

You can return a Resource from a @ReadOperation as we do in Boot's LogFileWebEndpoint:

@ReadOperation(produces = "text/plain; charset=UTF-8")
public Resource logFile() {

I guess a missing piece is the ability to set a Content-Disposition header which WebEndpointResponse does not allow you to do at the moment.

Likewise, the reverse of this operation is handled as an import endpoint in the same component that reads the request.getInputStream(), and stores the processed data somewhere, returning some http status endpoint

On first impressions, this doesn't sound like something that you'd do as part of operating an app that's running in production so it isn't necessarily something that we'd expect to support in Actuator. Can you expand a bit on the use case please?

@mmoayyed
Copy link
Contributor

Sure. I'd have to go back a few steps, so please bear with me.

This project ships a number auto-starters that control the implementation of a particular repository type. The ultimate build script (put together by an adopter in form of a overlay), allows an adopter to include the appropriate starter in their build and let that repository manage their data. I suppose there are scenarios where one would want to take data from one repository in one version, and move it over to another repository in the next version. Let's say JSON to Cassandra, Mongo to DynamoDb, etc. So the import/export operations provide a facility to do that as a point of convenience, behind an actuator endpoint that of course needs to be enabled, protected, used likely once or twice in dev, and then turned off in production as you note. Certainly, it's not something to actively use in production.

I suppose one could always use native tooling, if and when available, to handle the data migration task, or fall back onto dedicated libraries and frameworks that take care of such things. While all should be better options in theory, (and surely there is nothing stopping the advanced user to play around with such things) they add additional points of complexity and learning curve that in this particular context, a very select group of users would want to deal with. The overwhelming preference generally is to deal with an endpoint that handles such things (IIRC, there is a command-line utility available that does this as well in form a spring shell command, but I digress).

@frederic-kneier
Copy link

We currently use the management endpoints as a means of providing an internal API that is served on a different port. These annotations allow us to actually provide dedicated endpoints on the management port as well as handling security on these differently.

This way we can provide an Kubernetes ingress for the public API on one port while providing management actions an internal API without ingress on another.

@wilkinsona
Copy link
Member

Thanks, @frederic-kneier. Would it be fair to say that you're using an Actuator endpoint as a convenient way of exposing the API on a different port rather than because it's used as part of operating the application in production?

@frederic-kneier
Copy link

frederic-kneier commented Jul 26, 2022

Not entirely, but regarding @ControllerEndpoint and @RestControllerEndpoint I think that is true. It would be ok to use simple @Controller and @RestController annotations if we could specify the actual endpoint/port to be used and if security could be configured based on the endpoint/port. There are still management endpoints that do not match this description, but I think they could be implemented using the normal endpoints / operations.

@spencergibb
Copy link
Member

We have a few in spring cloud. I'll get details in a bit

@mmoayyed
Copy link
Contributor

mmoayyed commented Aug 3, 2022

One other thing that might be useful here: @RestControllerEndpoint allows for overloading methods that address different requirements. For example, one could have multiple @GetMapping methods that handle different concerns, with different parameters or path variables. This seems more natural vs one @ReadOperation with 2-3 parameters some marked as selectors and some marked as not. Perhaps the replacement could provide the same programming model.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 15, 2023
@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jan 15, 2024
@bclozel bclozel self-assigned this Apr 15, 2024
@bclozel bclozel added status: noteworthy A noteworthy issue to call out in the release notes and removed status: pending-design-work Needs design work before any code can be developed labels Apr 15, 2024
@bclozel bclozel modified the milestones: 3.x, 3.3.0-RC1 Apr 15, 2024
@bclozel bclozel changed the title Consider deprecating @ControllerEndpoint and @RestControllerEndpoint Deprecate @ServletEndpoint, @ControllerEndpoint and @RestControllerEndpoint Apr 15, 2024
@bclozel
Copy link
Member

bclozel commented Apr 15, 2024

We have discussed this again earlier today and decided that we should deprecate this for 3.3.0-RC1.
We think that since this was introduced, the @Endpoint / @*Operation model has evolved quite a bit and that we declined low-level web support feature requests in this area. Right now it seems that the abstraction leaks here and that it prevents us from evolving the implementation.

We don't plan on removing this feature soon - we will honor at least our 2 minor releases policy. In the meantime, we will also gather feedback from the community.

@bclozel
Copy link
Member

bclozel commented Apr 16, 2024

We had a look before scheduling this, and it seems this endpoint would work well with @ReadOperation and @WriteOperation.

@wilkinsona
Copy link
Member

It looks to me like your ApiProxyEndpoint should be a @RestController and use management.endpoints.web.base-path to map it to /actuator/api. If you're running actuator on the same port as the rest of the application, a standard @RestController should be all that you need. If you're running actuator on a separate port, you'll need to use a @ManagementContextConfiguration class registered in META-INF/spring.factories to define the controller as a bean.

@johanhaleby
Copy link

@wilkinsona I'm using a different port so I guess I should try the @ManagementContextConfiguration path. I'll let you know how it goes. Thank you for your suggestion.

@rainerfrey-inxmail
Copy link

Unfortunately I only found this ticket after the 3.3 release. Our reason for using @RestControllerEndpoint - and on a casual read this seems to have not come up in the discussion - is because we have several custom endpoints that require a JSON object with a couple of nested properties as input. These are actuator endpoints and not regular RestControllers as they serve operational purposes, are accessed on a separate management port and require our operational Basic auth whereas the application uses OAuth2 with a completely separate user base - configured conveniently with the EndpointRequest matchers. The approach to register a regular controller in the management context seems complicated and low-level in contrast, and make configuration and security configuration more complicated and less declarative. Removing this is a functional regression for us. Supporting POJO (and/or records) as input for @WebEndpoint would mitigate this for our use case.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 27, 2024
@philwebb
Copy link
Member

philwebb commented Jul 3, 2024

@rainerfrey-inxmail Are you able to share a sample application that shows one of your more complicated inputs? It doesn't need to be your production code, just something that gives us an idea of how you're currently using @RestControllerEndpoint. We can then consider how we might be able to evolve @WebEndpoint.

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 3, 2024
@lfvJonas
Copy link

lfvJonas commented Jul 4, 2024

@johanhaleby We are currently having the same issue did you get it to work using the @ManagementContextConfiguration method?

@johanhaleby
Copy link

@IfvJonas I haven't tried yet unfortunately.

@kschlesselmann
Copy link

@philwebb I second @rainerfrey-inxmail use case. We're synchronizing data between our environments and use actuator endpoints for that.

Right now we have an endpoint like

    @PostMapping
    fun synchronize(@RequestBody model: SynchronizationRequestModel): Mono<ResponseEntity<Void>> =

which basically accepts a list of entities that should be synced. It seems that this is no longer possible with @WebEndpoint and @WriteOperation.

@chicobento
Copy link
Contributor

Unfortunately I only found this ticket after the 3.3 release. Our reason for using @RestControllerEndpoint - and on a casual read this seems to have not come up in the discussion - is because we have several custom endpoints that require a JSON object with a couple of nested properties as input. These are actuator endpoints and not regular RestControllers as they serve operational purposes, are accessed on a separate management port and require our operational Basic auth whereas the application uses OAuth2 with a completely separate user base - configured conveniently with the EndpointRequest matchers. The approach to register a regular controller in the management context seems complicated and low-level in contrast, and make configuration and security configuration more complicated and less declarative. Removing this is a functional regression for us. Supporting POJO (and/or records) as input for @WebEndpoint would mitigate this for our use case.

We have the same issue.
Replacing @WebEndpoint with @RestController as suggested by @wilkinsona does not work as it also stops the "fake actuator" endpoint from being discovered via the /actuator endpoint which we also use for auto-discover of our management interfaces (similar tool as Spring Admin).

@rainerfrey-inxmail Are you able to share a sample application that shows one of your more complicated inputs? It doesn't need to be your production code, just something that gives us an idea of how you're currently using @RestControllerEndpoint. We can then consider how we might be able to evolve @WebEndpoint.

I can also help providing a minimal app for that but there's nothing fancy there, just:

@RestControllerEndpoint (id = "customer")
@Component
public class CustomerEndpoint {

    @PostMapping ("/{id}")
    public Customer createCustomer(final @PathVariable (value = "id") Integer id, final @Valid @RequestBody Customer customer)
    {
    	return customer;
    }
    public static record Customer(String name, Address address){}
    
    public static record Address(String street){}
}

Now you can POST to /actuator/customer/1 with

{ "name": "cust", "address": {"street": "1"}}

And placing a get to /actuator, it returns:

    "customer": {
      "href": "http://localhost:8081/actuator/customer",
      "templated": false
    }

There's a limitation where the templated will always be false when using RestControllerEndpoint so auto discovery does not not work 100% semantically correct for those endpoints.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 22, 2024
@kschlesselmann
Copy link

@philwebb Any news on this. This is a blocker on our side as well :-(

@rainerfrey-inxmail
Copy link

@rainerfrey-inxmail Are you able to share a sample application that shows one of your more complicated inputs?

Sorry I was otherwise engaged for a couple of weeks. Do you need a runnable application or is code of endpoint and request body sufficient?

@philwebb
Copy link
Member

@kschlesselmann We've not had a chance to discuss it again yet, but the issue is flagged so we won't forget.

@rainerfrey-inxmail Ideally something that we can run so we can investigate alternative API ideas.

@juliojgd
Copy link
Contributor

@philwebb Maybe this issue should be re-opened to show that there is an active discussion (an a pending one as well) here?

@wilkinsona
Copy link
Member

Thanks for the suggestion. This issue was tracking the deprecation which has been done and shipped in 3.3.0-RC1. It doesn't make sense to have an open issue for work that has been completed so it won't be re-opened. Please rest assured that the feedback that people have provided is still very much on our radar and it will all be considered before anything is actually removed.

@talasila-sairam
Copy link

talasila-sairam commented Oct 2, 2024

Thanks @wilkinsona Using RestControllerEndpoint solved the problem of creating a custom endpoint with context type as event-stream., Since we are deprecating these annotationsit from 3.3.0 and removing these annotations (@RestControllerEndpoint and @ControllerEndpoint), how are we going to create a custom actuator endpoint with context type as event-stream, since it is not working for both @Endpoint and @WebEndpoint?

@wilkinsona
Copy link
Member

how are we going to create a custom actuator endpoint with context type as event-stream, since it is not working for both @Endpoint and @WebEndpoint?

We may consider it as a future enhancement to the @WebEndpoint support, but we'd need a compelling use case for event stream responses from Actuator. What's your need for it?

@talasila-sairam
Copy link

We were actually in the midst of constructing a custom actuator endpoint since we have many circuit breakers and we wanted to send our circuit breaker events to the developer management portal managing bby us. Although we can accomplish the same thing with a controller or rest controller, this isn't what I presume it's meant for. Given that this is management configuration data, it made sense to expose it on the actuator endpoint.

@johanhaleby
Copy link

We had the same use case as @talasila-sairam before when using Hystrix and Turbine.

@slem1
Copy link

slem1 commented Oct 17, 2024

Hi we use Rest Actuator for some tech operation at work. We do not use jmx exposure, so we use all the flexibility a REST controller could provide such as this kind of actuator:

@Component
@RestControllerEndpoint(id = "kafka")
class KafkaActuator {

  @GetMapping("/consumers")
  List<ConsumerGroupWithStatus> consumers() {
 //...
 }

  @PostMapping("/suspend/{consumerGroup}")
  void suspend(@PathVariable String consumerGroup) {
    LOGGER.info("ask suspend for consumer group {}", consumerGroup);
    MessageListenerContainer consumer = getConsumer(consumerGroup);
    consumer.pause();
    LOGGER.info("consumer group {} suspended", consumerGroup);
  }

  @PostMapping("/resume/{consumerGroup}")
  void resume(@PathVariable String consumerGroup) {
    LOGGER.info("ask resume for consumer group {}", consumerGroup);
    MessageListenerContainer consumer = getConsumer(consumerGroup);
    consumer.resume();
    LOGGER.info("consumer group {} resumed", consumerGroup);
  }

  @PostMapping("/dump")
  ResponseEntity<DumpCommandResult> dump(@RequestBody DumpOrder dumpOrder) {
...
  }

  @DeleteMapping("/dump")
  ResponseEntity<DumpCommandResult> stopDump() {
  ...
  }

Now, RestControllerEndpoint is deprecated, but I didn't find any equivalent migration path in spring documentation. Of course, we can convert this actuator to a RestController, but we'll lose actuator built-in configuration, such as exposure properties:
management.endpoints.web.base-path=/management, management.endpoints.web.exposure.include=*...

@zxuanhong
Copy link

zxuanhong commented Dec 12, 2024

At present, we still need to pass the body parameter, but @WriteOperation does not support it. I don't know how to migrate either.It seems that abandoning @RestControlEndpoint did not have sufficient consideration now

@wilkinsona
Copy link
Member

@zxuanhong can you please be more specific? A concrete example of what you're doing with @RestControllerEndpoint, as others have shared above, would be useful.

@spencergibb
Copy link
Member

spencergibb commented Dec 12, 2024

@wilkinsona
Copy link
Member

I suspect we may have overlooked the inherited post mappings. Apologies. @WriteOperation does not support complex objects at the moment so we'd need to address that to allow Gateway to move away from @RestControllerEndpoint.

@zxuanhong
Copy link

zxuanhong commented Dec 16, 2024

@wilkinsona For example, how to migrate @RestControlEndpoint to @WebEndpoint (we need to pass the body parameter).There are over 20 fields in ChangeInfo. I don't want to use query parameters

@Component
@RestControllerEndpoint(id = "systemconfig")
public class ExportersEndpoint {

  @PostMapping(path = "/change")
  public CompletableFuture<ResponseEntity<?>> changeConfig(
      @RequestBody final ChangeInfo info) {
    ...
  }
}

@lenaschoenburg
Copy link

We also use RestControllerEndpoint for a management actuator. This actuator is part of our public API that we can't easily break.

https://github.com/camunda/camunda/blob/9328d9eaec774f5f077361074dbc830c2b854577/dist/src/main/java/io/camunda/zeebe/shared/management/ClusterEndpoint.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests