ACCTZ-7.1: gNSI.acctz.v1 (Accounting) Test Accounting Authentication Failure - Multi-transaction#5476
ACCTZ-7.1: gNSI.acctz.v1 (Accounting) Test Accounting Authentication Failure - Multi-transaction#5476mihaitomaro wants to merge 5 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new test suite to validate that the gNSI accounting service correctly logs authentication failures across multiple SSH attempts. It also includes a critical fix for Arista device configuration, ensuring that AAA command authorization is applied in a way that does not block the initial setup of users and roles. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite for gNSI accounting, specifically verifying that records are correctly generated for multiple failed SSH authentication attempts using both passwords and keys. It also refactors the Arista-specific AAA configuration to split command authorization into two steps, preventing implicit commit failures during gNMI sessions. The review feedback correctly identified critical concurrency issues in the test's record collection loop, including a potential goroutine leak and a data race, and provided actionable suggestions to ensure thread safety and efficiency.
…l variables, no data race on outer err)
Add delays between attempts, increase timeout to 30s, track skip reasons with counters and user identity in logs, and provide collection summary stats and per-record details for troubleshooting platform-specific behavior.
| // Verify local_address and local_port. On Arista, local = DUT (server), remote = client. | ||
| if la := sessionInfo.GetLocalAddress(); la != "" { | ||
| if la != expectedRemoteAddr { | ||
| t.Logf("local_address is %q (DUT), expected SSH target %q: platform-dependent interpretation", la, expectedRemoteAddr) |
| t.Logf("local_address is %q (DUT), expected SSH target %q: platform-dependent interpretation", la, expectedRemoteAddr) | ||
| } | ||
| } else { | ||
| t.Logf("local_address not populated by DUT (platform-dependent)") |
There was a problem hiding this comment.
same here and the ones below, should they all be Logf?
| r.GetSessionInfo().GetAuthn().GetCause()) | ||
| } | ||
|
|
||
| // Verify each authentication failure record. |
There was a problem hiding this comment.
this verification logic is too large and difficult to read. it'd be easier to read if you could split to multiple verify...() functions
| } | ||
|
|
||
| // AcctzStream_RecordSubscribeClient is a local interface for the RecordSubscribe gRPC stream. | ||
| type AcctzStream_RecordSubscribeClient interface { |
There was a problem hiding this comment.
Is it common in this directory to use underscore for types or other IDs?
it's not a Golang best practice
| return &nokiaAcctzClient{conn: conn} | ||
| } | ||
|
|
||
| // func getGrpcTarget(t *testing.T, dut *ondatra.DUTDevice, service introspect.Service) string { |
There was a problem hiding this comment.
Is this commented function serving a purpose? or should we clean it up?
3 participants
Readme Location: https://github.com/openconfig/featureprofiles/blob/main/feature/gnsi/acctz/tests/AccountingAuthenFailMulti/README.md
Attached logs here: https://partnerissuetracker.corp.google.com/u/2/issues/513266214