Skip to content
This repository was archived by the owner on Jul 31, 2025. It is now read-only.

Modify http cred provider resolving logic#4961

Merged
lucix-aws merged 18 commits intomainfrom
feat-http-cred
Nov 13, 2023
Merged

Modify http cred provider resolving logic#4961
lucix-aws merged 18 commits intomainfrom
feat-http-cred

Conversation

@wty-Bryant
Copy link
Copy Markdown
Contributor

Modify http cred provider resolving logic to allow EKS/ECS host url and auth token from a file, which can be retrieved from env var

@wty-Bryant wty-Bryant marked this pull request as ready for review August 23, 2023 19:53
aws-sdk-go-automation and others added 5 commits August 29, 2023 14:21
Release v1.44.332 (2023-08-25)
===

### Service Client Updates
* `service/cloudtrail`: Updates service API and documentation
  * Add ThrottlingException with error code 429 to handle CloudTrail Delegated Admin request rate exceeded on organization resources.
* `service/detective`: Updates service API
* `service/monitoring`: Updates service documentation
  * Doc-only update to get doc bug fixes into the SDK docs
Release v1.44.333 (2023-08-28)
===

### Service Client Updates
* `service/backup`: Updates service API and documentation
* `service/compute-optimizer`: Updates service API and documentation
* `service/organizations`: Updates service documentation
  * Documentation updates for permissions and links.
* `service/securitylake`: Updates service API
* `service/service-quotas`: Updates service API and documentation
* `service/workspaces-web`: Updates service API and documentation
Release v1.44.334 (2023-08-29)
===

### Service Client Updates
* `service/cognito-idp`: Updates service API, documentation, and examples
* `service/fsx`: Updates service documentation
* `service/omics`: Updates service API and documentation
* `service/sesv2`: Updates service API, documentation, paginators, and examples
Copy link
Copy Markdown
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.

Looks good overall. Approved with minor changes, but let's hold this in review until EKS has tested w/ it.

@isaiahvita
Copy link
Copy Markdown
Contributor

i dont understand the PR description. what does this mean:

allow EKS/ECS host url and auth token from a file

@wty-Bryant
Copy link
Copy Markdown
Contributor Author

i dont understand the PR description. what does this mean:

allow EKS/ECS host url and auth token from a file

It means (1) allow eks/ecs host url while creating http credential provider (2) enable retrieving authorization token from both env var and auth token file


### SDK Bugs
* `aws/defaults`: Modify http credential provider resolving logic to allow EKS/ECS container host and auth token file.
* Fix http cred provider only allows local hosts endpoint and auth token directly set in env var No newline at end of file
Copy link
Copy Markdown
Contributor

@isaiahvita isaiahvita Sep 1, 2023

Choose a reason for hiding this comment

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

i understand that short-hand is often effective in our own internal conversations. but with customer-facing release notes, lets use correct grammar with complete thoughts. Additionally, since this is more of a feature and not a simple bug fix, the language should emphasize the overall problem being solved.

Modify http credential provider resolving logic to allow EKS/ECS container host and auth token file.

The way this is written sounds like this PR is just fixing a bug. However, as i understand it, were actually adding a feature to the HTTP credential provider. Since this is a feature, and not merely a bug-fix, it should relate back to the higher-level problem. For example,

In the HTTP credential provider, support is added for the EKS container host URL along with support for dynamic loading of an authorization token from a file or environment variable.

And this

Fix http cred provider only allows local hosts endpoint and auth token directly set in env var

This reads like a run-on sentence and sounds like its just explaining implementation details. And because its a run-on sentence, im not sure if auth token directly set in env var is something that already exists or is something that you are adding.

Since most of the feature is explained in what I wrote above, you just add a clarification here for the motivation. For example:

These changes enables use of IRSAv2 AuthNZ in EKS.

if p.AuthorizationTokenProvider != nil {
authToken, err = p.AuthorizationTokenProvider.GetToken()
if err != nil {
return nil, fmt.Errorf("get authorization token: %v", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i dont understand what this error message is conveying

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is just standard error wrapping to include context for the underlying error. I had previously asked that we get better about doing this, it makes identifying failure points a lot easier.

@lucix-aws lucix-aws merged commit a07f333 into main Nov 13, 2023
@lucix-aws lucix-aws deleted the feat-http-cred branch November 13, 2023 22:29
ameukam added a commit to ameukam/kops that referenced this pull request Jan 19, 2024
This is done to fix authentication issues related to EKS Pod identity.

Picking aws/aws-sdk-go#4961

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
zetaab pushed a commit to zetaab/kops that referenced this pull request Jan 22, 2024
This is done to fix authentication issues related to EKS Pod identity.

Picking aws/aws-sdk-go#4961

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants