Introduce SA check before configuring impersonation

[maxgio92]

This PR adds a preliminary check on existence of the ServiceAccount to be impersonated on reconciliation, during the RESTClient Config setup. The ImpersonationConfig with SA User was previously set on RESTclient Config no matter if the ServiceAccount exists.

This check avoids consequent otherwise failure for missing privileges on the missing SA's User, and improves error explanation.

Moreover, tests are updated accordingly.

Fixes #682.

[maxgio92]

Hold on, impersonation "fails to reconcile impersonating the default service account" test fails.

[maxgio92]

Fixed it and added a test for this specific use case. PR I think is ready.

[maxgio92]

Hi @stefanprodan, WDYT about this? Thanks

[stefanprodan]

Hey @maxgio92 this changes the RBAC needed on remote clusters, instead of just access to the impersonation API, with this, users have to add serviceaccounts get, list, watch at cluster level, but since we do this for local clusters it may be ok for remote clusters too.

PS. Testes are failing:

=== CONT  TestKustomizationReconciler_Impersonation
    kustomization_impersonation_test.go:159: 
        Timed out after 30.001s.
        Expected
            <bool>: false
        to be true

[maxgio92]

Thank you @stefanprodan. Going to check them

[maxgio92]

Hi @stefanprodan I re-triggered CI as timeout on the new e2e test case seems curious, because locally the test is passing 100% of the time. Could you appove GH workflow to tun?

[maxgio92]

@stefanprodan as expected now e2e tests pass :-) Thank you. This client config setup stage (i.e. impersonation) is executed before the Kustomizations reconciliation, and by default with cluster-admin ClusterRole, isn't it? If so, practically speaking these privileges are already in place with standard GOTK installation.

[stefanprodan]

This client config setup stage (i.e. impersonation) is executed before the Kustomizations reconciliation, and by default with cluster-admin ClusterRole, isn't it?

We impersonate the SA on the remote cluster, where there is no Flux. Isn't what this PR does? if it checks the SA on the local cluster then it will make Flux fail for everyone using kubeconfig.

[maxgio92]

Yes @stefanprodan, to be honest I didn't consider the kubeconfig in this scenario. I think a dedicated e2e test case is needed here, besides the SA impersonation-only case. TL;DR:

• SA impersonation
• kubeconfig + SA impersonation

[maxgio92]

Or maybe this can be considered already covered from tests. Simply the otherwise identity specified in kubeconfig, needs:

• list
• get
• watch

over ServiceAccount resources, when specified a ServiceAccount to be impersonated.

[maxgio92]

Hey @stefanprodan I think now is correctly managed. It costs a new client per impersonation configuration call, but I think is the right way. That's because the client requested by the reconciler's reconcile() with impersonation.GetClient() is in the process to be created - at the moment of creating the rest.Config from the optional kubeconfig specified in the Kustomization - and there is no client still built from that client config.

TL;DR when specifying a kubeconfig and an impersonation ServiceAccount, the check on the existence of that SA is done with a client built from the specified kubeconfig.

WDYT?

[maxgio92]

Besides this, as a general idea it could be worthy to setup e2e tests for cases where kubeconfig refer to remote clusters, plus impersonation.

[maxgio92]

Hi @stefanprodan, can this be still applicable? WDYT? Thank you

[stefanprodan]

I think we need an RFC to motivate this permissions changes for remote clusters and ask the community for feedback. This has the potential of breaking existing Flux setups where only impersonation is granted to kustomize-controller SA on remote clusters.

[maxgio92]

Thanks @stefanprodan, I'll work to provide an RFC and share it