Skip to content

Commit e0bbf3e

Browse files
danail-branekovgeorgethebeatle
danail-branekov
and
georgethebeatle
committed
Improve the unique name of CFServiceBinding
- Up to now the unique name of a binding would only include the app and service instance guids - This means that we could only bind a service instance to an app once - This change improves the unique name of the binding by: - Including the service instance space guid. This has been accidentally omitted before, but given that the resource guids are already globally unique has not hit us yet. However we can fix it while we are at it - Including the service binding display name in the binding uinque name. If the diplay name is not set, we default it to the name of the service instance This commit also migrates existing CFServiceBindings by using the name registry to handle deregistering the "old" binding unique name and register the "new" unique name. Under the hood this takes care to delete the old lease and create a new one fixes #4175 Co-authored-by: Georgi Sabev <[email protected]>
1 parent d9ba050 commit e0bbf3e

File tree

5 files changed

+242
-49
lines changed

5 files changed

+242
-49
lines changed

‎controllers/api/v1alpha1/cfservicebinding_types.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package v1alpha1
1919
import (
2020
"fmt"
2121

22+
"code.cloudfoundry.org/korifi/tools"
2223
v1 "k8s.io/api/core/v1"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2425
)
@@ -118,11 +119,11 @@ func (b *CFServiceBinding) StatusConditions() *[]metav1.Condition {
118119
}
119120

120121
func (b CFServiceBinding) UniqueName() string {
121-
return fmt.Sprintf("sb::%s::%s::%s", b.Spec.AppRef.Name, b.Spec.Service.Namespace, b.Spec.Service.Name)
122+
return fmt.Sprintf("sb::%s::%s::%s::%s", b.Spec.AppRef.Name, b.Namespace, b.Spec.Service.Name, *tools.IfNil(b.Spec.DisplayName, &b.Spec.Service.Name))
122123
}
123124

124125
func (b CFServiceBinding) UniqueValidationErrorMessage() string {
125-
return fmt.Sprintf("Service binding already exists: App: %s Service Instance: %s", b.Spec.AppRef.Name, b.Spec.Service.Name)
126+
return fmt.Sprintf("Service binding already exists: App: %s Service Instance: %s Service Binding: %s", b.Spec.AppRef.Name, b.Spec.Service.Name, *tools.IfNil(b.Spec.DisplayName, &b.Spec.Service.Name))
126127
}
127128

128129
func init() {

‎controllers/webhooks/services/bindings/validator_test.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,23 @@ var _ = Describe("CFServiceBindingValidatingWebhook", func() {
7676
_, _, actualNamespace, actualResource := duplicateValidator.ValidateCreateArgsForCall(0)
7777
Expect(actualNamespace).To(Equal(defaultNamespace))
7878
Expect(actualResource).To(Equal(serviceBinding))
79-
Expect(actualResource.UniqueValidationErrorMessage()).To(Equal("Service binding already exists: App: " + appGUID + " Service Instance: " + serviceInstanceGUID))
79+
Expect(actualResource.UniqueName()).To(Equal("sb::" + appGUID + "::" + defaultNamespace + "::" + serviceInstanceGUID + "::" + serviceInstanceGUID))
80+
Expect(actualResource.UniqueValidationErrorMessage()).To(Equal("Service binding already exists: App: " + appGUID + " Service Instance: " + serviceInstanceGUID + " Service Binding: " + serviceInstanceGUID))
81+
})
82+
83+
When("the service binding has a display name", func() {
84+
BeforeEach(func() {
85+
serviceBinding.Spec.DisplayName = tools.PtrTo("my-binding")
86+
})
87+
88+
It("uses the display name in the unique name", func() {
89+
Expect(duplicateValidator.ValidateCreateCallCount()).To(Equal(1))
90+
_, _, actualNamespace, actualResource := duplicateValidator.ValidateCreateArgsForCall(0)
91+
Expect(actualNamespace).To(Equal(defaultNamespace))
92+
Expect(actualResource).To(Equal(serviceBinding))
93+
Expect(actualResource.UniqueName()).To(Equal("sb::" + appGUID + "::" + defaultNamespace + "::" + serviceInstanceGUID + "::my-binding"))
94+
Expect(actualResource.UniqueValidationErrorMessage()).To(Equal("Service binding already exists: App: " + appGUID + " Service Instance: " + serviceInstanceGUID + " Service Binding: my-binding"))
95+
})
8096
})
8197

8298
When("a duplicate service binding already exists", func() {

‎migration/main.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"runtime"
88

99
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
10+
"code.cloudfoundry.org/korifi/controllers/coordination"
11+
bindingswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/services/bindings"
1012
"code.cloudfoundry.org/korifi/migration/migration"
1113
"code.cloudfoundry.org/korifi/tools"
1214
"k8s.io/client-go/kubernetes/scheme"
@@ -33,7 +35,8 @@ func main() {
3335

3436
workersCount := tools.Max(1, runtime.NumCPU()/2)
3537

36-
err = migration.New(k8sClient, korifiVersion, workersCount).Run(context.Background())
38+
migrator := migration.New(k8sClient, korifiVersion, workersCount, coordination.NewNameRegistry(k8sClient, bindingswebhook.ServiceBindingEntityType))
39+
err = migrator.Run(context.Background())
3740
if err != nil {
3841
panic(err)
3942
}

‎migration/migration/executor.go‎

Lines changed: 54 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"time"
1010

1111
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
12+
"code.cloudfoundry.org/korifi/controllers/coordination"
1213
"code.cloudfoundry.org/korifi/tools"
1314
"code.cloudfoundry.org/korifi/tools/k8s"
1415
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -17,21 +18,22 @@ import (
1718
const MigratedByLabelKey = "korifi.cloudfoundry.org/migrated-by"
1819

1920
var korifiObjectLists = []client.ObjectList{
20-
&korifiv1alpha1.CFRouteList{},
2121
&korifiv1alpha1.CFServiceBindingList{},
2222
}
2323

2424
type Migrator struct {
25-
k8sClient client.Client
26-
korifiVersion string
27-
workersCount int
25+
k8sClient client.Client
26+
korifiVersion string
27+
workersCount int
28+
serviceBindingNameRegistry coordination.NameRegistry
2829
}
2930

30-
func New(k8sClient client.Client, korifiVersion string, workersCount int) *Migrator {
31+
func New(k8sClient client.Client, korifiVersion string, workersCount int, serviceBindingNameRegistry coordination.NameRegistry) *Migrator {
3132
return &Migrator{
32-
k8sClient: k8sClient,
33-
korifiVersion: korifiVersion,
34-
workersCount: workersCount,
33+
k8sClient: k8sClient,
34+
korifiVersion: korifiVersion,
35+
workersCount: workersCount,
36+
serviceBindingNameRegistry: serviceBindingNameRegistry,
3537
}
3638
}
3739

@@ -40,7 +42,7 @@ func (m *Migrator) Run(ctx context.Context) error {
4042

4143
objectsToUpdate, err := m.collectObjects(ctx, korifiObjectLists)
4244
if err != nil {
43-
panic(err)
45+
return fmt.Errorf("failed to collect objects to migrate: %v", err)
4446
}
4547

4648
fmt.Println("==========================================================")
@@ -61,8 +63,18 @@ func (m *Migrator) Run(ctx context.Context) error {
6163
defer wg.Done()
6264

6365
for obj := range objectChan {
64-
if err := m.setMigratedByLabel(ctx, obj); err != nil {
65-
fmt.Fprintf(os.Stderr, "Failed to set label on object %v %s/%s: %v\n", obj.GetObjectKind(), obj.GetNamespace(), obj.GetName(), err)
66+
binding, ok := obj.(*korifiv1alpha1.CFServiceBinding)
67+
if !ok {
68+
continue
69+
}
70+
71+
if binding.Labels[MigratedByLabelKey] == m.korifiVersion {
72+
continue
73+
}
74+
75+
if err := m.migrateServiceBindingUniqueName(ctx, binding); err != nil {
76+
fmt.Fprintf(os.Stderr, "failed to migrate lease for service binding %s/%s: %v\n", binding.Namespace, binding.Name, err)
77+
continue
6678
}
6779
fmt.Fprintf(os.Stdout, "%s %s/%s migrated\n", obj.GetObjectKind().GroupVersionKind().GroupKind().Kind, obj.GetNamespace(), obj.GetName())
6880
}
@@ -78,6 +90,37 @@ func (m *Migrator) Run(ctx context.Context) error {
7890
return nil
7991
}
8092

93+
func (m *Migrator) migrateServiceBindingUniqueName(ctx context.Context, binding *korifiv1alpha1.CFServiceBinding) error {
94+
err := m.registerServiceBindingUniqueName(ctx, binding)
95+
if err != nil {
96+
return fmt.Errorf("failed to register the new unique name for service binding %s/%s: %v", binding.Namespace, binding.Name, err)
97+
}
98+
99+
err = m.serviceBindingNameRegistry.DeregisterName(ctx, binding.Namespace, oldUniqueName(binding))
100+
if err != nil {
101+
return fmt.Errorf("failed to unregister old unique name for service binding %s/%s: %v", binding.Namespace, binding.Name, err)
102+
}
103+
104+
return m.setMigratedByLabel(ctx, binding)
105+
}
106+
107+
func (m *Migrator) registerServiceBindingUniqueName(ctx context.Context, binding *korifiv1alpha1.CFServiceBinding) error {
108+
isOwned, err := m.serviceBindingNameRegistry.CheckNameOwnership(ctx, binding.Namespace, binding.UniqueName(), binding.Namespace, binding.Name)
109+
// NotFound means that there is no lease for that unique name
110+
if client.IgnoreNotFound(err) != nil {
111+
return fmt.Errorf("failed to check ownership of the new binding unique name: %w", err)
112+
}
113+
114+
if isOwned {
115+
return nil
116+
}
117+
return m.serviceBindingNameRegistry.RegisterName(ctx, binding.Namespace, binding.UniqueName(), binding.Namespace, binding.Name)
118+
}
119+
120+
func oldUniqueName(binding *korifiv1alpha1.CFServiceBinding) string {
121+
return fmt.Sprintf("sb::%s::%s::%s", binding.Spec.AppRef.Name, binding.Spec.Service.Namespace, binding.Spec.Service.Name)
122+
}
123+
81124
func (m *Migrator) setMigratedByLabel(ctx context.Context, obj client.Object) error {
82125
return k8s.PatchResource(ctx, m.k8sClient, obj, func() {
83126
obj.SetLabels(tools.SetMapValue(obj.GetLabels(), MigratedByLabelKey, m.korifiVersion))

‎migration/migration/executor_test.go‎

Lines changed: 164 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,191 @@
11
package migration_test
22

33
import (
4+
"crypto/sha1"
5+
"fmt"
6+
47
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
8+
"code.cloudfoundry.org/korifi/controllers/coordination"
9+
bindingswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/services/bindings"
510
"code.cloudfoundry.org/korifi/migration/migration"
11+
"code.cloudfoundry.org/korifi/tools"
12+
"code.cloudfoundry.org/korifi/tools/k8s"
613
"github.com/google/uuid"
714
. "github.com/onsi/ginkgo/v2"
815
. "github.com/onsi/gomega"
16+
coordinationv1 "k8s.io/api/coordination/v1"
917
corev1 "k8s.io/api/core/v1"
18+
k8serrors "k8s.io/apimachinery/pkg/api/errors"
19+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1020
"sigs.k8s.io/controller-runtime/pkg/client"
1121
)
1222

1323
var _ = Describe("Executor", func() {
14-
Describe("CFRoute", test(&korifiv1alpha1.CFRoute{
15-
Spec: korifiv1alpha1.CFRouteSpec{
16-
Host: "example",
17-
Path: "/example",
18-
DomainRef: corev1.ObjectReference{
19-
Name: "example.com",
20-
},
21-
},
22-
}))
23-
24-
Describe("CFServiceBinding", test(&korifiv1alpha1.CFServiceBinding{
25-
Spec: korifiv1alpha1.CFServiceBindingSpec{
26-
Type: "key",
27-
},
28-
}))
29-
})
30-
31-
func test(testObj client.Object) func() {
32-
GinkgoHelper()
33-
34-
return func() {
24+
Describe("CFServiceBinding Lease", func() {
3525
var (
36-
obj client.Object
37-
migrator *migration.Migrator
26+
serviceBinding *korifiv1alpha1.CFServiceBinding
27+
migrator *migration.Migrator
28+
existingLease *coordinationv1.Lease
29+
bindingsNameRegistry coordination.NameRegistry
3830
)
3931

4032
BeforeEach(func() {
41-
migrator = migration.New(k8sClient, "abc123", 1)
33+
bindingsNameRegistry = coordination.NewNameRegistry(k8sClient, bindingswebhook.ServiceBindingEntityType)
34+
migrator = migration.New(k8sClient, "test-version", 1, bindingsNameRegistry)
4235

43-
obj = testObj.DeepCopyObject().(client.Object)
36+
serviceBinding = &korifiv1alpha1.CFServiceBinding{
37+
ObjectMeta: metav1.ObjectMeta{
38+
Namespace: testNamespace,
39+
Name: uuid.NewString(),
40+
},
41+
Spec: korifiv1alpha1.CFServiceBindingSpec{
42+
Type: "app",
43+
Service: corev1.ObjectReference{
44+
Kind: "CFServiceInstance",
45+
Name: uuid.NewString(),
46+
},
47+
AppRef: corev1.LocalObjectReference{
48+
Name: uuid.NewString(),
49+
},
50+
},
51+
}
52+
Expect(k8sClient.Create(ctx, serviceBinding)).To(Succeed())
4453

45-
obj.SetName(uuid.NewString())
46-
obj.SetNamespace(testNamespace)
47-
Expect(k8sClient.Create(ctx, obj)).To(Succeed())
54+
existingLeaseName := fmt.Sprintf("servicebinding::sb::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Spec.Service.Namespace, serviceBinding.Spec.Service.Name)
55+
existingLease = &coordinationv1.Lease{
56+
ObjectMeta: metav1.ObjectMeta{
57+
Namespace: testNamespace,
58+
Name: fmt.Sprintf("n-%x", sha1.Sum([]byte(existingLeaseName))),
59+
},
60+
}
61+
Expect(k8sClient.Create(ctx, existingLease)).To(Succeed())
4862
})
4963

5064
JustBeforeEach(func() {
51-
Expect(migrator.Run(ctx)).To(Succeed())
65+
err := migrator.Run(ctx)
66+
Expect(err).NotTo(HaveOccurred())
5267
})
5368

54-
It("sets the migrated_by label", func() {
69+
It("sets the migrated-by label", func() {
5570
Eventually(func(g Gomega) {
56-
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
57-
g.Expect(obj.GetLabels()).To(HaveKeyWithValue(migration.MigratedByLabelKey, "abc123"))
71+
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(serviceBinding), serviceBinding)).To(Succeed())
72+
g.Expect(serviceBinding.Labels).To(HaveKeyWithValue(migration.MigratedByLabelKey, "test-version"))
5873
}).Should(Succeed())
5974
})
60-
}
61-
}
75+
76+
It("deletes the existing lease", func() {
77+
Eventually(func(g Gomega) {
78+
err := k8sClient.Get(ctx, client.ObjectKeyFromObject(existingLease), existingLease)
79+
g.Expect(k8serrors.IsNotFound(err)).To(BeTrue())
80+
}).Should(Succeed())
81+
})
82+
83+
It("creates a new lease in the new format by using the service name", func() {
84+
leaseName := fmt.Sprintf("servicebinding::sb::%s::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Namespace, serviceBinding.Spec.Service.Name, serviceBinding.Spec.Service.Name)
85+
newLease := &coordinationv1.Lease{
86+
ObjectMeta: metav1.ObjectMeta{
87+
Namespace: testNamespace,
88+
Name: fmt.Sprintf("n-%x", sha1.Sum([]byte(leaseName))),
89+
},
90+
}
91+
Eventually(func(g Gomega) {
92+
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(newLease), newLease)).To(Succeed())
93+
g.Expect(newLease.Annotations).To(SatisfyAll(
94+
HaveKeyWithValue(coordination.EntityTypeAnnotation, bindingswebhook.ServiceBindingEntityType),
95+
HaveKeyWithValue(coordination.NameAnnotation, fmt.Sprintf("sb::%s::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Namespace, serviceBinding.Spec.Service.Name, serviceBinding.Spec.Service.Name)),
96+
HaveKeyWithValue(coordination.NamespaceAnnotation, serviceBinding.Namespace),
97+
HaveKeyWithValue(coordination.OwnerNamespaceAnnotation, serviceBinding.Namespace),
98+
HaveKeyWithValue(coordination.OwnerNameAnnotation, serviceBinding.Name),
99+
))
100+
}).Should(Succeed())
101+
})
102+
103+
When("the service binding has a display name", func() {
104+
BeforeEach(func() {
105+
Expect(k8s.Patch(ctx, k8sClient, serviceBinding, func() {
106+
serviceBinding.Spec.DisplayName = tools.PtrTo(uuid.NewString())
107+
})).To(Succeed())
108+
})
109+
110+
It("creates a new lease in the new format by using the binding display name", func() {
111+
leaseName := fmt.Sprintf("servicebinding::sb::%s::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Namespace, serviceBinding.Spec.Service.Name, *serviceBinding.Spec.DisplayName)
112+
newLease := &coordinationv1.Lease{
113+
ObjectMeta: metav1.ObjectMeta{
114+
Namespace: testNamespace,
115+
Name: fmt.Sprintf("n-%x", sha1.Sum([]byte(leaseName))),
116+
},
117+
}
118+
Eventually(func(g Gomega) {
119+
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(newLease), newLease)).To(Succeed())
120+
g.Expect(newLease.Annotations).To(SatisfyAll(
121+
HaveKeyWithValue(coordination.EntityTypeAnnotation, bindingswebhook.ServiceBindingEntityType),
122+
HaveKeyWithValue(coordination.NameAnnotation, fmt.Sprintf("sb::%s::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Namespace, serviceBinding.Spec.Service.Name, *serviceBinding.Spec.DisplayName)),
123+
HaveKeyWithValue(coordination.NamespaceAnnotation, serviceBinding.Namespace),
124+
HaveKeyWithValue(coordination.OwnerNamespaceAnnotation, serviceBinding.Namespace),
125+
HaveKeyWithValue(coordination.OwnerNameAnnotation, serviceBinding.Name),
126+
))
127+
}).Should(Succeed())
128+
})
129+
})
130+
131+
When("the binding unique name has been already registered for that binding", func() {
132+
BeforeEach(func() {
133+
Expect(bindingsNameRegistry.RegisterName(ctx, serviceBinding.Namespace, serviceBinding.UniqueName(), serviceBinding.Namespace, serviceBinding.Name)).To(Succeed())
134+
})
135+
136+
It("preserves the new unique name lease", func() {
137+
leaseName := fmt.Sprintf("servicebinding::sb::%s::%s::%s::%s", serviceBinding.Spec.AppRef.Name, serviceBinding.Namespace, serviceBinding.Spec.Service.Name, serviceBinding.Spec.Service.Name)
138+
newLease := &coordinationv1.Lease{
139+
ObjectMeta: metav1.ObjectMeta{
140+
Namespace: testNamespace,
141+
Name: fmt.Sprintf("n-%x", sha1.Sum([]byte(leaseName))),
142+
},
143+
}
144+
Consistently(func(g Gomega) {
145+
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(newLease), newLease)).To(Succeed())
146+
}).Should(Succeed())
147+
})
148+
149+
It("deletes the existing lease", func() {
150+
Eventually(func(g Gomega) {
151+
err := k8sClient.Get(ctx, client.ObjectKeyFromObject(existingLease), existingLease)
152+
g.Expect(k8serrors.IsNotFound(err)).To(BeTrue())
153+
}).Should(Succeed())
154+
})
155+
})
156+
157+
When("the binding has been already migrated by the current version", func() {
158+
BeforeEach(func() {
159+
Expect(k8s.Patch(ctx, k8sClient, serviceBinding, func() {
160+
serviceBinding.Labels = map[string]string{
161+
migration.MigratedByLabelKey: "test-version",
162+
}
163+
})).To(Succeed())
164+
})
165+
166+
It("does not migrate again", func() {
167+
Consistently(func(g Gomega) {
168+
err := k8sClient.Get(ctx, client.ObjectKeyFromObject(existingLease), existingLease)
169+
g.Expect(err).NotTo(HaveOccurred())
170+
}).Should(Succeed())
171+
})
172+
})
173+
174+
When("the binding has been migrated by another version", func() {
175+
BeforeEach(func() {
176+
Expect(k8s.Patch(ctx, k8sClient, serviceBinding, func() {
177+
serviceBinding.Labels = map[string]string{
178+
migration.MigratedByLabelKey: "another-version",
179+
}
180+
})).To(Succeed())
181+
})
182+
183+
It("migrates the binding", func() {
184+
Eventually(func(g Gomega) {
185+
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(serviceBinding), serviceBinding)).To(Succeed())
186+
g.Expect(serviceBinding.Labels).To(HaveKeyWithValue(migration.MigratedByLabelKey, "test-version"))
187+
}).Should(Succeed())
188+
})
189+
})
190+
})
191+
})

0 commit comments

Comments
 (0)