Argo Server with SSO enabled should not require List on all Secrets

[jessesuen]

Summary

When Argo Server starts up with SSO, it will call cache.NewResourceCache():

func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error) {
	configController := config.NewController(opts.Namespace, opts.ConfigName, opts.Clients.Kubernetes)
	var resourceCache *cache.ResourceCache = nil
	ssoIf := sso.NullSSO
	if opts.AuthModes[auth.SSO] {
		c, err := configController.Get(ctx)
		if err != nil {
			return nil, err
		}
		ssoIf, err = sso.New(c.SSO, opts.Clients.Kubernetes.CoreV1().Secrets(opts.Namespace), opts.BaseHRef, opts.TLSConfig != nil)
		if err != nil {
			return nil, err
		}
		resourceCache = cache.NewResourceCache(opts.Clients.Kubernetes, ctx, getResourceCacheNamespace(opts))
		log.Info("SSO enabled")
	} else {
		log.Info("SSO disabled")
	}

But NewResourceCache wants the ability to list all service accounts and all secrets of the entire cluster.

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
}

There are two problems with this:

1. While list ServiceAccounts might be acceptable, listing all secrets of the cluster is generally unacceptable.
2. This does not work with an Argo Server running with --namespaced mode

Instead of listing all secrets, I think we could look up the secret of the ServiceAccount on-demand, to find the bearer token. This will allow administrators to only give the controller limited permissions to Secrets. If the use of a Lister was because of performance, we could cache this in-memory rather than rely on a lister.

Use Cases

When would you use this?

In a secure, multi-tenant environment.

---

Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

[jessesuen]

This does not work with an Argo Server running with --namespaced mode

ResourceCache was introduced in #6990

@basanthjenuhb -- I think this could be considered a regression because --namespaced flag is broken when SSO is enabled.

[jessesuen]

Also, having a cluster-wide informer on Secrets (and/or ConfigMaps) is never really a good idea. It will balloon the memory requirements of the server unnecessarily because these two objects tend to contain a lot of data. We learned this the hard way with Argo Rollout controller.

At the very least, this can be restricted to label filters. But as I mentioned in the description, I think list/watch on Secrets is an unnecessary requirement that should be removed.

[basanthjenuhb]

we had discusses label filters.
We cannot do label filters because the secrets we are looking for are created automatically by serviceaccounts, and they don't have any label for them.

I am open to removing the list and just do get for every call
My initial thought process was the secrets are generally very small in size, and won't have much impact on memory
@alexec should I proceed with this change

[alexec]

Yes please. If we can remove the list secrets, that’d be great. You could use an lru cache to hold them.

[basanthjenuhb]

Using a cache would again lead us to unnecessary cache invalidation issues .
Will just do get everytime

[basanthjenuhb]

@alexec @jessesuen Please review. #8555