Skip to content

fix(client-s3-control): remove host prefix behavior #7025

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

Merged
merged 4 commits into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,12 @@ test-integration: build-s3-browser-bundle
rm -rf ./clients/client-sso/node_modules/\@smithy # todo(yarn) incompatible redundant nesting.
yarn g:vitest run -c vitest.config.integ.ts
npx jest -c jest.config.integ.js
make test-protocols;
make test-types;
make test-protocols
make test-types
make test-endpoints

test-endpoints:
npx jest -c ./tests/endpoints-2.0/jest.config.js --bail

test-e2e: build-s3-browser-bundle
yarn g:vitest run -c vitest.config.e2e.ts --retry=4
Expand Down
1 change: 0 additions & 1 deletion clients/client-s3-control/src/models/models_1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
StorageLensTag,
Tag,
} from "./models_0";

import { S3ControlServiceException as __BaseException } from "./S3ControlServiceException";

/**
Expand Down
1,110 changes: 1 addition & 1,109 deletions clients/client-s3-control/src/protocols/Aws_restXml.ts

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,12 @@
import software.amazon.smithy.model.shapes.TimestampShape;
import software.amazon.smithy.model.traits.DeprecatedTrait;
import software.amazon.smithy.model.traits.DocumentationTrait;
import software.amazon.smithy.model.traits.EndpointTrait;
import software.amazon.smithy.model.traits.HttpHeaderTrait;
import software.amazon.smithy.model.traits.HttpPayloadTrait;
import software.amazon.smithy.model.traits.StreamingTrait;
import software.amazon.smithy.model.transform.ModelTransformer;
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
import software.amazon.smithy.typescript.codegen.LanguageTarget;
import software.amazon.smithy.typescript.codegen.TypeScriptDependency;
import software.amazon.smithy.typescript.codegen.TypeScriptSettings;
Expand Down Expand Up @@ -79,6 +82,19 @@ public final class AddS3Config implements TypeScriptIntegration {
+ " you need to install the \"@aws-sdk/signature-v4-crt\" package to your project dependencies. \n"
+ "For more information, please go to https://github.com/aws/aws-sdk-js-v3#known-issues</p>";

/**
* Remove the hostPrefix functionality by removing the endpoint traits.
* Only applied if endpointRuleSet trait present on S3/S3Control services.
*/
public static Shape removeHostPrefixTrait(Shape shape) {
return shape.asOperationShape()
.map(OperationShape::shapeToBuilder)
.map(builder -> ((OperationShape.Builder) builder).removeTrait(EndpointTrait.ID))
.map(OperationShape.Builder::build)
.map(s -> (Shape) s)
.orElse(shape);
}

@Override
public List<String> runAfter() {
return List.of(
Expand All @@ -96,6 +112,7 @@ public Model preprocessModel(Model model, TypeScriptSettings settings) {
}

Model.Builder modelBuilder = model.toBuilder();
boolean hasRuleset = !model.getServiceShapesWithTrait(EndpointRuleSetTrait.class).isEmpty();

TopDownIndex topDownIndex = TopDownIndex.of(model);
Set<StructureShape> inputShapes = new HashSet<>();
Expand Down Expand Up @@ -206,7 +223,13 @@ public Model preprocessModel(Model model, TypeScriptSettings settings) {
}
}

return modelBuilder.addShapes(inputShapes).build();
Model builtModel = modelBuilder.addShapes(inputShapes).build();
if (hasRuleset) {
ModelTransformer.create().mapShapes(
builtModel, AddS3Config::removeHostPrefixTrait
);
}
return builtModel;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.function.Function;
import software.amazon.smithy.aws.traits.ServiceTrait;
import software.amazon.smithy.codegen.core.SymbolProvider;
import software.amazon.smithy.model.Model;
Expand All @@ -32,6 +33,7 @@
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.RequiredTrait;
import software.amazon.smithy.model.transform.ModelTransformer;
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
import software.amazon.smithy.typescript.codegen.LanguageTarget;
import software.amazon.smithy.typescript.codegen.TypeScriptSettings;
import software.amazon.smithy.typescript.codegen.TypeScriptWriter;
Expand Down Expand Up @@ -90,15 +92,19 @@ public Model preprocessModel(Model model, TypeScriptSettings settings) {
if (!isS3Control(settings.getService(model))) {
return model;
}
boolean hasRuleset = !model.getServiceShapesWithTrait(EndpointRuleSetTrait.class).isEmpty();
ServiceShape serviceShape = model.expectShape(settings.getService(), ServiceShape.class);
return ModelTransformer.create().mapShapes(model, shape -> {
Function<Shape, Shape> removeRequired = shape -> {
Optional<MemberShape> modified = shape.asMemberShape()
.filter(memberShape -> memberShape.getTarget().getName(serviceShape).equals("AccountId"))
.filter(memberShape -> model.expectShape(memberShape.getTarget()).isStringShape())
.filter(memberShape -> memberShape.isRequired())
.map(memberShape -> Shape.shapeToBuilder(memberShape).removeTrait(RequiredTrait.ID).build());
.filter(memberShape -> memberShape.getTarget().getName(serviceShape).equals("AccountId"))
.filter(memberShape -> model.expectShape(memberShape.getTarget()).isStringShape())
.filter(MemberShape::isRequired)
.map(memberShape -> Shape.shapeToBuilder(memberShape).removeTrait(RequiredTrait.ID).build());
return modified.isPresent() ? modified.get() : shape;
});
};
return ModelTransformer.create().mapShapes(
model, hasRuleset ? removeRequired.andThen(AddS3Config::removeHostPrefixTrait) : removeRequired
);
}

@Override
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
HandlerExecutionContext,
HttpRequest,
Pluggable,
RelativeMiddlewareOptions,
SerializeHandler,
Expand All @@ -9,28 +8,21 @@ import {
SerializeMiddleware,
} from "@smithy/types";

import { deduplicateHostPrefix } from "./deduplicateHostPrefix";

/**
* @internal
* This customization handles an edge case where
* a hostprefix may be duplicated in the endpoint ruleset resolution
* and hostPrefix serialization via the pre-endpoints 2.0 trait,
* and which cannot be reconciled automatically.
* @deprecated - the middleware is no longer necessary since hostPrefix was
* removed by S3Control codegen customization's model preprocessing.
*/
export const hostPrefixDeduplicationMiddleware = (): SerializeMiddleware<any, any> => {
return (next: SerializeHandler<any, any>, context: HandlerExecutionContext): SerializeHandler<any, any> =>
async (args: SerializeHandlerArguments<any>): Promise<SerializeHandlerOutput<any>> => {
const httpRequest: HttpRequest = (args.request ?? {}) as HttpRequest;
if (httpRequest?.hostname) {
httpRequest.hostname = deduplicateHostPrefix(httpRequest.hostname);
}
(args: SerializeHandlerArguments<any>): Promise<SerializeHandlerOutput<any>> => {
return next(args);
};
};

/**
* @internal
* @deprecated
*/
export const hostPrefixDeduplicationMiddlewareOptions: RelativeMiddlewareOptions = {
tags: ["HOST_PREFIX_DEDUPLICATION", "ENDPOINT_V2", "ENDPOINT"],
Expand All @@ -42,6 +34,7 @@ export const hostPrefixDeduplicationMiddlewareOptions: RelativeMiddlewareOptions

/**
* @internal
* @deprecated
*/
export const getHostPrefixDeduplicationPlugin = <T>(config: T): Pluggable<any, any> => ({
applyToStack: (clientStack) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ describe("middleware-sdk-s3-control", () => {

requireRequestsFrom(client).toMatch({
method: "GET",
hostname: "123456789012.s3-outposts.us-west-2.amazonaws.com",
hostname: "s3-outposts.us-west-2.amazonaws.com",
headers: {
host: "123456789012.s3-outposts.us-west-2.amazonaws.com",
Copy link
Contributor

@kuhe kuhe Apr 24, 2025

Choose a reason for hiding this comment

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

  • open question as to whether this is intended 🚧

Copy link
Contributor

Choose a reason for hiding this comment

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

I received confirmation that this change is ok.

host: "s3-outposts.us-west-2.amazonaws.com",
},
protocol: "https:",
path: "/v20180820/bucket",
Expand Down
92 changes: 86 additions & 6 deletions tests/endpoints-2.0/endpoints-integration.spec.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { resolveParams } from "@smithy/middleware-endpoint";
import { EndpointV2 } from "@smithy/types";
import { resolveEndpoint, EndpointParams } from "@smithy/util-endpoints";
import { EndpointV2, RelativeMiddlewareOptions } from "@smithy/types";
import { EndpointParams, resolveEndpoint } from "@smithy/util-endpoints";
import { existsSync, readdirSync } from "fs";
import { join } from "path";

import { EndpointExpectation, ServiceModel, ServiceNamespace } from "./integration-test-types";
import { HttpRequest } from "@smithy/protocol-http";

describe("client list", () => {
const root = join(__dirname, "..", "..");
const clientPackageNameList = readdirSync(join(root, "clients"));
const clientPackageNameList = readdirSync(join(root, "clients")).filter((f) => f.startsWith("client-"));

it("should be at least 300 clients", () => {
expect(clientPackageNameList.length).toBeGreaterThan(300);
Expand Down Expand Up @@ -37,6 +38,7 @@ describe("client list", () => {
function runTestCases(service: ServiceModel, namespace: ServiceNamespace) {
const serviceId = service.traits["aws.api#service"].serviceId;
const testCases = service.traits["smithy.rules#endpointTests"]?.testCases;
const Client: any = Object.entries(namespace).find(([k, v]) => k.endsWith("Client"))![1];

const ruleSet = service.traits["smithy.rules#endpointRuleSet"];
const defaultEndpointResolver = (endpointParams: EndpointParams) => resolveEndpoint(ruleSet, { endpointParams });
Expand All @@ -46,14 +48,23 @@ function runTestCases(service: ServiceModel, namespace: ServiceNamespace) {
const { documentation, params = {}, expect: expectation, operationInputs } = testCase;
params.serviceId = serviceId;

it(documentation || "undocumented testcase", async () => {
const test = Client.name === "DynamoDBClient" && "AccountId" in params ? it.skip : it;

test(documentation || "undocumented testcase", async () => {
if ("endpoint" in expectation) {
const { endpoint } = expectation;
if (operationInputs) {
for (const operationInput of operationInputs) {
const { operationName, operationParams = {} } = operationInput;
const command = namespace[`${operationName}Command`];
const endpointParams = await resolveParams(operationParams, command, params);
const Command = namespace[`${operationName}Command`];
const endpointParams = await resolveParams(operationParams, Command, mapClientConfig(params));

// todo: Use an actual client for a more integrated test.
// todo: This call returns an intercepted EndpointV2 object that can replace the one
// todo: used below.
void useClient;
void [Client, params, Command, operationParams];

const observed = defaultEndpointResolver(endpointParams as EndpointParams);
assertEndpointResolvedCorrectly(endpoint, observed);
}
Expand Down Expand Up @@ -106,3 +117,72 @@ function assertEndpointResolvedCorrectly(expected: EndpointExpectation["endpoint
expect(observed.properties?.authSchemes).toEqual(authSchemes);
}
}

/**
* Makes a client operation return its EndpointV2 instead of making a request.
*/
const requestInterceptorMiddleware = (next: any, context: any) => async (args: any) => {
const { request } = args;
if (HttpRequest.isInstance(request)) {
const endpoint = context.endpointV2;
return {
response: {
statusCode: 200,
},
output: {
...endpoint,
url: {
protocol: request.protocol,
hostname: request.hostname,
pathname: request.path,
href: `${request.protocol}//${request.hostname}${request.path}`,
} as URL,
},
} as {
output: EndpointV2;
};
}
throw new Error("Request must not continue beyond serialization step.");
};
const requestInterceptorMiddlewareOptions: RelativeMiddlewareOptions = {
name: "requestInterceptorMiddleware",
override: true,
toMiddleware: "serializerMiddleware",
relation: "after",
};

const paramMap = {
Region: "region",
UseFIPS: "useFipsEndpoint",
UseDualStack: "useDualstackEndpoint",
ForcePathStyle: "forcePathStyle",
Accelerate: "useAccelerateEndpoint",
DisableMRAP: "disableMultiregionAccessPoints",
DisableMultiRegionAccessPoints: "disableMultiregionAccessPoints",
UseArnRegion: "useArnRegion",
Endpoint: "endpoint",
UseGlobalEndpoint: "useGlobalEndpoint",
};

async function useClient(Client: any, Command: any, clientConfig: any, input: any): Promise<EndpointV2> {
const client = new Client({
...mapClientConfig(clientConfig),
credentials: {
accessKeyId: "ENDPOINTS_TEST",
secretAccessKey: "ENDPOINTS_TEST",
},
});
client.middlewareStack.addRelativeTo(requestInterceptorMiddleware, requestInterceptorMiddlewareOptions);
const command = new Command(input);
const observed: EndpointV2 = await client.send(command);
return observed;
}

function mapClientConfig(params: any) {
return Object.entries(params).reduce((acc: any, cur: [string, any]) => {
const [k, v] = cur;
const key = paramMap[k as keyof typeof paramMap] ?? k;
acc[key] = v;
return acc;
}, {} as any);
}
Loading