-
Notifications
You must be signed in to change notification settings - Fork 69
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
support AWS Web Identity provider #143
Conversation
@zjj2wry Thank you for your contribution. |
Thank you @zjj2wry for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
return nil, err | ||
} | ||
accessKeyID := c.GetProperty("AWS_ACCESS_KEY_ID", "accessKeyID") | ||
|
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.
May be, I would prefer to make it more explicit. If the EKS/serviceaccount authentication should be used it should explicitly be indicated by a dedicated field. This allows to explicitly express that this method should be be used and to provide an appropriate error message for missing credential information if this method is not consciously chosen.
BTW: nice feature for EKS
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.
- https://github.com/aws/amazon-eks-pod-identity-webhook
- EKS IAM Roles for Service Accounts (Pods) aws/containers-roadmap#23
I would prefer to make it more explicit. If the EKS/serviceaccount authentication should be used it should explicitly be indicated by a dedicated field.
@mandelsoft EKS uses a webhook(https://github.com/aws/amazon-eks-pod-identity-webhook) to automatically set the AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE environment variables. if AWS_ACCESS_KEY_ID values are null, will trying another provider. because I want AWS client will be trying all identity provider, so I remove the required AWS_ACCESS_KEY_ID validation.
provide an appropriate error message for missing credential information
AWS SDK will report NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors
if all identity providers can not work. and if permission is not enough, you can see from the log which provider is used
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.
AWS SDK will report
NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors
if all identity providers can not work. and if permission is not enough, you can see from the log which provider is used
This error message gives no clue about the real problem, i.e. that the credentials are missing. This is what @mandelsoft means with "making it more explicit".
I would suggest to change lines 66-90 to
var accessKeyID, secretAccessKey string | |
if os.Getenv("AWS_ROLE_ARN") == "" { | |
accessKeyID, err = c.GetRequiredProperty("AWS_ACCESS_KEY_ID", "accessKeyID") | |
if err != nil { | |
return nil, err | |
} | |
secretAccessKey, err = c.GetRequiredProperty("AWS_SECRET_ACCESS_KEY", "secretAccessKey") | |
if err != nil { | |
return nil, err | |
} | |
} else { | |
accessKeyID = c.GetProperty("AWS_ACCESS_KEY_ID", "accessKeyID") | |
secretAccessKey = c.GetProperty("AWS_SECRET_ACCESS_KEY", "secretAccessKey") | |
} | |
... | |
// only use aws static credentials when accessKeyID and secretAccessKey not null │ │ | |
if accessKeyID != "" && secretAccessKey != "" { | |
c.Logger.Infof("creating aws-route53 handler for %s", accessKeyID) | |
config.Credentials = credentials.NewStaticCredentials(accessKeyID, secretAccessKey, token) | |
} else { | |
c.Logger.Infof("using IAM roles for service accounts on Amazon EKS") | |
} |
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.
@MartinWeindel In this way, users can only use WebIdentityProvider or StaticCredential, if users want to use EC2 meta, we need to add extra code. I hope AWS SDK to control which authentication method is used.
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.
@zjj2wry We need to keep the existing behaviour if the controller is not deployed on AWS. There must be a clear message if the credentials are missing or there is a typo in the secret data keys. Therefore there should be some recognition if it runs on AWS EKS. Otherwise the credentials must be required.
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.
@zjj2wry, if I understand you correctly, you want to be able to use a DNSProvider configured in way so that the deployment environment of the controller (for example EKS webhook injection, or other statically configured credential providers configured for AWS as part of the controller pod) is used to lookup the credentials in some way supported by the SDK.
To make this intention explicit the provider secrets used for the DNSProvider should indicate whether regular credentials should be used (this is the default) or whether the implicit method of the SDK utilizing the deployment environment of the controller pod should be used.
So I would recommend an additional optional field authenticationMethod
/AUTHENTICATION_METHOD
which can have (so far) two values: credentials
(default if field is not specified) or automatic
. If automatic
is chosen, the credentials are not required and the SDK uses its own discovery methods.
In the scenario we are using the controller, it is
- not necessarily running on AWS
- not necessarily using the account used to provision then infrastructure
- using lost of providers configured for completely different accounts.
Therefore it is highly recommended to explicitly express the authentication method to be able to provide appropriate error messages to the end users.
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.
The following code is copied from gardener/machine-controller-manager. This is the way I hope, adding additional parameters will bring complexity. In addition, I hope I can fall back to the static credentials authentication method, now switch authentication I just need to modify the secretBinding values.
func (d *AWSDriver) createSVC() (*ec2.EC2, error) {
var (
config = &aws.Config{
Region: aws.String(d.AWSMachineClass.Spec.Region),
}
accessKeyID = ExtractCredentialsFromData(d.CredentialsData, v1alpha1.AWSAccessKeyID, v1alpha1.AWSAlternativeAccessKeyID)
secretAccessKey = ExtractCredentialsFromData(d.CredentialsData, v1alpha1.AWSSecretAccessKey, v1alpha1.AWSAlternativeSecretAccessKey)
)
if accessKeyID != "" && secretAccessKey != "" {
config.Credentials = credentials.NewStaticCredentialsFromCreds(credentials.Value{
AccessKeyID: accessKeyID,
SecretAccessKey: secretAccessKey,
})
}
s, err := session.NewSession(config)
if err != nil {
return nil, err
}
return ec2.New(s), nil
}
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html
https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-minimum-sdk.html
consider support AWS Web Identity provider,it would help remove static credentials. if you think it is helpful, I can create a PR for the master branch...
Release note: