Skip to content

Conversation

SSUday
Copy link

@SSUday SSUday commented Sep 12, 2025

Problem
When Docker returns 407 (Proxy Authentication Required), the response is often plain text, so the current code fails to surface a helpful error.

Solution

  • Special-case 407 in HttpClientTransport: if status == 407, read content and throw ProxyAuthenticationException with host/URI and details.
  • Parse JSON error if available; otherwise fall back to plain text.

Tests

  • Added ProxyAuthenticationExceptionTests to verify message/cause constructors.
  • (Happy to add/adjust HttpClientTransport tests per reviewer guidance.)

Notes

  • Only improves diagnostics; no success-path behavior changes.

Fixes: #46262

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 12, 2025

String detail = null;
Message json = deserializeMessage(content);
if (json != null && org.springframework.util.StringUtils.hasText(json.getMessage())) {
Copy link
Member

@wilkinsona wilkinsona Sep 18, 2025

Choose a reason for hiding this comment

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

StringUtils is already imported so there's no need to fully-qualify the type.


String msg = "Proxy authentication required for host: " + this.host.toHostString() + ", uri: "
+ request.getUri()
+ (org.springframework.util.StringUtils.hasText(detail) ? " - " + detail : "");
Copy link
Member

@wilkinsona wilkinsona Sep 18, 2025

Choose a reason for hiding this comment

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

StringUtils is already imported so there's no need to fully-qualify the type.


import static org.assertj.core.api.Assertions.assertThat;

class ProxyAuthenticationExceptionTests {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need tests for ProxyAuthenticationException. Please remove this class.

detail = json.getMessage();
}
else {
detail = new String(content, java.nio.charset.StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

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

StandardCharsets should be imported rather than using the fully-qualified type here.

@wilkinsona
Copy link
Member

Also, please sign your commits as described in the DCO check failure.

response.close();

if (statusCode == 407) {
response.close();
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong to me as the response will now only be closed when the status code is 407.

@SSUday SSUday force-pushed the 3.3.x branch 2 times, most recently from 7348c31 to 1bd6ffe Compare September 19, 2025 20:42
@SSUday
Copy link
Author

SSUday commented Sep 19, 2025

Update:

Close response on all error paths before throwing.

Use imports (no fully-qualified names).

Removed ProxyAuthenticationExceptionTests.

Rebased, squashed, DCO signed.

@wilkinsona
Copy link
Member

wilkinsona commented Sep 22, 2025

Those changes sound good, but I can't see them in the PR. Please check that you've pushed them.

@SSUday
Copy link
Author

SSUday commented Sep 22, 2025

Those changes sound good, but I can't see them in the PR. Please check that you've pushed them.

Pushed the updates: always close error responses, replaced FQNs with imports, removed ProxyAuthenticationExceptionTests.

Comment on lines -68 to -72
/**
* Perform an HTTP GET operation.
* @param uri the destination URI
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.

Comment on lines -78 to -82
/**
* Perform an HTTP POST operation.
* @param uri the destination URI
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.

Comment on lines -88 to -93
/**
* Perform an HTTP POST operation.
* @param uri the destination URI
* @param registryAuth registry authentication credentials
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.

Comment on lines -99 to -105
/**
* Perform an HTTP POST operation.
* @param uri the destination URI
* @param contentType the content type to write
* @param writer a content writer
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.

Comment on lines -111 to -117
/**
* Perform an HTTP PUT operation.
* @param uri the destination URI
* @param contentType the content type to write
* @param writer a content writer
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.

Comment on lines -123 to -127
/**
* Perform an HTTP DELETE operation.
* @param uri the destination URI
* @return the operation response
*/
Copy link
Member

Choose a reason for hiding this comment

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

This javadoc should not be removed.


if (statusCode >= 400 && statusCode <= 500) {
byte[] content = readContent(response);
// Always close the response for error paths
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not necessary. Please remove.

+ request.getUri()
+ (StringUtils.hasText(detail) ? " - " + detail : "");

throw new ProxyAuthenticationException(msg);
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure that this is needed. Could it be a DockerEngineException with a better message?

// Always close the response for error paths
response.close();

if (statusCode == 407) {
Copy link
Member

Choose a reason for hiding this comment

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

Use SC_PROXY_AUTHENTICATION_REQUIRED from org.apache.hc.core5.http.HttpStatus here rather than 407.

Comment on lines +123 to +126
Message json = deserializeMessage(content);
if (json != null && StringUtils.hasText(json.getMessage())) {
detail = json.getMessage();
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you handling JSON here? The issue states that the error has "a plain text message".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants