Skip to content
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

Replace Daemon Communication Mechanism #1308

Merged
merged 32 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
478ecc4
WIP Use mio for sockets and named pipes
t1m0thyj Feb 4, 2022
8a3ba53
Hopefully more progress for Windows
t1m0thyj Feb 4, 2022
8792232
Get named pipes working
t1m0thyj Feb 4, 2022
c12963b
Make named_pipe dependency Windows-only
t1m0thyj Feb 4, 2022
995dc62
Make named_pipe dependency Windows-only pt 2
t1m0thyj Feb 4, 2022
24c88b8
Fix number of parameters to format!
t1m0thyj Feb 4, 2022
637aef9
Use BufStream to allow simultaneous read/write
t1m0thyj Feb 7, 2022
71ea7f8
Hopefully fix Linux compilation errors
t1m0thyj Feb 7, 2022
a7e7963
Forgot reader must also be mutable
t1m0thyj Feb 7, 2022
59a6077
Fix prompting with named pipes
t1m0thyj Feb 8, 2022
9e46021
Make pipe name consistent on Windows
t1m0thyj Feb 8, 2022
ada5600
Support ZOWE_CLI_HOME variable for daemon socket
t1m0thyj Feb 8, 2022
fbe581a
Test removing complex shutdown logic for macOS
t1m0thyj Feb 8, 2022
9abd0f1
Fix unit tests for sockets and pipes.
awharn Feb 8, 2022
e1cf140
Update the location of the daemon socket on *nix
awharn Feb 8, 2022
0c06766
Revert "Test removing complex shutdown logic for macOS"
awharn Feb 8, 2022
a2d46df
Merge remote-tracking branch 'origin/next' into next-test-unix-socket…
awharn Feb 8, 2022
f740a97
Add changelog entry
awharn Feb 8, 2022
8a97ce4
Add unit tests
awharn Feb 8, 2022
e4a8676
Reset mock after use
awharn Feb 8, 2022
f47c0de
Just mock the return value instead of implementation
awharn Feb 8, 2022
539f7c0
Remove statement causing CI warning
awharn Feb 9, 2022
cfbcc5d
Use NPM ~8.3.2 because of Win CI error with ^8.4.0
awharn Feb 9, 2022
eca24ae
Make requested changes
awharn Feb 9, 2022
15ba33e
Merge remote-tracking branch 'origin/next' into next-test-unix-socket…
awharn Feb 9, 2022
60de50d
Put test under right describe
awharn Feb 9, 2022
25b8e7e
Fix *nix bug, limit concurrency to 1, fix pipeline
awharn Feb 11, 2022
905267a
Fix error due to response code bump
awharn Feb 11, 2022
3c8824d
Merge remote-tracking branch 'origin/next' into next-test-unix-socket…
awharn Feb 15, 2022
4ef3962
Make requested changes
awharn Feb 15, 2022
f0aaad3
Update daemon version.
awharn Feb 17, 2022
13f6250
Update cargo lock
awharn Feb 17, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

All notable changes to the Zowe CLI package will be documented in this file.

## Recent Changes

- **NEXT BREAKING** Enhancement: Use sockets and named pipes instead of ports for daemon communication for improved access control.

## `7.0.0-next.202202041954`

- BugFix: Fixed daemon binaries missing from package and Keytar binaries not found at install time.
Expand Down
201 changes: 195 additions & 6 deletions packages/cli/__tests__/daemon/__unit__/DaemonDecider.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,19 @@

jest.mock("net");
jest.mock("@zowe/imperative");
import * as fs from "fs";
import * as net from "net";
import * as os from "os";
import * as path from "path";
import Mock = jest.Mock;
import { Imperative } from "@zowe/imperative";
import { DaemonDecider } from "../../../src/daemon/DaemonDecider";
jest.mock("../../../src/daemon//DaemonClient");

describe("DaemonDecider tests", () => {
afterEach(() => {
delete process.env.ZOWE_DAEMON;
});

it("should call normal parse method if no daemon keyword", () => {

Expand Down Expand Up @@ -64,7 +70,7 @@ describe("DaemonDecider tests", () => {
// do nothing
});

const listen = jest.fn((port, hostname, method) => {
const listen = jest.fn((socket, method) => {
// do nothing
method();
});
Expand Down Expand Up @@ -109,7 +115,7 @@ describe("DaemonDecider tests", () => {
expect(err.message).toBe("data");
});

it("should set port based on env variable", () => {
it("should set socket based on env variable", () => {

const log = jest.fn(() => {
// do nothing
Expand All @@ -125,12 +131,195 @@ describe("DaemonDecider tests", () => {

const daemonDecider = new DaemonDecider(["anything"]);

const testPort = "1234";

const testSocket = "/fake/pipe/path";
(daemonDecider as any).mParms = ["one", "two", "--daemon"];
process.env.ZOWE_DAEMON = testPort;
process.env.ZOWE_DAEMON = testSocket;
(daemonDecider as any).initialParse();
expect((daemonDecider as any).mPort).toBe(parseInt(testPort, 10));
expect((daemonDecider as any).mSocket).toBe(process.platform === "win32" ? "\\\\.\\pipe\\" + testSocket : testSocket);

});

(process.platform !== "win32" ? it : it.skip)("should try to delete an existing socket on *nix", () => {
zFernand0 marked this conversation as resolved.
Show resolved Hide resolved
const log = jest.fn(() => {
// do nothing
});

const on = jest.fn((event, func) => {
// do nothing
});

const listen = jest.fn((event, func) => {
// do nothing
});

const parse = jest.fn( (data, context) => {
expect(data).toBe(undefined);
expect(context).toBe(undefined);
});

(Imperative as any) = {
api: {
appLogger: {
trace: log,
debug: log,
error: log
}
},
console: {
info: log
},
parse
};

const fn = net.createServer as Mock<typeof net.createServer>;
fn.mockImplementation((unusedclient, ...args: any[]) => {
return {on, listen};
});

const daemonDecider = new DaemonDecider(["node", "zowe", "--daemon"]);
const testSocket = "/fake/pipe/path";
process.env.ZOWE_DAEMON = testSocket;

const existsSyncSpy = jest.spyOn(fs, "existsSync");
existsSyncSpy.mockReturnValueOnce(true);

const unlinkSyncSpy = jest.spyOn(fs, "unlinkSync");
unlinkSyncSpy.mockReturnValueOnce(true);

daemonDecider.init();
daemonDecider.runOrUseDaemon();

expect(existsSyncSpy).toHaveBeenCalledTimes(1);
expect(unlinkSyncSpy).toHaveBeenCalledTimes(1);
});

(process.platform === "win32" ? it : it.skip)("should use the default socket location (win32)", () => {
const log = jest.fn(() => {
// do nothing
});

const on = jest.fn((event, func) => {
// do nothing
});

const listen = jest.fn((event, func) => {
// do nothing
});

const parse = jest.fn( (data, context) => {
expect(data).toBe(undefined);
expect(context).toBe(undefined);
});

(Imperative as any) = {
api: {
appLogger: {
trace: log,
debug: log,
error: log
}
},
console: {
info: log
},
parse
};

const fn = net.createServer as Mock<typeof net.createServer>;
fn.mockImplementation((unusedclient, ...args: any[]) => {
return {on, listen};
});

const daemonDecider = new DaemonDecider(["node", "zowe", "--daemon"]);
daemonDecider.init();

expect((daemonDecider as any).mSocket).toEqual(`\\\\.\\pipe\\${os.userInfo().username}\\ZoweDaemon`);
});

(process.platform !== "win32" ? it : it.skip)("should use the default socket location (not win32)", () => {
const log = jest.fn(() => {
// do nothing
});

const on = jest.fn((event, func) => {
// do nothing
});

const listen = jest.fn((event, func) => {
// do nothing
});

const parse = jest.fn( (data, context) => {
expect(data).toBe(undefined);
expect(context).toBe(undefined);
});

(Imperative as any) = {
api: {
appLogger: {
trace: log,
debug: log,
error: log
}
},
console: {
info: log
},
parse
};

const fn = net.createServer as Mock<typeof net.createServer>;
fn.mockImplementation((unusedclient, ...args: any[]) => {
return {on, listen};
});

const daemonDecider = new DaemonDecider(["node", "zowe", "--daemon"]);
daemonDecider.init();

expect((daemonDecider as any).mSocket).toEqual(path.join(os.homedir(), ".zowe-daemon.sock"));
});

it("should not start a daemon", () => {
const log = jest.fn(() => {
// do nothing
});

const on = jest.fn((event, func) => {
// do nothing
});

const listen = jest.fn((event, func) => {
// do nothing
});

const parse = jest.fn( (data, context) => {
expect(data).toBe(undefined);
expect(context).toBe(undefined);
});

(Imperative as any) = {
api: {
appLogger: {
trace: log,
debug: log,
error: log
}
},
console: {
info: log
},
parse
};

const fn = net.createServer as Mock<typeof net.createServer>;
fn.mockImplementation((unusedclient, ...args: any[]) => {
return {on, listen};
});

const daemonDecider = new DaemonDecider(["node", "zowe", "--help"]);
daemonDecider.init();

expect((daemonDecider as any).mSocket).toBeUndefined();
expect((daemonDecider as any).startServer).toBeUndefined();
});
});
69 changes: 37 additions & 32 deletions packages/cli/src/daemon/DaemonDecider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@
*
*/

import { Imperative } from "@zowe/imperative";
import * as fs from "fs";
import * as net from "net";
import { userInfo } from "os";
import * as os from "os";
import * as path from "path";
import { Imperative } from "@zowe/imperative";
import { DaemonClient } from "./DaemonClient";

// TODO(Kelosky): handle prompting cases from login command
Expand All @@ -26,15 +28,6 @@ import { DaemonClient } from "./DaemonClient";
* @class DaemonDecider
*/
export class DaemonDecider {

/**
* Default port number
* @private
* @static
* @memberof DaemonDecider
*/
private static readonly DEFAULT_PORT = 4000;

/**
* Undocumented paramter for launching in server mode
* @private
Expand All @@ -52,12 +45,12 @@ export class DaemonDecider {
private mServer: net.Server;

/**
* Hold current port number for the server
* Hold current socket path for the server
* @private
* @type {number}
* @memberof DaemonDecider
*/
private mPort: number;
private mSocket: string;

/**
* Hold current owner for the server
Expand Down Expand Up @@ -90,7 +83,7 @@ export class DaemonDecider {

this.initialParse();
if (this.startServer) {
this.mUser = userInfo().username;
this.mUser = os.userInfo().username;
this.mServer = net.createServer((c) => {
new DaemonClient(c, this.mServer, this.mUser).run();
});
Expand All @@ -107,9 +100,17 @@ export class DaemonDecider {
*/
public runOrUseDaemon() {
if (this.mServer) {
this.mServer.listen(this.mPort, "127.0.0.1", () => {
Imperative.api.appLogger.debug(`daemon server bound ${this.mPort}`);
Imperative.console.info(`server bound ${this.mPort}`);
if (process.platform !== "win32" && fs.existsSync(this.mSocket)) {
fs.unlinkSync(this.mSocket);
}

["exit", "SIGINT", "SIGQUIT", "SIGTERM"].forEach((eventType: any) => {
process.on(eventType, this.close.bind(this, true));
});

this.mServer.listen(this.mSocket, () => {
Imperative.api.appLogger.debug(`daemon server bound ${this.mSocket}`);
Imperative.console.info(`server bound ${this.mSocket}`);
});
} else {
Imperative.parse();
Expand All @@ -121,8 +122,11 @@ export class DaemonDecider {
* @private
* @memberof DaemonDecider
*/
private close() {
private close(shouldExit?: boolean) {
Imperative.api.appLogger.debug(`server closed`);
if (shouldExit) {
process.exit();
}
}

/**
Expand All @@ -142,28 +146,29 @@ export class DaemonDecider {
* @memberof DaemonDecider
*/
private initialParse() {
const numOfParms = this.mParms.length - 2;
this.mPort = DaemonDecider.DEFAULT_PORT;

if (numOfParms > 0) {
const parm = this.mParms[2];

if (this.mParms.length > 2) {
/**
* NOTE(Kelosky): For now, we use an undocumented paramter `--daemon`. If found first,
* NOTE(Kelosky): For now, we use an undocumented parameter `--daemon`. If found first,
* we bypass `yargs` and begin running this as a persistent Processor.
*/
const portOffset = parm.indexOf(DaemonDecider.DAEMON_KEY);
const parm = this.mParms[2];
const daemonOffset = parm.indexOf(DaemonDecider.DAEMON_KEY);

if (portOffset > -1) {
if (daemonOffset > -1) {
this.startServer = true;

if (process.env.ZOWE_DAEMON) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our default values for named pipe or domain socket is less likely to conflict with something that the user is already using than the old port number. Another small benefit for the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely is. While it probably won't be used often, we've left the environment variable in place because it can be used to override where the socket file is being placed in a *nix environment. Ideally, the socket will be located at .zowe-daemon.sock in the user's home directory, but it can be moved in case the environment is strange and a problem occurs (i.e. a user logged into multiple workstations, all of which share the same user home directory on an NFS share.)

try {
this.mPort = parseInt(process.env.ZOWE_DAEMON, 10);
} catch (err) {
// do nothing
this.mSocket = process.env.ZOWE_DAEMON;
if (process.platform === "win32") {
this.mSocket = `\\\\.\\pipe\\${this.mSocket}`;
}
} else if (process.platform !== "win32") {
this.mSocket = path.join(os.homedir(), ".zowe-daemon.sock");
} else {
this.mSocket = `\\\\.\\pipe\\${os.userInfo().username}\\ZoweDaemon`;
}
Imperative.api.appLogger.debug(`daemon server port ${this.mPort}`);

Imperative.api.appLogger.debug(`daemon server socket ${this.mSocket}`);
zFernand0 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
Loading