-
Notifications
You must be signed in to change notification settings - Fork 377
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
Allow auth using IAM Roles for Service Accounts on EKS #126
Conversation
I think the fact that authentication is done per pod doesn't really fit how Crossplane works in general. It essentially means you can only use one SA-enabled provider for the whole Crossplane cluster because there is only 1 Is there a way to overcome this? I image that if we had a controller for
Another possibility is that when we see an SA-enabled provider, we could somehow create another stack-aws |
@muvaf thanks for your thoughts here! Yes, you do understand correctly. While this is a different pattern, it is less about trying to enable using service accounts in general and more about enabling using service accounts and IAM roles the way that EKS does internally. I agree that most Crossplane users will want to use multiple providers, which you could still do here by managing a "core" provider that uses this service account model, and then letting other providers that reference secrets exist as well (potentially with finer scoped permissions). In general, I think your points are valuable for a conversation about how auth works across Crossplane. It might be worth exploring if the current secret model is sustainable long-term. However, I do not think enabling the functionality in this PR hurts our authentication story, and it does enable users who prefer this pattern to utilize it. |
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.
@hasheddan This direction looks good to me. I know you said you wanted a review before we get into style details, but I figured I'd leave a few now rather than later. :)
@@ -33,7 +34,7 @@ type Client interface { | |||
} | |||
|
|||
type cloudFormationClient struct { | |||
cloudformation cfiface.CloudFormationAPI | |||
cloudformation cfiface.ClientAPI |
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.
What's the reason behind this interface change?
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 interfaces were renamed somewhere in the bump from v0.5 to v0.19 in the AWS SDK
amiClient AMIClient | ||
sts *sts.STS | ||
sts *sts.Client |
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.
Ditto here - what's the context for this change?
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.
Same as above
pkg/clients/rds/rds.go
Outdated
cfg, err := awsclients.LoadConfig(credentials, awsclients.DefaultSection, region) | ||
if err != nil { | ||
return nil, err | ||
func NewClient(ctx context.Context, credentials []byte, region string, useSA bool) (Client, error) { |
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.
Style nit: I'd prefer an alternate NewServiceAccountClient
or a varaidic option like UsePodServiceAccount
over this boolean argument.
@muvaf I agree that this is pretty different from how Crossplane is typically configured, but I do think it's a reasonable MVP to solve the immediate problems a few folks have come to us with. It sounds like some orgs aren't permitted to expose their credentials as a Kubernetes |
Alright, but let's make sure this meets their requirements. @hasheddan I assume they use credentials only in this way. Are they OK with using only one IAM role for resource provisioning in a given cluster? They may think that they can deploy more than one Crossplane instance to the same cluster, which is not possible. So, it'd be great if we can confirm what we give is what they want. Ideally, I'd like us to discuss the final place we'd like to be in and how we would upgrade the clusters working with this MVP to that final place. Not really in a defined and planned way but at least very general path like |
@muvaf that’s a great point! @phanimullapudi do the constraints mentioned above still slow for the functionality you desire? |
Note: PR to implement similar functionality in |
Yes using one single IAM role should work for us. |
…ccounts Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
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.
LGTM! Thanks for driving this @hasheddan. I left a few comments, but nothing that needs to block merging. Let me know whether you want to address them or go straight to merge.
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@negz thanks for review! Should be ready for merge now ✔️ |
Could someone please document how to use this feature ? |
@alemairebe I have added a doc describing how to enable this feature. The |
Thank you for this quick answer ! |
@alemairebe So, you'd like to use Crossplane to create an IAM Role with a few policies attached and then assume it in the app pod to access to the RDS. Is my understanding correct? You can create the role via How do you plan to enable pod to assume this role? By attaching an annotation on its service account? |
@muvaf Yes, I would like to use Crossplane to :
|
@alemairebe It should be possible to make this scenario work in Crossplane except for the policy creation part that needs to be done for the IAM role created. I'm working on a set of YAMLs to see how it'd unfold e2e. Will let you know when it's ready. |
Allow auth using IAM Roles for Service Accounts on EKS
Allow auth using IAM Roles for Service Accounts on EKS
glue: regenerate to get update
Description of your changes
This is initial implementation of allowing an AWS
Provider
to authenticate using IAM roles for Service Accounts when runningcrossplane
andstack-aws
on EKS. A few things to note on this early WIP:RDSInstance
here, but other resources will look essentially the samecontext
to be explicitly passed to.Send()
methods. I have updated across all controllers and clients, but am just passingcontext.TODO()
into those controllers that are not yet using the managed reconciler. I think this should be addressed in a separate PR.aws-sdk-go-v2
(which is what we use instack-aws
) does not yet have the functionality implemented in v1 here, I am doing STS auth manually. However, there is indication that v2 will have the new features soon. I normally would say we should wait until it is contributed upstream before adding this feature, but given that we have an explicit request here and we can easily update to consume v2 implementation without changing our API, I think it would be reasonable to go ahead and move forward.Fixes #109 & #110
/cc @phanimullapudi @mfacenet
Checklist
I have:
make reviewable
to ensure this PR is ready for review.app.yaml
to include any new role permissions.