Adding certz baseline test#5286
Adding certz baseline test#5286ASHNA-AGGARWAL-KEYSIGHT wants to merge 6 commits intoopenconfig:mainfrom
Conversation
Pull Request Functional Test Report for #5286 / 80e3ab8Virtual Devices
Hardware Devices
|
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 comprehensive set of baseline tests for the gNSI Certz service. It establishes foundational test cases for managing TLS service profiles on a Device Under Test (DUT), including adding, deleting, and retrieving profile details. Additionally, it provides robust helper functions for generating and cleaning up test certificates and validating the connectivity and functionality of various gRPC services, ensuring the integrity of the certificate management operations. 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 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 counter productive. 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 baseline tests for the gNSI certz service, including functionality for adding, deleting, and retrieving TLS service profiles, as well as a placeholder for telemetry validation. The review identified several issues: the getTLSProfileDetailsTest fails to actually invoke the GetProfile RPC, the mismatch parameter is incorrectly handled in VerifyGnoi, the use of global variables in the setupservice package is discouraged, the busy-wait loop in CertzRotate is redundant, and the code uses time.Sleep for configuration propagation instead of the recommended gnmi.Watch with .Await as per the repository style guide.
| func getTLSProfileDetailsTest(t *testing.T, dut *ondatra.DUTDevice) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), rpcTimeout) | ||
| defer cancel() | ||
| certzClient := dut.RawAPIs().GNSI(t).Certz() | ||
|
|
||
| _, err := certzClient.AddProfile(ctx, &certzpb.AddProfileRequest{ | ||
| SslProfileId: testProfile, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| // If the profile already exists from a prior run, that is also acceptable. | ||
| if st, ok := status.FromError(err); ok && st.Code() == codes.AlreadyExists { | ||
| t.Logf("profile %q exists on DUT", testProfile) | ||
| } | ||
| } | ||
|
|
||
| //Get ssl profile list. | ||
| if getResp := setupService.GetSslProfilelist(ctx, t, certzClient, &certzpb.GetProfileListRequest{}); slices.Contains(getResp.SslProfileIds, testProfile) { | ||
| t.Logf("profile: %s already added.", testProfile) | ||
| } else { | ||
| t.Errorf("profile: %s not found in profile list", testProfile) | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of getTLSProfileDetailsTest does not actually call the GetProfile RPC, which is what the test name and description imply. It currently only verifies the profile's presence in the list returned by GetProfileList. Please update the test to call GetProfile and validate the returned details.
| if err != nil { | ||
| // If the profile already exists from a prior run, that is also acceptable. | ||
| if st, ok := status.FromError(err); ok && st.Code() == codes.AlreadyExists { | ||
| t.Errorf("profile %q already exists on DUT - skipping add", testProfile) |
There was a problem hiding this comment.
The comment on line 107 states that the profile already existing is acceptable, but t.Errorf is used here, which will cause the test to fail. If this condition is indeed acceptable, consider using t.Logf instead.
| t.Errorf("profile %q already exists on DUT - skipping add", testProfile) | |
| t.Logf("profile %q already exists on DUT - skipping add", testProfile) |
2 participants
Readme Location: https://github.com/openconfig/featureprofiles/blob/1268185c988fed56eeba55296d32fe577bc60e5e/feature/security/gnsi/certz/baseline/README.md
Raised an issue for the test not passing completely: https://partnerissuetracker.corp.google.com/issues/489348277
Logs attached: