Apply review comments of #1507

[andresmgot]

More info at #1507

Note that the default namespace has been changed from default to All Namespaces (until we have a better solution).

[andresmgot]

@absoludity I've found a interesting issue. Since the backend was returning a 403 if the user doesn't have permissions to list namespaces (and neither the backend serviceaccount) it was considered an oauth-proxy error. I have changed the check but can you verify this works as expected? (I don't have an OIDC setup right now).

[absoludity]

It's not clear to me why you've gone and used a string instead of all the occurrences of definedNamespaces.default? If we want the UI to use _all as the selected namespace in the UI, then why not explicitly use definedNamespaces.all at that one point in the code where we set that. Ah, maybe you were going to delete definedNamespaces.default in that case, since it would no longer be useful (if it was only a default for the UI).

It's also unclear why we're doing the change of the default, rather than updating to select the first namespace to which the user has access as soon as we have that data? Perhaps the error would flash on the screen? If so, I'm OK with _all being used, but keen to see us switch to the first namespace to which the user has access once the response comes back?

[absoludity]

What's the expectation here? There's no matcher following the expect(...). Did you mean to use .toBe(true)?

[andresmgot]

oops, thanks for catching, it should be false

[absoludity]

Woah - these defaults were dangerous!? Great to remove them.

[absoludity]

@absoludity I've found a interesting issue. Since the backend was returning a 403 if the user doesn't have permissions to list namespaces (and neither the backend serviceaccount) it was considered an oauth-proxy error. I have changed the check but can you verify this works as expected? (I don't have an OIDC setup right now).

It appears to work as expected. I removed the ClusterRoleBinding so that the backend does cannot list namespaces and then logged in with my kubeapps-user account. I see the 403 for the request to namespaces, but the user just defaults to All Namespaces without error.

There does seem to be one issue though, in that _all is also appearing in the dropdown, in addition to All Namespaces:

[absoludity]

Actually, another issue: when I later re-logged in with my operator user, I still only see "All Namespaces" and "_all" in the select options, even though I can see the request to the namespaces backend returned with all namespaces:

[andresmgot]

It's not clear to me why you've gone and used a string instead of all the occurrences of definedNamespaces.default?

Well, I have just changed that in the tests for not changing its data. In the functional code I have tried to use definedNamespaces.all when possible.

If we want the UI to use _all as the selected namespace in the UI, then why not explicitly use definedNamespaces.all at that one point in the code where we set that. Ah, maybe you were going to delete definedNamespaces.default in that case, since it would no longer be useful (if it was only a default for the UI).

Yes, that's my goal, the problem is that I wan unable to use it here: https://github.com/kubeapps/kubeapps/pull/1510/files#diff-05b790603bf81c810b2fc07b7bda81feR8 so I had to hardcode it. I was unable to use the value there but I am not sure why, I can investigate more but I assume there is a reason for that.

[EDIT]

I don't know what issue I found but I was able to just use definedNamespaces.all in the code I pointed 🤔

[EDIT2]

Finally found the issue (seems a typescript issue). Importing definedNamespaces in Auth caused the static property from the class to be undefined (I am not sure why TBH). Fixed in 962d223

It's also unclear why we're doing the change of the default, rather than updating to select the first namespace to which the user has access as soon as we have that data? Perhaps the error would flash on the screen? If so, I'm OK with _all being used, but keen to see us switch to the first namespace to which the user has access once the response comes back?

The problem with that is that it's not trivial to differentiate between the "loading" state and the case in which the namespaces are being re-fetched. Also, the first thing that the application does when loading the root path / is redirecting to the default namespace -> /apps/ns/<ns> so we either don't do that redirect until we have the list of namespaces or we redirect twice.

[andresmgot]

There does seem to be one issue though, in that _all is also appearing in the dropdown, in addition to All Namespaces:

That shouldn't happen, let me investigate.

Actually, another issue: when I later re-logged in with my operator user, I still only see "All Namespaces" and "_all" in the select options, even though I can see the request to the namespaces backend returned with all namespaces:

Neither that. It seems I need to set up properly the environment to test this before moving forward.

[andresmgot]

There does seem to be one issue though, in that _all is also appearing in the dropdown, in addition to All Namespaces:

That shouldn't happen, let me investigate.

Actually, another issue: when I later re-logged in with my operator user, I still only see "All Namespaces" and "_all" in the select options, even though I can see the request to the namespaces backend returned with all namespaces:

Neither that. It seems I need to set up properly the environment to test this before moving forward.

Actually it was easier than expected, I was able to reproduce the issue. For the first issue, I changed the NamespaceSelector to avoid to show the "default" namespace if the list of namespaces is empty. I was able to reproduce the second issue with an "old" kubeops image, you need to rebuild kubeops with the changes of this branch in order to make the list work.

[andresmgot]

I am going to merge this to send the next PR

[absoludity]

I am going to merge this to send the next PR

Yep, I'd already +1'd it for that reason :) Thanks for the replies to my questions.