fix!: remove list and watch on secrets. Fixes #8534

[basanthjenuhb]

Signed-off-by: bjenuhb Basanth_JenuHB@intuit.com

Fixes #8534

[basanthjenuhb]

Reason to introduce this method is if we want to introduce caching in future, we can easily change its implementation and keep external contract same

[alexec]

We added to avoid repeated API calls. Prior to this there was a rought 1-to-1 mapping between Argo Server API calls and Kubernetes API calls O(1). This is now O(2).

Please use a cache (e.g lru) so that this is mitigated.

[basanthjenuhb]

The problem with a cache is cache invalidation,
But got to know one info,
If the secret for that service account is deleted,
a new secret with a different name is recreated
so, since the name would be different, cache would be automatically invalidated.
Will add an LRU cache with 20 mins of refresh time

[alexec]

It can be less than 20m. We only want to avoid doing 1 per call. A cache of 1m would work.

[basanthjenuhb]

Just wanted to keep the timeout same as the informer
Since now cache invalidation is not an issue. 20m seems fine

[jessesuen]

There was one bug that is not addressed in this PR: getResourceCacheNamespace() needs to consider --namespaced as well as --managed-namespace.

As you can see, the code only considers --namespaced option:

func getResourceCacheNamespace(opts ArgoServerOpts) string {
	if opts.Namespaced {
		return opts.SSONameSpace
	}
	return v1.NamespaceAll
}

[basanthjenuhb]

There was one bug that is not addressed in this PR: getResourceCacheNamespace() needs to consider --namespaced as well as --managed-namespace.

As you can see, the code only considers --namespaced option:

func getResourceCacheNamespace(opts ArgoServerOpts) string {
	if opts.Namespaced {
		return opts.SSONameSpace
	}
	return v1.NamespaceAll
}

Actually that piece of code is handled here
You can specify --sso-namespace. To determine which namespace to lookup service accounts from
https://github.com/argoproj/argo-workflows/blob/master/cmd/argo/commands/server.go#L146-L155

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
    }
}

It can either be instalation namespace or managed namespace

[basanthjenuhb]

@alexec I feel 20 min should be fine since we don't have cache invalidation issue.
Let me know if you want it to be 1 min.

[jessesuen]

so if Namespaced is set to false, there won't be a namespaced installation anyway
Basically, I just wanted to honour ManagedNamespace only if Namespaced is true

I see. I think something should be done to improve the UX since this was something the user we worked with overlooked (and even myself). I understand now that currently --managed-namespace is ignored if --namespaced is not set.

I would suggest one of two things to improve:

1. Presence of --managed-namespace=foo automatically implying that server is running in a namespace mode, going so far as setting o.Namspaced=true in the code.
2. Adding validation/usage error that --managed-namespace=foo and --namespaced=false is not a valid combination of flags and quitting with a usage error (instead of a log message).

@alexec WDYT?

[basanthjenuhb]

@jessesuen
Can you have another look at the PR
I think the change regarding --managed-namespac and --namespaced could be done in a separate PR and needs discussion.

[basanthjenuhb]

@jessesuen Anything else needed in this PR. want to take the PR to closure

[agilgur5]

There appear to have been some undocumented breaking changes in this PR 😕

Those changes were unrelated to the goals and title of this PR too, which themselves were fully backward-compatible (as they only reduced permissions) 😕

[agilgur5]

This is a breaking change. I see it was discussed above, but it wasn't mentioned in the changelog or release notes (nor even the PR description). The PR title is also missing an exclamation point indicating breakage 😕 (I added the exclamation point myself post-hoc just now)

[agilgur5]

this actually is different from the logic on line 151 and line 160 below. Those set the SSO namespace to the installation namespace when using a managed namespace. That makes sense, since your Argo configuration and Argo Server are in the installation namespace. Only your Workflows are in the managed namespace.

This change is actually quite confusing as a result, as due to this, with a managed namespace you now configure SSO RBAC there and not in the installation namespace. Normally that only happens when you enable SSO namespace delegation, which is named fairly explicitly.

This resulted in an undocumented breaking change that caused a regression too: #9989 (comment) 😕