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

HLD for cli sessions feature #1367

Merged

Conversation

i-davydenko
Copy link
Contributor

@i-davydenko i-davydenko commented Jun 23, 2023

Prepared HLD documents for CLI sessions feature

Module name PR state context
sonic-buildimage Dev cli sessions GitHub issue/pull request detail GitHub pull request check contexts
sonic-host-services cli-sessions GitHub issue/pull request detail GitHub pull request check contexts
sonic-utilities SONIC CLI for CLI-Sessions feature #3175 GitHub issue/pull request detail GitHub pull request check contexts

Updated ssh_config.md with auto-logout and max-syslogins parameters.
Created serial-console-HLD.md

Signed-off-by: Ivan Davydenko ivanda@nvidia.com

Prepare HLD documents for CLI sessions feature

Update ssh_config.md with auto-logout and max-syslogins parameters.
Create serial-console-HLD.md
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -161,6 +175,20 @@ module sonic-ssh-server {
}
}
}
leaf auto_logout {
description "inactivity timeoout (min unit)";
Copy link
Collaborator

Choose a reason for hiding this comment

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

min unit -> in minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

default 15;
type uint32 {
range 0..35000;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add description for 0 value to indicate no auto logout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

leaf max_syslogins {
description "limit of concurrent system logins";
default 100;
type uint32 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

uint32 -> uint8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to leave it as 32, to not limit it in case we will extend the upper limit in future.

leaf auto_logout {
description "inactivity timeoout (min unit)";
default 15;
type uint32 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

uint32 -> uint16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, prefer not to limit by u16 in case we will extend in future.

@lguohan lguohan requested a review from liuh-80 June 27, 2023 15:54
@lguohan lguohan requested a review from qiluo-msft June 27, 2023 15:54
| Policy | Action | Param values | Default |
|--------------------|-----------------------------------------------------------------------|---------------------|--------------|
| auto logout | Inactivity timeout for serial-console session | 0-35000 (min) | 15 |
| sysrq capabilities | Enabling or disabling SysRq functionality for serial-consoles | enabled/disabled | disabled |
Copy link
Contributor

Choose a reason for hiding this comment

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

i saw some test are using sysrq in sonic-mgmt.

tests/scripts/sai_qualify/sai_warmboot.sh: echo 1 > /proc/sys/kernel/sysrq

can we check if this won't affect the tst?

@vaibhavhd for visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will overwrite any existing configuration and perform reboot (in the next line: 'echo b > /proc/sysrq-trigger' )
So the test wont break by proposed changes.
Thanks.

/* end of module sonic-serial-console */

```
### 6.4. <a name='ConfigDBEnhancements'></a>Config DB Enhancements
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the native markdown format instead of http marker? there are other cases, please correct as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@zhangyanzhao
Copy link
Collaborator

community review recording https://zoom.us/rec/share/4Wwy3t9ogqaeABDgjvYQ6IOwAovCEqdamwVzTeR9dxlvMK2GlOhNhrkcWfOgpeyc.dXhqq6g4IUDmdjxa. The starting point of this HLD is in the middle of this recording.

@zhangyanzhao
Copy link
Collaborator

zhangyanzhao commented Jun 27, 2023

MSFT register as reviewer for this feature.

@@ -80,6 +86,8 @@ We want to enable configuring the following policies, with default values are ta
| authentication retries | Number of attempts to try to log in before rejecting the session | 3-100 | 6 |
| login timeout | SSH session timeout | 1-600 (secs) | 120 |
| ports | Port numbers for SSH | 1-65535 | 22 |
| auto logout | Inactivity timeout for SSH session | 0-35000 (min) | 15 |
| max sessions | Max number of concurrent logins | 3-100 | 100 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we already have template files and hostcfgd code to support generate session limit by hwsku:

sonic-net/sonic-buildimage#10177

In the public repo this is a empty template.

So for the implementation detail of these limit, please consider reuse and improve existed code and template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also for the auto logout, currently there are code in this file do some pre-configurition:

https://github.com/sonic-net/sonic-buildimage/blob/master/build_debian.sh

So the implementation should not break those code, or need migrate those config to the new design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we will reuse existing infra to keep all limits.conf file updates in one pllace.
Added section to clarify this. Thank you for commens.

Update serial-console.hld according to the comments.
Update ssh_config HLD according to the community comments.

### 1.2. Definitions/Abbreviations

serial - secure shell
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 14, 2023

Choose a reason for hiding this comment

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

secure shell

this is for ssh? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this section covers the serial connection.
Fixed in 9e32292.

### 3.1 Flow description
When the feature is enabled, by modifying the DB manually, user will set serial-console configurations by modifing CONFIG_DB in SERIAL_CONSOLE table.

The hostcfgd daemon will be extended to listen to confogurations from SERIAL_CONSOLE table and restarts the serial_console.service. Serial console script will read SERIAL_CONSOLE table and update config files accordingly.
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 14, 2023

Choose a reason for hiding this comment

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

confogurations

typo #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in 9e32292.

### 3.1 Flow description
When the feature is enabled, by modifying the DB manually, user will set serial-console configurations by modifing CONFIG_DB in SERIAL_CONSOLE table.

The hostcfgd daemon will be extended to listen to confogurations from SERIAL_CONSOLE table and restarts the serial_console.service. Serial console script will read SERIAL_CONSOLE table and update config files accordingly.
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 14, 2023

Choose a reason for hiding this comment

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

serial_console.service

Is it a vanilla Debian service, or part of this HLD? If latter, please explain what is its functionality?

Why single hostcfgd service is not enough? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new service, not a vanilla Debian one.

The rationale behind it creation is to update serial console config files (sysrq config, $TMOUT, and others that could be added in the future) before user allowed to start serial connection.

We will achieve this by starting serial-console.service Before=getty-pre.target

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to move its functionality into hostcfgd service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe hostcfgd starts too late for this purpose, and it will impact the users config if the serial/ssh was started too early. Thats why I think in this case moving logic to hostcfgd is not a good solution.

Fix HLD with missed "POLICIES" container in config/cli tree.
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please check with other active reviewers.

@liat-grozovik
Copy link
Collaborator

@zhangyanzhao as other reviewers has no further comment, i believe we should move to the next step and merge
@i-davydenko please fix the easyCLA to unblock it

@Meir-renford
Copy link
Contributor

@venkatmahalingam can you please approve this PR if there are no more comments from your side?

@zhangyanzhao
Copy link
Collaborator

@i-davydenko can you please help to add the code PRs to this HLD by refering to #806 ? Thanks.

@liat-grozovik
Copy link
Collaborator

@i-davydenko please add a table with all the PRs under this feature in the PR description.

@i-davydenko
Copy link
Contributor Author

@liat-grozovik Done

@zhangyanzhao
Copy link
Collaborator

code PR review is on-going. Still target 202405 release

@zhangyanzhao
Copy link
Collaborator

code PRs are not approved yet, move to backlog for future release

qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Sep 16, 2024
sonic-net/SONiC#1367

Why I did it
Give ability to:
1. configure limit for active login sessions.
2. configure ssh-server / serial console autologout timeout
3. configure sysrq-capabilities (enable / disable)

Work item tracking
Microsoft ADO (number only):

How I did it
Add new service that responsible for serial configuration;
Update existing flows for extended ssh-server configurations in hostcfgd;
Add YANG model to support new configuration.

How to verify it
Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

Description for the changelog
Link to config_db schema for YANG module changes
[ssh_server](https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md#ssh_server)
[serial_console](https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md#serial_console)
qiluo-msft pushed a commit to sonic-net/sonic-utilities that referenced this pull request Sep 16, 2024
HLD: sonic-net/SONiC#1367

| Module name  | PR  | state | context |
| ------------- | ------------- | ----|-----|
| [sonic-buildimage](https://github.com/sonic-net/sonic-buildimage) | [Dev cli sessions](sonic-net/sonic-buildimage#17623) | ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/sonic-net/sonic-buildimage/17623) | ![GitHub pull request check contexts](https://img.shields.io/github/status/contexts/pulls/sonic-net/sonic-buildimage/17623) |
| [sonic-host-services](https://github.com/sonic-net/sonic-host-services)  | [cli-sessions](sonic-net/sonic-host-services#99) | ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/sonic-net/sonic-host-services/99) | ![GitHub pull request check contexts](https://img.shields.io/github/status/contexts/pulls/sonic-net/sonic-host-services/99) |
| [sonic-utilities](https://github.com/sonic-net/sonic-utilities)  | [SONIC CLI for CLI-Sessions feature #3175](#3175) | ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/sonic-net/sonic-utilities/3175)   | ![GitHub pull request check contexts](https://img.shields.io/github/status/contexts/pulls/sonic-net/sonic-utilities/3175)  |

#### What I did

Implement next commands for CLI-sessions feature:

- config serial-console inactivity-timeout  
- config serial-console sysrq-capabilities
- show serial-console

- config ssh  max-sessions
- config ssh  inactivity-timeout 
- show ssh

#### How I did it
Write handlers in config/main.py for serial-console and ssh commands to cover configuration set;
Write handlers in show/main.py for serial-console and ssh to cover show commands.
#### How to verify it
Manual tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: MovedToBacklog
Status: 🏗 In Progress
Development

Successfully merging this pull request may close these issues.

8 participants