Skip to content

Commit 680596b

Browse files
lunarwhitelunarwhite
committed
Fix IstioCSR cache sync race condition by using unified manager cache
1 parent b099c2f commit 680596b

File tree

3 files changed

+55
-111
lines changed

3 files changed

+55
-111
lines changed

‎pkg/controller/istiocsr/client.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@ type ctrlClient interface {
3131
}
3232

3333
func NewClient(m manager.Manager) (ctrlClient, error) {
34-
c, err := BuildCustomClient(m)
35-
if err != nil {
36-
return nil, fmt.Errorf("failed to build custom client: %w", err)
37-
}
34+
// Use the manager's client directly instead of creating a custom client.
35+
// The manager's client uses the manager's cache, which ensures the reconciler
36+
// reads from the same cache that the controller's watches use, preventing
37+
// cache mismatch issues.
3838
return &ctrlClientImpl{
39-
Client: c,
39+
Client: m.GetClient(),
4040
}, nil
4141
}
4242

‎pkg/controller/istiocsr/controller.go‎

Lines changed: 47 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"k8s.io/apimachinery/pkg/runtime"
1616
"k8s.io/apimachinery/pkg/selection"
1717
"k8s.io/apimachinery/pkg/types"
18+
"k8s.io/client-go/rest"
1819
"k8s.io/client-go/tools/record"
1920

2021
ctrl "sigs.k8s.io/controller-runtime"
@@ -56,6 +57,52 @@ type Reconciler struct {
5657
// +kubebuilder:rbac:groups=operator.openshift.io,resources=istiocsrs/status,verbs=get;update;patch
5758
// +kubebuilder:rbac:groups=operator.openshift.io,resources=istiocsrs/finalizers,verbs=update
5859

60+
// NewCacheBuilder returns a cache builder function configured with label selectors
61+
// for managed resources. This function is used by the manager to create its cache
62+
// to ensure the reconciler reads from the same cache that the controller's watches use.
63+
func NewCacheBuilder(config *rest.Config, opts cache.Options) (cache.Cache, error) {
64+
managedResourceLabelReq, _ := labels.NewRequirement(requestEnqueueLabelKey, selection.Equals, []string{requestEnqueueLabelValue})
65+
managedResourceLabelReqSelector := labels.NewSelector().Add(*managedResourceLabelReq)
66+
67+
// Configure cache with label selectors for managed resources
68+
opts.ByObject = map[client.Object]cache.ByObject{
69+
// Explicitly include IstioCSR to ensure the cache properly watches and syncs all IstioCSR objects
70+
&v1alpha1.IstioCSR{}: {},
71+
// Resources managed by controller (with label selectors)
72+
&certmanagerv1.Certificate{}: {
73+
Label: managedResourceLabelReqSelector,
74+
},
75+
&appsv1.Deployment{}: {
76+
Label: managedResourceLabelReqSelector,
77+
},
78+
&rbacv1.ClusterRole{}: {
79+
Label: managedResourceLabelReqSelector,
80+
},
81+
&rbacv1.ClusterRoleBinding{}: {
82+
Label: managedResourceLabelReqSelector,
83+
},
84+
&rbacv1.Role{}: {
85+
Label: managedResourceLabelReqSelector,
86+
},
87+
&rbacv1.RoleBinding{}: {
88+
Label: managedResourceLabelReqSelector,
89+
},
90+
&corev1.Service{}: {
91+
Label: managedResourceLabelReqSelector,
92+
},
93+
&corev1.ServiceAccount{}: {
94+
Label: managedResourceLabelReqSelector,
95+
},
96+
// Resources watched but not filtered
97+
&corev1.Secret{}: {},
98+
&corev1.ConfigMap{}: {},
99+
&certmanagerv1.Issuer{}: {},
100+
&certmanagerv1.ClusterIssuer{}: {},
101+
}
102+
103+
return cache.New(config, opts)
104+
}
105+
59106
// New returns a new Reconciler instance.
60107
func New(mgr ctrl.Manager) (*Reconciler, error) {
61108
c, err := NewClient(mgr)
@@ -71,106 +118,6 @@ func New(mgr ctrl.Manager) (*Reconciler, error) {
71118
}, nil
72119
}
73120

74-
func BuildCustomClient(mgr ctrl.Manager) (client.Client, error) {
75-
managedResourceLabelReq, _ := labels.NewRequirement(requestEnqueueLabelKey, selection.Equals, []string{requestEnqueueLabelValue})
76-
managedResourceLabelReqSelector := labels.NewSelector().Add(*managedResourceLabelReq)
77-
78-
customCacheOpts := cache.Options{
79-
HTTPClient: mgr.GetHTTPClient(),
80-
Scheme: mgr.GetScheme(),
81-
Mapper: mgr.GetRESTMapper(),
82-
ByObject: map[client.Object]cache.ByObject{
83-
&certmanagerv1.Certificate{}: {
84-
Label: managedResourceLabelReqSelector,
85-
},
86-
&appsv1.Deployment{}: {
87-
Label: managedResourceLabelReqSelector,
88-
},
89-
&rbacv1.ClusterRole{}: {
90-
Label: managedResourceLabelReqSelector,
91-
},
92-
&rbacv1.ClusterRoleBinding{}: {
93-
Label: managedResourceLabelReqSelector,
94-
},
95-
&rbacv1.Role{}: {
96-
Label: managedResourceLabelReqSelector,
97-
},
98-
&rbacv1.RoleBinding{}: {
99-
Label: managedResourceLabelReqSelector,
100-
},
101-
&corev1.Service{}: {
102-
Label: managedResourceLabelReqSelector,
103-
},
104-
&corev1.ServiceAccount{}: {
105-
Label: managedResourceLabelReqSelector,
106-
},
107-
},
108-
ReaderFailOnMissingInformer: true,
109-
}
110-
customCache, err := cache.New(mgr.GetConfig(), customCacheOpts)
111-
if err != nil {
112-
return nil, err
113-
}
114-
if _, err = customCache.GetInformer(context.Background(), &v1alpha1.IstioCSR{}); err != nil {
115-
return nil, err
116-
}
117-
if _, err = customCache.GetInformer(context.Background(), &certmanagerv1.Certificate{}); err != nil {
118-
return nil, err
119-
}
120-
if _, err = customCache.GetInformer(context.Background(), &appsv1.Deployment{}); err != nil {
121-
return nil, err
122-
}
123-
if _, err = customCache.GetInformer(context.Background(), &rbacv1.ClusterRole{}); err != nil {
124-
return nil, err
125-
}
126-
if _, err = customCache.GetInformer(context.Background(), &rbacv1.ClusterRoleBinding{}); err != nil {
127-
return nil, err
128-
}
129-
if _, err = customCache.GetInformer(context.Background(), &rbacv1.Role{}); err != nil {
130-
return nil, err
131-
}
132-
if _, err = customCache.GetInformer(context.Background(), &rbacv1.RoleBinding{}); err != nil {
133-
return nil, err
134-
}
135-
if _, err = customCache.GetInformer(context.Background(), &corev1.Service{}); err != nil {
136-
return nil, err
137-
}
138-
if _, err = customCache.GetInformer(context.Background(), &corev1.ServiceAccount{}); err != nil {
139-
return nil, err
140-
}
141-
if _, err = customCache.GetInformer(context.Background(), &corev1.Secret{}); err != nil {
142-
return nil, err
143-
}
144-
if _, err = customCache.GetInformer(context.Background(), &corev1.ConfigMap{}); err != nil {
145-
return nil, err
146-
}
147-
if _, err = customCache.GetInformer(context.Background(), &certmanagerv1.Issuer{}); err != nil {
148-
return nil, err
149-
}
150-
if _, err = customCache.GetInformer(context.Background(), &certmanagerv1.ClusterIssuer{}); err != nil {
151-
return nil, err
152-
}
153-
154-
err = mgr.Add(customCache)
155-
if err != nil {
156-
return nil, err
157-
}
158-
159-
customClient, err := client.New(mgr.GetConfig(), client.Options{
160-
HTTPClient: mgr.GetHTTPClient(),
161-
Scheme: mgr.GetScheme(),
162-
Mapper: mgr.GetRESTMapper(),
163-
Cache: &client.CacheOptions{
164-
Reader: customCache,
165-
},
166-
})
167-
if err != nil {
168-
return nil, err
169-
}
170-
171-
return customClient, nil
172-
}
173-
174121
// SetupWithManager sets up the controller with the Manager.
175122
func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
176123
mapFunc := func(ctx context.Context, obj client.Object) []reconcile.Request {

‎pkg/operator/setup_manager.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,9 @@ import (
1010
"k8s.io/apimachinery/pkg/runtime"
1111
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
1212
clientscheme "k8s.io/client-go/kubernetes/scheme"
13-
"k8s.io/client-go/rest"
1413
"k8s.io/klog/v2"
1514

1615
ctrl "sigs.k8s.io/controller-runtime"
17-
"sigs.k8s.io/controller-runtime/pkg/client"
1816
ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
1917
"sigs.k8s.io/controller-runtime/pkg/manager"
2018

@@ -54,10 +52,9 @@ func NewControllerManager() (*Manager, error) {
5452

5553
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
5654
Scheme: scheme,
57-
NewClient: func(config *rest.Config, options client.Options) (client.Client, error) {
58-
return client.New(config, options)
59-
},
60-
Logger: ctrl.Log.WithName("operator-manager"),
55+
// Use custom cache builder to configure label selectors for managed resources
56+
NewCache: istiocsr.NewCacheBuilder,
57+
Logger: ctrl.Log.WithName("operator-manager"),
6158
})
6259
if err != nil {
6360
return nil, fmt.Errorf("failed to create manager: %w", err)

0 commit comments

Comments
 (0)