-
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
Views management refactoring #669
Changes from 1 commit
7d3e876
8925186
a78d5a0
5452fdb
09c765c
2287332
2894f31
1204e9e
3e86729
9818c07
795a98c
7364cb7
0b80032
ceca2d7
8e6d886
7b5092d
6f6680b
4ef26be
d2be0e4
8e626ee
b3f97a6
6293dad
3a50255
df764ca
26e93e7
aa63624
bb382d2
503888a
39391e7
6c40d33
93b42ff
631826a
916a101
ae3fdea
260bbbd
e7d4317
7903eba
f5a978e
2228957
77066ee
c4ae765
bc339e4
877fe18
1987c12
161b356
8275e15
a28d671
38aefc5
50a2e1f
fb124ad
0353ee7
948421b
07c3143
fe42219
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Roman <ixrock@gmail.com> Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import type http from "http" | ||
import { autorun } from "mobx"; | ||
import { apiKubePrefix } from "../common/vars"; | ||
import { ClusterId, clusterStore } from "../common/cluster-store" | ||
import { Cluster } from "./cluster" | ||
import { clusterIpc } from "../common/cluster-ipc"; | ||
|
@@ -50,23 +49,8 @@ export class ClusterManager { | |
} | ||
|
||
getClusterForRequest(req: http.IncomingMessage): Cluster { | ||
let cluster: Cluster = null | ||
|
||
// lens-server is connecting to 127.0.0.1:<port>/<uid> | ||
if (req.headers.host.startsWith("127.0.0.1")) { | ||
const clusterId = req.url.split("/")[1] | ||
if (clusterId) { | ||
cluster = this.getCluster(clusterId) | ||
if (cluster) { | ||
// we need to swap path prefix so that request is proxied to kube api | ||
req.url = req.url.replace(`/${clusterId}`, apiKubePrefix) | ||
} | ||
} | ||
} else { | ||
const id = req.headers.host.split(".")[0] | ||
cluster = this.getCluster(id) | ||
} | ||
|
||
return cluster; | ||
logger.info(`getClusterForRequest(): ${req.headers.host}${req.url}`) | ||
const clusterId = req.headers.host.split(".")[0] | ||
return this.getCluster(clusterId) | ||
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 logic has been removed? Don't we use 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. Seems so.. Looks like outdated code from very beginning of lens-app. 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 asked because I 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. Ok, reverted back. Someone should check if this is actually works, cause top comment: |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,8 @@ | |
import "../common/system-ca" | ||
import "../common/prometheus-providers" | ||
import { app, dialog } from "electron" | ||
import { appName, appProto, staticDir, staticProto } from "../common/vars"; | ||
import { appName, staticDir } from "../common/vars"; | ||
import path from "path" | ||
import { initMenu } from "./menu" | ||
import { LensProxy } from "./lens-proxy" | ||
import { WindowManager } from "./window-manager"; | ||
import { ClusterManager } from "./cluster-manager"; | ||
|
@@ -41,8 +40,7 @@ async function main() { | |
const updater = new AppUpdater() | ||
updater.start(); | ||
|
||
registerFileProtocol(appProto, app.getPath("userData")); | ||
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 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 it's never used 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. Might be a dumb question but how do we display uploaded cluster icons then? 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 switched to saving the base64 encoding of them in the user store 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. It removes the filename issue that we were seeing with some unicode characters, and it removes the security problems when running on dev mode. 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.
Ok, do we have a migration for that? 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 did not code one up, I should do that then 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.
Yes we should have a migration. Otherwise Lens will drop all the existing cluster icons on upgrade. |
||
registerFileProtocol(staticProto, staticDir); | ||
registerFileProtocol("static", staticDir); | ||
|
||
// find free port | ||
let proxyPort: number | ||
|
@@ -74,7 +72,6 @@ async function main() { | |
|
||
// create window manager and open app | ||
windowManager = new WindowManager(proxyPort); | ||
initMenu(windowManager); | ||
} | ||
|
||
app.on("ready", main); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,11 @@ import { openShell } from "./node-shell-session"; | |
import { Router } from "./router" | ||
import { ClusterManager } from "./cluster-manager" | ||
import { ContextHandler } from "./context-handler"; | ||
import { apiKubePrefix, noClustersHost } from "../common/vars"; | ||
import { apiKubePrefix } from "../common/vars"; | ||
import logger from "./logger" | ||
|
||
export class LensProxy { | ||
protected origin: string | ||
protected proxyServer: http.Server | ||
protected router: Router | ||
protected closed = false | ||
|
@@ -21,12 +22,13 @@ export class LensProxy { | |
} | ||
|
||
private constructor(protected port: number, protected clusterManager: ClusterManager) { | ||
this.origin = `http://localhost:${port}` | ||
this.router = new Router(); | ||
} | ||
|
||
listen(port = this.port): this { | ||
this.proxyServer = this.buildCustomProxy().listen(port); | ||
logger.info(`LensProxy server has started http://localhost:${port}`); | ||
logger.info(`LensProxy server has started at ${this.origin}`); | ||
return this; | ||
} | ||
|
||
|
@@ -117,26 +119,17 @@ export class LensProxy { | |
} | ||
|
||
protected async handleRequest(proxy: httpProxy, req: http.IncomingMessage, res: http.ServerResponse) { | ||
if (req.headers.host.split(":")[0] === noClustersHost) { | ||
this.router.handleStaticFile(req.url, res); | ||
return; | ||
} | ||
const cluster = this.clusterManager.getClusterForRequest(req) | ||
if (!cluster) { | ||
const reqId = this.getRequestId(req); | ||
logger.error("Got request to unknown cluster", { reqId }) | ||
res.statusCode = 503 | ||
res.end() | ||
return | ||
} | ||
const contextHandler = cluster.contextHandler | ||
await contextHandler.ensureServer(); | ||
const proxyTarget = await this.getProxyTarget(req, contextHandler) | ||
if (proxyTarget) { | ||
proxy.web(req, res, proxyTarget) | ||
} else { | ||
this.router.route(cluster, req, res); | ||
if (cluster) { | ||
await cluster.contextHandler.ensureServer(); | ||
const proxyTarget = await this.getProxyTarget(req, cluster.contextHandler) | ||
if (proxyTarget) { | ||
// allow to fetch apis in "clusterId.localhost:port" from "localhost:port" | ||
res.setHeader("Access-Control-Allow-Origin", this.origin); | ||
return proxy.web(req, res, proxyTarget); | ||
} | ||
} | ||
this.router.route(cluster, req, res); | ||
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 route on an undefined cluster? Wouldn't it be better to change line 123 to be |
||
} | ||
|
||
protected async handleWsUpgrade(req: http.IncomingMessage, socket: net.Socket, head: Buffer) { | ||
|
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 this change, the older version is more clear
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.
Not for me, sorry. Older version more like functional-style, but I would not say it's more readable (for humans) + there is no need to run multiple cycles on what's can be updated in one run.
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 think this is matter of taste. I would revert this change as it's not related to views management at all, right?