Skip to content

Commit 8cbb499

Browse files
kerenlahavkerenlahav
authored
BUG fix - OOM crash when cluster contains a lot of secrets (#610)
1 parent b0554dd commit 8cbb499

File tree

9 files changed

+84
-52
lines changed

9 files changed

+84
-52
lines changed

‎.github/workflows/go.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@ jobs:
1313
runs-on: ubuntu-latest
1414
steps:
1515

16-
- name: Set up Go 1.25.6
16+
- name: Set up Go 1.25.7
1717
uses: actions/setup-go@v2
1818
with:
19-
go-version: 1.25.6
19+
go-version: 1.25.7
2020

2121
- name: Check out code into the Go module directory
2222
uses: actions/checkout@v2

‎Dockerfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# Build the manager binary
2-
FROM --platform=$BUILDPLATFORM golang:1.25.6-alpine3.23 as builder
2+
FROM --platform=$BUILDPLATFORM golang:1.25.7-alpine3.23 as builder
33

44
WORKDIR /workspace
55
# Copy the Go Modules manifests

‎api/common/consts.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const (
55
ClusterSecretLabel = "services.cloud.sap.com/cluster-secret"
66
InstanceSecretRefLabel = "services.cloud.sap.com/secret-ref_"
77
WatchSecretAnnotation = "services.cloud.sap.com/watch-secret-"
8+
WatchSecretLabel = "services.cloud.sap.com/watch-secret"
89

910
NamespaceLabel = "_namespace"
1011
K8sNameLabel = "_k8sname"

‎controllers/secret_controller.go‎

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ import (
66
"reflect"
77

88
"github.com/SAP/sap-btp-service-operator/internal/utils/logutils"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"sigs.k8s.io/controller-runtime/pkg/builder"
911
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
10-
1112
"sigs.k8s.io/controller-runtime/pkg/event"
1213
"sigs.k8s.io/controller-runtime/pkg/predicate"
1314

@@ -78,50 +79,40 @@ func (r *SecretReconciler) Reconcile(ctx context.Context, req reconcile.Request)
7879

7980
// SetupWithManager sets up the controller with the Manager.
8081
func (r *SecretReconciler) SetupWithManager(mgr ctrl.Manager) error {
81-
predicates := predicate.Funcs{
82+
labelSelectorPredicate, err := predicate.LabelSelectorPredicate(metav1.LabelSelector{
83+
MatchLabels: map[string]string{common.WatchSecretLabel: "true"},
84+
})
85+
if err != nil {
86+
return err
87+
}
88+
89+
dataChangedPredicate := predicate.Funcs{
8290
UpdateFunc: func(e event.UpdateEvent) bool {
83-
return (utils.IsSecretWatched(e.ObjectNew.GetAnnotations()) && isSecretDataChanged(e)) || isSecretInDelete(e)
91+
oldSecret := e.ObjectOld.(*corev1.Secret)
92+
newSecret := e.ObjectNew.(*corev1.Secret)
93+
return (utils.IsSecretWatched(newSecret) && isSecretDataChanged(oldSecret, newSecret)) || isSecretInDelete(newSecret)
8494
},
8595
CreateFunc: func(e event.CreateEvent) bool {
86-
return utils.IsSecretWatched(e.Object.GetAnnotations())
96+
return utils.IsSecretWatched(e.Object)
8797
},
8898
DeleteFunc: func(e event.DeleteEvent) bool {
89-
return utils.IsSecretWatched(e.Object.GetAnnotations())
99+
return utils.IsSecretWatched(e.Object)
90100
},
91101
GenericFunc: func(e event.GenericEvent) bool {
92-
return utils.IsSecretWatched(e.Object.GetAnnotations())
102+
return utils.IsSecretWatched(e.Object)
93103
},
94104
}
95105

96106
return ctrl.NewControllerManagedBy(mgr).
97-
For(&corev1.Secret{}).
98-
WithEventFilter(predicates).
107+
For(&corev1.Secret{}, builder.WithPredicates(labelSelectorPredicate, dataChangedPredicate)).
99108
WithOptions(controller.Options{MaxConcurrentReconciles: 1}).
100109
Complete(r)
101110
}
102111

103-
func isSecretDataChanged(e event.UpdateEvent) bool {
104-
// Type assert to *v1.Secret
105-
oldSecret, okOld := e.ObjectOld.(*corev1.Secret)
106-
newSecret, okNew := e.ObjectNew.(*corev1.Secret)
107-
if !okOld || !okNew {
108-
// If the objects are not Secrets, skip the event
109-
return false
110-
}
111-
112-
// Compare the Data field (byte slices)
112+
func isSecretDataChanged(oldSecret, newSecret *corev1.Secret) bool {
113113
return !reflect.DeepEqual(oldSecret.Data, newSecret.Data) || !reflect.DeepEqual(oldSecret.StringData, newSecret.StringData)
114114
}
115115

116-
func isSecretInDelete(e event.UpdateEvent) bool {
117-
// Type assert to *v1.Secret
118-
119-
newSecret, okNew := e.ObjectNew.(*corev1.Secret)
120-
if !okNew {
121-
// If the objects are not Secrets, skip the event
122-
return false
123-
}
124-
125-
// Compare the Data field (byte slices)
126-
return !newSecret.GetDeletionTimestamp().IsZero() && controllerutil.ContainsFinalizer(newSecret, common.FinalizerName)
116+
func isSecretInDelete(secret *corev1.Secret) bool {
117+
return !secret.GetDeletionTimestamp().IsZero() && controllerutil.ContainsFinalizer(secret, common.FinalizerName)
127118
}

‎controllers/serviceinstance_controller.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626

2727
"github.com/SAP/sap-btp-service-operator/internal/utils/logutils"
2828
"github.com/pkg/errors"
29+
corev1 "k8s.io/api/core/v1"
2930
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3031

3132
"k8s.io/apimachinery/pkg/types"
@@ -143,12 +144,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
143144
}
144145

145146
if isFinalState(ctx, serviceInstance) {
146-
if len(serviceInstance.Status.HashedSpec) == 0 {
147-
updateHashedSpecValue(serviceInstance)
148-
return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
149-
}
150-
151-
return ctrl.Result{}, nil
147+
return r.maintainFinalState(ctx, serviceInstance)
152148
}
153149

154150
if controllerutil.AddFinalizer(serviceInstance, common.FinalizerName) {
@@ -574,7 +570,6 @@ func (r *ServiceInstanceReconciler) buildSMRequestParameters(ctx context.Context
574570
log.Error(err, fmt.Sprintf("failed to mark secret for watch %s", secret.Name))
575571
return nil, err
576572
}
577-
578573
}
579574
}
580575

@@ -603,6 +598,34 @@ func (r *ServiceInstanceReconciler) buildSMRequestParameters(ctx context.Context
603598
return instanceParameters, nil
604599
}
605600

601+
func (r *ServiceInstanceReconciler) maintainFinalState(ctx context.Context, serviceInstance *v1.ServiceInstance) (ctrl.Result, error) {
602+
log := logutils.GetLogger(ctx)
603+
604+
if serviceInstance.IsSubscribedToParamSecretsChanges() {
605+
log.Info("instance is in final state, WatchParametersFromChanges is true, validating that all parameters secrets are watched")
606+
for _, param := range serviceInstance.Spec.ParametersFrom {
607+
if param.SecretKeyRef != nil {
608+
secret := &corev1.Secret{}
609+
if err := r.Get(ctx, types.NamespacedName{Name: param.SecretKeyRef.Name, Namespace: serviceInstance.Namespace}, secret); err != nil {
610+
log.Error(err, fmt.Sprintf("failed to get secret %s", param.SecretKeyRef.Name))
611+
return ctrl.Result{}, err
612+
}
613+
if err := utils.AddWatchForSecretIfNeeded(ctx, r.Client, secret, string(serviceInstance.UID)); err != nil {
614+
log.Error(err, fmt.Sprintf("failed to mark secret for watch %s", param.SecretKeyRef.Name))
615+
return ctrl.Result{}, err
616+
}
617+
}
618+
}
619+
}
620+
621+
if len(serviceInstance.Status.HashedSpec) == 0 {
622+
log.Info("instance is missing HashedSpec value, updating it")
623+
updateHashedSpecValue(serviceInstance)
624+
return ctrl.Result{}, utils.UpdateStatus(ctx, r.Client, serviceInstance)
625+
}
626+
return ctrl.Result{}, nil
627+
}
628+
606629
func isFinalState(ctx context.Context, serviceInstance *v1.ServiceInstance) bool {
607630
log := logutils.GetLogger(ctx)
608631

‎controllers/serviceinstance_controller_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1663,10 +1663,11 @@ func checkSecretAnnotationsAndLabels(ctx context.Context, k8sClient client.Clien
16631663
if len(instances) == 0 {
16641664
Eventually(func() bool {
16651665
Expect(k8sClient.Get(ctx, getResourceNamespacedName(paramsSecret), paramsSecret)).To(Succeed())
1666-
return !utils.IsSecretWatched(paramsSecret.Annotations) && len(paramsSecret.Finalizers) == 0
1666+
return !utils.IsSecretWatched(paramsSecret) && len(paramsSecret.Finalizers) == 0
16671667
}, timeout, interval).Should(BeTrue())
16681668
} else {
16691669
Expect(k8sClient.Get(ctx, getResourceNamespacedName(paramsSecret), paramsSecret)).To(Succeed())
1670+
Expect(paramsSecret.Labels[common.WatchSecretLabel]).To(Equal("true"))
16701671
for _, instance := range instances {
16711672
Expect(k8sClient.Get(ctx, getResourceNamespacedName(instance), instance)).To(Succeed())
16721673
Expect(instance.Labels[utils.GetLabelKeyForInstanceSecret(paramsSecret.Name)]).To(Equal(paramsSecret.Name))

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
module github.com/SAP/sap-btp-service-operator
22

3-
go 1.25.6
3+
go 1.25.7
44

55
require (
66
github.com/Masterminds/sprig/v3 v3.3.0

‎internal/utils/controller_util.go‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import (
2323

2424
"k8s.io/apimachinery/pkg/util/rand"
2525

26-
v1 "k8s.io/api/authentication/v1"
26+
authv1 "k8s.io/api/authentication/v1"
2727
)
2828

2929
const (
@@ -80,7 +80,7 @@ func NormalizeCredentials(credentialsJSON json.RawMessage) (map[string][]byte, [
8080
return normalized, metadata, nil
8181
}
8282

83-
func BuildUserInfo(ctx context.Context, userInfo *v1.UserInfo) string {
83+
func BuildUserInfo(ctx context.Context, userInfo *authv1.UserInfo) string {
8484
log := logutils.GetLogger(ctx)
8585
if userInfo == nil {
8686
return ""
@@ -177,13 +177,25 @@ func RemoveAnnotations(ctx context.Context, k8sClient client.Client, object comm
177177

178178
func AddWatchForSecretIfNeeded(ctx context.Context, k8sClient client.Client, secret *corev1.Secret, instanceUID string) error {
179179
log := logutils.GetLogger(ctx)
180+
updateRequired := false
180181
if secret.Annotations == nil {
181182
secret.Annotations = make(map[string]string)
182183
}
183184
if len(secret.Annotations[common.WatchSecretAnnotation+string(instanceUID)]) == 0 {
184-
log.Info(fmt.Sprintf("adding secret watch for secret %s", secret.Name))
185+
log.Info(fmt.Sprintf("adding secret watch annotation for instance %s on secret %s", instanceUID, secret.Name))
185186
secret.Annotations[common.WatchSecretAnnotation+instanceUID] = "true"
187+
updateRequired = true
188+
}
189+
if secret.Labels == nil {
190+
secret.Labels = make(map[string]string)
191+
}
192+
if secret.Labels[common.WatchSecretLabel] != "true" {
193+
log.Info(fmt.Sprintf("adding watch label for secret %s", secret.Name))
194+
secret.Labels[common.WatchSecretLabel] = "true"
186195
controllerutil.AddFinalizer(secret, common.FinalizerName)
196+
updateRequired = true
197+
}
198+
if updateRequired {
187199
return k8sClient.Update(ctx, secret)
188200
}
189201

@@ -197,19 +209,22 @@ func RemoveWatchForSecret(ctx context.Context, k8sClient client.Client, secretKe
197209
}
198210

199211
delete(secret.Annotations, common.WatchSecretAnnotation+instanceUID)
200-
if !IsSecretWatched(secret.Annotations) {
212+
existInstanceAnnotation := false
213+
for key := range secret.Annotations {
214+
if strings.HasPrefix(key, common.WatchSecretAnnotation) {
215+
existInstanceAnnotation = true
216+
break
217+
}
218+
}
219+
if !existInstanceAnnotation {
220+
delete(secret.Labels, common.WatchSecretLabel)
201221
controllerutil.RemoveFinalizer(secret, common.FinalizerName)
202222
}
203223
return k8sClient.Update(ctx, secret)
204224
}
205225

206-
func IsSecretWatched(secretAnnotations map[string]string) bool {
207-
for key := range secretAnnotations {
208-
if strings.HasPrefix(key, common.WatchSecretAnnotation) {
209-
return true
210-
}
211-
}
212-
return false
226+
func IsSecretWatched(secret client.Object) bool {
227+
return secret.GetLabels() != nil && secret.GetLabels()[common.WatchSecretLabel] == "true"
213228
}
214229

215230
func GetLabelKeyForInstanceSecret(secretName string) string {

‎internal/utils/controller_util_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ var _ = Describe("Controller Util", func() {
185185
Expect(err).ToNot(HaveOccurred())
186186
Expect(updatedSecret.Finalizers[0]).To(Equal(common.FinalizerName))
187187
Expect(updatedSecret.Annotations[common.WatchSecretAnnotation+"123"]).To(Equal("true"))
188+
Expect(updatedSecret.Labels[common.WatchSecretLabel]).To(Equal("true"))
188189

189190
})
190191
})

0 commit comments

Comments
 (0)