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

#7552 Allow adding a custom context for path management #9820

Closed
wants to merge 2 commits into from

Conversation

viniciusxyz
Copy link
Contributor

@viniciusxyz viniciusxyz commented Sep 2, 2023

#7552 The idea of ​​this MR is to create the endpoints.all.context.path property that can be used to separate the management context-path from the server.context-path

With this change, the behavior of the paths is as follows:

config:

micronaut.server.context-path=/foo
endpoints.all.context-path=/fff
endpoints.all.path=/eee

Now:

Default app -> http://localhost:8080/foo
Management -> http://localhost:8080/fff/eee

Before

Default app -> http://localhost:8080/foo
Management -> http://localhost:8080/foo/eee

config:

micronaut.server.context-path=/foo
endpoints.all.path=/eee

Now:

Default app -> http://localhost:8080/foo
Management -> http://localhost:8080/foo/eee

Before

Default app -> http://localhost:8080/foo
Management -> http://localhost:8080/foo/eee

With the addition of this property there should be no change in behavior in relation to the current properties.

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

This will be a breaking change.

@sdelamo sdelamo added status: next major version The issue will be considered for the next major version type: breaking Introduces a breaking change labels Sep 11, 2023
@graemerocher
Copy link
Contributor

not sure this makes sense to be honest. There should be a separate context path setting for endpoints in which case it wouldn't be breaking

@graemerocher
Copy link
Contributor

To make this non-breaking IMO we should support endpoints.all.context-path

@viniciusxyz
Copy link
Contributor Author

To make this non-breaking IMO we should support endpoints.all.context-path

I will try to update MR over the weekend by adding this property.

@viniciusxyz
Copy link
Contributor Author

To make this non-breaking IMO we should support endpoints.all.context-path

I will try to update MR over the weekend by adding this property.

I apologize, but due to Black Friday I had to work on the weekend and was unable to make the necessary changes, I will try to make it as soon as possible

@viniciusxyz viniciusxyz marked this pull request as draft December 9, 2023 15:26
@viniciusxyz viniciusxyz changed the base branch from 4.1.x to 4.2.x December 9, 2023 18:51
@viniciusxyz
Copy link
Contributor Author

viniciusxyz commented Dec 10, 2023

@sdelamo I made the changes based on the comments made and instead of changing the behavior of endpoints.all.path the idea is to add the endpoints.all.context-path property, I am finishing the implementation, but I came across a difficulty that I still haven't been able to overcome, for some reason when adding the new property the EndpointsFilter filter is never executed, I'm investigating this and as soon as possible I'll add the documentation and remove the draft PR.

If you have any idea why a filter with the /** pattern is not filtering a request after adding a path, the tip would be very welcome.

@viniciusxyz viniciusxyz changed the title #7552 Separate micronaut.server.context-path configuration from endpoints.all.path #7552 Allow adding a custom context for path management Dec 10, 2023
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Can you add a controller to the test to verify the controller is still accessible in the expected path?

Moreover, can you modify your test to test an endpoint which does not return 401. The Unauthorized makes the test confusing.

@sdelamo sdelamo changed the base branch from 4.2.x to 4.3.x December 12, 2023 12:38
@sdelamo
Copy link
Contributor

sdelamo commented Dec 12, 2023

can you merge 4.3.x against your branch. I changed the base to be 4.3.x

@viniciusxyz
Copy link
Contributor Author

viniciusxyz commented Dec 16, 2023

Can you add a controller to the test to verify the controller is still accessible in the expected path?

Moreover, can you modify your test to test an endpoint which does not return 401. The Unauthorized makes the test confusing.

As the test refers to the context path of the management endpoints and they are configured as sensitive by default, I have to guarantee that even with the change in context there is no change in the security model that the framework adds today, which is why I am following the applied testing model in the EndpointsBasePathSpec test which has a very similar objective, I still haven't been able to define why the security tests aren't passing, it's as if there was some trigger to activate the EndpointsFilter that I can't see

@viniciusxyz
Copy link
Contributor Author

viniciusxyz commented Dec 17, 2023

@sdelamo The problem currently is that when defining micronaut.server.context-path and endpoints.all.context-path as in this test, EndpointsFilter -> doFilter is never called, this occurs because of this code snippet ServerFilterRouteBuilder.java.

As a mental model, let’s think about the following:

Let's assume we have an application configured with Prometheus and the following properties:

  • micronaut.server.context-path: /any
  • endpoints.all.context-path: /test
  • endpoints.prometheus.sensitive: true
  1. A new enhancement of AbstractEndpointRouteBuilder.java creates the path as /test/prometheus ignoring the context path as it is what is desired.

  2. A class ServerFilterRouteBuilder changes the default pattern /** of EndpointsFilter to /any/**

This causes calls to the prometheus point, even if they are defined as sensitive true, to still be displayed without security. This is due to the filter's behavior. Is there any filter that I can use that does not have this behavior of append the contextPath?

@viniciusxyz
Copy link
Contributor Author

@sdelamo Since I couldn't get the filter to ignore the ContextPath in the filters, I added a new property to the ServerFilter to establish whether the ContextPath should be added or not. With this and adding the option to not put ContextPath in the security filter now the solution works and all tests pass.

If you have any comments about the solution or need me to modify or test something, please let me know.

@sdelamo sdelamo self-assigned this Feb 6, 2024
@altro3
Copy link
Contributor

altro3 commented Apr 11, 2024

Any news about this improvement?

@viniciusxyz
Copy link
Contributor Author

Any news about this improvement?

I'm still waiting for approval :)

@altro3
Copy link
Contributor

altro3 commented Apr 11, 2024

@graemerocher @sdelamo ping

@graemerocher
Copy link
Contributor

it is scheduled for 4.5

@viniciusxyz viniciusxyz changed the base branch from 4.3.x to 4.5.x May 26, 2024 15:53
@graemerocher graemerocher self-assigned this May 31, 2024
@graemerocher
Copy link
Contributor

Merged with #10870

Thanks for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: next major version The issue will be considered for the next major version type: breaking Introduces a breaking change
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants