Skip to content

Commit 65aba4e

Browse files
justinsbjustinsb
committed
gce: use typed ServiceAccount in IAM tasks
This gives us an automatic dependency in our evaluation, and lets us write out a dependency to terraform also.
1 parent 46a8ffe commit 65aba4e

File tree

10 files changed

+104
-62
lines changed

10 files changed

+104
-62
lines changed

‎pkg/model/gcemodel/service_accounts.go

+8-9
Original file line numberDiff line numberDiff line change
@@ -82,16 +82,15 @@ func (b *ServiceAccountsBuilder) Build(c *fi.CloudupModelBuilderContext) error {
8282
role = kops.InstanceGroupRoleControlPlane
8383
}
8484

85-
if err := b.addInstanceGroupServiceAccountPermissions(c, *serviceAccount.Email, role); err != nil {
85+
if err := b.addInstanceGroupServiceAccountPermissions(c, serviceAccount, role); err != nil {
8686
return err
8787
}
8888
}
8989

9090
return nil
9191
}
9292

93-
func (b *ServiceAccountsBuilder) addInstanceGroupServiceAccountPermissions(c *fi.CloudupModelBuilderContext, serviceAccountEmail string, role kops.InstanceGroupRole) error {
94-
member := "serviceAccount:" + serviceAccountEmail
93+
func (b *ServiceAccountsBuilder) addInstanceGroupServiceAccountPermissions(c *fi.CloudupModelBuilderContext, serviceAccount *gcetasks.ServiceAccount, role kops.InstanceGroupRole) error {
9594

9695
// Ideally we would use a custom role here, but the deletion of a custom role takes 7 days,
9796
// which means we can't easily recycle cluster names.
@@ -104,9 +103,9 @@ func (b *ServiceAccountsBuilder) addInstanceGroupServiceAccountPermissions(c *fi
104103
Name: s("serviceaccount-control-plane"),
105104
Lifecycle: b.Lifecycle,
106105

107-
Project: s(b.ProjectID),
108-
Member: s(member),
109-
Role: s("roles/container.serviceAgent"),
106+
Project: s(b.ProjectID),
107+
MemberServiceAccount: serviceAccount,
108+
Role: s("roles/container.serviceAgent"),
110109
})
111110

112111
case kops.InstanceGroupRoleNode:
@@ -120,9 +119,9 @@ func (b *ServiceAccountsBuilder) addInstanceGroupServiceAccountPermissions(c *fi
120119
Name: s("serviceaccount-nodes"),
121120
Lifecycle: b.Lifecycle,
122121

123-
Project: s(b.ProjectID),
124-
Member: s(member),
125-
Role: s("roles/compute.viewer"),
122+
Project: s(b.ProjectID),
123+
MemberServiceAccount: serviceAccount,
124+
Role: s("roles/compute.viewer"),
126125
})
127126
}
128127
return nil

‎pkg/model/gcemodel/storageacl.go

+13-15
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,14 @@ func (b *StorageAclBuilder) Build(c *fi.CloudupModelBuilderContext) error {
121121
}
122122

123123
type serviceAccountRole struct {
124-
Email string
125-
Role kops.InstanceGroupRole
124+
ServiceAccount *gcetasks.ServiceAccount
125+
Role kops.InstanceGroupRole
126126
}
127127
serviceAccountRoles := make(map[serviceAccountRole]bool)
128128

129129
for _, ig := range b.InstanceGroups {
130130
serviceAccount := b.LinkToServiceAccount(ig)
131-
132-
email := *serviceAccount.Email
133-
serviceAccountRoles[serviceAccountRole{Email: email, Role: ig.Spec.Role}] = true
131+
serviceAccountRoles[serviceAccountRole{ServiceAccount: serviceAccount, Role: ig.Spec.Role}] = true
134132
}
135133

136134
for serviceAccountRole := range serviceAccountRoles {
@@ -165,11 +163,11 @@ func (b *StorageAclBuilder) Build(c *fi.CloudupModelBuilderContext) error {
165163
klog.Warningf("adding bucket level write IAM for role %q to gs://%s to support etcd backup", role, bucket)
166164

167165
c.AddTask(&gcetasks.StorageBucketIAM{
168-
Name: s("objectadmin-" + bucket + "-serviceaccount-" + nameForTask),
169-
Lifecycle: b.Lifecycle,
170-
Bucket: s(bucket),
171-
Member: s("serviceAccount:" + serviceAccountRole.Email),
172-
Role: s("roles/storage.objectAdmin"),
166+
Name: s("objectadmin-" + bucket + "-serviceaccount-" + nameForTask),
167+
Lifecycle: b.Lifecycle,
168+
Bucket: s(bucket),
169+
MemberServiceAccount: serviceAccountRole.ServiceAccount,
170+
Role: s("roles/storage.objectAdmin"),
173171
})
174172
}
175173

@@ -201,11 +199,11 @@ func (b *StorageAclBuilder) Build(c *fi.CloudupModelBuilderContext) error {
201199
klog.Warningf("adding bucket level read IAM to gs://%s for role %q", bucket, role)
202200

203201
c.AddTask(&gcetasks.StorageBucketIAM{
204-
Name: s("objectviewer-" + bucket + "-serviceaccount-" + nameForTask),
205-
Lifecycle: b.Lifecycle,
206-
Bucket: s(bucket),
207-
Member: s("serviceAccount:" + serviceAccountRole.Email),
208-
Role: s("roles/storage.objectViewer"),
202+
Name: s("objectviewer-" + bucket + "-serviceaccount-" + nameForTask),
203+
Lifecycle: b.Lifecycle,
204+
Bucket: s(bucket),
205+
MemberServiceAccount: serviceAccountRole.ServiceAccount,
206+
Role: s("roles/storage.objectViewer"),
209207
})
210208
}
211209
}

‎tests/integration/update_cluster/many-addons-gce/kubernetes.tf

+2-2
Original file line numberDiff line numberDiff line change
@@ -560,13 +560,13 @@ resource "google_compute_subnetwork" "us-test1-minimal-example-com" {
560560
}
561561

562562
resource "google_project_iam_binding" "serviceaccount-control-plane" {
563-
members = ["serviceAccount:control-plane[email protected]"]
563+
members = [format("serviceAccount:%s", google_service_account.control-plane.email)]
564564
project = "testproject"
565565
role = "roles/container.serviceAgent"
566566
}
567567

568568
resource "google_project_iam_binding" "serviceaccount-nodes" {
569-
members = ["serviceAccount:node[email protected]"]
569+
members = [format("serviceAccount:%s", google_service_account.node.email)]
570570
project = "testproject"
571571
role = "roles/compute.viewer"
572572
}

‎tests/integration/update_cluster/minimal_gce_dns-none/kubernetes.tf

+2-2
Original file line numberDiff line numberDiff line change
@@ -631,13 +631,13 @@ resource "google_compute_subnetwork" "us-test1-minimal-gce-example-com" {
631631
}
632632

633633
resource "google_project_iam_binding" "serviceaccount-control-plane" {
634-
members = ["serviceAccount:control-plane[email protected]"]
634+
members = [format("serviceAccount:%s", google_service_account.control-plane.email)]
635635
project = "testproject"
636636
role = "roles/container.serviceAgent"
637637
}
638638

639639
resource "google_project_iam_binding" "serviceaccount-nodes" {
640-
members = ["serviceAccount:node[email protected]"]
640+
members = [format("serviceAccount:%s", google_service_account.node.email)]
641641
project = "testproject"
642642
role = "roles/compute.viewer"
643643
}

‎upup/pkg/fi/cloudup/gcetasks/instancetemplate.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ func mapServiceAccountsToTerraform(serviceAccounts []*ServiceAccount, saScopes [
605605
var out []*terraformTemplateServiceAccount
606606
for _, serviceAccount := range serviceAccounts {
607607
tsa := &terraformTemplateServiceAccount{
608-
Email: serviceAccount.TerraformLink(),
608+
Email: serviceAccount.TerraformLink_Email(),
609609
Scopes: scopes,
610610
}
611611
out = append(out, tsa)

‎upup/pkg/fi/cloudup/gcetasks/projectiambinding.go

+16-12
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"k8s.io/kops/upup/pkg/fi"
2626
"k8s.io/kops/upup/pkg/fi/cloudup/gce"
2727
"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
28+
"k8s.io/kops/upup/pkg/fi/cloudup/terraformWriter"
2829
)
2930

3031
// ProjectIAMBinding represents an IAM rule on a project
@@ -33,9 +34,9 @@ type ProjectIAMBinding struct {
3334
Name *string
3435
Lifecycle fi.Lifecycle
3536

36-
Project *string
37-
Member *string
38-
Role *string
37+
Project *string
38+
MemberServiceAccount *ServiceAccount
39+
Role *string
3940
}
4041

4142
var _ fi.CompareWithID = &ProjectIAMBinding{}
@@ -50,7 +51,7 @@ func (e *ProjectIAMBinding) Find(c *fi.CloudupContext) (*ProjectIAMBinding, erro
5051
cloud := c.T.Cloud.(gce.GCECloud)
5152

5253
projectID := fi.ValueOf(e.Project)
53-
member := fi.ValueOf(e.Member)
54+
member := "serviceAccount:" + fi.ValueOf(e.MemberServiceAccount.Email)
5455
role := fi.ValueOf(e.Role)
5556

5657
klog.V(2).Infof("Checking IAM for project %q", projectID)
@@ -70,7 +71,7 @@ func (e *ProjectIAMBinding) Find(c *fi.CloudupContext) (*ProjectIAMBinding, erro
7071

7172
actual := &ProjectIAMBinding{}
7273
actual.Project = e.Project
73-
actual.Member = e.Member
74+
actual.MemberServiceAccount = e.MemberServiceAccount
7475
actual.Role = e.Role
7576

7677
// Ignore "system" fields
@@ -88,8 +89,11 @@ func (_ *ProjectIAMBinding) CheckChanges(a, e, changes *ProjectIAMBinding) error
8889
if fi.ValueOf(e.Project) == "" {
8990
return fi.RequiredField("Project")
9091
}
91-
if fi.ValueOf(e.Member) == "" {
92-
return fi.RequiredField("Member")
92+
if e.MemberServiceAccount == nil {
93+
return fi.RequiredField("MemberServiceAccount")
94+
}
95+
if fi.ValueOf(e.MemberServiceAccount.Email) == "" {
96+
return fi.RequiredField("MemberServiceAccount.Email")
9397
}
9498
if fi.ValueOf(e.Role) == "" {
9599
return fi.RequiredField("Role")
@@ -101,7 +105,7 @@ func (_ *ProjectIAMBinding) RenderGCE(t *gce.GCEAPITarget, a, e, changes *Projec
101105
ctx := context.TODO()
102106

103107
projectID := fi.ValueOf(e.Project)
104-
member := fi.ValueOf(e.Member)
108+
member := "serviceAccount:" + fi.ValueOf(e.MemberServiceAccount.Email)
105109
role := fi.ValueOf(e.Role)
106110

107111
// Avoid concurrent operations
@@ -132,16 +136,16 @@ func (_ *ProjectIAMBinding) RenderGCE(t *gce.GCEAPITarget, a, e, changes *Projec
132136

133137
// terraformProjectIAMBinding is the model for a terraform google_project_iam_binding rule
134138
type terraformProjectIAMBinding struct {
135-
Project string `cty:"project"`
136-
Role string `cty:"role"`
137-
Members []string `cty:"members"`
139+
Project string `cty:"project"`
140+
Role string `cty:"role"`
141+
Members []*terraformWriter.Literal `cty:"members"`
138142
}
139143

140144
func (_ *ProjectIAMBinding) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *ProjectIAMBinding) error {
141145
tf := &terraformProjectIAMBinding{
142146
Project: fi.ValueOf(e.Project),
143147
Role: fi.ValueOf(e.Role),
144-
Members: []string{fi.ValueOf(e.Member)},
148+
Members: []*terraformWriter.Literal{e.MemberServiceAccount.TerraformLink_Member()},
145149
}
146150

147151
return t.RenderResource("google_project_iam_binding", *e.Name, tf)

‎upup/pkg/fi/cloudup/gcetasks/projectiambinding_test.go

+11-4
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,23 @@ func TestProjectIAMBinding(t *testing.T) {
3434

3535
// We define a function so we can rebuild the tasks, because we modify in-place when running
3636
buildTasks := func() map[string]fi.CloudupTask {
37+
serviceAccount := &ServiceAccount{
38+
Lifecycle: fi.LifecycleSync,
39+
40+
Email: fi.PtrTo("[email protected]"),
41+
}
42+
3743
binding := &ProjectIAMBinding{
3844
Lifecycle: fi.LifecycleSync,
3945

40-
Project: fi.PtrTo("testproject"),
41-
Member: fi.PtrTo("serviceAccount:[email protected]"),
42-
Role: fi.PtrTo("roles/owner"),
46+
Project: fi.PtrTo("testproject"),
47+
MemberServiceAccount: serviceAccount,
48+
Role: fi.PtrTo("roles/owner"),
4349
}
4450

4551
return map[string]fi.CloudupTask{
46-
"binding": binding,
52+
"serviceAccount": serviceAccount,
53+
"binding": binding,
4754
}
4855
}
4956

‎upup/pkg/fi/cloudup/gcetasks/serviceaccount.go

+20-1
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func (_ *ServiceAccount) RenderTerraform(t *terraform.TerraformTarget, a, e, cha
194194
return t.RenderResource("google_service_account", *e.Name, tf)
195195
}
196196

197-
func (e *ServiceAccount) TerraformLink() *terraformWriter.Literal {
197+
func (e *ServiceAccount) TerraformLink_Email() *terraformWriter.Literal {
198198
shared := fi.ValueOf(e.Shared)
199199
if shared {
200200
email := fi.ValueOf(e.Email)
@@ -208,3 +208,22 @@ func (e *ServiceAccount) TerraformLink() *terraformWriter.Literal {
208208

209209
return terraformWriter.LiteralProperty("google_service_account", *e.Name, "email")
210210
}
211+
212+
func (e *ServiceAccount) TerraformLink_Member() *terraformWriter.Literal {
213+
shared := fi.ValueOf(e.Shared)
214+
if shared {
215+
email := fi.ValueOf(e.Email)
216+
if email == "" {
217+
klog.Fatalf("Email must be set, if ServiceAccount is shared: %#v", e)
218+
}
219+
220+
klog.V(4).Infof("reusing existing ServiceAccount %q", email)
221+
return terraformWriter.LiteralFromStringValue(email)
222+
}
223+
224+
return terraformWriter.LiteralFunctionExpression(
225+
"format",
226+
terraformWriter.LiteralFromStringValue("serviceAccount:%s"),
227+
terraformWriter.LiteralProperty("google_service_account", *e.Name, "email"),
228+
)
229+
}

‎upup/pkg/fi/cloudup/gcetasks/storagebucketiam.go

+20-12
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"k8s.io/kops/upup/pkg/fi"
2626
"k8s.io/kops/upup/pkg/fi/cloudup/gce"
2727
"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
28+
"k8s.io/kops/upup/pkg/fi/cloudup/terraformWriter"
2829
)
2930

3031
// StorageBucketIAM represents an IAM rule on a google cloud storage bucket
@@ -33,9 +34,9 @@ type StorageBucketIAM struct {
3334
Name *string
3435
Lifecycle fi.Lifecycle
3536

36-
Bucket *string
37-
Member *string
38-
Role *string
37+
Bucket *string
38+
MemberServiceAccount *ServiceAccount
39+
Role *string
3940
}
4041

4142
var _ fi.CompareWithID = &StorageBucketIAM{}
@@ -50,7 +51,7 @@ func (e *StorageBucketIAM) Find(c *fi.CloudupContext) (*StorageBucketIAM, error)
5051
cloud := c.T.Cloud.(gce.GCECloud)
5152

5253
bucket := fi.ValueOf(e.Bucket)
53-
member := fi.ValueOf(e.Member)
54+
member := "serviceAccount:" + fi.ValueOf(e.MemberServiceAccount.Email)
5455
role := fi.ValueOf(e.Role)
5556

5657
klog.V(2).Infof("Checking GCS bucket IAM for gs://%s for %s", bucket, member)
@@ -69,7 +70,7 @@ func (e *StorageBucketIAM) Find(c *fi.CloudupContext) (*StorageBucketIAM, error)
6970

7071
actual := &StorageBucketIAM{}
7172
actual.Bucket = e.Bucket
72-
actual.Member = e.Member
73+
actual.MemberServiceAccount = e.MemberServiceAccount
7374
actual.Role = e.Role
7475

7576
// Ignore "system" fields
@@ -87,7 +88,10 @@ func (_ *StorageBucketIAM) CheckChanges(a, e, changes *StorageBucketIAM) error {
8788
if fi.ValueOf(e.Bucket) == "" {
8889
return fi.RequiredField("Bucket")
8990
}
90-
if fi.ValueOf(e.Member) == "" {
91+
if e.MemberServiceAccount == nil {
92+
return fi.RequiredField("MemberServiceAccount")
93+
}
94+
if fi.ValueOf(e.MemberServiceAccount.Email) == "" {
9195
return fi.RequiredField("Member")
9296
}
9397
if fi.ValueOf(e.Role) == "" {
@@ -100,7 +104,7 @@ func (_ *StorageBucketIAM) RenderGCE(t *gce.GCEAPITarget, a, e, changes *Storage
100104
ctx := context.TODO()
101105

102106
bucket := fi.ValueOf(e.Bucket)
103-
member := fi.ValueOf(e.Member)
107+
member := "serviceAccount:" + fi.ValueOf(e.MemberServiceAccount.Email)
104108
role := fi.ValueOf(e.Role)
105109

106110
klog.V(2).Infof("Creating GCS bucket IAM for gs://%s for %s as %s", bucket, member, role)
@@ -126,19 +130,23 @@ func (_ *StorageBucketIAM) RenderGCE(t *gce.GCEAPITarget, a, e, changes *Storage
126130

127131
// terraformStorageBucketIAM is the model for a terraform google_storage_bucket_iam_member rule
128132
type terraformStorageBucketIAM struct {
129-
Bucket string `cty:"bucket"`
130-
Role string `cty:"role"`
131-
Member string `cty:"member"`
133+
Bucket string `cty:"bucket"`
134+
Role string `cty:"role"`
135+
Member *terraformWriter.Literal `cty:"member"`
132136
}
133137

134138
func (_ *StorageBucketIAM) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *StorageBucketIAM) error {
135139
tf := &terraformStorageBucketIAM{
136140
Bucket: fi.ValueOf(e.Bucket),
137141
Role: fi.ValueOf(e.Role),
138-
Member: fi.ValueOf(e.Member),
142+
Member: e.MemberServiceAccount.TerraformLink_Member(),
143+
}
144+
145+
if err := t.RenderResource("google_storage_bucket_iam_member", *e.Name, tf); err != nil {
146+
return err
139147
}
140148

141-
return t.RenderResource("google_storage_bucket_iam_member", *e.Name, tf)
149+
return nil
142150
}
143151

144152
func patchPolicy(policy *storage.Policy, wantMember string, wantRole string) bool {

‎upup/pkg/fi/cloudup/gcetasks/storagebucketiam_test.go

+11-4
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,23 @@ func TestStorageBucketIAM(t *testing.T) {
3434

3535
// We define a function so we can rebuild the tasks, because we modify in-place when running
3636
buildTasks := func() map[string]fi.CloudupTask {
37+
serviceAccount := &ServiceAccount{
38+
Lifecycle: fi.LifecycleSync,
39+
40+
Email: fi.PtrTo("[email protected]"),
41+
}
42+
3743
binding := &StorageBucketIAM{
3844
Lifecycle: fi.LifecycleSync,
3945

40-
Bucket: fi.PtrTo("bucket1"),
41-
Member: fi.PtrTo("serviceAccount:[email protected]"),
42-
Role: fi.PtrTo("roles/owner"),
46+
Bucket: fi.PtrTo("bucket1"),
47+
MemberServiceAccount: serviceAccount,
48+
Role: fi.PtrTo("roles/owner"),
4349
}
4450

4551
return map[string]fi.CloudupTask{
46-
"binding": binding,
52+
"serviceAccount": serviceAccount,
53+
"binding": binding,
4754
}
4855
}
4956

0 commit comments

Comments
 (0)