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.

Sign up for GitHub

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

Jump to bottom

Control Apache HttpComponents' protocol upgrades w/ a property #4399

Open
wants to merge 2 commits into
base: 4.2.x
Choose a base branch
from
Open

Control Apache HttpComponents' protocol upgrades w/ a property #4399

wants to merge 2 commits into from

Conversation

mamachanko
Copy link

Provides a configuration property

eureka:
  client:
    http-components:
      enable-protocol-upgrades: false

to make #4391 even easier building on top of #4394

@mamachanko mamachanko force-pushed the topic/mamachanko/4.2.x/http-components-properties branch 2 times, most recently from 8263e69 to e9faf7f Compare January 31, 2025 15:11
mamachanko
mamachanko commented Jan 31, 2025
View reviewed changes
...c/test/java/org/springframework/cloud/netflix/eureka/EurekaClientAutoConfigurationTests.java Outdated
Comment on lines 621 to 645
@Test
void TODO_shouldOverrideProtocolUpgrades() {
TestPropertyValues.of("eureka.client.http-components.enable-protocol-upgrades=false").applyTo(context);
context.register(TestEmptyRequestCustomizerConfiguration.class);
setupContext(RefreshAutoConfiguration.class, AutoServiceRegistrationConfiguration.class);

assertThat(context.getBeansOfType(EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer.class))
.hasSize(1);
EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer testRequestCustomizer = context
.getBean(EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer.class);

RequestConfig.Builder requestConfigBuilder = RequestConfig.custom().setProtocolUpgradeEnabled(true);
testRequestCustomizer.customize(requestConfigBuilder);
assertThat(requestConfigBuilder.build().isProtocolUpgradeEnabled()).isFalse();
}

@Configuration
protected static class TestEmptyRequestCustomizerConfiguration {

@Bean
EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer emptyRequestCustomizer() {
return builder -> {};
}

}
Copy link
Author

Choose a reason for hiding this comment

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

[thought]:

As suggested by you, @OlgaMaciaszek, in #4396 (comment) user-provided request customizers override HTTP components configuration by way of @ConditionalOnMissingBean.

(for simplicity I am referring to EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer as plain ærequest customizer")

Assuming if I have understood your remarks correctly, then I see the following issues. When user-provided request customizers override HTTP components configuration, ...

  • Then any request customizer - regardless of whether it controls protocol upgrades or not - will override eureka.client.http-components.enable-protocol-upgrades. That's because request customizers are opaque. We can't easily tell what they customize. The test TODO_shouldOverrideProtocolUpgrades demonstrates the issue with an empty request customizer which overrides.

  • Then it seems as if it's breaking the priority order respective externalized configuration. Conventionally, I would expect a configuration property to have the last word, not a bean. Arguably, at a lower level the two become beans all the same. But I find it unintuitive. It is making the mental model more involved, doesn't it? Furthermore, if there are users who have - knowingly or not - have a request customizer, then they may set eureka.client.http-components.enable-protocol-upgrades and wonder why its not working as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your point. Actually, given that it's a customizer type bean, we may want to change it so that we expect >= 1 customiser beans, then we'd add ours, but also pick the user's beans, and we should ensure ours have lower order of precedence than the user-created ones (so possibly should be applied first and then overwritten by user changes where appropriate); wdyt? CC @spencergibb

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have consulted with @spencergibb and he's approved the ordered list approach.

@mamachanko mamachanko mentioned this pull request Jan 31, 2025
Closed
@mamachanko mamachanko marked this pull request as ready for review January 31, 2025 15:30
OlgaMaciaszek
OlgaMaciaszek requested changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @mamachanko. Have added some comments.

...c/test/java/org/springframework/cloud/netflix/eureka/EurekaClientAutoConfigurationTests.java
}

@Test
void TODO_shouldOverrideProtocolUpgrades() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove TODO

Copy link
Author

@mamachanko mamachanko Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@OlgaMaciaszek I'd like to keep it until we resolve https://github.com/spring-cloud/spring-cloud-netflix/pull/4399/files#r1937493242, because it's exemplifying the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, the review is done with the final version in thought.

...c/test/java/org/springframework/cloud/netflix/eureka/EurekaClientAutoConfigurationTests.java Outdated
Comment on lines 621 to 645
@Test
void TODO_shouldOverrideProtocolUpgrades() {
TestPropertyValues.of("eureka.client.http-components.enable-protocol-upgrades=false").applyTo(context);
context.register(TestEmptyRequestCustomizerConfiguration.class);
setupContext(RefreshAutoConfiguration.class, AutoServiceRegistrationConfiguration.class);

assertThat(context.getBeansOfType(EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer.class))
.hasSize(1);
EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer testRequestCustomizer = context
.getBean(EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer.class);

RequestConfig.Builder requestConfigBuilder = RequestConfig.custom().setProtocolUpgradeEnabled(true);
testRequestCustomizer.customize(requestConfigBuilder);
assertThat(requestConfigBuilder.build().isProtocolUpgradeEnabled()).isFalse();
}

@Configuration
protected static class TestEmptyRequestCustomizerConfiguration {

@Bean
EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer emptyRequestCustomizer() {
return builder -> {};
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your point. Actually, given that it's a customizer type bean, we may want to change it so that we expect >= 1 customiser beans, then we'd add ours, but also pick the user's beans, and we should ensure ours have lower order of precedence than the user-created ones (so possibly should be applied first and then overwritten by user changes where appropriate); wdyt? CC @spencergibb

@mamachanko mamachanko force-pushed the topic/mamachanko/4.2.x/http-components-properties branch from e9faf7f to b4e392e Compare February 10, 2025 12:56
@mamachanko mamachanko force-pushed the topic/mamachanko/4.2.x/http-components-properties branch from b4e392e to f650158 Compare February 10, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek requested changes

Assignees
No one assigned
Labels
waiting-for-triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mamachanko @OlgaMaciaszek @spring-cloud-issues