-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(anta): Added the test case to verify SNMP groups #886
base: main
Are you sure you want to change the base?
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
827047c
to
cb1d3d7
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
239ea7d
to
c383c57
Compare
CodSpeed Performance ReportMerging #886 will not alter performanceComparing Summary
|
anta/input_models/snmp.py
Outdated
"""Model for a SNMP group.""" | ||
|
||
group_name: str | ||
"""SNMP group for the user.""" |
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.
"""SNMP group for the user.""" | |
"""SNMP group name.""" |
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.
Updated
anta/input_models/snmp.py
Outdated
authentication: Literal["v3Auth", "v3Priv", "v3NoAuth"] | None = None | ||
"""Advanced authentication in v3 SNMP version. Defaults to None. | ||
- v3Auth: Group using authentication but not privacy | ||
- v3Priv: Group using both authentication and privacy | ||
- v3NoAuth: Group using neither authentication nor privacy |
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 being rendered properly in the doc:
I know we provide the default everywhere, e.g., "Defaults to None" but it's not necessary since we have a Default
column in our documentation.
Also, since this field is required with SNMPv3, we should update the docstring with something like "SNMPv3 authentication settings. Required when version = v3
"
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.
Hi, Carl, I agree with your point. Since we already have a default column, I've removed the string 'Defaults to None.'
As for the content rendering in the document, I've made several attempts to modify it, but it’s still not rendering as expected. Therefore, for the time being, I’ve removed the informative strings from the description to avoid confusion. Please share your thoughts/suggestion on this approach.
Thanks
Geetanjali
anta/tests/snmp.py
Outdated
1. Verifies that the SNMP group is configured for the specified version. | ||
2. For SNMP version 3, verify that the security model matches the expected value. | ||
3. Ensures that the SNMP views, the read, write and notify settings aligning with version-specific requirements. |
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.
3. Ensures that the SNMP views, the read, write and notify settings aligning with version-specific requirements. | |
3. Ensures that SNMP group configurations, including read, write, and notify views, align with version-specific requirements. |
anta/tests/snmp.py
Outdated
"""Validate the inputs provided to the SnmpGroup class.""" | ||
for snmp_group in snmp_groups: | ||
if snmp_group.version == "v3" and snmp_group.authentication is None: | ||
msg = f"{snmp_group}; advanced `authentication` is required." |
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.
msg = f"{snmp_group}; advanced `authentication` is required." | |
msg = f"{snmp_group}; `authentication` field is required for `version: v3`" |
anta/tests/snmp.py
Outdated
# Verify SNMP views, the read, write and notify settings aligning with version-specific requirements. | ||
if group.read_view and not all( | ||
[(act_view := group_details.get("readView")) == group.read_view, (view_configured := group_details.get("readViewConfig"))] | ||
): | ||
self.result.is_failure(f"{group}, ReadView: {group.read_view} - View configuration mismatch - ReadView: {act_view}, Configured: {view_configured}") | ||
|
||
if group.write_view and not all( | ||
[(act_view := group_details.get("writeView")) == group.write_view, (view_configured := group_details.get("writeViewConfig"))] | ||
): | ||
self.result.is_failure( | ||
f"{group}, WriteView: {group.write_view} - View configuration mismatch - WriteView: {act_view}, Configured: {view_configured}" | ||
) | ||
|
||
if group.notify_view and not all( | ||
[(act_view := group_details.get("notifyView")) == group.notify_view, (view_configured := group_details.get("notifyViewConfig"))] | ||
): | ||
self.result.is_failure( | ||
f"{group}, NotifyView: {group.notify_view} - View configuration mismatch - NotifyView: {act_view}, Configured: {view_configured}" | ||
) |
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.
You could simplify this logic by looping over read, write, notify. Something like:
for view_type in ["read", "write", "notify"]:
expected_view = getattr(group, f"{view_type}_view")
if expected_view and not all([
(act_view := group_details.get(f"{view_type}View")) == expected_view,
(view_configured := group_details.get(f"{view_type}ViewConfig"))
]):
self.result.is_failure(
f"{group} {view_type.title()} View: {expected_view} - "
f"View configuration mismatch - {view_type.title()} View: {act_view}, "
f"Configured: {view_configured}"
)
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.
Also, since group_details.get()
could return an empty string, we should handle it in the failure message to avoid having something empty. You should also add a unit test for an output with it:
{
"groups": {
"ARISTA_SNMPV3_RO_GROUP": {
"versions": {
"v3": {
"secModel": "v3Priv",
"readView": "ARISTA_FULL_SNMP_TREE_VIEW",
"readViewConfig": true,
"writeView": "", # Empty string
"notifyView": ""
}
}
}
}
}
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.
Yeah!..Thanks a lot for the suggestion, @carl-baillargeon! I've updated the test case with the new logic and added unit tests for the empty string check.
6ceb261
to
819c92b
Compare
Quality Gate passedIssues Measures |
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.
@carl-baillargeon let us know if you disagree with splitting the error messages
Expected Results | ||
---------------- | ||
* Success: The test will pass if the provided SNMP group and all specified parameters are correctly configured. | ||
* Failure: The test will fail if the provided SNMP group is not configured or specified parameters are not correctly configured. |
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.
* Failure: The test will fail if the provided SNMP group is not configured or specified parameters are not correctly configured. | |
* Failure: The test will fail if the provided SNMP group is not configured or if any specified parameter is not correctly configured. |
anta.tests.snmp: | ||
- VerifySnmpGroup: | ||
snmp_groups: | ||
- group_name: Group1 |
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.
show other versions in example - especially with the security model for v3
"""Validate the inputs provided to the SnmpGroup class.""" | ||
for snmp_group in snmp_groups: | ||
if snmp_group.version == "v3" and snmp_group.authentication is None: | ||
msg = f"{snmp_group}; `authentication` field is required for `version: v3`" |
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.
Please don't use semi columns
msg = f"{snmp_group}; `authentication` field is required for `version: v3`" | |
msg = f"{snmp_group}: `authentication` field is required for `version: v3`" |
elif expected_view and not all( | ||
[(act_view := group_details.get(f"{view_type}View")) == expected_view, (view_configured := group_details.get(f"{view_type}ViewConfig"))] | ||
): | ||
self.result.is_failure( | ||
f"{group} {view_type.title()} View: {expected_view} - " | ||
f"View configuration mismatch - {view_type.title()} View: {act_view}, " | ||
f"Configured: {view_configured}" | ||
) |
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.
I would rather make two error messages, 1 to indicate the view name is not correct (matcging the expecting one) and 2. to indicate that when the view name is matching, it is not properly configured (Configured: False)
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.
Agreed
Description
Verifies the SNMP group configurations for specified version(s).
Fixes #853
Checklist:
pre-commit run
)tox -e testenv
)