-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export type Disposer = () => void; |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 commentThe 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 |
||||||||||
let cancelReloading: IReactionDisposer = noop; | ||||||||||
let ac = new AbortController(); | ||||||||||
|
||||||||||
const subscribe = () => { | ||||||||||
if (isUnsubscribed) return; | ||||||||||
const subscribe = async (signal: AbortSignal) => { | ||||||||||
if (isUnsubscribed || signal.aborted) return; | ||||||||||
|
||||||||||
stores.forEach((store) => { | ||||||||||
unsubscribeList.push(store.subscribe()); | ||||||||||
}); | ||||||||||
for (const store of stores) { | ||||||||||
if (!signal.aborted) { | ||||||||||
unsubscribeList.push(await store.subscribe()); | ||||||||||
} | ||||||||||
} | ||||||||||
Comment on lines
+74
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fail quickly if we shouldn't load the stores |
||||||||||
}; | ||||||||||
|
||||||||||
let subscribing: Promise<void> = Promise.resolve(); | ||||||||||
|
||||||||||
if (preloading) { | ||||||||||
if (waitUntilLoaded) { | ||||||||||
preloading.loading.then(subscribe, error => { | ||||||||||
this.log({ | ||||||||||
message: new Error("Loading stores has failed"), | ||||||||||
meta: { stores, error, options: opts }, | ||||||||||
subscribing = preloading.loading | ||||||||||
.then(() => subscribe(ac.signal)) | ||||||||||
.catch(error => { | ||||||||||
this.log({ | ||||||||||
message: new Error("Loading stores has failed"), | ||||||||||
meta: { stores, error, options: opts }, | ||||||||||
}); | ||||||||||
}); | ||||||||||
}); | ||||||||||
} else { | ||||||||||
subscribe(); | ||||||||||
subscribing = subscribe(ac.signal); | ||||||||||
} | ||||||||||
|
||||||||||
// reload stores only for context namespaces change | ||||||||||
cancelReloading = reaction(() => this.context?.contextNamespaces, namespaces => { | ||||||||||
preloading?.cancelLoading(); | ||||||||||
unsubscribeList.forEach(unsubscribe => unsubscribe()); | ||||||||||
unsubscribeList.length = 0; | ||||||||||
preloading = load(namespaces); | ||||||||||
preloading.loading.then(subscribe); | ||||||||||
cancelReloading = reaction(() => this.context?.selectedNamespaces, namespaces => { | ||||||||||
if (typeof preloading === "object") { | ||||||||||
preloading.cancelLoading(); | ||||||||||
} | ||||||||||
Comment on lines
+99
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my note about the actually typing of |
||||||||||
ac.abort(); | ||||||||||
subscribing.then(() => { | ||||||||||
unsubscribeList.forEach(unsubscribe => unsubscribe()); | ||||||||||
unsubscribeList.length = 0; | ||||||||||
Comment on lines
+104
to
+105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This list is only "full" after |
||||||||||
|
||||||||||
ac = new AbortController(); | ||||||||||
preloading = load(namespaces); | ||||||||||
preloading.loading | ||||||||||
.then(() => subscribing = subscribe(ac.signal)); | ||||||||||
}); | ||||||||||
}, { | ||||||||||
equals: comparer.shallow, | ||||||||||
}); | ||||||||||
|
@@ -104,9 +119,15 @@ export class KubeWatchApi { | |||||||||
if (isUnsubscribed) return; | ||||||||||
isUnsubscribed = true; | ||||||||||
cancelReloading(); | ||||||||||
preloading?.cancelLoading(); | ||||||||||
unsubscribeList.forEach(unsubscribe => unsubscribe()); | ||||||||||
unsubscribeList.length = 0; | ||||||||||
|
||||||||||
if (typeof preloading === "object") { | ||||||||||
preloading.cancelLoading(); | ||||||||||
} | ||||||||||
ac.abort(); | ||||||||||
subscribing.then(() => { | ||||||||||
unsubscribeList.forEach(unsubscribe => unsubscribe()); | ||||||||||
unsubscribeList.length = 0; | ||||||||||
}); | ||||||||||
}; | ||||||||||
} | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ export class NamespaceStore extends KubeObjectStore<Namespace> { | |
].flat())); | ||
} | ||
|
||
@computed get contextNamespaces(): string[] { | ||
@computed get selectedNamespaces(): string[] { | ||
const namespaces = Array.from(this.contextNs); | ||
|
||
if (!namespaces.length) { | ||
|
@@ -108,9 +108,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 commentThe reason will be displayed to describe this comment to others. Learn more. Why it's became There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we need to wait for |
||
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) { | ||
Comment on lines
+111
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
return []; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. I can revert this change if wanted. But |
||
} | ||
|
||
export const clusterContext: ClusterContext = { | ||
|
@@ -17,7 +17,7 @@ export const clusterContext: ClusterContext = { | |
return this.cluster?.allowedNamespaces ?? []; | ||
}, | ||
|
||
get contextNamespaces(): string[] { | ||
return namespaceStore.contextNamespaces ?? []; | ||
get selectedNamespaces(): string[] { | ||
return namespaceStore.selectedNamespaces ?? []; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import type { ClusterContext } from "./components/context"; | ||
|
||
import { action, computed, observable, reaction, when } from "mobx"; | ||
import { autobind } from "./utils"; | ||
import { autobind, Disposer } from "./utils"; | ||
import { KubeObject, KubeStatus } from "./api/kube-object"; | ||
import { IKubeWatchEvent } from "./api/kube-watch-api"; | ||
import { ItemStore } from "./item.store"; | ||
|
@@ -35,7 +35,7 @@ export abstract class KubeObjectStore<T extends KubeObject = any> extends ItemSt | |
} | ||
|
||
@computed get contextItems(): T[] { | ||
const namespaces = this.context?.contextNamespaces ?? []; | ||
const namespaces = this.context?.selectedNamespaces ?? []; | ||
|
||
return this.items.filter(item => { | ||
const itemNamespace = item.getNs(); | ||
|
@@ -111,7 +111,7 @@ export abstract class KubeObjectStore<T extends KubeObject = any> extends ItemSt | |
|
||
const isLoadingAll = this.context.allNamespaces.every(ns => namespaces.includes(ns)); | ||
|
||
if (isLoadingAll) { | ||
if (isLoadingAll && this.context.cluster?.listedNamespaces) { | ||
this.loadedNamespaces = []; | ||
|
||
return api.list({}, this.query); | ||
|
@@ -264,11 +264,15 @@ export abstract class KubeObjectStore<T extends KubeObject = any> extends ItemSt | |
}); | ||
} | ||
|
||
getSubscribeApis(): KubeApi[] { | ||
async getSubscribeApis(): Promise<KubeApi[]> { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Why moving to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my note |
||
|
||
await this.contextReady; | ||
apis ??= await this.getSubscribeApis(); | ||
|
||
const abortController = new AbortController(); | ||
const namespaces = [...this.loadedNamespaces]; | ||
|
||
|
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
andcluster.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?
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 700There 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:
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 aftergetAllowedNamespaces
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)?