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

fix: add UnsupportedSigningAlgorithmInterceptor to all sigV4a services #1245

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

0marperez
Copy link
Contributor

@0marperez 0marperez commented Mar 6, 2024

Issue #

Description of changes

The UnsupportedSigningAlgorithmInterceptor, which deals with the default signer not supporting sigV4a was only being applied to a hard coded list of services, now the interceptor is applied based on the sigV4a trait.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@0marperez 0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

A new generated diff is ready to view.

@0marperez 0marperez marked this pull request as ready for review March 6, 2024 18:11
@0marperez 0marperez requested a review from a team as a code owner March 6, 2024 18:11
Copy link

github-actions bot commented Mar 6, 2024

A new generated diff is ready to view.

}

// Needs to happen after the SigV4AsymmetricTraitCustomization (-60), which adds the sigV4a trait to models missing it
override val order: Byte = -50
Copy link
Member

Choose a reason for hiding this comment

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

question: Why not just leave it as the default (zero)?

Comment on lines 28 to 30
// Set to true rather than looking for sigV4a in the model here because this integration would be filtered out
// before the application of the trait is done for some services via SigV4AsymmetricTraitCustomization.
// See: CodegenVisitor
Copy link
Member

@lauzadis lauzadis Mar 6, 2024

Choose a reason for hiding this comment

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

This should be a simple fix and I see we have a FIXME already in CodegenVisitor, if you have the time I would recommend including it in this PR. Then we don't need the hardcoded true and we can tighten the conditional.

CodegenVisitor.kt

        integrations = ServiceLoader.load(KotlinIntegration::class.java, classLoader)
            .onEach { integration -> LOGGER.info("Loaded KotlinIntegration: ${integration.javaClass.name}") }
            .sortedBy(KotlinIntegration::order)
...
        for (integration in integrations) {
            if (integration.enabledForService(resolvedModel, settings)) { 
                resolvedModel = integration.preprocessModel(resolvedModel, settings)
            }
        }

Copy link

sonarqubecloud bot commented Mar 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

github-actions bot commented Mar 7, 2024

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@0marperez 0marperez merged commit 786ce03 into main Mar 11, 2024
16 checks passed
@0marperez 0marperez deleted the sigv4a-signer-exception branch March 11, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants