-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Modify http cred provider resolving logic #4961
Changes from 16 commits
a71bedc
f08fc0b
f4f3fc7
b4fc961
d5735f7
0be20ae
4755c89
ecfb8f5
8e3829b
807c9ee
c8ccaaf
e3b83f4
460e716
bcc4451
98515f3
47a4701
7195c34
0b32bd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,8 @@ package endpointcreds | |
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
"time" | ||
|
||
"github.com/aws/aws-sdk-go/aws" | ||
|
@@ -69,7 +71,37 @@ type Provider struct { | |
|
||
// Optional authorization token value if set will be used as the value of | ||
// the Authorization header of the endpoint credential request. | ||
// | ||
// When constructed from environment, the provider will use the value of | ||
// AWS_CONTAINER_AUTHORIZATION_TOKEN environment variable as the token | ||
// | ||
// Will be overridden if AuthorizationTokenProvider is configured | ||
AuthorizationToken string | ||
|
||
// Optional auth provider func to dynamically load the auth token from a file | ||
// everytime a credential is retrieved | ||
// | ||
// When constructed from environment, the provider will read and use the content | ||
// of the file pointed to by AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE environment variable | ||
// as the auth token everytime credentials are retrieved | ||
// | ||
// Will override AuthorizationToken if configured | ||
AuthorizationTokenProvider AuthTokenProvider | ||
} | ||
|
||
// AuthTokenProvider defines an interface to dynamically load a value to be passed | ||
// for the Authorization header of a credentials request. | ||
type AuthTokenProvider interface { | ||
GetToken() (string, error) | ||
} | ||
|
||
// TokenProviderFunc is a func type implementing AuthTokenProvider interface | ||
// and enables customizing token provider behavior | ||
type TokenProviderFunc func() (string, error) | ||
|
||
// GetToken func retrieves auth token according to TokenProviderFunc implementation | ||
func (p TokenProviderFunc) GetToken() (string, error) { | ||
return p() | ||
} | ||
|
||
// NewProviderClient returns a credentials Provider for retrieving AWS credentials | ||
|
@@ -164,7 +196,20 @@ func (p *Provider) getCredentials(ctx aws.Context) (*getCredentialsOutput, error | |
req := p.Client.NewRequest(op, nil, out) | ||
req.SetContext(ctx) | ||
req.HTTPRequest.Header.Set("Accept", "application/json") | ||
if authToken := p.AuthorizationToken; len(authToken) != 0 { | ||
|
||
authToken := p.AuthorizationToken | ||
lucix-aws marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var err error | ||
if p.AuthorizationTokenProvider != nil { | ||
authToken, err = p.AuthorizationTokenProvider.GetToken() | ||
if err != nil { | ||
return nil, fmt.Errorf("get authorization token: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont understand what this error message is conveying There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
|
||
if strings.ContainsAny(authToken, "\r\n") { | ||
return nil, fmt.Errorf("authorization token contains invalid newline sequence") | ||
} | ||
if len(authToken) != 0 { | ||
req.HTTPRequest.Header.Set("Authorization", authToken) | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.
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,
And this
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: