New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix allowed resources checks on GKE #6657

Merged

Iku-turso merged 25 commits into master from fix-cluster-accessibility

Dec 20, 2022

Collaborator

Nokel81 commented Nov 28, 2022 •

edited

Signed-off-by: Sebastian Malton sebastian@malton.name

NOTE: This is a rather large PR that fixes the GKE issue but also does some much need refactoring with fixing some circular dependencies.

The first commit fixes the bug on GKE and the rest of the commits work to do the following refactorings:

Convert and simplify the retrival of known and allowed resources on the Cluster type so that the shouldShowResource method is more correct. Namely, just the apiName of a resource is not enough to uniquely specify it. Furthermore, this updated naming better describes how the information should be used (it is also what the Kube docs say to use for it).
1. Furthermore, this leads to a natural addition to disable this check and just show all resources.
Expands the ClusterContext type to include some helpful functions that are used in several places (more DRY)
Make two different contexts, once for namespaced resources (which can work with cluster scoped resources as well) and one just for cluster scoped resources (which is much simpler in implementation). The benefit of this is that it removes a circular dependency with NamespaceStore
Moves the KubeObjectStore.context static to the legacy extension API type and instead passes the context in as a dependency. This removed some more global shared state and permitted the removal of the circular dependency.
Remove the last uses of the legacy global isMac, etc...
Start moving more things into runnables for bootstrap

Nokel81 added the bug label

Nokel81 added this to the 6.3.0 milestone

Nokel81 requested a review from a team as a code owner

November 28, 2022 17:11

Nokel81 requested review from ixrock and aleksfront and removed request for a team

November 28, 2022 17:11

github-actions bot added the PR: needs rebase label

Contributor

github-actions bot commented Nov 28, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 force-pushed the fix-cluster-accessibility branch from 6c31d85 to 92d41b6 Compare

November 28, 2022 17:20

github-actions bot removed the PR: needs rebase label

Contributor

github-actions bot commented Nov 28, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions bot added the PR: needs rebase label

Contributor

github-actions bot commented Dec 1, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 force-pushed the fix-cluster-accessibility branch from 15d3957 to cf105dd Compare

December 1, 2022 14:33

github-actions bot removed the PR: needs rebase label

Contributor

github-actions bot commented Dec 1, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions bot added the PR: needs rebase label

Contributor

github-actions bot commented Dec 2, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 force-pushed the fix-cluster-accessibility branch from 1a34fef to 8916ce1 Compare

December 2, 2022 15:40

github-actions bot removed the PR: needs rebase label

Contributor

github-actions bot commented Dec 2, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

Nokel81 mentioned this pull request

Secrets not visible in navigation #6732

Closed

github-actions bot added the PR: needs rebase label

Contributor

github-actions bot commented Dec 15, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 force-pushed the fix-cluster-accessibility branch from e75c3de to d426ed8 Compare

December 15, 2022 16:43

Contributor

github-actions bot commented Dec 15, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions bot added PR: needs rebase and removed PR: needs rebase labels

Contributor

github-actions bot commented Dec 16, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.


          Add check for incomplete SelfSubjectRulesReview to fix GKE

51d71f2

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 added 12 commits

December 19, 2022 08:16


          Fix lint

4847cfc

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fixing namespace-select-filter tests

31f0b8e

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix other tests requiring overrides

d442de5

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix kludge in cluster-frame tests

408f811

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix remaining test failures

97bf4ae

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix integration test due to incorrect casting

883bd36

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix integration test and kube watches not working at all

c0ba030

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix secret details test

cedbc62

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix lint

b9f6bad

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix non-ApplicationBuilder tests by adding overrides

3dbe01e

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix crash due to trying to read hostedCluster too soon

266e43b

Signed-off-by: Sebastian Malton <sebastian@malton.name>


          Fix crash due to timing issues

c7ead83

- Make injectable phases more explicit for renderer

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 force-pushed the fix-cluster-accessibility branch from 4e1ec5e to c7ead83 Compare

December 19, 2022 13:17

github-actions bot removed the PR: needs rebase label

Contributor

github-actions bot commented Dec 19, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

Iku-turso reviewed

View reviewed changes

src/common/cluster/request-api-resources.injectable.ts

+                  const k8sRequest = di.inject(k8SRequestInjectable);
+                  const logger = di.inject(loggerInjectable);
+                  return async (cluster) => {

Contributor

Iku-turso Dec 20, 2022 •

edited

Code as seen here could probably benefit from reducing its nesting. Eg. instead of mutating resourceListGroups and kubeApiResources inside for-loops inside async-IIFEs inside concurrency protected decorator, inside Promise.all, it could use some functional patterns to get something like. const resourceListGroups = await pipeline(clusters, map(toK8ApiRequestPromise), awaitAll, flatMap(toResourceListGroup)); and const kubeApiResources = await pipeline(resourceListGroups, map(toGeneralK8RequestCall), map(withApiLimit), map(toCalledFunction), awaitAll, flatMap(toKubeApiResources));.

Probably made an error somewhere there, but the point is there :)

Contributor

Iku-turso Dec 20, 2022

That would be a conversation-starter :)

Iku-turso reviewed

View reviewed changes

src/common/k8s-api/kube-object.store.ts Show resolved Hide resolved

Iku-turso reviewed

View reviewed changes

src/common/k8s-api/kube-object.store.ts Show resolved Hide resolved

Iku-turso reviewed

View reviewed changes

src/common/k8s-api/kube-object.store.ts Show resolved Hide resolved

Iku-turso reviewed

View reviewed changes

src/extensions/common-api/k8s-api.ts

+                    console.warn("Setting KubeObjectStore.context is no longer supported");
+                    void ctx;
+                  },
+                  get: () => asLegacyGlobalForExtensionApi(clusterFrameContextForNamespacedResourcesInjectable),

Contributor

Iku-turso Dec 20, 2022

One day we'll get rid of these.

Iku-turso approved these changes

View reviewed changes

Iku-turso merged commit 4430814 into master

Iku-turso deleted the fix-cluster-accessibility branch

December 20, 2022 15:20

This was referenced Dec 20, 2022

Make request-api-resources flatter in implementation #6802

Merged

Release 6.3.0 #6804

Merged

KillianH mentioned this pull request

[BUG][v6.3.0] GKE Cluster - No UI displayed when connected to a GKE Cluster #6852

Closed

farodin91 reviewed

View reviewed changes

src/common/rbac.ts

+                },
+                nodes: {
+                  kind: "Node",
+                  group: "v1",

Contributor

farodin91 Jan 5, 2023

@Nokel81 Should the group be an empty string? It seems only v1 stuff is not showing up for me.

Collaborator Author

Nokel81 Jan 5, 2023

Actually this probably isn't the place that you need to fix but instead the uses of shouldShowResourceInjectionToken

Collaborator Author

Nokel81 Jan 5, 2023

But yes they probably should be empty here too

dmarjanovic mentioned this pull request

OpenLens 6.3.0 on macOS does not show pods and config/network/storage sections are empty, all work fine in 6.2.6. #6858

Closed

This was referenced Mar 5, 2023

Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Merged

Fix namespaces not respecting accessible namespace config #7279

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

farodin91 farodin91 left review comments

Iku-turso Iku-turso approved these changes

ixrock Awaiting requested review from ixrock ixrock is a code owner automatically assigned from lensapp/lens-ide

aleksfront Awaiting requested review from aleksfront aleksfront is a code owner automatically assigned from lensapp/lens-ide