From e6afd370ffbb8fae5c697ec540b982db19967a44 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Sat, 30 Apr 2022 21:21:14 +0530 Subject: [PATCH 01/21] fix: remove secrets Signed-off-by: bjenuhb --- server/auth/gatekeeper.go | 2 +- server/cache/cache.go | 13 +++++++++++-- server/cache/cache_test.go | 4 ++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/server/auth/gatekeeper.go b/server/auth/gatekeeper.go index ce936c0922b2..d8e6257030e9 100644 --- a/server/auth/gatekeeper.go +++ b/server/auth/gatekeeper.go @@ -318,7 +318,7 @@ func (s *gatekeeper) authorizationForServiceAccount(serviceAccount *corev1.Servi if len(serviceAccount.Secrets) == 0 { return "", fmt.Errorf("expected at least one secret for SSO RBAC service account: %s", serviceAccount.GetName()) } - secret, err := s.cache.SecretLister.Secrets(serviceAccount.GetNamespace()).Get(serviceAccount.Secrets[0].Name) + secret, err := s.cache.GetSecret(serviceAccount.GetNamespace(), serviceAccount.Secrets[0].Name) if err != nil { return "", fmt.Errorf("failed to get service account secret: %w", err) } diff --git a/server/cache/cache.go b/server/cache/cache.go index cd5540efb03f..7a3cb0b5946c 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -2,6 +2,8 @@ package cache import ( "context" + v12 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "time" "k8s.io/client-go/informers" @@ -10,17 +12,24 @@ import ( ) type ResourceCache struct { + ctx context.Context + client kubernetes.Interface v1.ServiceAccountLister - v1.SecretLister } func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) cache := &ResourceCache{ + ctx: ctx, + client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), - SecretLister: informerFactory.Core().V1().Secrets().Lister(), } informerFactory.Start(ctx.Done()) informerFactory.WaitForCacheSync(ctx.Done()) return cache } + +func (c *ResourceCache) GetSecret(namespace string, secretName string) (*v12.Secret, error) { + options := metav1.GetOptions{} + return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, options) +} diff --git a/server/cache/cache_test.go b/server/cache/cache_test.go index 42407d5391f5..34320b46d72d 100644 --- a/server/cache/cache_test.go +++ b/server/cache/cache_test.go @@ -87,7 +87,7 @@ func TestServer_K8sUtilsCache(t *testing.T) { assert.Equal(t, 1, len(sa)) assert.True(t, checkServiceAccountExists(sa, "sa3")) - secrets, _ := cache.SecretLister.Secrets("ns1").List(labels.Everything()) - assert.Equal(t, 1, len(secrets)) + secret, _ := cache.GetSecret("ns1", "s1") + assert.NotNil(t, secret) }) } From c06ea3e5849e8b489aecc5c4ac8bb580e7463a8d Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Sat, 30 Apr 2022 21:26:44 +0530 Subject: [PATCH 02/21] feat: remove list and watch on secrets Signed-off-by: bjenuhb --- .../argo-server-rbac/argo-server-clusterole.yaml | 2 -- manifests/install.yaml | 2 -- manifests/namespace-install.yaml | 2 -- .../namespace-install/argo-server-rbac/argo-server-role.yaml | 2 -- manifests/quick-start-minimal.yaml | 2 -- manifests/quick-start-mysql.yaml | 2 -- manifests/quick-start-postgres.yaml | 2 -- 7 files changed, 14 deletions(-) diff --git a/manifests/cluster-install/argo-server-rbac/argo-server-clusterole.yaml b/manifests/cluster-install/argo-server-rbac/argo-server-clusterole.yaml index 74e7849f901a..0882c9a7b9c9 100644 --- a/manifests/cluster-install/argo-server-rbac/argo-server-clusterole.yaml +++ b/manifests/cluster-install/argo-server-rbac/argo-server-clusterole.yaml @@ -18,8 +18,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/install.yaml b/manifests/install.yaml index 01e233f2867c..5c9d9af8cbff 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -944,8 +944,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index df01bb5e6723..22689f3a7f71 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -853,8 +853,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/namespace-install/argo-server-rbac/argo-server-role.yaml b/manifests/namespace-install/argo-server-rbac/argo-server-role.yaml index 0470b138d9d3..314177a3ef8e 100644 --- a/manifests/namespace-install/argo-server-rbac/argo-server-role.yaml +++ b/manifests/namespace-install/argo-server-rbac/argo-server-role.yaml @@ -18,8 +18,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/quick-start-minimal.yaml b/manifests/quick-start-minimal.yaml index 98d490d23cc8..8cdd6422c80d 100644 --- a/manifests/quick-start-minimal.yaml +++ b/manifests/quick-start-minimal.yaml @@ -882,8 +882,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/quick-start-mysql.yaml b/manifests/quick-start-mysql.yaml index b22be6cde2a3..ed94fa6bfa2f 100644 --- a/manifests/quick-start-mysql.yaml +++ b/manifests/quick-start-mysql.yaml @@ -882,8 +882,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/quick-start-postgres.yaml b/manifests/quick-start-postgres.yaml index eb06894ac303..466697b3547b 100644 --- a/manifests/quick-start-postgres.yaml +++ b/manifests/quick-start-postgres.yaml @@ -882,8 +882,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: From 65ac3e90fd9f75a551bd9ea3dc361bf84fac7730 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Sun, 1 May 2022 10:46:47 +0530 Subject: [PATCH 03/21] fix: lint Signed-off-by: bjenuhb --- server/cache/cache.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/cache/cache.go b/server/cache/cache.go index 7a3cb0b5946c..04893818940f 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -2,9 +2,10 @@ package cache import ( "context" + "time" + v12 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "time" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" From e096ce8f058f282e07abe1e1765ef189ce728a60 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Mon, 2 May 2022 17:56:27 +0530 Subject: [PATCH 04/21] feat: initial draft of timed cache Signed-off-by: bjenuhb --- server/cache/cache.go | 40 ++++++++++++++++++++++++------ server/cache/timed_cache.go | 49 +++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 8 deletions(-) create mode 100644 server/cache/timed_cache.go diff --git a/server/cache/cache.go b/server/cache/cache.go index 04893818940f..cd1f1af07a8e 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -4,7 +4,7 @@ import ( "context" "time" - v12 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/informers" @@ -13,15 +13,17 @@ import ( ) type ResourceCache struct { - ctx context.Context - client kubernetes.Interface + ctx context.Context + secretCache *timedCache[string, *corev1.Secret] + client kubernetes.Interface v1.ServiceAccountLister } -func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { - informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) +func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { + informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) cache := &ResourceCache{ ctx: ctx, + secretCache: NewTimedCache[string, *corev1.Secret](timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), } @@ -30,7 +32,29 @@ func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespac return cache } -func (c *ResourceCache) GetSecret(namespace string, secretName string) (*v12.Secret, error) { - options := metav1.GetOptions{} - return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, options) +func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { + return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*20) +} + +func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { + cacheKey := c.getSecretCacheKey(namespace, secretName) + if secret, ok := c.secretCache.Get(cacheKey); ok { + return secret, nil + } + + secret, err := c.getSecretFromServer(namespace, secretName) + if err != nil { + return nil, err + } + + c.secretCache.Add(cacheKey, secret) + return secret, nil +} + +func (c *ResourceCache) getSecretFromServer(namespace string, secretName string) (*corev1.Secret, error) { + return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, metav1.GetOptions{}) +} + +func (c *ResourceCache) getSecretCacheKey(namespace string, secretName string) string { + return namespace + ":secret:" + secretName } diff --git a/server/cache/timed_cache.go b/server/cache/timed_cache.go new file mode 100644 index 000000000000..641f547f746e --- /dev/null +++ b/server/cache/timed_cache.go @@ -0,0 +1,49 @@ +package cache + +import ( + "time" + + "k8s.io/utils/lru" +) + +type timedCache[Key comparable, Value any] struct { + timeout time.Duration + *lru.Cache +} + +type timeValueHolder struct { + createTime time.Time + value interface{} +} + +func NewTimedCache[key comparable, value any](timeout time.Duration, size int) *timedCache[key, value] { + return &timedCache[key, value]{ + timeout: timeout, + Cache: lru.New(size), + } +} + +func (c *timedCache[Key, Value]) Get(key Key) (Value, bool) { + if data, ok := c.Cache.Get(key); ok { + holder := data.(*timeValueHolder) + deadline := holder.createTime.Add(c.timeout) + if c.getCurrentTime().Before(deadline) { + if value, ok := holder.value.(Value); ok { + return value, true + } + } + c.Cache.Remove(key) + } + return *new(Value), false +} + +func (c *timedCache[Key, Value]) Add(key Key, value Value) { + c.Cache.Add(key, &timeValueHolder{ + createTime: c.getCurrentTime(), + value: value, + }) +} + +func (c *timedCache[Key, Value]) getCurrentTime() time.Time { + return time.Now().UTC() +} From fa5a098fa1196d70c1dc9caecb871393d2abb0b2 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Mon, 2 May 2022 18:02:38 +0530 Subject: [PATCH 05/21] feat: initial draft of timed cache Signed-off-by: bjenuhb --- server/cache/timed_cache.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/cache/timed_cache.go b/server/cache/timed_cache.go index 641f547f746e..f3f5dfa233fb 100644 --- a/server/cache/timed_cache.go +++ b/server/cache/timed_cache.go @@ -8,7 +8,7 @@ import ( type timedCache[Key comparable, Value any] struct { timeout time.Duration - *lru.Cache + cache *lru.Cache } type timeValueHolder struct { @@ -19,12 +19,12 @@ type timeValueHolder struct { func NewTimedCache[key comparable, value any](timeout time.Duration, size int) *timedCache[key, value] { return &timedCache[key, value]{ timeout: timeout, - Cache: lru.New(size), + cache: lru.New(size), } } func (c *timedCache[Key, Value]) Get(key Key) (Value, bool) { - if data, ok := c.Cache.Get(key); ok { + if data, ok := c.cache.Get(key); ok { holder := data.(*timeValueHolder) deadline := holder.createTime.Add(c.timeout) if c.getCurrentTime().Before(deadline) { @@ -32,13 +32,13 @@ func (c *timedCache[Key, Value]) Get(key Key) (Value, bool) { return value, true } } - c.Cache.Remove(key) + c.cache.Remove(key) } return *new(Value), false } func (c *timedCache[Key, Value]) Add(key Key, value Value) { - c.Cache.Add(key, &timeValueHolder{ + c.cache.Add(key, &timeValueHolder{ createTime: c.getCurrentTime(), value: value, }) From ada3ef48e406e2a8402360100726c35d279db702 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Mon, 2 May 2022 20:05:03 +0530 Subject: [PATCH 06/21] feat: add tests Signed-off-by: bjenuhb --- server/cache/timed_cache_test.go | 54 ++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 server/cache/timed_cache_test.go diff --git a/server/cache/timed_cache_test.go b/server/cache/timed_cache_test.go new file mode 100644 index 000000000000..953bdc18cc84 --- /dev/null +++ b/server/cache/timed_cache_test.go @@ -0,0 +1,54 @@ +package cache + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestNewTimedCache(t *testing.T) { + + t.Run("NewTimedCache should return a new instance", func(t *testing.T) { + cache := NewTimedCache[string, string](time.Second, 1) + assert.NotNil(t, cache) + }) + + t.Run("TimedCache should cache based on LRU size", func(t *testing.T) { + cache := NewTimedCache[string, string](time.Second*10, 2) + cache.Add("one", "one") + cache.Add("two", "two") + + // Both "one" and "two" should be available since maxSize is 2 + _, ok := cache.Get("one") + assert.True(t, ok) + + _, ok = cache.Get("two") + assert.True(t, ok) + + // "three" should be available since its newly added + cache.Add("three", "three") + _, ok = cache.Get("three") + assert.True(t, ok) + + // "one" should not be available since maxSize is 2 + _, ok = cache.Get("one") + assert.False(t, ok) + }) + + t.Run("TimedCache should cache based on timeout", func(t *testing.T) { + cache := NewTimedCache[string, string](time.Millisecond*5, 2) + cache.Add("one", "one") + + _, ok := cache.Get("one") + assert.True(t, ok) + + time.Sleep(time.Millisecond * 10) + + // "one" should not be available since timeout is 5 ms + _, ok = cache.Get("one") + assert.False(t, ok) + + }) + +} From 41830bda3bf98733bd575fd5d9d62a0febd2aa79 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Tue, 3 May 2022 12:23:18 +0530 Subject: [PATCH 07/21] fix: remove generics to fix build Signed-off-by: bjenuhb --- server/cache/cache.go | 8 +++++--- server/cache/timed_cache.go | 24 +++++++++++------------- server/cache/timed_cache_test.go | 6 +++--- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/server/cache/cache.go b/server/cache/cache.go index cd1f1af07a8e..9afb4548b04f 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -14,7 +14,7 @@ import ( type ResourceCache struct { ctx context.Context - secretCache *timedCache[string, *corev1.Secret] + secretCache *timedCache client kubernetes.Interface v1.ServiceAccountLister } @@ -23,7 +23,7 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) cache := &ResourceCache{ ctx: ctx, - secretCache: NewTimedCache[string, *corev1.Secret](timeout, 2000), + secretCache: NewTimedCache(timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), } @@ -39,7 +39,9 @@ func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespac func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { cacheKey := c.getSecretCacheKey(namespace, secretName) if secret, ok := c.secretCache.Get(cacheKey); ok { - return secret, nil + if secret, ok := secret.(*corev1.Secret); ok { + return secret, nil + } } secret, err := c.getSecretFromServer(namespace, secretName) diff --git a/server/cache/timed_cache.go b/server/cache/timed_cache.go index f3f5dfa233fb..1764ba655ded 100644 --- a/server/cache/timed_cache.go +++ b/server/cache/timed_cache.go @@ -6,44 +6,42 @@ import ( "k8s.io/utils/lru" ) -type timedCache[Key comparable, Value any] struct { +type timedCache struct { timeout time.Duration cache *lru.Cache } type timeValueHolder struct { createTime time.Time - value interface{} + value any } -func NewTimedCache[key comparable, value any](timeout time.Duration, size int) *timedCache[key, value] { - return &timedCache[key, value]{ +func NewTimedCache(timeout time.Duration, size int) *timedCache { + return &timedCache{ timeout: timeout, cache: lru.New(size), } } -func (c *timedCache[Key, Value]) Get(key Key) (Value, bool) { +func (c *timedCache) Get(key string) (any, bool) { if data, ok := c.cache.Get(key); ok { holder := data.(*timeValueHolder) deadline := holder.createTime.Add(c.timeout) - if c.getCurrentTime().Before(deadline) { - if value, ok := holder.value.(Value); ok { - return value, true - } + if getCurrentTime().Before(deadline) { + return holder.value, true } c.cache.Remove(key) } - return *new(Value), false + return nil, false } -func (c *timedCache[Key, Value]) Add(key Key, value Value) { +func (c *timedCache) Add(key string, value any) { c.cache.Add(key, &timeValueHolder{ - createTime: c.getCurrentTime(), + createTime: getCurrentTime(), value: value, }) } -func (c *timedCache[Key, Value]) getCurrentTime() time.Time { +func getCurrentTime() time.Time { return time.Now().UTC() } diff --git a/server/cache/timed_cache_test.go b/server/cache/timed_cache_test.go index 953bdc18cc84..56ed848dc81d 100644 --- a/server/cache/timed_cache_test.go +++ b/server/cache/timed_cache_test.go @@ -10,12 +10,12 @@ import ( func TestNewTimedCache(t *testing.T) { t.Run("NewTimedCache should return a new instance", func(t *testing.T) { - cache := NewTimedCache[string, string](time.Second, 1) + cache := NewTimedCache(time.Second, 1) assert.NotNil(t, cache) }) t.Run("TimedCache should cache based on LRU size", func(t *testing.T) { - cache := NewTimedCache[string, string](time.Second*10, 2) + cache := NewTimedCache(time.Second*10, 2) cache.Add("one", "one") cache.Add("two", "two") @@ -37,7 +37,7 @@ func TestNewTimedCache(t *testing.T) { }) t.Run("TimedCache should cache based on timeout", func(t *testing.T) { - cache := NewTimedCache[string, string](time.Millisecond*5, 2) + cache := NewTimedCache(time.Millisecond*5, 2) cache.Add("one", "one") _, ok := cache.Get("one") From 83ed08b391724bddad7b27e02db0625f755a9a17 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Tue, 3 May 2022 12:49:51 +0530 Subject: [PATCH 08/21] feat: rename variables Signed-off-by: bjenuhb --- server/cache/cache.go | 17 ++++++++++------- server/cache/timed_cache.go | 10 +++++----- server/cache/timed_cache_test.go | 8 ++++---- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/server/cache/cache.go b/server/cache/cache.go index 9afb4548b04f..9eabb1568b5e 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -7,15 +7,16 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + log "github.com/sirupsen/logrus" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" v1 "k8s.io/client-go/listers/core/v1" ) type ResourceCache struct { - ctx context.Context - secretCache *timedCache - client kubernetes.Interface + ctx context.Context + cache *lruTtlCache + client kubernetes.Interface v1.ServiceAccountLister } @@ -23,7 +24,7 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) cache := &ResourceCache{ ctx: ctx, - secretCache: NewTimedCache(timeout, 2000), + cache: NewLruTtlCache(timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), } @@ -33,13 +34,14 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex } func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { - return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*20) + return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*1) } func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { cacheKey := c.getSecretCacheKey(namespace, secretName) - if secret, ok := c.secretCache.Get(cacheKey); ok { + if secret, ok := c.cache.Get(cacheKey); ok { if secret, ok := secret.(*corev1.Secret); ok { + log.Infof("Get secret %s from cache", cacheKey) return secret, nil } } @@ -49,7 +51,8 @@ func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1. return nil, err } - c.secretCache.Add(cacheKey, secret) + log.Infof("Get secret %s from server", cacheKey) + c.cache.Add(cacheKey, secret) return secret, nil } diff --git a/server/cache/timed_cache.go b/server/cache/timed_cache.go index 1764ba655ded..00e4120b6f46 100644 --- a/server/cache/timed_cache.go +++ b/server/cache/timed_cache.go @@ -6,7 +6,7 @@ import ( "k8s.io/utils/lru" ) -type timedCache struct { +type lruTtlCache struct { timeout time.Duration cache *lru.Cache } @@ -16,14 +16,14 @@ type timeValueHolder struct { value any } -func NewTimedCache(timeout time.Duration, size int) *timedCache { - return &timedCache{ +func NewLruTtlCache(timeout time.Duration, size int) *lruTtlCache { + return &lruTtlCache{ timeout: timeout, cache: lru.New(size), } } -func (c *timedCache) Get(key string) (any, bool) { +func (c *lruTtlCache) Get(key string) (any, bool) { if data, ok := c.cache.Get(key); ok { holder := data.(*timeValueHolder) deadline := holder.createTime.Add(c.timeout) @@ -35,7 +35,7 @@ func (c *timedCache) Get(key string) (any, bool) { return nil, false } -func (c *timedCache) Add(key string, value any) { +func (c *lruTtlCache) Add(key string, value any) { c.cache.Add(key, &timeValueHolder{ createTime: getCurrentTime(), value: value, diff --git a/server/cache/timed_cache_test.go b/server/cache/timed_cache_test.go index 56ed848dc81d..6067608ab625 100644 --- a/server/cache/timed_cache_test.go +++ b/server/cache/timed_cache_test.go @@ -9,13 +9,13 @@ import ( func TestNewTimedCache(t *testing.T) { - t.Run("NewTimedCache should return a new instance", func(t *testing.T) { - cache := NewTimedCache(time.Second, 1) + t.Run("NewLruTtlCache should return a new instance", func(t *testing.T) { + cache := NewLruTtlCache(time.Second, 1) assert.NotNil(t, cache) }) t.Run("TimedCache should cache based on LRU size", func(t *testing.T) { - cache := NewTimedCache(time.Second*10, 2) + cache := NewLruTtlCache(time.Second*10, 2) cache.Add("one", "one") cache.Add("two", "two") @@ -37,7 +37,7 @@ func TestNewTimedCache(t *testing.T) { }) t.Run("TimedCache should cache based on timeout", func(t *testing.T) { - cache := NewTimedCache(time.Millisecond*5, 2) + cache := NewLruTtlCache(time.Millisecond*5, 2) cache.Add("one", "one") _, ok := cache.Get("one") From 11551021d69a4ae2855782c69754d24567b65b0a Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Tue, 3 May 2022 12:52:20 +0530 Subject: [PATCH 09/21] feat: rename variables Signed-off-by: bjenuhb --- server/cache/cache.go | 10 ++++++---- server/cache/{timed_cache.go => lru_ttl_cache.go} | 0 .../{timed_cache_test.go => lru_ttl_cache_test.go} | 0 3 files changed, 6 insertions(+), 4 deletions(-) rename server/cache/{timed_cache.go => lru_ttl_cache.go} (100%) rename server/cache/{timed_cache_test.go => lru_ttl_cache_test.go} (100%) diff --git a/server/cache/cache.go b/server/cache/cache.go index 9eabb1568b5e..66f3caa2f63b 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -7,15 +7,19 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - log "github.com/sirupsen/logrus" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" v1 "k8s.io/client-go/listers/core/v1" ) +type Cache interface { + Get(key string) (any, bool) + Add(key string, value any) +} + type ResourceCache struct { ctx context.Context - cache *lruTtlCache + cache Cache client kubernetes.Interface v1.ServiceAccountLister } @@ -41,7 +45,6 @@ func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1. cacheKey := c.getSecretCacheKey(namespace, secretName) if secret, ok := c.cache.Get(cacheKey); ok { if secret, ok := secret.(*corev1.Secret); ok { - log.Infof("Get secret %s from cache", cacheKey) return secret, nil } } @@ -51,7 +54,6 @@ func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1. return nil, err } - log.Infof("Get secret %s from server", cacheKey) c.cache.Add(cacheKey, secret) return secret, nil } diff --git a/server/cache/timed_cache.go b/server/cache/lru_ttl_cache.go similarity index 100% rename from server/cache/timed_cache.go rename to server/cache/lru_ttl_cache.go diff --git a/server/cache/timed_cache_test.go b/server/cache/lru_ttl_cache_test.go similarity index 100% rename from server/cache/timed_cache_test.go rename to server/cache/lru_ttl_cache_test.go From 901ea65f15f051b4e34fd8f73b30a3778b63e90a Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Tue, 3 May 2022 12:55:11 +0530 Subject: [PATCH 10/21] feat: rename variables Signed-off-by: bjenuhb --- server/cache/cache.go | 61 ------------------ server/cache/resource_cache.go | 62 +++++++++++++++++++ .../{cache_test.go => resource_cache_test.go} | 0 3 files changed, 62 insertions(+), 61 deletions(-) create mode 100644 server/cache/resource_cache.go rename server/cache/{cache_test.go => resource_cache_test.go} (100%) diff --git a/server/cache/cache.go b/server/cache/cache.go index 66f3caa2f63b..18eb9752208a 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -1,67 +1,6 @@ package cache -import ( - "context" - "time" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - v1 "k8s.io/client-go/listers/core/v1" -) - type Cache interface { Get(key string) (any, bool) Add(key string, value any) } - -type ResourceCache struct { - ctx context.Context - cache Cache - client kubernetes.Interface - v1.ServiceAccountLister -} - -func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { - informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) - cache := &ResourceCache{ - ctx: ctx, - cache: NewLruTtlCache(timeout, 2000), - client: client, - ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), - } - informerFactory.Start(ctx.Done()) - informerFactory.WaitForCacheSync(ctx.Done()) - return cache -} - -func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { - return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*1) -} - -func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { - cacheKey := c.getSecretCacheKey(namespace, secretName) - if secret, ok := c.cache.Get(cacheKey); ok { - if secret, ok := secret.(*corev1.Secret); ok { - return secret, nil - } - } - - secret, err := c.getSecretFromServer(namespace, secretName) - if err != nil { - return nil, err - } - - c.cache.Add(cacheKey, secret) - return secret, nil -} - -func (c *ResourceCache) getSecretFromServer(namespace string, secretName string) (*corev1.Secret, error) { - return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, metav1.GetOptions{}) -} - -func (c *ResourceCache) getSecretCacheKey(namespace string, secretName string) string { - return namespace + ":secret:" + secretName -} diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go new file mode 100644 index 000000000000..b6e5caad8842 --- /dev/null +++ b/server/cache/resource_cache.go @@ -0,0 +1,62 @@ +package cache + +import ( + "context" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + v1 "k8s.io/client-go/listers/core/v1" +) + +type ResourceCache struct { + ctx context.Context + cache Cache + client kubernetes.Interface + v1.ServiceAccountLister +} + +func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { + informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) + cache := &ResourceCache{ + ctx: ctx, + cache: NewLruTtlCache(timeout, 2000), + client: client, + ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), + } + informerFactory.Start(ctx.Done()) + informerFactory.WaitForCacheSync(ctx.Done()) + return cache +} + +func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { + return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*1) +} + +func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { + cacheKey := c.getSecretCacheKey(namespace, secretName) + if secret, ok := c.cache.Get(cacheKey); ok { + if secret, ok := secret.(*corev1.Secret); ok { + return secret, nil + } + } + + secret, err := c.getSecretFromServer(namespace, secretName) + if err != nil { + return nil, err + } + + c.cache.Add(cacheKey, secret) + return secret, nil +} + +func (c *ResourceCache) getSecretFromServer(namespace string, secretName string) (*corev1.Secret, error) { + return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, metav1.GetOptions{}) +} + +func (c *ResourceCache) getSecretCacheKey(namespace string, secretName string) string { + return namespace + ":secret:" + secretName +} diff --git a/server/cache/cache_test.go b/server/cache/resource_cache_test.go similarity index 100% rename from server/cache/cache_test.go rename to server/cache/resource_cache_test.go From 8b0b3622623917b17995430dbce80823f81f5afe Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Wed, 4 May 2022 09:37:38 +0530 Subject: [PATCH 11/21] fix: remove sso-namespace option Signed-off-by: bjenuhb --- cmd/argo/commands/server.go | 19 ------------------- server/apiserver/argoserver.go | 12 +++++++----- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/cmd/argo/commands/server.go b/cmd/argo/commands/server.go index 5ae79908478b..9724d8cc4ec0 100644 --- a/cmd/argo/commands/server.go +++ b/cmd/argo/commands/server.go @@ -49,7 +49,6 @@ func NewServerCommand() *cobra.Command { htst bool namespaced bool // --namespaced managedNamespace string // --managed-namespace - ssoNamespace string enableOpenBrowser bool eventOperationQueueSize int eventWorkerCount int @@ -143,29 +142,12 @@ See %s`, help.ArgoServer), log.Warn("You are running without client authentication. Learn how to enable client authentication: https://argoproj.github.io/argo-workflows/argo-server-auth-mode/") } - if namespaced { - // Case 1: If ssoNamespace is not specified, default it to installation namespace - if ssoNamespace == "" { - ssoNamespace = namespace - } - // Case 2: If ssoNamespace is not equal to installation or managed namespace, default it to installation namespace - if ssoNamespace != namespace && ssoNamespace != managedNamespace { - log.Warn("--sso-namespace should be equal to --managed-namespace or the installation namespace") - ssoNamespace = namespace - } - } else { - if ssoNamespace != "" { - log.Warn("ignoring --sso-namespace because --namespaced is false") - } - ssoNamespace = namespace - } opts := apiserver.ArgoServerOpts{ BaseHRef: baseHRef, TLSConfig: tlsConfig, HSTS: htst, Namespaced: namespaced, Namespace: namespace, - SSONameSpace: ssoNamespace, Clients: clients, RestConfig: config, AuthModes: modes, @@ -224,7 +206,6 @@ See %s`, help.ArgoServer), command.Flags().StringVar(&configMap, "configmap", common.ConfigMapName, "Name of K8s configmap to retrieve workflow controller configuration") command.Flags().BoolVar(&namespaced, "namespaced", false, "run as namespaced mode") command.Flags().StringVar(&managedNamespace, "managed-namespace", "", "namespace that watches, default to the installation namespace") - command.Flags().StringVar(&ssoNamespace, "sso-namespace", "", "namespace that will be used for SSO RBAC. Defaults to installation namespace. Used only in namespaced mode") command.Flags().BoolVarP(&enableOpenBrowser, "browser", "b", false, "enable automatic launching of the browser [local mode]") command.Flags().IntVar(&eventOperationQueueSize, "event-operation-queue-size", 16, "how many events operations that can be queued at once") command.Flags().IntVar(&eventWorkerCount, "event-worker-count", 4, "how many event workers to run") diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index 322df92e4ab6..87fb7fd706b1 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -99,7 +99,6 @@ type ArgoServerOpts struct { // config map name ConfigName string ManagedNamespace string - SSONameSpace string HSTS bool EventOperationQueueSize int EventWorkerCount int @@ -116,9 +115,12 @@ func init() { } } -func getResourceCacheNamespace(opts ArgoServerOpts) string { +func getSSONamespace(opts ArgoServerOpts) string { if opts.Namespaced { - return opts.SSONameSpace + if opts.ManagedNamespace != "" { + return opts.ManagedNamespace + } + return opts.Namespace } return v1.NamespaceAll } @@ -136,12 +138,12 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error if err != nil { return nil, err } - resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts)) + resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getSSONamespace(opts)) log.Info("SSO enabled") } else { log.Info("SSO disabled") } - gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace, opts.SSONameSpace, opts.Namespaced, resourceCache) + gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace, getSSONamespace(opts), opts.Namespaced, resourceCache) if err != nil { return nil, err } From 56e064c8f3556b4e8eac2d1ccc065b4894aca374 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Wed, 4 May 2022 09:40:27 +0530 Subject: [PATCH 12/21] fix: set cache TTL to 1m Signed-off-by: bjenuhb --- server/cache/resource_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go index b6e5caad8842..d7f26a9993a6 100644 --- a/server/cache/resource_cache.go +++ b/server/cache/resource_cache.go @@ -20,7 +20,7 @@ type ResourceCache struct { } func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { - informerFactory := informers.NewSharedInformerFactoryWithOptions(client, timeout, informers.WithNamespace(namespace)) + informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) cache := &ResourceCache{ ctx: ctx, cache: NewLruTtlCache(timeout, 2000), From 0e57a3229863cc14c7402f77e1b2b3cee4bffda4 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Wed, 4 May 2022 14:25:29 +0530 Subject: [PATCH 13/21] fix: docs Signed-off-by: bjenuhb --- docs/cli/argo_server.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/cli/argo_server.md b/docs/cli/argo_server.md index 7d101d8a11c7..16251f1e2cb0 100644 --- a/docs/cli/argo_server.md +++ b/docs/cli/argo_server.md @@ -30,7 +30,6 @@ See https://argoproj.github.io/argo-workflows/argo-server/ --managed-namespace string namespace that watches, default to the installation namespace --namespaced run as namespaced mode -p, --port int Port to listen on (default 2746) - --sso-namespace string namespace that will be used for SSO RBAC. Defaults to installation namespace. Used only in namespaced mode --x-frame-options string Set X-Frame-Options header in HTTP responses. (default "DENY") ``` From 22e6bdb6a4f0e13287b92383586069be44ce0efd Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Thu, 5 May 2022 00:02:56 +0530 Subject: [PATCH 14/21] feat: fix namespaces Signed-off-by: bjenuhb --- server/apiserver/argoserver.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index 87fb7fd706b1..6d1e397d44aa 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -116,11 +116,15 @@ func init() { } func getSSONamespace(opts ArgoServerOpts) string { - if opts.Namespaced { - if opts.ManagedNamespace != "" { - return opts.ManagedNamespace - } - return opts.Namespace + if opts.ManagedNamespace != "" { + return opts.ManagedNamespace + } + return opts.Namespace +} + +func getResourceCacheNamespace(opts ArgoServerOpts) string { + if opts.ManagedNamespace != "" { + return opts.ManagedNamespace } return v1.NamespaceAll } @@ -138,7 +142,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error if err != nil { return nil, err } - resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getSSONamespace(opts)) + resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts)) log.Info("SSO enabled") } else { log.Info("SSO disabled") From 17a649f781e198f77b25d6c58ebfe1d7339de94d Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Thu, 5 May 2022 00:50:10 +0530 Subject: [PATCH 15/21] fix: rename variable Signed-off-by: bjenuhb --- server/cache/lru_ttl_cache.go | 2 +- server/cache/lru_ttl_cache_test.go | 8 ++++---- server/cache/resource_cache.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/cache/lru_ttl_cache.go b/server/cache/lru_ttl_cache.go index 00e4120b6f46..cf73d54fe729 100644 --- a/server/cache/lru_ttl_cache.go +++ b/server/cache/lru_ttl_cache.go @@ -16,7 +16,7 @@ type timeValueHolder struct { value any } -func NewLruTtlCache(timeout time.Duration, size int) *lruTtlCache { +func NewLRUTtlCache(timeout time.Duration, size int) *lruTtlCache { return &lruTtlCache{ timeout: timeout, cache: lru.New(size), diff --git a/server/cache/lru_ttl_cache_test.go b/server/cache/lru_ttl_cache_test.go index 6067608ab625..d87512b98697 100644 --- a/server/cache/lru_ttl_cache_test.go +++ b/server/cache/lru_ttl_cache_test.go @@ -9,13 +9,13 @@ import ( func TestNewTimedCache(t *testing.T) { - t.Run("NewLruTtlCache should return a new instance", func(t *testing.T) { - cache := NewLruTtlCache(time.Second, 1) + t.Run("NewLRUTtlCache should return a new instance", func(t *testing.T) { + cache := NewLRUTtlCache(time.Second, 1) assert.NotNil(t, cache) }) t.Run("TimedCache should cache based on LRU size", func(t *testing.T) { - cache := NewLruTtlCache(time.Second*10, 2) + cache := NewLRUTtlCache(time.Second*10, 2) cache.Add("one", "one") cache.Add("two", "two") @@ -37,7 +37,7 @@ func TestNewTimedCache(t *testing.T) { }) t.Run("TimedCache should cache based on timeout", func(t *testing.T) { - cache := NewLruTtlCache(time.Millisecond*5, 2) + cache := NewLRUTtlCache(time.Millisecond*5, 2) cache.Add("one", "one") _, ok := cache.Get("one") diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go index d7f26a9993a6..956b6c7feb41 100644 --- a/server/cache/resource_cache.go +++ b/server/cache/resource_cache.go @@ -23,7 +23,7 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) cache := &ResourceCache{ ctx: ctx, - cache: NewLruTtlCache(timeout, 2000), + cache: NewLRUTtlCache(timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), } From 629f3ddcc562e53fd30cdc467265bf5d8598c9e2 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Thu, 5 May 2022 10:39:11 +0530 Subject: [PATCH 16/21] fix: minor Signed-off-by: bjenuhb --- server/cache/cache.go | 2 +- server/cache/lru_ttl_cache.go | 15 +++++++-------- server/cache/resource_cache.go | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/server/cache/cache.go b/server/cache/cache.go index 18eb9752208a..4dcf99335073 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -1,6 +1,6 @@ package cache -type Cache interface { +type Interface interface { Get(key string) (any, bool) Add(key string, value any) } diff --git a/server/cache/lru_ttl_cache.go b/server/cache/lru_ttl_cache.go index cf73d54fe729..8173fefdf1ae 100644 --- a/server/cache/lru_ttl_cache.go +++ b/server/cache/lru_ttl_cache.go @@ -11,8 +11,8 @@ type lruTtlCache struct { cache *lru.Cache } -type timeValueHolder struct { - createTime time.Time +type item struct { + expiryTime time.Time value any } @@ -25,10 +25,9 @@ func NewLRUTtlCache(timeout time.Duration, size int) *lruTtlCache { func (c *lruTtlCache) Get(key string) (any, bool) { if data, ok := c.cache.Get(key); ok { - holder := data.(*timeValueHolder) - deadline := holder.createTime.Add(c.timeout) - if getCurrentTime().Before(deadline) { - return holder.value, true + item := data.(*item) + if getCurrentTime().Before(item.expiryTime) { + return item.value, true } c.cache.Remove(key) } @@ -36,8 +35,8 @@ func (c *lruTtlCache) Get(key string) (any, bool) { } func (c *lruTtlCache) Add(key string, value any) { - c.cache.Add(key, &timeValueHolder{ - createTime: getCurrentTime(), + c.cache.Add(key, &item{ + expiryTime: getCurrentTime().Add(c.timeout), value: value, }) } diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go index 956b6c7feb41..5b240480e441 100644 --- a/server/cache/resource_cache.go +++ b/server/cache/resource_cache.go @@ -14,7 +14,7 @@ import ( type ResourceCache struct { ctx context.Context - cache Cache + cache Interface client kubernetes.Interface v1.ServiceAccountLister } From edc69b3bcb691943ed5052a88e9016cbbb62d5dc Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Thu, 5 May 2022 23:34:05 +0530 Subject: [PATCH 17/21] feat: start informerFactory in a separate method Signed-off-by: bjenuhb --- server/apiserver/argoserver.go | 1 + server/cache/resource_cache.go | 9 +++++++-- server/cache/resource_cache_test.go | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index 6d1e397d44aa..1096a5e8ceb4 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -143,6 +143,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error return nil, err } resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts)) + resourceCache.Run() log.Info("SSO enabled") } else { log.Info("SSO disabled") diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go index 5b240480e441..67186605d2a1 100644 --- a/server/cache/resource_cache.go +++ b/server/cache/resource_cache.go @@ -17,6 +17,7 @@ type ResourceCache struct { cache Interface client kubernetes.Interface v1.ServiceAccountLister + informerFactory informers.SharedInformerFactory } func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { @@ -26,9 +27,8 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex cache: NewLRUTtlCache(timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), + informerFactory: informerFactory, } - informerFactory.Start(ctx.Done()) - informerFactory.WaitForCacheSync(ctx.Done()) return cache } @@ -36,6 +36,11 @@ func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespac return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*1) } +func (c *ResourceCache) Run() { + c.informerFactory.Start(c.ctx.Done()) + c.informerFactory.WaitForCacheSync(c.ctx.Done()) +} + func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { cacheKey := c.getSecretCacheKey(namespace, secretName) if secret, ok := c.cache.Get(cacheKey); ok { diff --git a/server/cache/resource_cache_test.go b/server/cache/resource_cache_test.go index 34320b46d72d..844abdabd831 100644 --- a/server/cache/resource_cache_test.go +++ b/server/cache/resource_cache_test.go @@ -76,6 +76,7 @@ func TestServer_K8sUtilsCache(t *testing.T) { }, }) cache := NewResourceCache(kubeClient, context.TODO(), v1.NamespaceAll) + cache.Run() t.Run("List Service Accounts in different namespaces", func(t *testing.T) { sa, _ := cache.ServiceAccountLister.ServiceAccounts("ns1").List(labels.Everything()) From 56fbaaf20fa3e0b7729e226333ebbee9515d4f65 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Fri, 6 May 2022 00:09:46 +0530 Subject: [PATCH 18/21] feat: fix time tests Signed-off-by: bjenuhb --- server/cache/lru_ttl_cache.go | 10 ++++------ server/cache/lru_ttl_cache_test.go | 20 +++++++++++++++----- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/server/cache/lru_ttl_cache.go b/server/cache/lru_ttl_cache.go index 8173fefdf1ae..381f75ff9bdd 100644 --- a/server/cache/lru_ttl_cache.go +++ b/server/cache/lru_ttl_cache.go @@ -6,6 +6,8 @@ import ( "k8s.io/utils/lru" ) +var currentTime = time.Now + type lruTtlCache struct { timeout time.Duration cache *lru.Cache @@ -26,7 +28,7 @@ func NewLRUTtlCache(timeout time.Duration, size int) *lruTtlCache { func (c *lruTtlCache) Get(key string) (any, bool) { if data, ok := c.cache.Get(key); ok { item := data.(*item) - if getCurrentTime().Before(item.expiryTime) { + if currentTime().Before(item.expiryTime) { return item.value, true } c.cache.Remove(key) @@ -36,11 +38,7 @@ func (c *lruTtlCache) Get(key string) (any, bool) { func (c *lruTtlCache) Add(key string, value any) { c.cache.Add(key, &item{ - expiryTime: getCurrentTime().Add(c.timeout), + expiryTime: currentTime().Add(c.timeout), value: value, }) } - -func getCurrentTime() time.Time { - return time.Now().UTC() -} diff --git a/server/cache/lru_ttl_cache_test.go b/server/cache/lru_ttl_cache_test.go index d87512b98697..1600645589e3 100644 --- a/server/cache/lru_ttl_cache_test.go +++ b/server/cache/lru_ttl_cache_test.go @@ -37,18 +37,28 @@ func TestNewTimedCache(t *testing.T) { }) t.Run("TimedCache should cache based on timeout", func(t *testing.T) { - cache := NewLRUTtlCache(time.Millisecond*5, 2) + tempCurrentTime := currentTime + + cache := NewLRUTtlCache(time.Minute*1, 2) + + currentTime = getTimeFunc(0, 0) cache.Add("one", "one") + currentTime = getTimeFunc(0, 30) _, ok := cache.Get("one") assert.True(t, ok) - time.Sleep(time.Millisecond * 10) - - // "one" should not be available since timeout is 5 ms + currentTime = getTimeFunc(1, 30) + // "one" should not be available since timeout is 1 min _, ok = cache.Get("one") assert.False(t, ok) - + currentTime = tempCurrentTime }) } + +func getTimeFunc(min int, sec int) func() time.Time { + return func() time.Time { + return time.Date(0, 0, 0, 0, min, sec, 0, time.UTC) + } +} From e232df44051247bf41a6906308ef931afcc34a35 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Fri, 6 May 2022 00:18:18 +0530 Subject: [PATCH 19/21] fix: resolve ssonamespace in Signed-off-by: bjenuhb --- cmd/argo/commands/server.go | 7 +++++++ server/apiserver/argoserver.go | 10 ++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/cmd/argo/commands/server.go b/cmd/argo/commands/server.go index 9724d8cc4ec0..5c5466a09c2a 100644 --- a/cmd/argo/commands/server.go +++ b/cmd/argo/commands/server.go @@ -97,10 +97,16 @@ See %s`, help.ArgoServer), managedNamespace = namespace } + ssoNamespace := namespace + if managedNamespace != "" { + ssoNamespace = managedNamespace + } + log.WithFields(log.Fields{ "authModes": authModes, "namespace": namespace, "managedNamespace": managedNamespace, + "ssoNamespace": ssoNamespace, "baseHRef": baseHRef, "secure": secure, }).Info() @@ -152,6 +158,7 @@ See %s`, help.ArgoServer), RestConfig: config, AuthModes: modes, ManagedNamespace: managedNamespace, + SSONamespace: ssoNamespace, ConfigName: configMap, EventOperationQueueSize: eventOperationQueueSize, EventWorkerCount: eventWorkerCount, diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index 1096a5e8ceb4..b5b0a185beb0 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -99,6 +99,7 @@ type ArgoServerOpts struct { // config map name ConfigName string ManagedNamespace string + SSONamespace string HSTS bool EventOperationQueueSize int EventWorkerCount int @@ -115,13 +116,6 @@ func init() { } } -func getSSONamespace(opts ArgoServerOpts) string { - if opts.ManagedNamespace != "" { - return opts.ManagedNamespace - } - return opts.Namespace -} - func getResourceCacheNamespace(opts ArgoServerOpts) string { if opts.ManagedNamespace != "" { return opts.ManagedNamespace @@ -148,7 +142,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error } else { log.Info("SSO disabled") } - gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace, getSSONamespace(opts), opts.Namespaced, resourceCache) + gatekeeper, err := auth.NewGatekeeper(opts.AuthModes, opts.Clients, opts.RestConfig, ssoIf, auth.DefaultClientForAuthorization, opts.Namespace, opts.SSONamespace, opts.Namespaced, resourceCache) if err != nil { return nil, err } From 91271ebefec0741906cdbec2873d0a0c04bb0abe Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Fri, 6 May 2022 08:42:17 +0530 Subject: [PATCH 20/21] fix: pass ctx as function argument Signed-off-by: bjenuhb --- server/apiserver/argoserver.go | 4 ++-- server/auth/gatekeeper.go | 14 +++++++------- server/auth/gatekeeper_test.go | 2 +- server/cache/resource_cache.go | 22 ++++++++++------------ server/cache/resource_cache_test.go | 7 ++++--- 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index b5b0a185beb0..7008f45b7652 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -136,8 +136,8 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error if err != nil { return nil, err } - resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts)) - resourceCache.Run() + resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, getResourceCacheNamespace(opts)) + resourceCache.Run(ctx.Done()) log.Info("SSO enabled") } else { log.Info("SSO disabled") diff --git a/server/auth/gatekeeper.go b/server/auth/gatekeeper.go index d8e6257030e9..5f684c024578 100644 --- a/server/auth/gatekeeper.go +++ b/server/auth/gatekeeper.go @@ -207,7 +207,7 @@ func (s gatekeeper) getClients(ctx context.Context, req interface{}) (*servertyp return nil, nil, status.Error(codes.Unauthenticated, err.Error()) } if s.ssoIf.IsRBACEnabled() { - clients, err := s.rbacAuthorization(claims, req) + clients, err := s.rbacAuthorization(ctx, claims, req) if err != nil { log.WithError(err).Error("failed to perform RBAC authorization") return nil, nil, status.Error(codes.PermissionDenied, "not allowed") @@ -279,8 +279,8 @@ func (s *gatekeeper) canDelegateRBACToRequestNamespace(req interface{}) bool { return len(namespace) != 0 && s.ssoNamespace != namespace } -func (s *gatekeeper) getClientsForServiceAccount(claims *types.Claims, serviceAccount *corev1.ServiceAccount) (*servertypes.Clients, error) { - authorization, err := s.authorizationForServiceAccount(serviceAccount) +func (s *gatekeeper) getClientsForServiceAccount(ctx context.Context, claims *types.Claims, serviceAccount *corev1.ServiceAccount) (*servertypes.Clients, error) { + authorization, err := s.authorizationForServiceAccount(ctx, serviceAccount) if err != nil { return nil, err } @@ -292,7 +292,7 @@ func (s *gatekeeper) getClientsForServiceAccount(claims *types.Claims, serviceAc return clients, nil } -func (s *gatekeeper) rbacAuthorization(claims *types.Claims, req interface{}) (*servertypes.Clients, error) { +func (s *gatekeeper) rbacAuthorization(ctx context.Context, claims *types.Claims, req interface{}) (*servertypes.Clients, error) { ssoDelegationAllowed, ssoDelegated := false, false loginAccount, err := s.getServiceAccount(claims, s.ssoNamespace) if err != nil { @@ -311,14 +311,14 @@ func (s *gatekeeper) rbacAuthorization(claims *types.Claims, req interface{}) (* } // important! write an audit entry (i.e. log entry) so we know which user performed an operation log.WithFields(log.Fields{"serviceAccount": delegatedAccount.Name, "loginServiceAccount": loginAccount.Name, "subject": claims.Subject, "email": claims.Email, "ssoDelegationAllowed": ssoDelegationAllowed, "ssoDelegated": ssoDelegated}).Info("selected SSO RBAC service account for user") - return s.getClientsForServiceAccount(claims, delegatedAccount) + return s.getClientsForServiceAccount(ctx, claims, delegatedAccount) } -func (s *gatekeeper) authorizationForServiceAccount(serviceAccount *corev1.ServiceAccount) (string, error) { +func (s *gatekeeper) authorizationForServiceAccount(ctx context.Context, serviceAccount *corev1.ServiceAccount) (string, error) { if len(serviceAccount.Secrets) == 0 { return "", fmt.Errorf("expected at least one secret for SSO RBAC service account: %s", serviceAccount.GetName()) } - secret, err := s.cache.GetSecret(serviceAccount.GetNamespace(), serviceAccount.Secrets[0].Name) + secret, err := s.cache.GetSecret(ctx, serviceAccount.GetNamespace(), serviceAccount.Secrets[0].Name) if err != nil { return "", fmt.Errorf("failed to get service account secret: %w", err) } diff --git a/server/auth/gatekeeper_test.go b/server/auth/gatekeeper_test.go index 2dce0e36d9b1..895e5544789c 100644 --- a/server/auth/gatekeeper_test.go +++ b/server/auth/gatekeeper_test.go @@ -105,7 +105,7 @@ func TestServer_GetWFClient(t *testing.T) { }, }, ) - resourceCache := cache.NewResourceCache(kubeClient, context.TODO(), corev1.NamespaceAll) + resourceCache := cache.NewResourceCache(kubeClient, corev1.NamespaceAll) var clientForAuthorization ClientForAuthorization = func(authorization string) (*rest.Config, *servertypes.Clients, error) { return &rest.Config{}, &servertypes.Clients{Workflow: &fakewfclientset.Clientset{}, Kubernetes: &kubefake.Clientset{}}, nil } diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go index 67186605d2a1..c9b4e7126f35 100644 --- a/server/cache/resource_cache.go +++ b/server/cache/resource_cache.go @@ -13,17 +13,15 @@ import ( ) type ResourceCache struct { - ctx context.Context cache Interface client kubernetes.Interface v1.ServiceAccountLister informerFactory informers.SharedInformerFactory } -func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Context, namespace string, timeout time.Duration) *ResourceCache { +func NewResourceCacheWithTimeout(client kubernetes.Interface, namespace string, timeout time.Duration) *ResourceCache { informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) cache := &ResourceCache{ - ctx: ctx, cache: NewLRUTtlCache(timeout, 2000), client: client, ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), @@ -32,16 +30,16 @@ func NewResourceCacheWithTimeout(client kubernetes.Interface, ctx context.Contex return cache } -func NewResourceCache(client kubernetes.Interface, ctx context.Context, namespace string) *ResourceCache { - return NewResourceCacheWithTimeout(client, ctx, namespace, time.Minute*1) +func NewResourceCache(client kubernetes.Interface, namespace string) *ResourceCache { + return NewResourceCacheWithTimeout(client, namespace, time.Minute*1) } -func (c *ResourceCache) Run() { - c.informerFactory.Start(c.ctx.Done()) - c.informerFactory.WaitForCacheSync(c.ctx.Done()) +func (c *ResourceCache) Run(stopCh <-chan struct{}) { + c.informerFactory.Start(stopCh) + c.informerFactory.WaitForCacheSync(stopCh) } -func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1.Secret, error) { +func (c *ResourceCache) GetSecret(ctx context.Context, namespace string, secretName string) (*corev1.Secret, error) { cacheKey := c.getSecretCacheKey(namespace, secretName) if secret, ok := c.cache.Get(cacheKey); ok { if secret, ok := secret.(*corev1.Secret); ok { @@ -49,7 +47,7 @@ func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1. } } - secret, err := c.getSecretFromServer(namespace, secretName) + secret, err := c.getSecretFromServer(ctx, namespace, secretName) if err != nil { return nil, err } @@ -58,8 +56,8 @@ func (c *ResourceCache) GetSecret(namespace string, secretName string) (*corev1. return secret, nil } -func (c *ResourceCache) getSecretFromServer(namespace string, secretName string) (*corev1.Secret, error) { - return c.client.CoreV1().Secrets(namespace).Get(c.ctx, secretName, metav1.GetOptions{}) +func (c *ResourceCache) getSecretFromServer(ctx context.Context, namespace string, secretName string) (*corev1.Secret, error) { + return c.client.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{}) } func (c *ResourceCache) getSecretCacheKey(namespace string, secretName string) string { diff --git a/server/cache/resource_cache_test.go b/server/cache/resource_cache_test.go index 844abdabd831..ae93ce51c9a4 100644 --- a/server/cache/resource_cache_test.go +++ b/server/cache/resource_cache_test.go @@ -75,8 +75,9 @@ func TestServer_K8sUtilsCache(t *testing.T) { "token": {}, }, }) - cache := NewResourceCache(kubeClient, context.TODO(), v1.NamespaceAll) - cache.Run() + cache := NewResourceCache(kubeClient, v1.NamespaceAll) + ctx := context.TODO() + cache.Run(ctx.Done()) t.Run("List Service Accounts in different namespaces", func(t *testing.T) { sa, _ := cache.ServiceAccountLister.ServiceAccounts("ns1").List(labels.Everything()) @@ -88,7 +89,7 @@ func TestServer_K8sUtilsCache(t *testing.T) { assert.Equal(t, 1, len(sa)) assert.True(t, checkServiceAccountExists(sa, "sa3")) - secret, _ := cache.GetSecret("ns1", "s1") + secret, _ := cache.GetSecret(ctx, "ns1", "s1") assert.NotNil(t, secret) }) } From d0541c1021b19f264f810a6cbb14611e00c84a69 Mon Sep 17 00:00:00 2001 From: bjenuhb Date: Fri, 6 May 2022 08:49:55 +0530 Subject: [PATCH 21/21] fix: pass ctx as function argument Signed-off-by: bjenuhb --- server/auth/gatekeeper_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/auth/gatekeeper_test.go b/server/auth/gatekeeper_test.go index 895e5544789c..b13bcfba5f8f 100644 --- a/server/auth/gatekeeper_test.go +++ b/server/auth/gatekeeper_test.go @@ -106,6 +106,7 @@ func TestServer_GetWFClient(t *testing.T) { }, ) resourceCache := cache.NewResourceCache(kubeClient, corev1.NamespaceAll) + resourceCache.Run(context.TODO().Done()) var clientForAuthorization ClientForAuthorization = func(authorization string) (*rest.Config, *servertypes.Clients, error) { return &rest.Config{}, &servertypes.Clients{Workflow: &fakewfclientset.Clientset{}, Kubernetes: &kubefake.Clientset{}}, nil }