-
Notifications
You must be signed in to change notification settings - Fork 16
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
Specification for SSH-key configuration utils #142
Conversation
@shtripat @nnDarshan @nthomas-redhat @brainfunked @r0h4n please review |
specs/ssh_configuration.adoc
Outdated
name: jsmith | ||
generate_ssh_key: yes | ||
ssh_key_bits: 2048 | ||
ssh_key_file: .ssh/id_rsa |
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.
The key file should be named specifically for tendrl. Don't rely on the defaults.
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.
@brainfunked ok
specs/ssh_configuration.adoc
Outdated
``` | ||
* Create a new a function is utils to find ssh daemon status and port number | ||
``` | ||
- Port number is identifiable using cat /var/run/sshd.pid, run the command |
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.
Nope. systemctl show sshd.service
. Grab the value of MainPID
.
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.
@brainfunked ok i will check this
specs/ssh_configuration.adoc
Outdated
p.name() # to print service name | ||
p.connections() | ||
sample output: | ||
[pconn(fd=3, family=10, type=1, laddr=('::1', 22), raddr=('::1', 54960), status='LISTEN')] |
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 bit is good.
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.
@brainfunked ok
specs/ssh_configuration.adoc
Outdated
sample output: | ||
[pconn(fd=3, family=10, type=1, laddr=('::1', 22), raddr=('::1', 54960), status='LISTEN')] | ||
- Parse the output and return the port number | ||
- Checkout the status in output if is listen then return result like port is open |
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.
Err? Listening on the port isn't the same as the port being open. Port being open means that the firewall configuration is proper. However, that's not in the scope of this specification, so for now, just report that the service is running and listening on a specific port.
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.
@brainfunked i understand clearly
specs/ssh_configuration.adoc
Outdated
a mandatory parameter, while the user name should be optional. User name | ||
should default to root. | ||
3. Check if the SSH daemon is running and get it's port number. | ||
4. Check if the SSH incoming port is open. |
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 possible unless we support firewall configuration parsing. For now, we can only report on what port the sshd process listens on. Ensure that the port number and pid is returned from the method that checks for the running daemon.
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.
@brainfunked ok
specs/ssh_configuration.adoc
Outdated
|
||
== Proposed change | ||
|
||
SSH public-key configuration have four major process: |
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 not supposed to be an all-in-one method. Each of the following steps is supposed to be a separate utility method that always works locally on a specific node. Each of these methods must execute ansible in local mode. The entire workflow will need to be tied together with flows, which are not part of this specification.
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.
@brainfunked i understand
specs/ssh_configuration.adoc
Outdated
|
||
== Implementation | ||
|
||
* Create ansible module in commons utils to generate SSH with user. If user is present |
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.
Write down each of the methods being added and their inputs and outputs.
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.
@brainfunked ok
@GowthamShanmugam Do I understand it right that Tendrl will create ssh key on node with Etcd and then distribute it to storage nodes? How will be key distributed to storage nodes? |
@mkudlej By creating new job, ssh-key is copied to particular nodes, we have to create flow and atoms for that job later. But those are out of this scope, this spec is only for utils to create and copy the ssh-keys on local node. |
37b9280
to
ccead2b
Compare
@brainfunked @shtripat @nthomas-redhat @r0h4n @nnDarshan please review |
ccead2b
to
1c3e13a
Compare
@shtripat @brainfunked @nnDarshan @r0h4n please review |
1c3e13a
to
dd39da4
Compare
specs/ssh_configuration.adoc
Outdated
@@ -0,0 +1,271 @@ | |||
= SSH configuration utilities for provisioning cluster | |||
|
|||
SSH public-key utilities are used to authenticate cluster nodes with |
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.
Somehow this description is not very correct I feel. This utility is more to do with helping tendrl to setup a password less ssh between the provisioning node and other cluster nodes.
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.
@shtripat ok will change it
specs/ssh_configuration.adoc
Outdated
(default path is "(homedir)+/.ssh/authorized_keys") | ||
path: "Alternate path to the authorized_keys file" (optional) | ||
``` | ||
* Create a new a function is utils to find ssh daemon status and port number. |
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.
Grammar?
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.
@shtripat i will correct it
specs/ssh_configuration.adoc
Outdated
|
||
== Implementation | ||
|
||
* Create a new utility class called GenerateSSH in commons with filename generate_ssh_utils.py. |
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.
Class name could be SSHUtil
may be
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.
@shtripat all the functionality is different utility so i have named utility based on its functionality, so SSHUtil not correct i feel
|
||
return status and portnumber from this | ||
``` | ||
* There is one more utility for identifying port is open or not. It will not part of this |
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.
Do we need separate classes for each functionality? Can there be one class with more generic name and have individual functions to do the job?
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.
@shtripat this is the requirement from @brainfunked he wants all the functionality should be a different utility
* Sanity check for flow. | ||
* Check SSH-key is generated successfully or not | ||
* Check SSH-key is copied successfully | ||
* Check the user is created successfully |
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.
May be manual verification of connection to nodes using ssh which doesnt ask passwd.
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.
@shtripat done
b9d0e96
to
61d620f
Compare
@shtripat @brainfunked @r0h4n please review |
specs/ssh_configuration.adoc
Outdated
} | ||
``` | ||
* Create a new utility class called AuthorizedKey in commons with filename authorized_key_utils.py. | ||
* AuthorizedKey should be initialized with username(default root) and pair of keys(mandatory). |
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.
what are the pair of keys that you have mentioned ? can you please elaborate ?
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.
@nnDarshan as we discussed i will change the sentence
* Create a new utility class called GenerateSSH in commons with filename generate_ssh_utils.py. | ||
* GenerateSSH should take username and exec_path path as parameter to initialize object. | ||
* Default user is root. | ||
* Create a method called __run_module in GenerateSSH class to call ansible runner with |
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 run module should return public key back right ? so that, it can be used by the user to write to authorized keys file ?
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.
@nnDarshan i will return the newly generated ssh key
specs/ssh_configuration.adoc
Outdated
None | ||
|
||
== Developer impact | ||
None |
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.
Can you please add how a developer will be using these utils, By adding some code snippets
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.
@nnDarshan sure i will add some sample code
61d620f
to
6d73d60
Compare
@shtripat @brainfunked @r0h4n @nnDarshan review please |
Looks good to me. |
6a6ba28
to
d5a455c
Compare
tendrl-bug-id: Tendrl#141 Signed-off-by: GowthamShanmugam <gshanmug@redhat.com>
d5a455c
to
f365a67
Compare
@GowthamShanmugam please confirm all action items are implemented |
@r0h4n all of the actions are done, it contains three major actions
All three are done https://github.com/Tendrl/commons/tree/develop/tendrl/commons/utils/ssh. |
tendrl-bug-id: #141
Signed-off-by: GowthamShanmugam gshanmug@redhat.com