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

Add support for "derived" execution attributes. #4384

Conversation

millems
Copy link
Contributor

@millems millems commented Aug 31, 2023

These execution attributes do not get stored in the execution attribute map. Instead, they read from or write to OTHER execution attributes in the execution attribute map. This allows us to deprecate old execution attributes and keep them in sync with the execution attributes that have replaced them without needing to read or write to the old attributes.

Included in this PR is adding derived execution attributes for all AwsSignerExecutionAttributes, SdkTokenExecutionAttributes and S3SignerExecutionAttributes, as well as deprecation of those classes and their attributes.

Other notable changes:

  1. When the SELECTED_AUTH_SCHEME is already set when we execute the auth scheme interceptor, carry forward any properties that were already set in the selected auth scheme. This allows setting those properties using the old execution attributes early in the request pipeline.
  2. Add dependency on http-auth-aws and http-auth from auth. This allows the mapping from old attributes to new properties to be defined where the old attributes are created. This also sets us up for a potential future where our existing signers can return a new-style signer.
  3. Simplify the decision of when to use the old signing path or the new signing path. For presigners, always use the old signers. For everything else, use the old signers when the customer has overridden the signer OR we're using an old client that doesn't define any auth schemes.
  4. Disallow duplicate signer and identity property names. This prevents weird bugs where setting one property could set another property (e.g. signing name for v4a setting the signing name for v4). This global-uniqueness property already exists in execution attributes.

@gosar
Copy link
Contributor

gosar commented Sep 5, 2023

Disallow duplicate signer and identity property names. This prevents weird bugs where setting one property could set another property (e.g. signing name for v4a setting the signing name for v4).

I think the global-uniqueness may be a bad limitation to have. What if 2 services build their own runtime plugins where they use the same name that's intuitive to each, and now when a customer tries to use both plugins in same application, they get errors? Even if there was some central coordination for AWS services, this will happen more easily with generic SDKs, even without runtime plugins, just with their own signers/identity providers. Say a customer used both AWS and a generic SDK from company1, or multiple generic SDKs from different companies.

This global-uniqueness property already exists in execution attributes.

But identity/signer properties are used as a property bag per AuthSchemeOption, so at a high level, it seems fine if same name is used across signers / identity providers.

Is this a necessary constraint for this PR?

These execution attributes do not get stored in the execution attribute map. Instead, they read
from or write to OTHER execution attributes in the execution attribute map. This allows us to
deprecate old execution attributes and keep them in sync with the execution attributes that have
replaced them without needing to read or write to the old attributes.

Included in this PR is adding derived execution attributes for all AwsSignerExecutionAttributes,
SdkTokenExecutionAttributes and S3SignerExecutionAttributes, as well as deprecation of those
classes and their attributes.

Other notable changes:
1. When the SELECTED_AUTH_SCHEME is already set when we execute the auth scheme interceptor, carry
forward any properties that were already set in the selected auth scheme. This allows setting those
properties using the old execution attributes early in the request pipeline.
2. Add dependency on http-auth-aws and http-auth from auth. This allows the mapping from old
attributes to new properties to be defined where the old attributes are created. This also sets us
up for a potential future where our existing signers can return a new-style signer.
3. Simplify the decision of when to use the old signing path or the new signing path. For
presigners, always use the old signers. For everything else, use the old signers when the customer
has overridden the signer OR we're using an old client that doesn't define any auth schemes.
4. Disallow duplicate signer and identity property names. This prevents weird bugs where setting
one property could set another property (e.g. signing name for v4a setting the signing name for v4).
This global-uniqueness property already exists in execution attributes.
@millems millems force-pushed the backup/millem/derived-execution-attributes branch from c19cc1f to da98374 Compare September 5, 2023 20:51
@millems
Copy link
Contributor Author

millems commented Sep 5, 2023

Closing in favor of non-draft or backup source branch.

@millems millems closed this Sep 5, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 4 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 380 Code Smells

0.0% 0.0% Coverage
5.7% 5.7% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@millems millems deleted the backup/millem/derived-execution-attributes branch February 5, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants