Skip to content

Conversation

@dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Oct 8, 2018

Hi,

this PR fixes a missing entry for the caches endpoint in the production-ready-endpoints-exposing-endpoints section.

While doing that, I noticed that the integrationgraph endpoint is documented to be available via Web by default, which doesn't seem to be the case. Maybe I'm missing something here though (in which case I'm happy to revert that particular change).

Let me know what you think.

Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 8, 2018
@snicoll snicoll self-requested a review October 8, 2018 10:06
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. integrationgraph is enabled by default, I've added a comment.

|`integrationgraph`
|Yes
|Yes
|No
Copy link
Member

Choose a reason for hiding this comment

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

What makes you think this is not enabled by default? There is a graph only if you're actually using Spring Integration. If you have no graph, there's not much to show so the endpoint is conditional on that.

So the endpoint is enabled by default but only shows up if you're using SI. Is that confusing?

Copy link
Member

Choose a reason for hiding this comment

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

This table is for exposure rather than enablement. The endpoint is enabled by default, but I don't believe it is exposed by default. In other words, I agree with @dreis2211 that the table is currently incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit different from exposure or enablement. I think adding the integration graph as No in the exposure table, might lead to the misconception that it will be available if management.endpoints.web.exposure is set to *. But it is conditionally available on SI and setting that flag won't make it available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flyway endpoint is very similar here. It requires Flyway of course, but also states a No. I don't care too much in the end and do what the team prefers, but setting integrationgraph to No seems more consistent at the moment.

Setting it to No also backs the code handling default exposure. E.g. WebEndpointAutoConfiguration

	@Bean
	public ExposeExcludePropertyEndpointFilter<ExposableWebEndpoint> webExposeExcludePropertyEndpointFilter() {
		WebEndpointProperties.Exposure exposure = this.properties.getExposure();
		return new ExposeExcludePropertyEndpointFilter<>(ExposableWebEndpoint.class,
				exposure.getInclude(), exposure.getExclude(), "info", "health");
	}

Or ManagementWebSecurityConfigurerAdapter

	@Override
	protected void configure(HttpSecurity http) throws Exception {
		http.authorizeRequests()
				.requestMatchers(
						EndpointRequest.to(HealthEndpoint.class, InfoEndpoint.class))
				.permitAll().anyRequest().authenticated().and().formLogin().and()
				.httpBasic();
	}

Both only expose the health and the info endpoint.

Having that said, I think we could do the following:

  1. Accept the PR as it is to be more consistent
  2. Clarify both the enablement and exposure tables documentation with an additional hint, that some endpoints can be conditional on something. Flyway having Flyway available, integrationgraph having SI on the classpath, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

@dreis2211 Ignore my comment. I misread the change. Thanks for the clarification.

Copy link
Member

Choose a reason for hiding this comment

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

This table is for exposure rather than enablement.

I misread that, sorry.

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue type: documentation A documentation update and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 8, 2018
@snicoll snicoll added this to the 2.1.x milestone Oct 8, 2018
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged label Oct 9, 2018
@snicoll snicoll self-assigned this Oct 9, 2018
@snicoll snicoll closed this in 6a66a33 Oct 9, 2018
snicoll added a commit that referenced this pull request Oct 9, 2018
* pr/14712:
  Fix actuator endpoint tables in docs
@wilkinsona wilkinsona modified the milestones: 2.1.x, 2.1.0.RC1 Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: documentation A documentation update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants