Skip to content

Commit e435f0d

Browse files
BlaineEXEBlaineEXE
committed
add bucketaccess sidecar reconciliation
Implement the final portion of BucketAccess provisioning after handoff to the Sidecar. Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>
1 parent 819f6d1 commit e435f0d

File tree

24 files changed

+1278
-380
lines changed

24 files changed

+1278
-380
lines changed

‎client/apis/objectstorage/v1alpha2/definitions.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ const (
2525

2626
// Annotations
2727
const (
28+
// BucketClaimBeingDeletedAnnotation : This annotation is applied by the COSI Controller to a
29+
// Bucket when its BucketClaim is being deleted.
30+
BucketClaimBeingDeletedAnnotation = `objectstorage.k8.io/bucketclaim-being-deleted`
31+
2832
// HasBucketAccessReferencesAnnotation : This annotation is applied by the COSI Controller to a
2933
// BucketClaim when a BucketAccess that references the BucketClaim is created. The annotation
3034
// remains for as long as any BucketAccess references the BucketClaim. Once all BucketAccesses

‎controller/internal/reconciler/bucketaccess.go‎

Lines changed: 4 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ import (
3535

3636
cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
3737
objectstoragev1alpha2 "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
38+
"sigs.k8s.io/container-object-storage-interface/internal/bucketaccess"
3839
cosierr "sigs.k8s.io/container-object-storage-interface/internal/errors"
39-
"sigs.k8s.io/container-object-storage-interface/internal/handoff"
4040
cosipredicate "sigs.k8s.io/container-object-storage-interface/internal/predicate"
4141
)
4242

@@ -66,7 +66,7 @@ func (r *BucketAccessReconciler) Reconcile(ctx context.Context, req ctrl.Request
6666
return ctrl.Result{}, err
6767
}
6868

69-
if handoff.BucketAccessManagedBySidecar(access) {
69+
if bucketaccess.ManagedBySidecar(access) {
7070
logger.V(1).Info("not reconciling BucketAccess that should be managed by sidecar")
7171
return ctrl.Result{}, nil
7272
}
@@ -138,12 +138,12 @@ func (r *BucketAccessReconciler) reconcile(
138138
return cosierr.NonRetryableError(fmt.Errorf("deletion is not yet implemented")) // TODO
139139
}
140140

141-
needInit, err := needsControllerInitialization(&access.Status)
141+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status)
142142
if err != nil {
143143
logger.Error(err, "processed a degraded BucketAccess")
144144
return cosierr.NonRetryableError(fmt.Errorf("processed a degraded BucketAccess: %w", err))
145145
}
146-
if !needInit {
146+
if initialized {
147147
// BucketAccessClass info should only be copied to the BucketAccess status once, upon
148148
// initial provisioning. After the info is copied, make no attempt to fill in any missing or
149149
// lost info because we don't know whether the current Class is compatible with the info
@@ -237,38 +237,6 @@ func (r *BucketAccessReconciler) reconcile(
237237
return nil
238238
}
239239

240-
// Return true if the Controller needs to initialize the BucketAccess with BucketClaim and
241-
// BucketAccessClass info. Return false if required info is set.
242-
// Return an error if any required info is only partially set. This indicates some sort of
243-
// degradation or bug.
244-
func needsControllerInitialization(s *cosiapi.BucketAccessStatus) (bool, error) {
245-
requiredFields := map[string]bool{}
246-
requiredFieldIsSet := func(fieldName string, isSet bool) {
247-
requiredFields[fieldName] = isSet
248-
}
249-
250-
requiredFieldIsSet("status.accessedBuckets", len(s.AccessedBuckets) > 0)
251-
requiredFieldIsSet("status.driverName", s.DriverName != "")
252-
requiredFieldIsSet("status.authenticationType", string(s.AuthenticationType) != "")
253-
254-
set := []string{}
255-
for field, isSet := range requiredFields {
256-
if isSet {
257-
set = append(set, field)
258-
}
259-
}
260-
261-
if len(set) == 0 {
262-
return true, nil
263-
}
264-
265-
if len(set) == len(requiredFields) {
266-
return false, nil
267-
}
268-
269-
return false, fmt.Errorf("required Controller-managed fields are only partially set: %v", requiredFields)
270-
}
271-
272240
// Get all BucketClaims that this BucketAccess references.
273241
// If any claims don't exist, assume they don't exist YET; mark them nil in the resulting map
274242
// without treating nonexistence as an error.

‎controller/internal/reconciler/bucketaccess_test.go‎

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import (
2828
"k8s.io/apimachinery/pkg/runtime"
2929
"k8s.io/apimachinery/pkg/types"
3030
cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
31-
"sigs.k8s.io/container-object-storage-interface/internal/handoff"
31+
"sigs.k8s.io/container-object-storage-interface/internal/bucketaccess"
3232
ctrl "sigs.k8s.io/controller-runtime"
3333
"sigs.k8s.io/controller-runtime/pkg/client"
3434
"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -194,10 +194,10 @@ func TestBucketAccessReconcile(t *testing.T) {
194194
status.Parameters,
195195
)
196196

197-
assert.True(t, handoff.BucketAccessManagedBySidecar(access)) // MUST hand off to sidecar
198-
needInit, err := needsControllerInitialization(&access.Status) // MUST be fully initialized
197+
assert.True(t, bucketaccess.ManagedBySidecar(access)) // MUST hand off to sidecar
198+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST be fully initialized
199199
assert.NoError(t, err)
200-
assert.False(t, needInit)
200+
assert.True(t, initialized)
201201

202202
crw := &cosiapi.BucketClaim{}
203203
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -266,10 +266,10 @@ func TestBucketAccessReconcile(t *testing.T) {
266266
assert.Empty(t, status.AuthenticationType)
267267
assert.Empty(t, status.Parameters)
268268

269-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
270-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
269+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
270+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
271271
assert.NoError(t, err)
272-
assert.True(t, needInit)
272+
assert.False(t, initialized)
273273

274274
crw := &cosiapi.BucketClaim{}
275275
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -314,10 +314,10 @@ func TestBucketAccessReconcile(t *testing.T) {
314314
assert.Empty(t, status.AuthenticationType)
315315
assert.Empty(t, status.Parameters)
316316

317-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
318-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
317+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
318+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
319319
assert.NoError(t, err)
320-
assert.True(t, needInit)
320+
assert.False(t, initialized)
321321

322322
crw := &cosiapi.BucketClaim{}
323323
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -370,10 +370,10 @@ func TestBucketAccessReconcile(t *testing.T) {
370370
assert.Empty(t, status.AuthenticationType)
371371
assert.Empty(t, status.Parameters)
372372

373-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
374-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
373+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
374+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
375375
assert.NoError(t, err)
376-
assert.True(t, needInit)
376+
assert.False(t, initialized)
377377

378378
crw := &cosiapi.BucketClaim{}
379379
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -423,10 +423,10 @@ func TestBucketAccessReconcile(t *testing.T) {
423423
assert.Empty(t, status.AuthenticationType)
424424
assert.Empty(t, status.Parameters)
425425

426-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
427-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
426+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
427+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
428428
assert.NoError(t, err)
429-
assert.True(t, needInit)
429+
assert.False(t, initialized)
430430

431431
crw := &cosiapi.BucketClaim{}
432432
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -473,10 +473,10 @@ func TestBucketAccessReconcile(t *testing.T) {
473473
assert.Empty(t, status.AuthenticationType)
474474
assert.Empty(t, status.Parameters)
475475

476-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
477-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
476+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
477+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
478478
assert.NoError(t, err)
479-
assert.True(t, needInit)
479+
assert.False(t, initialized)
480480

481481
crw := &cosiapi.BucketClaim{}
482482
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -525,10 +525,10 @@ func TestBucketAccessReconcile(t *testing.T) {
525525
assert.Empty(t, status.AuthenticationType)
526526
assert.Empty(t, status.Parameters)
527527

528-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
529-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
528+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
529+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
530530
assert.NoError(t, err)
531-
assert.True(t, needInit)
531+
assert.False(t, initialized)
532532

533533
crw := &cosiapi.BucketClaim{}
534534
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -593,10 +593,10 @@ func TestBucketAccessReconcile(t *testing.T) {
593593
status.Parameters,
594594
)
595595

596-
assert.True(t, handoff.BucketAccessManagedBySidecar(access)) // MUST hand off to sidecar
597-
needInit, err := needsControllerInitialization(&access.Status) // MUST be fully initialized
596+
assert.True(t, bucketaccess.ManagedBySidecar(access)) // MUST hand off to sidecar
597+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST be fully initialized
598598
assert.NoError(t, err)
599-
assert.False(t, needInit)
599+
assert.True(t, initialized)
600600

601601
crw := &cosiapi.BucketClaim{}
602602
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -649,10 +649,10 @@ func TestBucketAccessReconcile(t *testing.T) {
649649
assert.Empty(t, status.AuthenticationType)
650650
assert.Empty(t, status.Parameters)
651651

652-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
653-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
652+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
653+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
654654
assert.NoError(t, err)
655-
assert.True(t, needInit)
655+
assert.False(t, initialized)
656656

657657
crw := &cosiapi.BucketClaim{}
658658
err = c.Get(ctx, readWriteClaimNsName, crw)
@@ -705,10 +705,10 @@ func TestBucketAccessReconcile(t *testing.T) {
705705
assert.Empty(t, status.AuthenticationType)
706706
assert.Empty(t, status.Parameters)
707707

708-
assert.False(t, handoff.BucketAccessManagedBySidecar(access)) // MUST NOT hand off to sidecar
709-
needInit, err := needsControllerInitialization(&access.Status) // MUST NOT be initialized
708+
assert.False(t, bucketaccess.ManagedBySidecar(access)) // MUST NOT hand off to sidecar
709+
initialized, err := bucketaccess.SidecarRequirementsPresent(&access.Status) // MUST NOT be initialized
710710
assert.NoError(t, err)
711-
assert.True(t, needInit)
711+
assert.False(t, initialized)
712712

713713
crw := &cosiapi.BucketClaim{}
714714
err = c.Get(ctx, readWriteClaimNsName, crw)

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ require (
1313
github.com/go-logr/logr v1.4.3
1414
github.com/stretchr/testify v1.11.1
1515
google.golang.org/grpc v1.75.1
16+
k8s.io/api v0.34.2
1617
k8s.io/apimachinery v0.34.2
1718
k8s.io/client-go v0.34.2
1819
sigs.k8s.io/container-object-storage-interface/client v0.0.0-20250925174816-5fce7c365e9c
@@ -98,7 +99,6 @@ require (
9899
gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect
99100
gopkg.in/inf.v0 v0.9.1 // indirect
100101
gopkg.in/yaml.v3 v3.0.1 // indirect
101-
k8s.io/api v0.34.2 // indirect
102102
k8s.io/apiextensions-apiserver v0.34.1 // indirect
103103
k8s.io/apiserver v0.34.1 // indirect
104104
k8s.io/component-base v0.34.1 // indirect

internal/handoff/handoff.go renamed to internal/bucketaccess/bucketaccess.go

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,18 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
// Package handoff defines logic needed for handing off control of resources between Controller and
18-
// Sidecar.
19-
package handoff
17+
// Package bucketaccess defines logic for BucketAccess resources needed by both Controller and
18+
// Sidecar. BucketAccesses need to be handed off between the two, and some logic is shared.
19+
package bucketaccess
2020

2121
import (
22+
"fmt"
23+
2224
cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
2325
)
2426

25-
// BucketAccessManagedBySidecar returns true if a BucketAccess should be managed by the Sidecar.
26-
// A false return value indicates that it should be managed by the Controller instead.
27+
// ManagedBySidecar returns true if a BucketAccess should be managed by the Sidecar.
28+
// It returns false if it should be managed by the Controller instead.
2729
//
2830
// In order for COSI Controller and any given Sidecar to work well together, they should avoid
2931
// managing the same BucketAccess resource at the same time. This will help prevent the Controller
@@ -39,19 +41,19 @@ import (
3941
// 2. Sidecar version low, Controller version high
4042
// 3. Sidecar version high, Controller version low
4143
// 4. Sidecar version high, Controller version high
42-
func BucketAccessManagedBySidecar(ba *cosiapi.BucketAccess) bool {
44+
func ManagedBySidecar(ba *cosiapi.BucketAccess) bool {
4345
// Allow a future-compatible mechanism by which the Controller can override the normal
4446
// BucketAccess management handoff logic in order to resolve a bug.
4547
// Instances where this is utilized should be infrequent -- ideally, never used.
4648
if _, ok := ba.Annotations[cosiapi.ControllerManagementOverrideAnnotation]; ok {
4749
return false
4850
}
4951

50-
// During provisioning, there are several status fields that the Controller needs to set before
51-
// the Sidecar can provision an access. However, tying this function's logic to ALL of the
52-
// status items could make long-term Controller-Sidecar handoff logic fragile. More logic means
53-
// more risk of unmanaged resources and more difficulty reasoning about how changes will impact
54-
// ownership during version skew. Minimize risk by relying on a single determining status field.
52+
// During provisioning, there are several status fields that the Controller needs to initialize
53+
// before the Sidecar can provision an access. However, tying this function's logic to ALL of
54+
// the status items could make long-term Controller-Sidecar handoff logic fragile. More logic
55+
// means more risk of unmanaged resources and more difficulty reasoning about how changes will
56+
// impact ownership during version skew. Minimize risk by relying on a single status field.
5557
if ba.Status.DriverName == "" {
5658
return false
5759
}
@@ -66,3 +68,40 @@ func BucketAccessManagedBySidecar(ba *cosiapi.BucketAccess) bool {
6668

6769
return true
6870
}
71+
72+
// SidecarRequirementsPresent verifies that BucketAccess status information required by the Sidecar
73+
// to provision the BucketAccess is fully set.
74+
//
75+
// Return true if the fields needed by the Sidecar are all set (Controller initialization finished).
76+
// Return false if the fields needed by the Sidecar are all unset (needs Controller initialization).
77+
// Return an error if required info is only partially set, indicating some sort of degradation/bug.
78+
//
79+
// Do not use this function to determine whether a BucketAccess should be managed by the Sidecar or
80+
// Controller, or whether handoff has occurred. Use ManagedBySidecar() for that purpose instead.
81+
// This function is appropriate for use within a controller to check requirements before/after
82+
// initialization/provisioning.
83+
func SidecarRequirementsPresent(s *cosiapi.BucketAccessStatus) (bool, error) {
84+
requiredFields := map[string]bool{}
85+
set := []string{}
86+
87+
requiredFieldIsSet := func(fieldName string, isSet bool) {
88+
requiredFields[fieldName] = isSet
89+
if isSet {
90+
set = append(set, fieldName)
91+
}
92+
}
93+
94+
requiredFieldIsSet("status.accessedBuckets", len(s.AccessedBuckets) > 0)
95+
requiredFieldIsSet("status.driverName", s.DriverName != "")
96+
requiredFieldIsSet("status.authenticationType", string(s.AuthenticationType) != "")
97+
98+
if len(set) == 0 {
99+
return false, nil
100+
}
101+
102+
if len(set) == len(requiredFields) {
103+
return true, nil
104+
}
105+
106+
return false, fmt.Errorf("fields required for sidecar provisioning are only partially set: %v", requiredFields)
107+
}

internal/handoff/handoff_test.go renamed to internal/bucketaccess/bucketaccess_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
package handoff
17+
package bucketaccess
1818

1919
import (
2020
"testing"
@@ -27,7 +27,7 @@ import (
2727
cosiapi "sigs.k8s.io/container-object-storage-interface/client/apis/objectstorage/v1alpha2"
2828
)
2929

30-
func TestBucketAccessManagedBySidecar(t *testing.T) {
30+
func TestManagedBySidecar(t *testing.T) {
3131
tests := []struct {
3232
name string // description of this test case
3333
// input parameters for target function.
@@ -125,8 +125,8 @@ func TestBucketAccessManagedBySidecar(t *testing.T) {
125125
if tt.isHandedOffToSidecar {
126126
copy.Status.AccessedBuckets = []cosiapi.AccessedBucket{
127127
{
128-
BucketName: "bc-asdfgh",
129-
AccessMode: cosiapi.BucketAccessModeReadWrite,
128+
BucketName: "bc-asdfgh",
129+
BucketClaimName: "bc-1",
130130
},
131131
}
132132
copy.Status.DriverName = "some.driver.io"
@@ -142,12 +142,12 @@ func TestBucketAccessManagedBySidecar(t *testing.T) {
142142
copy.Annotations[cosiapi.SidecarCleanupFinishedAnnotation] = ""
143143
}
144144

145-
got := BucketAccessManagedBySidecar(copy)
145+
got := ManagedBySidecar(copy)
146146
assert.Equal(t, tt.want, got)
147147

148148
// for all cases,applying the controller override annotation makes it controller-managed
149149
copy.Annotations[cosiapi.ControllerManagementOverrideAnnotation] = ""
150-
withOverride := BucketAccessManagedBySidecar(copy)
150+
withOverride := ManagedBySidecar(copy)
151151
assert.False(t, withOverride)
152152
})
153153
}

0 commit comments

Comments
 (0)