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

Add mgmt-svcs HLD #108

Open
wants to merge 348 commits into
base: master
Choose a base branch
from
Open

Add mgmt-svcs HLD #108

wants to merge 348 commits into from

Conversation

seiferteric
Copy link

This is the HLD for the management services CLI for configuring REST and telemetry server options.

Ravi Vasanthm and others added 30 commits October 8, 2019 16:57
* Address more review comments

Signed-off-by: Garrick He <[email protected]>
* Add more information regarding agent-id and sample-rate default values
* Add more information about SONiC sFlow YANG.

Signed-off-by: Garrick He <[email protected]>
Signed-off-by: Garrick He <[email protected]>
Signed-off-by: Tejaswi Goel <[email protected]>
Address Review comment - Libraries used in sonic-telemetry
  - Backed out since this has to go in a separate PR.
jeff-yin and others added 28 commits July 23, 2020 16:04
Update to developer guide sections.
Add new XLM tag information.
Add more examples for development.
Add section on command order.
Add Table of Contents.
Updated OC Interface HLD regarding rate utilization counters enhancements.
 CLI "show running configuration" HLD
HLD for PIM-SSM (IPv4) Management-framework support
In-band management via mgmt VRF
Added details for response_type in Generic client
Developer Guide: Update DB access layer with link to HLD
|--------------------------|-------------------------------------|
| REST | Representational state transfer |
| gNMI | Google Network Managemnt Interface |
| JWT | Java Web Token |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be "JSON Web Token" ??


# 1 Feature Overview

A CLI is needed to manage the options for running the management services, REST and telemetry. The CLI will include setting the client authentication modes, token validity, token refresh interval and service operational state. This also requires the creation of a sonic YANG model to contain these fields which will be named sonic-mgmt-svcs. With the model defined, these fields will also be configurable via REST and gNMI interfaces. Along with the CLI and YANG model, changes will be required to REST server, sonic-buildimage and sonic-utilities.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need options to configure certificate paths also.

* Sonic model will contain the fields defined above for REST and gNMI and map to the existing redis fields. The model will be called sonic-mgmt-svcs.
* A CLI (XML and actioner) will be created to modify these fields via the CLI.
* The fields can also be modified via REST and gNMI.
* Services will be restarted when their configurations are changed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to avoid service restart. KLISH CLI also will loose connection here. If admin replays a running config file which includes REST server configs, all commands following that command will fail. It will look bad.

We should be able to change authentication modes and jwt refresh intervals dynamically, without affecting existing connections. Change of port requires restart of the TLS server. I think all these can be achieved without service restart -- by introducing a db monitoring module in REST and gNMI main.

* The CLI will be available on the Management interface(s) only
4. Configurability
* This feature is for the configuration of mgmt services.
5. User Interfaces
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this section duplicate of "3. Interfaces" ?

* Disabling Telemetry service will shut down main telemetry process, but Dialout-Telemetry process may still run.
* There is not currently a CLI for setting host and CA certificates. This will be addressed in a later HLD when PKI is available.

2 Functionality
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a heading? Please cleanup

- password,cert -> PASSWORD_CERT
- jwt,cert -> JWT_CERT
- cert -> CERT

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we retain the comma separated list? This is a standard way of representing array in sonic db. It can be modeled as a leaf-list in sonic yang. We may have to change the field name as "client_auth@" to match swsssdk conventions. DB migration script needs to change the field name only.


The model should prevent invalid configurations from being created, such as invalid client_auth modes (like JWT alone) which is handled by using an ENUM of valid modes. For PORT field, valid ports are between 1-65535, 0 should not be allowed and should result in an error. Both jwt_refresh and jwt_valid should not allow 0 either. This is acheived by the YANG range statement. In the case an invalid value is specified on the CLI, the CLI will display the error.

In the case that the user specifies a port that is already in use, this will cause the service to not restart successfully. In this case the service should revert back to the default port.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may not be a good idea. If admin configures a port, he will not be expecting the service to come up with default port. Next restart may use the configured port depending on the system state. Clients/controllers cannot change their settings dynamically.

* All Platforms

# 9 Limitations
* This is available via the Management interface(s) only.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a limitation.
Also remove the indentation so that it gets rendered as bullet. Today it is rendered as code block due to indentation.

N/A

## 11.1 IS-CLI Compliance
N/A
Copy link
Collaborator

Choose a reason for hiding this comment

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

IS-CLI is supported. You may want to put a link to section 3.6.2.

@@ -0,0 +1,420 @@
# Management CLI
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Management Services CLI"

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.