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

Snmp configuation db schema proposal #231

Open
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from

Conversation

MichelMoriniaux
Copy link

Proposal for a config DB SNMP configuration table schema

* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
* propose schema for SNMP config in the DB
* list of changes to files

  Signed-off-by: michel.moriniaux@gmail.com
"location": LOCATION_STRING,
"contact": CONTACT_STRING,
"v2c": {
COMMUNITY_STRING: {
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

COMMUNITY_STRING [](start = 8, length = 16)

Is it true that COMMUNITY_STRING is only related to v2c? #Resolved

Copy link
Author

@MichelMoriniaux MichelMoriniaux Aug 21, 2018

Choose a reason for hiding this comment

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

I don't understand the comment
it is my understanding that in v1 and v2c the community is a 'password' and that in v3 this 'password' is replaced by user authentication+encryption key, so in this contect COMMUNITY_STRING should be part of v2c configuration only #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

I found "SNMPv3 also uses community strings, but allows for secure authentication and communication between SNMP manager and agent"
https://en.wikipedia.org/wiki/Simple_Network_Management_Protocol.
I am ok with the version string here.


In reply to: 211706220 [](ancestors = 211706220)

"v2c": {
COMMUNITY_STRING: {
"type": "rw"|"ro",
"acl": ACL_STRING
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

acl [](start = 13, length = 3)

Is it true that acl is not related to COMMUNITY_STRING? #Closed

Copy link
Author

Choose a reason for hiding this comment

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

current behavior of /usr/bin/snmpd-config-updater and only this configuration method is to read the ACL table to link a rocommunity to a host
for eg.: this writes in the snmpd.conf file:
rocommunity mysnmpcomm 172.16.10.0/24
rocommunity mysnmpcomm 122.168.5.12

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I believe this is implementation details. By design, we support one or several snmp community strings, and all whitelist IP will use the same set of community strings. I think your design is like

COMMUNITY_STRING1: { acl: acl1 },
COMMUNITY_STRING2: { acl: acl2 },

I believe it should be:

"community_strings": ["COMMUNITY1", "COMMUNITY2"],
"acl": [....]

@jleveque for more insight.


In reply to: 211704981 [](ancestors = 211704981)

Copy link
Author

Choose a reason for hiding this comment

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

makes sense, significantly simplifies config generation

Copy link
Contributor

@jleveque jleveque Aug 21, 2018

Choose a reason for hiding this comment

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

Qi is correct. the current SNMP ACL implementation disregards community strings, and works on a service-based level. We whitelist IPs for each service, so all whitelisted IPs for SNMP will apply regardless of the community string.

Also, the /usr/bin/snmpd-config-updater application currently only applies to Arista platforms, and I believe it will be removed in the coming months. Once this is done, all service ACLs (control plane ACLs) will be managed in the base image using the caclmgrd daemon. Therefore, there will be no need for ACL information inside the SNMP container. I believe you can simplify your design further and eliminate ACL information altogether.


New keys:
- "v2c": we define a "v2c" tree to allow for future expansion for other versions of the SNMP protocol, this spec only defines for SNMP v2
we could imagine the implementation of "v3" with the inclusion of users or references to central PAM methods.
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

imagine [](start = 18, length = 7)

Is there any reference for the imagination? I found v3 also supports COMMUNITY_STRING #Closed

Copy link
Author

@MichelMoriniaux MichelMoriniaux Aug 21, 2018

Choose a reason for hiding this comment

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

we could have for eg.:
"v3": { "key": ENCRYPTION_KEY, "pam": PAM_METHOD, "acl": ACL_STRING, "users": { USERNAME: PASSWORD, USERNAME: PASSWORD } }

"v2c": {
COMMUNITY_STRING: {
"type": "rw"|"ro",
"acl": ACL_STRING
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

ACL_STRING [](start = 19, length = 10)

Currently ConfigDB already contains "ACL_TABLE|SNMP_MGMT_ONLY", do you want to replace it or link to it?
@jleveque who developed it. #Closed

Copy link
Author

Choose a reason for hiding this comment

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

good point:
how about: if the "acl" key is present we override and if not present we used the default: "ACL_TABLE|SNMP_MGMT_ONLY"

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 refer to it, and keep the table contents in original place? This way your changes are minimized.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned above, the /usr/bin/snmpd-config-updater application currently only applies to Arista platforms, and I believe it will be removed in the coming months. Once this is done, all service ACLs (control plane ACLs) will be managed in the base image using the caclmgrd daemon. Therefore, there will be no need for ACL information inside the SNMP container. I believe you can simplify your design further and eliminate ACL information altogether.

All ACL information will continue to reside in the ACL_TABLE of ConfigDB.

@jipanyang
Copy link
Contributor

It is good to move the configuration to configDB. Eventually where are you going to store the schema data?

A large part of SONiC schema could be found in swss-schema.md file though some of them are a little outdated. My worry is that we may lose track of the schema soon if there is no well known central place to store and find them.

If there is mechanism to automatically generate code from the schema, it would be great, that will ensure the schema is always up to date and match the code implementation, but I guess this will a lot of effort.

- removed ACL requirements
- type becomes an optional element
- added requirement for swss-schema.md update
@MichelMoriniaux
Copy link
Author

@jipanyang added requirement to the design proposal

doc/snmp-schema-addition.md Outdated Show resolved Hide resolved
doc/snmp-schema-addition.md Outdated Show resolved Hide resolved
doc/snmp-schema-addition.md Outdated Show resolved Hide resolved
jleveque requested changes
@lguohan
Copy link
Contributor

lguohan commented Aug 28, 2018

@MichelMoriniaux can you add backward compatibility into the design, so that we can make sure the existing way still works.

@MichelMoriniaux
Copy link
Author

MichelMoriniaux commented Aug 28, 2018 via email

@tientienle
Copy link

tientienle commented Sep 9, 2018

I don't see any value that used to store the snmp server ip address. Is there any reason for this or do you use it in anywhere else?
For example, this is how I store it in config_db.json (or cfg data)
"SNMPINFORM": {
"192.168.0.1": {
"community-string": "public"
}
},
"SNMPTRAP2": {
"192.168.0.1": {
"community-string": "public"
}
},
"SNMPTRAP3": {
"1192.168.0.1": {
"username": "spine1",
"password": "password",
"auth": "MD5",
"priv": "DES",
"key": "testingkey"
}
},
"SNMPTRAP1": {
"192.168.0.1": {
"community-string": "public"
}
},
However, I still miss a few more values.

@RichardWu-Hebut
Copy link

I tried to write the SNMP configuration according to your proposal, and found that those SNMP configurations can not be automatically loaded from JSON file into DB. So, we need to handle this SNMP configuration specially?

@MichelMoriniaux
Copy link
Author

MichelMoriniaux commented Jan 30, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants