-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Fix: loading in namespaces under reduced RBAC #2130
Conversation
- don't assume that loading "all namespaces" that lens knows about is the same as loading "all namespaces" that exist on the cluster - Track when actually listing namespaces Signed-off-by: Sebastian Malton <sebastian@malton.name>
@@ -64,36 +64,51 @@ export class KubeWatchApi { | |||
let isUnsubscribed = false; | |||
|
|||
const load = (namespaces = subscribingNamespaces) => this.preloadStores(stores, { namespaces, loadOnce }); | |||
let preloading = preload && load(); | |||
let preloading: boolean | ReturnType<typeof load> = preload && load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the correct typing of this value. I assume that because we aren't using strict: true
it was typed as only ReturnType<typeof load>
for (const store of stores) { | ||
if (!signal.aborted) { | ||
unsubscribeList.push(await store.subscribe()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fail quickly if we shouldn't load the stores
unsubscribeList.forEach(unsubscribe => unsubscribe()); | ||
unsubscribeList.length = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list is only "full" after subscribeP
has "finished"
async getSubscribeApis() { | ||
await this.contextReady; | ||
|
||
// if user has given static list of namespaces let's not start watches because watch adds stuff that's not wanted | ||
if (this.context?.cluster.accessibleNamespaces.length > 0) { | ||
if (this.context.cluster.accessibleNamespaces.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this to be actually correct we need to always wait for the context to be ready. Otherwise we would return an API if this function was called before the context was ready.
@@ -5,7 +5,7 @@ import { namespaceStore } from "./+namespaces/namespace.store"; | |||
export interface ClusterContext { | |||
cluster?: Cluster; | |||
allNamespaces?: string[]; // available / allowed namespaces from cluster.ts | |||
contextNamespaces?: string[]; // selected by user (see: namespace-select.tsx) | |||
selectedNamespaces?: string[]; // selected by user (see: namespace-select.tsx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert this change if wanted. But contextNamespaces
is a very not descriptive name IMO.
@@ -108,9 +112,11 @@ export class NamespaceStore extends KubeObjectStore<Namespace> { | |||
return namespaces; | |||
} | |||
|
|||
getSubscribeApis() { | |||
async getSubscribeApis() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's became async
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need to wait for contextReady
* | ||
* @observable | ||
*/ | ||
@observable listedNamespaces = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this when we have already cluster.accessibleNamespaces
and cluster.allowedNamespaces
?
If accessibleNamespaces
is provided by user it must be used first.
Otherwise maybe better to use this as I understand it correct it serves same purpose?
@computed get listedNamespaces(): boolean {
return !this.accessibleNamespaces.length;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because allowedNamespaces
might also be from the context. See line 700
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tought allowedNamespaces
must be only from the context? No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or by listing via the kube api ( which I believe is the most common)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there are some inconsistency between 2 methods imho:
protected async getAllowedNamespaces() { // get allowed?
if (this.accessibleNamespaces.length) {
return this.accessibleNamespaces; // No, get accessible!
}
protected async getAllowedResources() {
try {
if (!this.allowedNamespaces.length) { // why not checking accessible as well?
return [];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but if getAllowedResources
is only ever called after getAllowedNamespaces
it "just works"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of things I would like to change to cluster.ts
and the above it part of it but I don't think that it is in scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the idea of yet-another-public-observable because it will go to the extension API and is really hard to take away later. Any possibility that we could resolve this from the existing information (as we did before)?
if (typeof preloading === "object") { | ||
preloading.cancelLoading(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (typeof preloading === "object") { | |
preloading.cancelLoading(); | |
} | |
preloading?.cancelLoading(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my note about the actually typing of preloading
and why the old version was actually wrong.
return [this.api]; | ||
} | ||
|
||
subscribe(apis = this.getSubscribeApis()) { | ||
async subscribe(apis?: KubeApi[]): Promise<Disposer> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why moving to async
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my note
Signed-off-by: Sebastian Malton <sebastian@malton.name>
don't assume that loading "all namespaces" that lens knows about is the same as loading "all namespaces" that exist on the cluster
Track when actually listing namespaces
Signed-off-by: Sebastian Malton sebastian@malton.name
Fixes #2111