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 WithUseFIPSEndpoint to aws.Config #5078

Merged
merged 2 commits into from
Dec 22, 2023

Conversation

reedloden
Copy link
Contributor

The ability to chain UseFIPSEndpoint was missing.

@reedloden
Copy link
Contributor Author

@lucix-aws What's the right way to get review on this?

Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

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

@reedloden This is fine but it needs to take a bool like WithUseDualStack does since it's an explicit call.

@lucix-aws lucix-aws added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 22, 2023
@reedloden
Copy link
Contributor Author

@reedloden This is fine but it needs to take a bool like WithUseDualStack does since it's an explicit call.

UseFIPSEndpoint is not a boolean, though. It's an enum. See

aws-sdk-go/aws/config.go

Lines 241 to 242 in 15ccb94

// UseFIPSEndpoint specifies the resolver must resolve a FIPS endpoint.
UseFIPSEndpoint endpoints.FIPSEndpointState
and
// FIPSEndpointState is a constant to describe the FIPS endpoint resolution behavior.
type FIPSEndpointState uint
const (
// FIPSEndpointStateUnset is the default value behavior for FIPS endpoint resolution.
FIPSEndpointStateUnset FIPSEndpointState = iota
// FIPSEndpointStateEnabled enables FIPS endpoint resolution for service endpoints.
FIPSEndpointStateEnabled
// FIPSEndpointStateDisabled disables FIPS endpoint resolution for endpoints.
FIPSEndpointStateDisabled
)
. Need to be able to explicitly set it to whichever state is needed.

UseDualStack does take a bool, but note that it's deprecated. UseDualStackEndpoint is its replacement, and it takes an enum as well.

@reedloden reedloden requested a review from lucix-aws December 22, 2023 18:53
@lucix-aws
Copy link
Contributor

Yes, Config.UseDualStack is deprecated, but its WithUseDualStack that takes a bool (again, since it's something you'd explicitly call) is not. That's what I'm looking to match here API-wise.

Copy link
Contributor

@lucix-aws lucix-aws left a comment

Choose a reason for hiding this comment

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

(see above)

@reedloden reedloden force-pushed the reed/withusefipsendpoint branch from 1325129 to f58c108 Compare December 22, 2023 19:35
@reedloden reedloden requested a review from lucix-aws December 22, 2023 19:40
@reedloden
Copy link
Contributor Author

Yes, Config.UseDualStack is deprecated, but its WithUseDualStack that takes a bool (again, since it's something you'd explicitly call) is not. That's what I'm looking to match here API-wise.

Ok. I was trying to provide all three options to users (to allow them to even unset it if it had been previously set), but this will still be an improvement. Updated based on what I believe you're looking for.

@lucix-aws
Copy link
Contributor

Yes, Config.UseDualStack is deprecated, but its WithUseDualStack that takes a bool (again, since it's something you'd explicitly call) is not. That's what I'm looking to match here API-wise.

Ok. I was trying to provide all three options to users (to allow them to even unset it if it had been previously set), but this will still be an improvement. Updated based on what I believe you're looking for.

Yeah I see where you were coming from. The whole State thing we have for a few fields is an unfortunate legacy decision we used to disambiguate between an unset value and the zero value, it's really not at all necessary given the default for these settings is usually false, but it's established.

@lucix-aws lucix-aws removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 22, 2023
@lucix-aws lucix-aws merged commit 04c15fc into aws:main Dec 22, 2023
9 checks passed
@reedloden reedloden deleted the reed/withusefipsendpoint branch December 22, 2023 21:12
aws-sdk-go-automation pushed a commit that referenced this pull request Dec 26, 2023
===

### Service Client Updates
* `service/iam`: Updates service documentation
  * Documentation updates for AWS Identity and Access Management (IAM).

### SDK Enhancements
* `aws`: Add `WithUseFIPSEndpoint` to `aws.Config`. ([#5078](#5078))
  * `WithUseFIPSEndpoint` can be used to explicitly enable or disable FIPS endpoint variants.
aws-sdk-go-automation added a commit that referenced this pull request Dec 26, 2023
Release v1.49.10 (2023-12-26)
===

### Service Client Updates
* `service/iam`: Updates service documentation
  * Documentation updates for AWS Identity and Access Management (IAM).

### SDK Enhancements
* `aws`: Add `WithUseFIPSEndpoint` to `aws.Config`. ([#5078](#5078))
  * `WithUseFIPSEndpoint` can be used to explicitly enable or disable FIPS endpoint variants.
reedloden added a commit to gravitational/teleport that referenced this pull request Jan 2, 2024
Decided to keep the current code as-is, so removing the TODOs

aws/aws-sdk-go#5078 was landed upstream, so bump `aws-sdk-go` to pick up the change.
Note that this new functionality is not used, but would like to keep it updated
so it can possibly be used in the future without needing to bump the SDK version.
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