Skip to content

Bind testing server to 0.0.0.0 #172

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

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Bind testing server to 0.0.0.0 #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 23 additions & 10 deletions trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/rest/TestProxiedErrorResponses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,16 @@
import io.trino.aws.proxy.server.remote.PathStyleRemoteS3Facade;
import io.trino.aws.proxy.server.testing.TestingRemoteS3Facade;
import io.trino.aws.proxy.server.testing.TestingTrinoAwsProxyServer.Builder;
import io.trino.aws.proxy.server.testing.TestingUtil.ForTesting;
import io.trino.aws.proxy.server.testing.harness.BuilderFilter;
import io.trino.aws.proxy.server.testing.harness.TrinoAwsProxyTest;
import io.trino.aws.proxy.spi.credentials.Credentials;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.auth.credentials.AwsSessionCredentials;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.S3Exception;

Expand All @@ -39,13 +43,13 @@
import java.util.Optional;

import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static io.trino.aws.proxy.server.testing.TestingUtil.clientBuilder;
import static io.trino.aws.proxy.server.testing.TestingUtil.createTestingHttpServer;
import static io.trino.aws.proxy.server.testing.TestingUtil.getFileFromStorage;
import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import static java.util.Objects.requireNonNull;
import static java.util.function.Function.identity;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
Expand Down Expand Up @@ -95,12 +99,21 @@ public Builder filter(Builder builder)
}

@Inject
public TestProxiedErrorResponses(S3Client internalClient, TestingRemoteS3Facade delegatingFacade, @ForErrorResponseTest TestingHttpServer httpErrorResponseServer)
public TestProxiedErrorResponses(TestingRemoteS3Facade delegatingFacade, @ForErrorResponseTest TestingHttpServer httpErrorResponseServer, @ForTesting Credentials testingCredentials)
{
this.internalClient = requireNonNull(internalClient, "internal client is null");
internalClient = clientBuilder(httpErrorResponseServer.getBaseUrl())
.forcePathStyle(true)
.credentialsProvider(() -> AwsSessionCredentials.create(testingCredentials.emulated().accessKey(), testingCredentials.emulated().secretKey(), testingCredentials.emulated().session().orElse("")))
.build();
delegatingFacade.setDelegate(new PathStyleRemoteS3Facade((_, _) -> httpErrorResponseServer.getBaseUrl().getHost(), false, Optional.of(httpErrorResponseServer.getBaseUrl().getPort())));
Comment on lines +104 to 108
Copy link
Contributor

Choose a reason for hiding this comment

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

In this state, this doesn't test anything, right?

The point was to test that aws-proxy propagates errors from the remote S3 correctly.

What this is doing currently is bypassing the aws-proxy and calling httpErrorResponseServer with an S3 client

Copy link
Member Author

Choose a reason for hiding this comment

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

Derp - I messed it up I think. Feel free to submit a fixup PR. I probably didn't understand the test.

}

@AfterEach
Copy link
Member

@martint martint Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is the constructor invoked once per test? It's not obvious from the test whether it's @TestInstance(PER_CLASS) or @TestInstance(PER_METHOD). I'd suggest annotating the class to make it explicit.

Copy link
Member Author

@Randgalt Randgalt Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@TrinoAwsProxyTest implies per class (and other settings).

@Retention(RUNTIME)
@Target(TYPE)
@Inherited
@ExtendWith(TrinoAwsProxyTestExtension.class)
@TestInstance(PER_CLASS)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... if its @TestInstance(PER_CLASS), then it's not going to work with this shutdown method, which is going to be called after each individual test. You need to use @AfterAll, instead.

public void shutdown()
{
internalClient.close();
}

@Test
public void test()
{
Expand All @@ -121,13 +134,13 @@ private static class HttpErrorResponseServlet
extends HttpServlet
{
private static final String RESPONSE_TEMPLATE = """
<?xml version="1.0" encoding="UTF-8"?>
<Error>
<Code>%s</Code>
<Message>Error Message</Message>
<Resource>%s</Resource>
<RequestId>123</RequestId>
</Error>""";
<?xml version="1.0" encoding="UTF-8"?>
<Error>
<Code>%s</Code>
<Message>Error Message</Message>
<Resource>%s</Resource>
<RequestId>123</RequestId>
</Error>""";

private static final Map<String, HttpStatus> PATH_STATUS_CODE_MAPPING = STATUS_CODES.stream().collect(toImmutableMap(status -> "/status/%d".formatted(status.code()), identity()));

Expand Down
4 changes: 3 additions & 1 deletion trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/TestingUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.airlift.http.server.HttpServerConfig;
import io.airlift.http.server.HttpServerInfo;
import io.airlift.http.server.testing.TestingHttpServer;
import io.airlift.node.NodeConfig;
import io.airlift.node.NodeInfo;
import io.trino.aws.proxy.spi.credentials.Credential;
import io.trino.aws.proxy.spi.credentials.Credentials;
Expand Down Expand Up @@ -157,7 +158,8 @@ public static String sha256(String content)
public static TestingHttpServer createTestingHttpServer(Servlet servlet)
throws IOException
{
NodeInfo nodeInfo = new NodeInfo("test");
NodeConfig nodeConfig = new NodeConfig().setNodeBindIp("0.0.0.0").setEnvironment("test");
NodeInfo nodeInfo = new NodeInfo(nodeConfig);
HttpServerConfig config = new HttpServerConfig().setHttpPort(0);
HttpServerInfo httpServerInfo = new HttpServerInfo(config, nodeInfo);
return new TestingHttpServer(httpServerInfo, nodeInfo, config, servlet);
Expand Down