diff --git a/cmd/argo/commands/server.go b/cmd/argo/commands/server.go index 5ae79908478b..5c5466a09c2a 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 @@ -98,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() @@ -143,33 +148,17 @@ 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, ManagedNamespace: managedNamespace, + SSONamespace: ssoNamespace, ConfigName: configMap, EventOperationQueueSize: eventOperationQueueSize, EventWorkerCount: eventWorkerCount, @@ -224,7 +213,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/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") ``` 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 01e18f33d85d..d9e4d49090c1 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -953,8 +953,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 6bb9370a094a..203b4e01c9b1 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -862,8 +862,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 7f490997471b..102a233261f0 100644 --- a/manifests/quick-start-minimal.yaml +++ b/manifests/quick-start-minimal.yaml @@ -891,8 +891,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/quick-start-mysql.yaml b/manifests/quick-start-mysql.yaml index 72b6e9c8b4f0..51b6959f224b 100644 --- a/manifests/quick-start-mysql.yaml +++ b/manifests/quick-start-mysql.yaml @@ -891,8 +891,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/manifests/quick-start-postgres.yaml b/manifests/quick-start-postgres.yaml index 45469c14cc08..c7a5cf472e4a 100644 --- a/manifests/quick-start-postgres.yaml +++ b/manifests/quick-start-postgres.yaml @@ -891,8 +891,6 @@ rules: verbs: - get - create - - list - - watch - apiGroups: - "" resources: diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index 49e4d7cca901..c506461aa727 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -99,7 +99,7 @@ type ArgoServerOpts struct { // config map name ConfigName string ManagedNamespace string - SSONameSpace string + SSONamespace string HSTS bool EventOperationQueueSize int EventWorkerCount int @@ -117,8 +117,8 @@ func init() { } func getResourceCacheNamespace(opts ArgoServerOpts) string { - if opts.Namespaced { - return opts.SSONameSpace + if opts.ManagedNamespace != "" { + return opts.ManagedNamespace } return v1.NamespaceAll } @@ -136,12 +136,13 @@ 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, getResourceCacheNamespace(opts)) + resourceCache.Run(ctx.Done()) 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, opts.SSONamespace, opts.Namespaced, resourceCache) if err != nil { return nil, err } diff --git a/server/auth/gatekeeper.go b/server/auth/gatekeeper.go index ce936c0922b2..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.SecretLister.Secrets(serviceAccount.GetNamespace()).Get(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..b13bcfba5f8f 100644 --- a/server/auth/gatekeeper_test.go +++ b/server/auth/gatekeeper_test.go @@ -105,7 +105,8 @@ func TestServer_GetWFClient(t *testing.T) { }, }, ) - resourceCache := cache.NewResourceCache(kubeClient, context.TODO(), corev1.NamespaceAll) + 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 } diff --git a/server/cache/cache.go b/server/cache/cache.go index cd5540efb03f..4dcf99335073 100644 --- a/server/cache/cache.go +++ b/server/cache/cache.go @@ -1,26 +1,6 @@ package cache -import ( - "context" - "time" - - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - v1 "k8s.io/client-go/listers/core/v1" -) - -type ResourceCache struct { - 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{ - ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), - SecretLister: informerFactory.Core().V1().Secrets().Lister(), - } - informerFactory.Start(ctx.Done()) - informerFactory.WaitForCacheSync(ctx.Done()) - return cache +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 new file mode 100644 index 000000000000..381f75ff9bdd --- /dev/null +++ b/server/cache/lru_ttl_cache.go @@ -0,0 +1,44 @@ +package cache + +import ( + "time" + + "k8s.io/utils/lru" +) + +var currentTime = time.Now + +type lruTtlCache struct { + timeout time.Duration + cache *lru.Cache +} + +type item struct { + expiryTime time.Time + value any +} + +func NewLRUTtlCache(timeout time.Duration, size int) *lruTtlCache { + return &lruTtlCache{ + timeout: timeout, + cache: lru.New(size), + } +} + +func (c *lruTtlCache) Get(key string) (any, bool) { + if data, ok := c.cache.Get(key); ok { + item := data.(*item) + if currentTime().Before(item.expiryTime) { + return item.value, true + } + c.cache.Remove(key) + } + return nil, false +} + +func (c *lruTtlCache) Add(key string, value any) { + c.cache.Add(key, &item{ + expiryTime: currentTime().Add(c.timeout), + value: value, + }) +} diff --git a/server/cache/lru_ttl_cache_test.go b/server/cache/lru_ttl_cache_test.go new file mode 100644 index 000000000000..1600645589e3 --- /dev/null +++ b/server/cache/lru_ttl_cache_test.go @@ -0,0 +1,64 @@ +package cache + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestNewTimedCache(t *testing.T) { + + 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.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) { + 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) + + 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) + } +} diff --git a/server/cache/resource_cache.go b/server/cache/resource_cache.go new file mode 100644 index 000000000000..c9b4e7126f35 --- /dev/null +++ b/server/cache/resource_cache.go @@ -0,0 +1,65 @@ +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 { + cache Interface + client kubernetes.Interface + v1.ServiceAccountLister + informerFactory informers.SharedInformerFactory +} + +func NewResourceCacheWithTimeout(client kubernetes.Interface, namespace string, timeout time.Duration) *ResourceCache { + informerFactory := informers.NewSharedInformerFactoryWithOptions(client, time.Minute*20, informers.WithNamespace(namespace)) + cache := &ResourceCache{ + cache: NewLRUTtlCache(timeout, 2000), + client: client, + ServiceAccountLister: informerFactory.Core().V1().ServiceAccounts().Lister(), + informerFactory: informerFactory, + } + return cache +} + +func NewResourceCache(client kubernetes.Interface, namespace string) *ResourceCache { + return NewResourceCacheWithTimeout(client, namespace, time.Minute*1) +} + +func (c *ResourceCache) Run(stopCh <-chan struct{}) { + c.informerFactory.Start(stopCh) + c.informerFactory.WaitForCacheSync(stopCh) +} + +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 { + return secret, nil + } + } + + secret, err := c.getSecretFromServer(ctx, namespace, secretName) + if err != nil { + return nil, err + } + + c.cache.Add(cacheKey, secret) + return secret, nil +} + +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 { + return namespace + ":secret:" + secretName +} diff --git a/server/cache/cache_test.go b/server/cache/resource_cache_test.go similarity index 93% rename from server/cache/cache_test.go rename to server/cache/resource_cache_test.go index 42407d5391f5..ae93ce51c9a4 100644 --- a/server/cache/cache_test.go +++ b/server/cache/resource_cache_test.go @@ -75,7 +75,9 @@ func TestServer_K8sUtilsCache(t *testing.T) { "token": {}, }, }) - cache := NewResourceCache(kubeClient, context.TODO(), v1.NamespaceAll) + 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()) @@ -87,7 +89,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(ctx, "ns1", "s1") + assert.NotNil(t, secret) }) }