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

HttpClient5 update packaged from Spring Boot 3.4/Spring Cloud 2024.0.0 + Envoy causing unexpected behaviours from 'protocolUpgradeEnabled' configuration #738

Open
Mingkongchan0 opened this issue Dec 10, 2024 · 7 comments

Comments

@Mingkongchan0
Copy link

Mingkongchan0 commented Dec 10, 2024

Describe the bug

Spring Boot 3.4.0
Spring Cloud Dependencies 2024.0.0 (Spring Cloud Starter Vault Config 4.2.0, Spring Cloud Vault Config 4.2.0)
Spring Vault Core 3.1.2
HttpClient5 5.4.1
Istio Envoy

This is a noted issue in the Spring Boot 3.4 Release notes, where Envoy is unable to parse the HTTP/1.1 TLS upgrades which were made the default as part of the current version of HttpClient5.

image

From the ClientHttpRequestFactoryFactory class within Spring Vault Core, given that Apache HttpComponents dependency exists, it would be the default client used for constructing the ClientHttpRequestFactory that is inadvertently used by VaultTemplate to create requests to Vault.
image

However, with reference to this issue raised on Istio, the ClientHttpRequestFactory spun up by ClientHttpRequestFactoryFactory has protocolUpgradeEnabled = true by default.
image

The resolution provided in the Spring Boot Release notes to define this bean to supersede the new defaults does not seem to be working as expected, as the Spring Vault instantiation happens before the Bean is registered.

Ultimately, this causes requests from VaultTemplate -> Istio -> Vault to fail.

Are there any workarounds to this issue?

Logs:
Istio error
{"upstream_cluster":"PassthroughCluster","requested_server_name":null,"upstream_local_address":null,"bytes_sent":0,"bytes_received":0,"duration":0,"protocol":"HTTP/1.1","request_id":"redacted","start_time":"2024-12-10T08:44:40.632Z","x_forwarded_for":null,"user_agent":"Apache-HttpClient/5.4.1 (Java/21.0.3)","path":"/v1/secret/vault-test","upstream_transport_failure_reason":null,"response_flags":"-","connection_termination_details":null,"response_code_details":"upgrade_failed","downstream_remote_address":"redacted","route_name":null,"downstream_local_address":"redacted","upstream_service_time":null,"method":"GET","authority":"redacted","response_code":403,"upstream_host":null}

Vault error
{"logTimeStamp":"2024-12-10T08:13:41.025079294Z","service":"vault-test","logger":"org.springframework.vault.core.lease.SecretLeaseEventPublisher$LoggingErrorListener","level":"WARN","class":"org.springframework.boot.logging.DeferredLog","method":"logTo","line":253,"file":"DeferredLog.java","thread":"main","stackTrace":"org.springframework.vault.VaultException: Status 403 Forbidden [secret/vault-test]

@mp911de
Copy link
Member

mp911de commented Dec 10, 2024

You can register a ClientHttpRequestFactory wrapped in org.springframework.vault.config.AbstractVaultConfiguration.ClientFactoryWrapper:

SpringApplication application = new SpringApplication(…);
application.addBootstrapRegistryInitializer(
		registry -> {

			registry.registerIfAbsent(AbstractVaultConfiguration.ClientFactoryWrapper.class, ctx -> {

				HttpComponentsClientHttpRequestFactory factory = …;
				return new AbstractVaultConfiguration.ClientFactoryWrapper(factory);

			});
		});

Let me know whether this helps.

@Mingkongchan0
Copy link
Author

Mingkongchan0 commented Dec 11, 2024

Hi @mp911de, I don't think it works.

image image

From what I see, the VaultConfigDataLoader class registers the clientHttpRequestFactory bean if its absent, and at this point in the bootstrap, there isn't a clientHttpRequestFactory instance yet.

Code below for reference. Please do advise if I'm misconfiguring this.

@SpringBootApplication
@Slf4j
public class VaultTestApplication {
    public static void main(String[] args) {
        SpringApplication application = new SpringApplication();
        application.addBootstrapRegistryInitializer(
                registry -> {
                    registry.registerIfAbsent(AbstractVaultConfiguration.ClientFactoryWrapper.class, ctx -> {
                        HttpComponentsClientHttpRequestFactory factory = ClientHttpRequestFactoryBuilder.httpComponents()
                                .withDefaultRequestConfigCustomizer(builder -> builder.setProtocolUpgradeEnabled(false)).build();
                        return new AbstractVaultConfiguration.ClientFactoryWrapper(factory);
                    });
                });
        SpringApplication.run(VaultTestApplication.class);
    }
}

@mp911de
Copy link
Member

mp911de commented Dec 11, 2024

Have you noticed that you're not using the customized application object, but instead, you're running SpringApplication.run(VaultTestApplication.class);? It should be application.run(args).

@v-ladynev
Copy link

v-ladynev commented Dec 12, 2024

@mp911de An example with application.addBootstrapRegistryInitializer() doesn't work even corrected.
addBootstrapRegistryInitializer() just adds InstanceSupplier not a bean.
And that InstanceSupplier never called because AbstractVaultConfiguration.ClientFactoryWrapper is already created during bootstrap.

I am using bootstrap.yaml for Vault configuration.

@mp911de
Copy link
Member

mp911de commented Dec 13, 2024

I am using bootstrap.yaml for Vault configuration.

This is an important detail because in that case, you need to define an override bean (@Bean ClientFactoryWrapper). To do so, create a boostrap autoconfiguration and register it with spring.factories under the org.springframework.cloud.bootstrap.BootstrapConfiguration key.

@v-ladynev
Copy link

@mp911de It works. Thank you very much.

Also this configuration parameter should be added to bootstrap.yaml

spring.main.allow-bean-definition-overriding: true

@reachjyo
Copy link

reachjyo commented Dec 17, 2024

@mp911de Even after adding the following fix , I still dont see the issue fixed , should I turn on the property spring.main.allow-bean-definition-overriding: true as well ?

We are not using Spring's Vault then should we still follow the suggestion you mentioned earlier , ie

This is an important detail because in that case, you need to define an override bean (@bean ClientFactoryWrapper). To do so, create a boostrap autoconfiguration and register it with spring.factories under the org.springframework.cloud.bootstrap.BootstrapConfiguration key.

    @Bean
    public HttpComponentsClientHttpRequestFactoryBuilder httpComponentsClientHttpRequestFactoryBuilder() {
        return ClientHttpRequestFactoryBuilder.httpComponents()
                .withDefaultRequestConfigCustomizer((builder) -> builder.setProtocolUpgradeEnabled(false));
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants