Skip to content

Commit 537d5a8

Browse files
authored
fix: remove volumes and mounts if access has expired (#107)
* fix: use correct error target type * fix: remove volumes and mounts if access has expired * fix: account for the curvature of spacetime * test: add some tests for volumes mutator
1 parent 63d2303 commit 537d5a8

File tree

6 files changed

+254
-14
lines changed

6 files changed

+254
-14
lines changed

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ require (
77
cloud.google.com/go/resourcemanager v1.10.5
88
cloud.google.com/go/storage v1.51.0
99
github.com/caarlos0/env/v11 v11.3.1
10+
github.com/go-test/deep v1.1.1
1011
github.com/onsi/ginkgo/v2 v2.23.3
1112
github.com/onsi/gomega v1.36.3
1213
golang.org/x/exp v0.0.0-20240808152545-0cdaa3abc0fa

go.sum

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ github.com/go-openapi/swag v0.23.0 h1:vsEVJDUo2hPJ2tu0/Xc+4noaxyEffXNIs3cOULZ+Gr
7777
github.com/go-openapi/swag v0.23.0/go.mod h1:esZ8ITTYEsH1V2trKHjAN8Ai7xHb8RV+YSZ577vPjgQ=
7878
github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI=
7979
github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8=
80+
github.com/go-test/deep v1.1.1 h1:0r/53hagsehfO4bzD2Pgr/+RgHqhmf+k1Bpse2cTu1U=
81+
github.com/go-test/deep v1.1.1/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE=
8082
github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
8183
github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
8284
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE=

internal/controller/serviceaccount_controller.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func (r *ServiceAccountReconciler) removeIamBinding(ctx context.Context, group s
146146
policy, err := r.Iam.Projects.ServiceAccounts.GetIamPolicy(gcpSaRef).OptionsRequestedPolicyVersion(3).Do()
147147
if err != nil {
148148
// We don't care about NotFound errors
149-
var gErr googleapi.Error
149+
var gErr *googleapi.Error
150150
if errors.As(err, &gErr) && gErr.Code == http.StatusNotFound {
151151
log.Info("service account no longer exists")
152152
return ctrl.Result{}, nil
@@ -189,7 +189,7 @@ func (r *ServiceAccountReconciler) handleGcpSa(ctx context.Context, sa *corev1.S
189189
policy, err := r.Iam.Projects.ServiceAccounts.GetIamPolicy(gcpSaRef).OptionsRequestedPolicyVersion(3).Do()
190190
if err != nil {
191191
// Ignore non-existent SAs, but log a warning
192-
var gErr googleapi.Error
192+
var gErr *googleapi.Error
193193
if errors.As(err, &gErr) && gErr.Code == http.StatusNotFound {
194194
log.Info("cannot set IAM policy on non-existent SA")
195195
return ctrl.Result{}, nil

internal/controller/statefulset_webhook.go

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http"
99
"slices"
1010
"strings"
11+
"time"
1112

1213
rm "cloud.google.com/go/resourcemanager/apiv3"
1314
rmpb "cloud.google.com/go/resourcemanager/apiv3/resourcemanagerpb"
@@ -93,8 +94,6 @@ func (m *StatefulsetMutator) Handle(ctx context.Context, req admission.Request)
9394
return admission.Errored(http.StatusBadRequest, err)
9495
}
9596

96-
bucketMounts := getExtraBucketMounts(sfs.Annotations)
97-
9897
saAnnotations := map[string]string{
9998
impersonateGroupAnnotation: group,
10099
accessReasonAnnotation: sfs.Annotations[accessReasonAnnotation],
@@ -130,20 +129,37 @@ func (m *StatefulsetMutator) Handle(ctx context.Context, req admission.Request)
130129
}
131130
}
132131

133-
if sharedBuckets, ok := sfs.Annotations[mountSharedBucketsAnnotation]; ok {
134-
if err := m.addSharedBuckets(bucketMounts, sharedBuckets); err != nil {
135-
log.Error(err, "failed to add shared buckets")
132+
accessExpired := false
133+
if groupConfig.ReasonRequired {
134+
parsedDuration, err := time.ParseDuration(sfs.Annotations[requestedServiceDurationAnnotation])
135+
if err != nil {
136+
log.Error(err, "failed to parse requested duration", "durationAnnotation", sfs.Annotations[requestedServiceDurationAnnotation])
137+
return admission.Denied("invalid requested duration format")
136138
}
139+
140+
accessExpired = time.Now().After(sfs.CreationTimestamp.Add(parsedDuration))
137141
}
138142

139-
// TODO: Use Dapla Team API for this?
140-
if sfs.Annotations[mountStandardBucketsAnnotation] == "true" {
141-
if err := m.addStandardBuckets(ctx, team, *groupConfig, bucketMounts); err != nil {
142-
log.Error(err, "failed to add standard buckets")
143+
if accessExpired {
144+
removeBucketsFromPodSpec(&sfs.Spec.Template.Spec, serviceContainer)
145+
} else {
146+
bucketMounts := getExtraBucketMounts(sfs.Annotations)
147+
148+
if sharedBuckets, ok := sfs.Annotations[mountSharedBucketsAnnotation]; ok {
149+
if err := m.addSharedBuckets(bucketMounts, sharedBuckets); err != nil {
150+
log.Error(err, "failed to add shared buckets")
151+
}
143152
}
144-
}
145153

146-
addBucketsToPodSpec(&sfs.Spec.Template.Spec, serviceContainer, bucketMounts, m.PrecreatorImage)
154+
// TODO: Use Dapla Team API for this?
155+
if sfs.Annotations[mountStandardBucketsAnnotation] == "true" {
156+
if err := m.addStandardBuckets(ctx, team, *groupConfig, bucketMounts); err != nil {
157+
log.Error(err, "failed to add standard buckets")
158+
}
159+
}
160+
161+
addBucketsToPodSpec(&sfs.Spec.Template.Spec, serviceContainer, bucketMounts, m.PrecreatorImage)
162+
}
147163

148164
if m.IamProbeImage != "" && sfs.Annotations[iamProbeStatus] != iamProbeDone {
149165
if err := m.launchIamProbe(ctx, sfs); err != nil {

internal/controller/volumes.go

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ import (
99
"k8s.io/utils/ptr"
1010
)
1111

12+
const (
13+
precreatorContainerName = "bucket-folders-precreator"
14+
)
15+
1216
func volumeNameIs(name string) func(v corev1.Volume) bool {
1317
return func(v corev1.Volume) bool {
1418
return v.Name == name
@@ -66,7 +70,7 @@ func addBucketsToPodSpec(podspec *corev1.PodSpec, container *corev1.Container, b
6670
if modified {
6771
precreator := corev1.Container{
6872
Image: precreatorImage,
69-
Name: "bucket-folders-precreator",
73+
Name: precreatorContainerName,
7074
}
7175
for mountPoint, bucket := range bucketMounts {
7276
volumeName := fmt.Sprintf("gcsfuse-%s", strings.ReplaceAll(mountPoint, "/", "--"))
@@ -84,3 +88,33 @@ func addBucketsToPodSpec(podspec *corev1.PodSpec, container *corev1.Container, b
8488

8589
return modified
8690
}
91+
92+
func removeBucketsFromPodSpec(podspec *corev1.PodSpec, container *corev1.Container) (shouldUpdate bool) {
93+
modified := false
94+
95+
podspec.Volumes = slices.DeleteFunc(podspec.Volumes, func(v corev1.Volume) bool {
96+
if strings.HasPrefix(v.Name, "gcsfuse-") {
97+
modified = true
98+
return true
99+
}
100+
return false
101+
})
102+
103+
container.VolumeMounts = slices.DeleteFunc(container.VolumeMounts, func(m corev1.VolumeMount) bool {
104+
if strings.HasPrefix(m.Name, "gcsfuse-") {
105+
modified = true
106+
return true
107+
}
108+
return false
109+
})
110+
111+
podspec.Containers = slices.DeleteFunc(podspec.Containers, func(c corev1.Container) bool {
112+
if c.Name == precreatorContainerName {
113+
modified = true
114+
return true
115+
}
116+
return false
117+
})
118+
119+
return modified
120+
}
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package controller
2+
3+
import (
4+
"testing"
5+
6+
"github.com/go-test/deep"
7+
corev1 "k8s.io/api/core/v1"
8+
)
9+
10+
func TestVolumeNameIs(t *testing.T) {
11+
type TestCase struct {
12+
Description string
13+
Volume corev1.Volume
14+
Name string
15+
Expected bool
16+
}
17+
18+
tests := []TestCase{
19+
{
20+
Description: "Should return false if name does not match",
21+
Volume: corev1.Volume{Name: "my-volume"},
22+
Name: "abc",
23+
Expected: false,
24+
},
25+
{
26+
Description: "Should return true if name matches",
27+
Volume: corev1.Volume{Name: "my-volume"},
28+
Name: "my-volume",
29+
Expected: true,
30+
},
31+
}
32+
33+
for _, tt := range tests {
34+
t.Run(tt.Description, func(t *testing.T) {
35+
if res := volumeNameIs(tt.Name)(tt.Volume); res != tt.Expected {
36+
t.Errorf("volumeNameIs=%v, expected=%v", res, tt.Expected)
37+
}
38+
})
39+
}
40+
}
41+
42+
func TestVolumeMountNameIs(t *testing.T) {
43+
type TestCase struct {
44+
Description string
45+
VolumeMount corev1.VolumeMount
46+
Name string
47+
Expected bool
48+
}
49+
50+
tests := []TestCase{
51+
{
52+
Description: "Should return false if name does not match",
53+
VolumeMount: corev1.VolumeMount{Name: "my-volume"},
54+
Name: "abc",
55+
Expected: false,
56+
},
57+
{
58+
Description: "Should return true if name matches",
59+
VolumeMount: corev1.VolumeMount{Name: "my-volume"},
60+
Name: "my-volume",
61+
Expected: true,
62+
},
63+
}
64+
65+
for _, tt := range tests {
66+
t.Run(tt.Description, func(t *testing.T) {
67+
if res := volumeMountNameIs(tt.Name)(tt.VolumeMount); res != tt.Expected {
68+
t.Errorf("volumeMountNameIs=%v, expected=%v", res, tt.Expected)
69+
}
70+
})
71+
}
72+
}
73+
74+
func TestRemoveBucketsFromPodSpec(t *testing.T) {
75+
type TestCase struct {
76+
Description string
77+
PodSpec corev1.PodSpec
78+
ExpectedPodSpec corev1.PodSpec
79+
ExpectedModified bool
80+
}
81+
82+
tests := []TestCase{
83+
{
84+
Description: "Should not change anything if no gcsfuse volumes/sidecars",
85+
PodSpec: corev1.PodSpec{
86+
Volumes: []corev1.Volume{
87+
{
88+
Name: "not-gcsfuse",
89+
},
90+
},
91+
Containers: []corev1.Container{
92+
{
93+
Name: "not-gcsfuse",
94+
VolumeMounts: []corev1.VolumeMount{
95+
{
96+
Name: "not-gcsfuse",
97+
},
98+
},
99+
},
100+
},
101+
},
102+
ExpectedPodSpec: corev1.PodSpec{
103+
Volumes: []corev1.Volume{
104+
{
105+
Name: "not-gcsfuse",
106+
},
107+
},
108+
Containers: []corev1.Container{
109+
{
110+
Name: "not-gcsfuse",
111+
VolumeMounts: []corev1.VolumeMount{
112+
{
113+
Name: "not-gcsfuse",
114+
},
115+
},
116+
},
117+
},
118+
},
119+
ExpectedModified: false,
120+
},
121+
{
122+
Description: "Should remove only relevant volumes and containers",
123+
PodSpec: corev1.PodSpec{
124+
Volumes: []corev1.Volume{
125+
{
126+
Name: "gcsfuse-1",
127+
},
128+
{
129+
Name: "not-gcsfuse",
130+
},
131+
{
132+
Name: "gcsfuse-2",
133+
},
134+
},
135+
Containers: []corev1.Container{
136+
{
137+
Name: "not-gcsfuse",
138+
VolumeMounts: []corev1.VolumeMount{
139+
{
140+
Name: "gcsfuse-1",
141+
},
142+
{
143+
Name: "not-gcsfuse",
144+
},
145+
{
146+
Name: "gcsfuse-2",
147+
},
148+
},
149+
},
150+
{
151+
Name: precreatorContainerName,
152+
},
153+
},
154+
},
155+
ExpectedPodSpec: corev1.PodSpec{
156+
Volumes: []corev1.Volume{
157+
{
158+
Name: "not-gcsfuse",
159+
},
160+
},
161+
Containers: []corev1.Container{
162+
{
163+
Name: "not-gcsfuse",
164+
VolumeMounts: []corev1.VolumeMount{
165+
{
166+
Name: "not-gcsfuse",
167+
},
168+
},
169+
},
170+
},
171+
},
172+
ExpectedModified: true,
173+
},
174+
}
175+
176+
for _, tt := range tests {
177+
t.Run(tt.Description, func(t *testing.T) {
178+
if modified := removeBucketsFromPodSpec(&tt.PodSpec, &tt.PodSpec.Containers[0]); modified != tt.ExpectedModified {
179+
t.Errorf("modified=%v, expected=%v", modified, tt.ExpectedModified)
180+
}
181+
if diff := deep.Equal(tt.PodSpec, tt.ExpectedPodSpec); diff != nil {
182+
t.Errorf("modified PodSpec does not equal expected: %v", diff)
183+
}
184+
})
185+
}
186+
187+
}

0 commit comments

Comments
 (0)