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

Conversation

awharn
Copy link
Member

@awharn awharn commented Feb 8, 2022

Changes the Daemon communication mechanism from port based communication to socket and named pipe based communication, allowing for the use of file permissions as an access control mechanism.

t1m0thyj and others added 15 commits February 4, 2022 10:43
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #1308 (13f6250) into next (4a34f94) will decrease coverage by 0.03%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1308      +/-   ##
==========================================
- Coverage   91.43%   91.39%   -0.04%     
==========================================
  Files         369      369              
  Lines        5742     5752      +10     
  Branches      822      826       +4     
==========================================
+ Hits         5250     5257       +7     
- Misses        486      489       +3     
  Partials        6        6              
Impacted Files Coverage Δ
packages/cli/src/daemon/DaemonDecider.ts 94.33% <88.88%> (-5.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae50e22...13f6250. Read the comment docs.

This reverts commit fbe581a.
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
…s-timothy

Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
@awharn awharn force-pushed the next-test-unix-sockets-timothy branch from 90ee1d1 to f740a97 Compare February 8, 2022 21:10
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
gejohnston
gejohnston previously approved these changes Feb 9, 2022
Copy link
Member

@gejohnston gejohnston left a comment

Choose a reason for hiding this comment

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

Looks good.

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.)

zFernand0
zFernand0 previously approved these changes Feb 9, 2022
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM

packages/cli/src/daemon/DaemonDecider.ts Outdated Show resolved Hide resolved
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
zFernand0
zFernand0 previously approved these changes Feb 10, 2022
@zFernand0 zFernand0 dismissed their stale review February 11, 2022 12:46

Will review again once the mechanism's behavior is confirmed in Unix

Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
…s-timothy

Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
gejohnston
gejohnston previously approved these changes Feb 15, 2022
process.on(eventType, this.close.bind(this, true));
});

this.mServer.maxConnections = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We will have to remember this when we are tasked with operating in a multi-tenant environment, but this seems like good way to solve today's problem.

zowex/src/main.rs Outdated Show resolved Hide resolved
zowex/src/main.rs Outdated Show resolved Hide resolved
zowex/src/main.rs Outdated Show resolved Hide resolved
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
t1m0thyj
t1m0thyj previously approved these changes Feb 15, 2022
zFernand0
zFernand0 previously approved these changes Feb 15, 2022
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/zowe-cli.yml Show resolved Hide resolved
.github/workflows/zowe-cli.yml Show resolved Hide resolved
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
@awharn awharn dismissed stale reviews from zFernand0 and t1m0thyj via f0aaad3 February 17, 2022 16:57
t1m0thyj
t1m0thyj previously approved these changes Feb 17, 2022
zFernand0
zFernand0 previously approved these changes Feb 17, 2022
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
@awharn awharn dismissed stale reviews from zFernand0 and t1m0thyj via 13f6250 February 17, 2022 17:08
@sonarcloud
Copy link

sonarcloud bot commented Feb 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.0% 85.0% Coverage
0.0% 0.0% Duplication

@t1m0thyj t1m0thyj merged commit 79bf53c into next Feb 17, 2022
@t1m0thyj t1m0thyj deleted the next-test-unix-sockets-timothy branch February 17, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants