Add feature: Custom IAM Roles #2139
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Hi @sp-borja-juncosa. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
I signed it! |
k8s-ci-robot
added
cncf-cla: yes
This way Cluster IAM roles can be managed externally, either manually, using cloudformation or any other tool.
sp-borja-juncosa
force-pushed
the
kops/custom_iam_role
branch
from
d3a9548 to
81229af
Compare
|
We do have the additional IAM policy for extending the "core" roles. Can I ask what the use-case is here? |
|
Hi @justinsb... In a lot of the larger companies role/user/group/policy management is limited to the security teams. I'd say this feature gives the user a lot more flexibility than the "additional policies" feature. To my company this is a must have feature if we want to use Kops. Furthermore, it's a very simple workaround for this bug: #2134 Cheers |
|
Hi @justinsb , as @tiadobatima stated, there are some companies where IAM roles can only be managed through 1 team or 1 software. That's why we thought it would be nice to have the ability to use already created IAM roles. On the other hand there's also the ability to limit the permissions to the minimum necessary for each cluster. For example, masters have the ability to do anything on ec2 and not all clusters may need those permissions. Best regards, |
|
Another use case is described in #1166 . Specifically, those wishing to use kube2iam are required to manually set up some trust relationships between roles after kops has created the "core" roles. If core roles could be setup before-hand with those relationships, and merely referenced when creating the cluster, this could all be automated. |
|
👍 on those above use cases. Currently within our organization, one team manages the creation of such roles, and in order for us to use kops - we would need the feature to use existing roles instead of creating them from scratch. |
|
Any idea when this issue will be accepted? |
|
+1 |
4 similar comments
|
+1 |
|
+1 |
|
+1 |
|
+1 |
|
This feature would be a welcome addition. |
|
I also really need it |
| @@ -147,6 +149,10 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command { | |||
|
|
|||
| cmd.Flags().StringVar(&options.MasterSize, "master-size", options.MasterSize, "Set instance size for masters") | |||
|
|
|||
| cmd.Flags().StringVar(&options.NodeIAMRole, "node-iam-role", options.NodeIAMRole, "Set IAM role for nodes") | |||
There was a problem hiding this comment.
I would say that we leave this at a yaml / API / clusterspec level. Not sure if we want more cli flags.
There was a problem hiding this comment.
Tested it myself with the cli flags, but fine if it ends up in the yaml
There was a problem hiding this comment.
Hi, @sp-borja-juncosa is on vacations, but I guess that it could be easy to remove the cli flags.
There was a problem hiding this comment.
@sp-andres-diaz that would be awesome ... please and thank-you
| @@ -132,6 +132,9 @@ type ClusterSpec struct { | |||
| // missing: default policy (currently OS security upgrades that do not require a reboot) | |||
| UpdatePolicy *string `json:"updatePolicy,omitempty"` | |||
|
|
|||
| // Use custom IAM roles for cluster roles | |||
| RoleCustomIamRoles map[string]string `json:"roleCustomIamRoles,omitempty"` | |||
There was a problem hiding this comment.
Do you mean to change the variable name?
There was a problem hiding this comment.
Yah ... sorry was not clear. How about the name CustomIamRoles or even IamRoles?
|
We are going to need something me documentation as well ;) I do not have the issue handy but I started to document the IAM perms needed. Thanks for the work, as I was just about to implement something like this as well. @justinsb any concerns? |
|
@justinsb do we want to feature flag this? |
|
@k8s-bot ok to test |
|
TODO List ... Cause we have like three separate threads going.
|
|
@sp-andres-diaz when is @@sp-borja-juncosa back? |
There was a problem hiding this comment.
Please use a feature flag on this.
It has been mentioned that there is concern that this functionality is going to give users of kops the power to completely mess up clusters. I am not saying we do not need this, I am asking how do we provide an awesome user experience.
@justinsb feedback?
Here are some challenges for you:
- How do we tell the user that the better know wth they are doing? Log the heck out of this? What else should we do?
- How do we provide a tool to make this easier? Like, create the roles? What would a users sec group maintain this? An idea is to output a JSON representation of the IAM role.
- I want to get this in and iterate, what can you take on?
I am not asking to do all of this work in the PR, just we need to plan, maintain and collaborate. Let me know and I will dump these comments into issues.
Bottom line is that we need this functionality. The question is how do we make it easy and bulletproof.
|
|
||
| // If we've specified an IAMRoleArn for this cluster role, | ||
| // do not create a new one | ||
| if customIamRoles[roleAsString] != "" { |
There was a problem hiding this comment.
Review Notes
I do need to have you put this functionality behind a feature flag.
- Here is our implementation of feature flags: https://github.com/kubernetes/kops/blob/77fbf9cbf9003d89d2e157027ac1e650c1798c60/pkg/featureflag/featureflag.go
- here is the documentation about one
kops/cmd/kops/rollingupdatecluster.go
Line 105 in 77fbf9c
- here is an example of use
kops/cmd/kops/rollingupdatecluster.go
Line 119 in 77fbf9c
|
Re good ux, you can run iam simulate API to validate custom role has permissions that will be used. |
|
Hi, Thanks for the interest of the PR. Unfortunately we don't have a lot of time to make all the changes proposed, but please, feel free to change or adapt the code as you need. The following code is the cloudformation template that we use to create custom roles with the minimum permissions (for masters and nodes) to start and operate the cluster (AFAIK), I hope that these are useful too. The masters one: {
"AWSTemplateFormatVersion": "2010-09-09",
"Description": "Custom masters Role",
"Parameters": {
},
"Mappings": {
},
"Resources": {
"MastersRole": {
"Type": "AWS::IAM::Role",
"Properties": {
"RoleName": "cmasters.kops.lab",
"AssumeRolePolicyDocument": {
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Service": [
"ec2.amazonaws.com"
]
},
"Action": [
"sts:AssumeRole"
]
}
]
},
"Path": "/"
}
},
"MastersPolicy": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyName": "cmasters.kops.lab",
"Roles": [ { "Ref": "MastersRole" } ],
"PolicyDocument": {
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"ecr:GetAuthorizationToken",
"ecr:BatchCheckLayerAvailability",
"ecr:GetDownloadUrlForLayer",
"ecr:GetRepositoryPolicy",
"ecr:DescribeRepositories",
"ecr:ListImages",
"ecr:BatchGetImage"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"ec2:Describe*",
"ec2:Attach*",
"ec2:Detach*",
"ec2:ModifyInstanceAttribute",
"ec2:CreateRoute",
"ec2:DeleteRoute",
"ec2:*SecurityGroup*",
"ec2:CreateTags"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"elasticloadbalancing:*"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"autoscaling:DescribeAutoScalingGroups",
"autoscaling:DescribeAutoScalingInstances",
"autoscaling:SetDesiredCapacity",
"autoscaling:TerminateInstanceInAutoScalingGroup"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
"route53:GetHostedZone"
],
"Resource": [
"arn:aws:route53:::hostedzone/XXXXXXXX"
]
},
{
"Effect": "Allow",
"Action": [
"route53:GetChange"
],
"Resource": [
"arn:aws:route53:::change/*"
]
},
{
"Effect": "Allow",
"Action": [
"route53:ListHostedZones"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"s3:*"
],
"Resource": [
"arn:aws:s3:::kops.lab/*kops.lab",
"arn:aws:s3:::kops.lab/*kops.lab/*"
]
},
{
"Effect": "Allow",
"Action": [
"s3:GetBucketLocation",
"s3:ListBucket"
],
"Resource": [
"arn:aws:s3:::kops.lab"
]
}
]
}
}
}
},
"Outputs": {
}
}
The nodes one: {
"AWSTemplateFormatVersion": "2010-09-09",
"Description": "Custom nodes Role",
"Parameters": {
},
"Mappings": {
},
"Resources": {
"NodesRole": {
"Type": "AWS::IAM::Role",
"Properties": {
"RoleName": "cnodes.kops.lab",
"AssumeRolePolicyDocument": {
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Service": [
"ec2.amazonaws.com"
]
},
"Action": [
"sts:AssumeRole"
]
}
]
},
"Path": "/"
}
},
"NodesPolicy": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyName": "cnodes.kops.lab",
"Roles": [ { "Ref": "NodesRole" } ],
"PolicyDocument": {
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"ec2:Describe*"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"ecr:GetAuthorizationToken",
"ecr:BatchCheckLayerAvailability",
"ecr:GetDownloadUrlForLayer",
"ecr:GetRepositoryPolicy",
"ecr:DescribeRepositories",
"ecr:ListImages",
"ecr:BatchGetImage"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"route53:ChangeResourceRecordSets",
"route53:ListResourceRecordSets",
"route53:GetHostedZone"
],
"Resource": [
"arn:aws:route53:::hostedzone/XXXXXXXXXX"
]
},
{
"Effect": "Allow",
"Action": [
"route53:GetChange"
],
"Resource": [
"arn:aws:route53:::change/*"
]
},
{
"Effect": "Allow",
"Action": [
"route53:ListHostedZones"
],
"Resource": [
"*"
]
},
{
"Effect": "Allow",
"Action": [
"s3:*"
],
"Resource": [
"arn:aws:s3:::kops.lab/*kops.lab",
"arn:aws:s3:::kops.lab/*kops.lab/*"
]
},
{
"Effect": "Allow",
"Action": [
"s3:GetBucketLocation",
"s3:ListBucket"
],
"Resource": [
"arn:aws:s3:::kops.lab"
]
}
]
}
}
}
},
"Outputs": {
}
}
|
|
No problemo! Can you leave you branch and I pull in a branch, and knock the work out! Thanks so much for starting the ball rolling, we will get this into kops in 1.6.1 or maybe sooner ;) Thanks |
|
per @sp-borja-juncosa closing this. opened #2440 |
|
Sorry for the delay on this one. I've been thinking about it, and I think the issue is that we need to make sure that we don't create an unmaintainable product. At the same time, we want to let users customize everything. I think the way we keep this maintainable is that we have to solve the problem of "how do I use a custom IAM role". So this is half of it, in that we can use a custom IAM role, but we should document and probably create a command for how that role should be created. We previously had This has a few advantages:
Again I apologize for the delay - I've been trying to formulate this, and it's taken a while and some discussions for it to crystallize... I do think this is a great feature, we just have to make sure that we don't add a feature and then all progress stops because it is too hard to use. For the record, I think we probably need to do the same thing for networking, because that also causes a lot of support time, because it is hard :-) |
|
Did not close this as I mentioned. I forked this PR and made some tweak, and it has been submitted. @justinsb I am going to copy you comment into an issue that I am working on so we can maintain the information till the work is completed. |
This way Cluster IAM roles can be managed externally, either manually,
using cloudformation or any other tool.
This change is