Skip to content

Commit 8bd91f7

Browse files
kishorjkishorj
authored
monitor secret resources ony if necessary (#2550)
* monitor secret resources ony if necessary * removes the default watch for all secret resources in the cluster * create watch for specific secret resource if required for OIDC feature * do not configure rbac permissions for secrets by default * provide helm chart configuration options for specific secret resources * remove option to configure specific secret names * use channel for secrets event
1 parent 7343672 commit 8bd91f7

File tree

13 files changed

+428
-22
lines changed

13 files changed

+428
-22
lines changed

‎config/rbac/role.yaml

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,6 @@ rules:
5252
verbs:
5353
- patch
5454
- update
55-
- apiGroups:
56-
- ""
57-
resources:
58-
- secrets
59-
verbs:
60-
- get
61-
- list
62-
- watch
6355
- apiGroups:
6456
- ""
6557
resources:

‎controllers/ingress/eventhandlers/secret_events.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ func (h *enqueueRequestsForSecretEvent) Delete(e event.DeleteEvent, _ workqueue.
6363
}
6464

6565
func (h *enqueueRequestsForSecretEvent) Generic(e event.GenericEvent, _ workqueue.RateLimitingInterface) {
66-
// we don't have any generic event for secrets.
66+
secretObj := e.Object.(*corev1.Secret)
67+
h.enqueueImpactedObjects(secretObj)
6768
}
6869

6970
func (h *enqueueRequestsForSecretEvent) enqueueImpactedObjects(secret *corev1.Secret) {

‎controllers/ingress/group_controller.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ type groupReconciler struct {
9393
stackMarshaller deploy.StackMarshaller
9494
stackDeployer deploy.StackDeployer
9595
backendSGProvider networkingpkg.BackendSGProvider
96+
secretsManager k8s.SecretsManager
9697

9798
groupLoader ingress.GroupLoader
9899
groupFinalizerManager ingress.FinalizerManager
@@ -160,7 +161,7 @@ func (r *groupReconciler) reconcile(ctx context.Context, req ctrl.Request) error
160161
}
161162

162163
func (r *groupReconciler) buildAndDeployModel(ctx context.Context, ingGroup ingress.Group) (core.Stack, *elbv2model.LoadBalancer, error) {
163-
stack, lb, err := r.modelBuilder.Build(ctx, ingGroup)
164+
stack, lb, secrets, err := r.modelBuilder.Build(ctx, ingGroup)
164165
if err != nil {
165166
r.recordIngressGroupEvent(ctx, ingGroup, corev1.EventTypeWarning, k8s.IngressEventReasonFailedBuildModel, fmt.Sprintf("Failed build model due to %v", err))
166167
return nil, nil, err
@@ -177,6 +178,7 @@ func (r *groupReconciler) buildAndDeployModel(ctx context.Context, ingGroup ingr
177178
return nil, nil, err
178179
}
179180
r.logger.Info("successfully deployed model", "ingressGroup", ingGroup.ID)
181+
r.secretsManager.MonitorSecrets(ingGroup.ID.String(), secrets)
180182
return stack, lb, err
181183
}
182184

@@ -229,7 +231,7 @@ func (r *groupReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager
229231
if err := r.setupIndexes(ctx, mgr.GetFieldIndexer(), ingressClassResourceAvailable); err != nil {
230232
return err
231233
}
232-
if err := r.setupWatches(ctx, c, ingressClassResourceAvailable); err != nil {
234+
if err := r.setupWatches(ctx, c, ingressClassResourceAvailable, clientSet); err != nil {
233235
return err
234236
}
235237
return nil
@@ -276,9 +278,10 @@ func (r *groupReconciler) setupIndexes(ctx context.Context, fieldIndexer client.
276278
return nil
277279
}
278280

279-
func (r *groupReconciler) setupWatches(_ context.Context, c controller.Controller, ingressClassResourceAvailable bool) error {
281+
func (r *groupReconciler) setupWatches(_ context.Context, c controller.Controller, ingressClassResourceAvailable bool, clientSet *kubernetes.Clientset) error {
280282
ingEventChan := make(chan event.GenericEvent)
281283
svcEventChan := make(chan event.GenericEvent)
284+
secretEventsChan := make(chan event.GenericEvent)
282285
ingEventHandler := eventhandlers.NewEnqueueRequestsForIngressEvent(r.groupLoader, r.eventRecorder,
283286
r.logger.WithName("eventHandlers").WithName("ingress"))
284287
svcEventHandler := eventhandlers.NewEnqueueRequestsForServiceEvent(ingEventChan, r.k8sClient, r.eventRecorder,
@@ -297,10 +300,9 @@ func (r *groupReconciler) setupWatches(_ context.Context, c controller.Controlle
297300
if err := c.Watch(&source.Kind{Type: &corev1.Service{}}, svcEventHandler); err != nil {
298301
return err
299302
}
300-
if err := c.Watch(&source.Kind{Type: &corev1.Secret{}}, secretEventHandler); err != nil {
303+
if err := c.Watch(&source.Channel{Source: secretEventsChan}, secretEventHandler); err != nil {
301304
return err
302305
}
303-
304306
if ingressClassResourceAvailable {
305307
ingClassEventChan := make(chan event.GenericEvent)
306308
ingClassParamsEventHandler := eventhandlers.NewEnqueueRequestsForIngressClassParamsEvent(ingClassEventChan, r.k8sClient, r.eventRecorder,
@@ -317,6 +319,7 @@ func (r *groupReconciler) setupWatches(_ context.Context, c controller.Controlle
317319
return err
318320
}
319321
}
322+
r.secretsManager = k8s.NewSecretsManager(clientSet, secretEventsChan, ctrl.Log.WithName("secrets-manager"))
320323
return nil
321324
}
322325

‎helm/aws-load-balancer-controller/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,3 +234,4 @@ The default values set by the application itself can be confirmed [here](https:/
234234
| `serviceMonitor.enabled` | Specifies whether a service monitor should be created, requires the ServiceMonitor CRD to be installed | `false` |
235235
| `serviceMonitor.additionalLabels` | Labels to add to the service account | `{}` |
236236
| `serviceMonitor.interval` | Prometheus scrape interval | `1m` |
237+
| `clusterSecretsPermissions.allowAllSecrets` | If `true`, controller has access to all secrets in the cluster. | `false` |

‎helm/aws-load-balancer-controller/templates/rbac.yaml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,13 @@ rules:
5757
resources: [services, ingresses]
5858
verbs: [get, list, patch, update, watch]
5959
- apiGroups: [""]
60-
resources: [nodes, secrets, namespaces, endpoints]
60+
resources: [nodes, namespaces, endpoints]
6161
verbs: [get, list, watch]
62+
{{- if .Values.clusterSecretsPermissions.allowAllSecrets }}
63+
- apiGroups: [""]
64+
resources: [secrets]
65+
verbs: [get, list, watch]
66+
{{- end }}
6267
- apiGroups: ["elbv2.k8s.aws", "", "extensions", "networking.k8s.io"]
6368
resources: [targetgroupbindings/status, pods/status, services/status, ingresses/status]
6469
verbs: [update, patch]

‎helm/aws-load-balancer-controller/values.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,3 +266,12 @@ serviceMonitor:
266266
additionalLabels: {}
267267
# Prometheus scrape interval
268268
interval: 1m
269+
270+
# clusterSecretsPermissions lets you configure RBAC permissions for secret resources
271+
# Access to secrets resource is required only if you use the OIDC feature, and instead of
272+
# enabling access to all secrets, we recommend configuring namespaced role/rolebinding.
273+
# This option is for backwards compatibility only, and will potentially be deprecated in future.
274+
clusterSecretsPermissions:
275+
# allowAllSecrets allows the controller to access all secrets in the cluster.
276+
# This is to get backwards compatible behavior, but *NOT* recommended for security reasons
277+
allowAllSecrets: false

‎pkg/config/runtime_config.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
package config
22

33
import (
4+
"time"
5+
46
"github.com/spf13/pflag"
57
corev1 "k8s.io/api/core/v1"
68
"k8s.io/apimachinery/pkg/runtime"
79
"k8s.io/client-go/rest"
810
"k8s.io/client-go/tools/clientcmd"
911
"k8s.io/client-go/tools/leaderelection/resourcelock"
1012
ctrl "sigs.k8s.io/controller-runtime"
11-
"time"
13+
"sigs.k8s.io/controller-runtime/pkg/client"
1214
)
1315

1416
const (
@@ -121,6 +123,7 @@ func BuildRuntimeOptions(rtCfg RuntimeConfig, scheme *runtime.Scheme) ctrl.Optio
121123
LeaderElectionNamespace: rtCfg.LeaderElectionNamespace,
122124
Namespace: rtCfg.WatchNamespace,
123125
SyncPeriod: &rtCfg.SyncPeriod,
126+
ClientDisableCacheFor: []client.Object{&corev1.Secret{}},
124127
}
125128
}
126129

‎pkg/ingress/model_build_actions.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,6 @@ func (t *defaultModelBuildTask) buildAuthenticateOIDCAction(ctx context.Context,
172172
if err := t.k8sClient.Get(ctx, secretKey, secret); err != nil {
173173
return elbv2model.Action{}, err
174174
}
175-
176175
rawClientID, ok := secret.Data["clientID"]
177176
// AWSALBIngressController looks for clientId, we should be backwards-compatible here.
178177
if !ok {
@@ -186,6 +185,7 @@ func (t *defaultModelBuildTask) buildAuthenticateOIDCAction(ctx context.Context,
186185
return elbv2model.Action{}, errors.Errorf("missing clientSecret, secret: %v", secretKey)
187186
}
188187

188+
t.secretKeys = append(t.secretKeys, secretKey)
189189
clientID := strings.TrimRightFunc(string(rawClientID), unicode.IsSpace)
190190
clientSecret := string(rawClientSecret)
191191
return elbv2model.Action{

‎pkg/ingress/model_builder.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ const (
3030
// ModelBuilder is responsible for build mode stack for a IngressGroup.
3131
type ModelBuilder interface {
3232
// build mode stack for a IngressGroup.
33-
Build(ctx context.Context, ingGroup Group) (core.Stack, *elbv2model.LoadBalancer, error)
33+
Build(ctx context.Context, ingGroup Group) (core.Stack, *elbv2model.LoadBalancer, []types.NamespacedName, error)
3434
}
3535

3636
// NewDefaultModelBuilder constructs new defaultModelBuilder.
@@ -97,7 +97,7 @@ type defaultModelBuilder struct {
9797
}
9898

9999
// build mode stack for a IngressGroup.
100-
func (b *defaultModelBuilder) Build(ctx context.Context, ingGroup Group) (core.Stack, *elbv2model.LoadBalancer, error) {
100+
func (b *defaultModelBuilder) Build(ctx context.Context, ingGroup Group) (core.Stack, *elbv2model.LoadBalancer, []types.NamespacedName, error) {
101101
stack := core.NewDefaultStack(core.StackID(ingGroup.ID))
102102
task := &defaultModelBuildTask{
103103
k8sClient: b.k8sClient,
@@ -143,9 +143,9 @@ func (b *defaultModelBuilder) Build(ctx context.Context, ingGroup Group) (core.S
143143
backendServices: make(map[types.NamespacedName]*corev1.Service),
144144
}
145145
if err := task.run(ctx); err != nil {
146-
return nil, nil, err
146+
return nil, nil, nil, err
147147
}
148-
return task.stack, task.loadBalancer, nil
148+
return task.stack, task.loadBalancer, task.secretKeys, nil
149149
}
150150

151151
// the default model build task
@@ -193,6 +193,7 @@ type defaultModelBuildTask struct {
193193
loadBalancer *elbv2model.LoadBalancer
194194
tgByResID map[string]*elbv2model.TargetGroup
195195
backendServices map[types.NamespacedName]*corev1.Service
196+
secretKeys []types.NamespacedName
196197
}
197198

198199
func (t *defaultModelBuildTask) run(ctx context.Context) error {

‎pkg/ingress/model_builder_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3900,7 +3900,7 @@ func Test_defaultModelBuilder_Build(t *testing.T) {
39003900
defaultSSLPolicy: "ELBSecurityPolicy-2016-08",
39013901
}
39023902

3903-
gotStack, _, err := b.Build(context.Background(), tt.args.ingGroup)
3903+
gotStack, _, _, err := b.Build(context.Background(), tt.args.ingGroup)
39043904
if tt.wantErr != nil {
39053905
assert.EqualError(t, err, tt.wantErr.Error())
39063906
} else {

‎pkg/k8s/secrets_manager.go

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
package k8s
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"k8s.io/apimachinery/pkg/types"
7+
"k8s.io/apimachinery/pkg/util/sets"
8+
"k8s.io/client-go/util/workqueue"
9+
"sigs.k8s.io/controller-runtime/pkg/event"
10+
"sync"
11+
12+
"github.com/go-logr/logr"
13+
14+
corev1 "k8s.io/api/core/v1"
15+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
16+
"k8s.io/apimachinery/pkg/fields"
17+
"k8s.io/apimachinery/pkg/runtime"
18+
"k8s.io/apimachinery/pkg/watch"
19+
"k8s.io/client-go/kubernetes"
20+
"k8s.io/client-go/tools/cache"
21+
)
22+
23+
// SecretsManager manages the secret resources needed by the controller
24+
type SecretsManager interface {
25+
// MonitorSecrets manages the individual watches for the given secrets
26+
MonitorSecrets(ingressGroupID string, secrets []types.NamespacedName)
27+
}
28+
29+
func NewSecretsManager(clientSet kubernetes.Interface, secretsEventChan chan<- event.GenericEvent, logger logr.Logger) *defaultSecretsManager {
30+
return &defaultSecretsManager{
31+
mutex: sync.Mutex{},
32+
secretMap: make(map[types.NamespacedName]*secretItem),
33+
secretsEventChan: secretsEventChan,
34+
clientSet: clientSet,
35+
logger: logger,
36+
}
37+
}
38+
39+
var _ SecretsManager = &defaultSecretsManager{}
40+
41+
type defaultSecretsManager struct {
42+
mutex sync.Mutex
43+
secretMap map[types.NamespacedName]*secretItem
44+
secretsEventChan chan<- event.GenericEvent
45+
clientSet kubernetes.Interface
46+
queue workqueue.RateLimitingInterface
47+
logger logr.Logger
48+
}
49+
50+
type secretItem struct {
51+
store cache.Store
52+
rt *cache.Reflector
53+
ingresses sets.String
54+
55+
stopCh chan struct{}
56+
}
57+
58+
func (m *defaultSecretsManager) MonitorSecrets(ingressGroupID string, secrets []types.NamespacedName) {
59+
m.logger.V(1).Info("Monitoring secrets", "groupID", ingressGroupID, "secrets", secrets)
60+
m.mutex.Lock()
61+
defer m.mutex.Unlock()
62+
63+
inputSecrets := make(sets.String)
64+
for _, secret := range secrets {
65+
inputSecrets.Insert(secret.String())
66+
item, exists := m.secretMap[secret]
67+
if !exists {
68+
m.logger.V(1).Info("secret is not being monitored, adding watch", "item", secret)
69+
item = m.newReflector(secret.Namespace, secret.Name)
70+
m.secretMap[secret] = item
71+
}
72+
item.ingresses.Insert(ingressGroupID)
73+
}
74+
75+
// Perform garbage collection
76+
var cleanupSecrets []types.NamespacedName
77+
for secret, secretItem := range m.secretMap {
78+
if inputSecrets.Has(secret.String()) {
79+
continue
80+
}
81+
if secretItem.ingresses.Has(ingressGroupID) {
82+
secretItem.ingresses.Delete(ingressGroupID)
83+
}
84+
if secretItem.ingresses.Len() == 0 {
85+
cleanupSecrets = append(cleanupSecrets, secret)
86+
}
87+
}
88+
for _, secret := range cleanupSecrets {
89+
m.logger.V(1).Info("secret no longer needs monitoring, stopping the watch", "item", secret)
90+
m.secretMap[secret].stopReflector()
91+
delete(m.secretMap, secret)
92+
}
93+
}
94+
95+
func (m *defaultSecretsManager) newReflector(namespace, name string) *secretItem {
96+
fieldSelector := fields.Set{"metadata.name": name}.AsSelector().String()
97+
listFunc := func(options metav1.ListOptions) (runtime.Object, error) {
98+
options.FieldSelector = fieldSelector
99+
return m.clientSet.CoreV1().Secrets(namespace).List(context.TODO(), options)
100+
}
101+
watchFunc := func(options metav1.ListOptions) (watch.Interface, error) {
102+
options.FieldSelector = fieldSelector
103+
return m.clientSet.CoreV1().Secrets(namespace).Watch(context.TODO(), options)
104+
}
105+
store := m.newStore()
106+
rt := cache.NewNamedReflector(
107+
fmt.Sprintf("secret-%s/%s", namespace, name),
108+
&cache.ListWatch{ListFunc: listFunc, WatchFunc: watchFunc},
109+
&corev1.Secret{},
110+
store,
111+
0,
112+
)
113+
item := &secretItem{
114+
store: store,
115+
rt: rt,
116+
ingresses: make(sets.String),
117+
stopCh: make(chan struct{}),
118+
}
119+
go item.startReflector()
120+
return item
121+
}
122+
123+
func (m *defaultSecretsManager) newStore() *SecretsStore {
124+
return NewSecretsStore(m.secretsEventChan, cache.MetaNamespaceKeyFunc, m.logger)
125+
}
126+
127+
func (s *secretItem) stopReflector() {
128+
close(s.stopCh)
129+
}
130+
131+
func (s *secretItem) startReflector() {
132+
s.rt.Run(s.stopCh)
133+
}

0 commit comments

Comments
 (0)