Split AWS/IAM policies for master and workers #370

rossf7 · 2017-08-08T15:15:54Z

Fixes #330

This PR splits the master and worker IAM policies and makes 2 permissions changes. These have been tested on a locally launched cluster.

The worker policy has reduced EC2 permissions as per the docs to only support attaching and detaching volumes.

https://github.com/kubernetes/kubernetes/tree/release-1.5/cluster/aws/templates/iam

ECR permissions are only applied to workers since the master cannot schedule pods.

rossf7 · 2017-08-08T15:24:24Z

resources/aws/policy.go

+		"Version": "2012-10-17",
+		"Statement": [
+			{
+				"Effect": "Allow",


Here are the reduced EC2 permissions.

rossf7 · 2017-08-08T15:24:33Z

resources/aws/policy.go

 	AWSEntity
 }

 func (p *Policy) clusterPolicyName() string {
-	return fmt.Sprintf("%s-%s", p.ClusterID, PolicyNameTemplate)
+	return fmt.Sprintf("%s-%s-%s", p.ClusterID, p.PolicyType, PolicyNameTemplate)


AWS identifiers now also contain the policy type.

rossf7 · 2017-08-08T15:24:42Z

resources/aws/policy.go


+	policyDocument := fmt.Sprintf(policyTemplate, p.KMSKeyArn, p.S3Bucket, p.S3Bucket)
+	clusterRoleName := p.clusterRoleName()


clusterRoleName now uses the helper func

rossf7 · 2017-08-08T15:25:08Z

resources/aws/policy.go

 	if _, err := p.Clients.IAM.PutRolePolicy(&iam.PutRolePolicyInput{
-		PolicyName:     aws.String(clusterPolicyName),
+		PolicyName:     aws.String(p.clusterPolicyName()),


As does the policy name.

rossf7 · 2017-08-08T15:25:23Z

service/create/service.go

 	bucketName := s.bucketName(cluster)

-	var policy resources.NamedResource
-	var policyErr error
+	// Create master IAM policy


Here both policies are now created.

Every time someone writes a sentence without using a full stop a pomsky puppy dies.

rossf7 · 2017-08-08T15:26:02Z

service/create/service.go

@@ -759,7 +777,7 @@ func (s *Service) addFunc(obj interface{}) {
 		securityGroup:       mastersSecurityGroup,
 		subnet:              publicSubnet,
 		keyPairName:         cluster.Name,
-		instanceProfileName: policy.GetName(),
+		instanceProfileName: masterPolicy.GetName(),


Here the master EC2 instance uses the new profile.

rossf7 · 2017-08-08T15:26:20Z

service/create/service.go

@@ -871,7 +889,7 @@ func (s *Service) addFunc(obj interface{}) {
 		securityGroup:       workersSecurityGroup,
 		subnet:              publicSubnet,
 		keypairName:         cluster.Name,
-		instanceProfileName: policy.GetName(),
+		instanceProfileName: workerPolicy.GetName(),


Here the workers launch config uses the new policy.

xh3b4sd

These changes make sense to me. Lots of details I am not into but it is ok now. Just left some comments to say something. :D Thanks for the efforts. <3

xh3b4sd · 2017-08-09T06:22:38Z

resources/aws/policy.go

+				"Action": [
+					"s3:ListBucket"
+				],
+				"Resource": "arn:aws:s3:::%s"


When we add more items that have to be dynamically placed here we should consider making this a real golang template. It is going to be hard to see how many format placeholders are in here.

Yes I think we should move to golang templates. The templates will need more logic when we make the ECR permissions configurable. We may also need to add templates for Cloud Formation.

xh3b4sd · 2017-08-09T06:24:26Z

service/create/service.go

 	bucketName := s.bucketName(cluster)

-	var policy resources.NamedResource
-	var policyErr error
+	// Create master IAM policy


Every time someone writes a sentence without using a full stop a pomsky puppy dies.

xh3b4sd · 2017-08-09T06:26:26Z

service/create/service.go

-			S3Bucket:  bucketName,
-			AWSEntity: awsresources.AWSEntity{Clients: clients},
+		masterPolicy = &awsresources.Policy{
+			ClusterID:  cluster.Spec.Cluster.Cluster.ID,


In some other operators we created key methods for data that is often accessed. That maybe helps at some point reducing the hustle to find out where what data is located within a structure. One day it also might help refactoring or changing the data's location within the structure.

Good point I've seen the getKeys methods in the other operators. There is much refactoring to be done I'll keep this in mind.

xh3b4sd · 2017-08-09T06:29:44Z

service/create/service.go

@@ -824,7 +842,7 @@ func (s *Service) addFunc(obj interface{}) {
 	// If the policy couldn't be created and some instances didn't exist before, that means that the cluster
 	// is inconsistent and most problably its deployment broke in the middle during the previous run of
 	// aws-operator.
-	if anyMastersCreated && (kmsKeyErr != nil || policyErr != nil) {
+	if anyMastersCreated && (kmsKeyErr != nil || masterPolicyErr != nil || workerPolicyErr != nil) {


I am wondering why we do not handle all these errors separately. I think these errors should be handled somehow even if there are no masters created. I also do not see where these errors come from, so this could be clearer. I am not into the details here but I think errors should immediately handled when they occur. This might be a candidate for improvements when we move to the operatorkit reconciliation framework.

There is an issue for whether this check is still needed. I added the new errors here for completeness.

#310

xh3b4sd · 2017-08-09T06:31:17Z

service/create/service.go

+		AWSEntity:  awsresources.AWSEntity{Clients: clients},
+	}
+	if err := masterPolicy.Delete(); err != nil {
+		s.logger.Log("error", fmt.Sprintf("%#v", err))


IMO we should retry on errors and not move forward. This would also come for free with the reconciliation framework.

At the moment the retry logic has only been added where needed. So I don't think its needed in this case. But using the framework so all resources have retry support would be a big improvement.

rossf7 · 2017-08-09T07:31:53Z

@xh3b4sd Comments are tidied up :)

There were many full stops missing in the add and delete funcs. So the lives of many puppies were saved ;)

hobti01 · 2017-08-09T08:02:08Z

Thanks for adding the permissions to pull images from AWS ECR (#364)!

rossf7 · 2017-08-09T08:06:22Z

@hobti01 You're welcome and thanks again for your PR!

rossf7 added 3 commits August 8, 2017 16:13

Policy resource creates separate master and worker policies

30250fc

Service create separate master and worker policies

89510d2

Delete both master and worker policies

cd4ca0a

rossf7 self-assigned this Aug 8, 2017

rossf7 commented Aug 8, 2017

View reviewed changes

rossf7 requested review from xh3b4sd and kopiczko August 8, 2017 15:28

xh3b4sd approved these changes Aug 9, 2017

View reviewed changes

Tidy up comments

a71f41e

rossf7 merged commit 3633f7a into master Aug 9, 2017

rossf7 deleted the split-iam-policies branch August 9, 2017 07:30

rossf7 mentioned this pull request Aug 9, 2017

WIP: Split AWS/IAM policies for master and workers #362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split AWS/IAM policies for master and workers #370

Split AWS/IAM policies for master and workers #370

rossf7 commented Aug 8, 2017 •

edited

Loading

rossf7 Aug 8, 2017

rossf7 Aug 8, 2017

rossf7 Aug 8, 2017

rossf7 Aug 8, 2017

rossf7 Aug 8, 2017

xh3b4sd Aug 9, 2017

rossf7 Aug 8, 2017

rossf7 Aug 8, 2017

xh3b4sd left a comment

xh3b4sd Aug 9, 2017

rossf7 Aug 9, 2017

xh3b4sd Aug 9, 2017

xh3b4sd Aug 9, 2017

rossf7 Aug 9, 2017

xh3b4sd Aug 9, 2017

rossf7 Aug 9, 2017

xh3b4sd Aug 9, 2017

rossf7 Aug 9, 2017

rossf7 commented Aug 9, 2017

hobti01 commented Aug 9, 2017

rossf7 commented Aug 9, 2017


		policyDocument := fmt.Sprintf(policyTemplate, p.KMSKeyArn, p.S3Bucket, p.S3Bucket)
		clusterRoleName := p.clusterRoleName()

Split AWS/IAM policies for master and workers #370

Split AWS/IAM policies for master and workers #370

Conversation

rossf7 commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xh3b4sd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossf7 commented Aug 9, 2017

hobti01 commented Aug 9, 2017

rossf7 commented Aug 9, 2017

rossf7 commented Aug 8, 2017 •

edited

Loading