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

JAVA-3033 - Consider handling IOException in a manner consistent with other exceptions in CloudConfigFactory.fetchProxyMetadata() #1609

Closed
wants to merge 2 commits into from
Closed

JAVA-3033 - Consider handling IOException in a manner consistent with other exceptions in CloudConfigFactory.fetchProxyMetadata() #1609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
680c7db
JAVA-3033 - Consider handling IOException in a manner consistent with…
Oct 1, 2022
f292c60
Small fixes to initial commit
hhughes Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions .../src/main/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
Expand Down Expand Up @@ -228,8 +229,18 @@ protected BufferedReader fetchProxyMetadata(
connection.setSSLSocketFactory(sslContext.getSocketFactory());
connection.setRequestMethod("GET");
connection.setRequestProperty("host", "localhost");
return new BufferedReader(
new InputStreamReader(connection.getInputStream(), StandardCharsets.UTF_8));
try {
return new BufferedReader(
new InputStreamReader(connection.getInputStream(), StandardCharsets.UTF_8));
} catch (FileNotFoundException e) {
throw new IllegalStateException(
"Metadata service request path not found. Please make sure the metadata service url is correct",
e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change relies on undocumented behaviour (HttpsURLConnection throwing FNFE in this case). We should either find a more conventional way to note this case or else remove the special logging for this specific code path.

} catch (IOException ioe) {
throw new IllegalStateException(
"Unable to read data from cloud metadata service. Please make sure your cluster is not parked or terminated",
ioe);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like for more targeted catch blocks here. As it is the exceptions indicated here could be thrown by either HttpsURLConneciton.getInputStream(), the InputStreamReader constructor or the BufferedReader constructor.

} catch (ConnectException e) {
throw new IllegalStateException(
"Unable to connect to cloud metadata service. Please make sure your cluster is not parked or terminated",
Expand Down
37 changes: 35 additions & 2 deletions .../test/java/com/datastax/oss/driver/internal/core/config/cloud/CloudConfigFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public void should_throw_when_bundle_not_readable() throws Exception {
Throwable t = catchThrowable(() -> cloudConfigFactory.createCloudConfig(configFile));
assertThat(t)
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("Invalid bundle: missing file config.json");
.hasMessage("Invalid bundle: missing file config.json");
}

@Test
Expand All @@ -134,7 +134,40 @@ public void should_throw_when_metadata_not_found() throws Exception {
URL configFile = new URL("http", "localhost", wireMockRule.port(), BUNDLE_PATH);
CloudConfigFactory cloudConfigFactory = new CloudConfigFactory();
Throwable t = catchThrowable(() -> cloudConfigFactory.createCloudConfig(configFile));
assertThat(t).isInstanceOf(FileNotFoundException.class).hasMessageContaining("metadata");
assertThat(t)
.isInstanceOf(IllegalStateException.class)
.hasMessage(
"Metadata service request path not found. Please make sure the metadata service url is correct");
}

@Test
public void should_throw_when_metadata_unauthorized() throws Exception {
// given
mockHttpSecureBundle(secureBundle());
stubFor(any(urlPathEqualTo("/metadata")).willReturn(aResponse().withStatus(401)));
// when
URL configFile = new URL("http", "localhost", wireMockRule.port(), BUNDLE_PATH);
CloudConfigFactory cloudConfigFactory = new CloudConfigFactory();
Throwable t = catchThrowable(() -> cloudConfigFactory.createCloudConfig(configFile));
assertThat(t)
.isInstanceOf(IllegalStateException.class)
.hasMessage(
"Unable to read data from cloud metadata service. Please make sure your cluster is not parked or terminated");
}

@Test
public void should_throw_when_metadata_bad_gateway() throws Exception {
// given
mockHttpSecureBundle(secureBundle());
stubFor(any(urlPathEqualTo("/metadata")).willReturn(aResponse().withStatus(502)));
// when
URL configFile = new URL("http", "localhost", wireMockRule.port(), BUNDLE_PATH);
CloudConfigFactory cloudConfigFactory = new CloudConfigFactory();
Throwable t = catchThrowable(() -> cloudConfigFactory.createCloudConfig(configFile));
assertThat(t)
.isInstanceOf(IllegalStateException.class)
.hasMessage(
"Unable to read data from cloud metadata service. Please make sure your cluster is not parked or terminated");
}

@Test
Expand Down
Loading