Certz4-modified timeout value in trust_bundle.go #5452
Open
priyacj wants to merge 198 commits into
Open
Conversation
Removed waiting period for large-scale cert propagation.
Contributor
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces customizable certificate generation in the trust bundle tests by adding command-line flags for certificate lists and timeouts, and updates the certificate generation script to accept these parameters. The review feedback focuses on improving code quality and security: it recommends avoiding shell execution to prevent command injection, removing redundant manual timestamps from test logs, and directly dereferencing command-line flags instead of using non-idiomatic package-level anonymous functions.
Comment on lines
72
to
109
| dut := ondatra.DUT(t, "dut") | ||
| serverAddr = dut.Name() //returns the device name. | ||
| if err := binding.DUTAs(dut.RawAPIs().BindingDUT(), &creds); err != nil { | ||
| t.Fatalf("%s:STATUS:Failed to get DUT credentials using binding.DUTAs: %v. The binding for %s must implement the DUTCredentialer interface.", logTime, err, dut.Name()) | ||
| t.Fatalf("%s:STATUS:Failed to get DUT credentials using binding.DUTAs: %v. The binding for %s must implement the DUTCredentialer interface.", time.Now().String(), err, dut.Name()) | ||
| } | ||
| username := creds.RPCUsername() | ||
| password := creds.RPCPassword() | ||
| t.Logf("%s:STATUS:Validation of all services that are using gRPC before certz rotation.", logTime) | ||
| t.Logf("%s:STATUS:Validation of all services that are using gRPC before certz rotation.", time.Now().String()) | ||
| gnmiClient, gnsiC := setup_service.PreInitCheck(context.Background(), t, dut) | ||
| //Generate testdata certificates. | ||
| t.Logf("%s:Creation of test data.", logTime) | ||
| if err := setup_service.TestdataMakeCleanup(t, dirPath, timeOutVar, "./mk_cas.sh"); err != nil { | ||
| t.Logf("%s:STATUS:Generation of testdata certificates failed!: %v", logTime, err) | ||
| t.Logf("%s:Creation of test data.", time.Now().String()) | ||
| // Execute mk_cas.sh to generate certificates | ||
| t.Logf("%s:STATUS:Generation of testdata certificates begins.", time.Now().String()) | ||
| command := fmt.Sprintf("./mk_cas.sh %v", certsString()) | ||
| if err := setup_service.TestdataMakeCleanup(t, dirPath, certsTimeOutVar(), command); err != nil { | ||
| t.Fatalf("%s:STATUS:Generation of testdata certificates failed!: %v", time.Now().String(), err) | ||
| } | ||
| //Create a certz client. | ||
| ctx := context.Background() | ||
| certzClient := gnsiC.Certz() | ||
| t.Logf("%s:STATUS:Precheck:checking baseline sslprofile list.", logTime) | ||
| t.Logf("%s:STATUS:Precheck:checking baseline sslprofile list.", time.Now().String()) | ||
| //Get sslprofile list. | ||
| if getResp := setup_service.GetSslProfilelist(ctx, t, certzClient, &certzpb.GetProfileListRequest{}); slices.Contains(getResp.SslProfileIds, testProfile) { | ||
| t.Fatalf("%s:STATUS:profileID %s already exists.", logTime, testProfile) | ||
| t.Fatalf("%s:STATUS:profileID %s already exists.", time.Now().String(), testProfile) | ||
| } | ||
| //Add new sslprofileID. | ||
| t.Logf("%s:Adding new empty sslprofile ID %s.", logTime, testProfile) | ||
| t.Logf("%s:Adding new empty sslprofile ID %s.", time.Now().String(), testProfile) | ||
| if addProfileResponse, err := certzClient.AddProfile(ctx, &certzpb.AddProfileRequest{SslProfileId: testProfile}); err != nil { | ||
| t.Fatalf("%s:STATUS:Add profile request failed with %v! ", logTime, err) | ||
| t.Fatalf("%s:STATUS:Add profile request failed with %v! ", time.Now().String(), err) | ||
| } else { | ||
| t.Logf("%s:STATUS:Received the AddProfileResponse %v.", logTime, addProfileResponse) | ||
| t.Logf("%s:STATUS:Received the AddProfileResponse %v.", time.Now().String(), addProfileResponse) | ||
| } | ||
| //Get sslprofile list after new sslprofile addition. | ||
| if getResp := setup_service.GetSslProfilelist(ctx, t, certzClient, &certzpb.GetProfileListRequest{}); !slices.Contains(getResp.SslProfileIds, testProfile) { | ||
| t.Fatalf("%s:STATUS:newly added profileID is not seen.", logTime) | ||
| t.Fatalf("%s:STATUS:newly added profileID is not seen.", time.Now().String()) | ||
| } else { | ||
| t.Logf("%s:STATUS:new profileID %s is seen in sslprofile list", logTime, testProfile) | ||
| t.Logf("%s:STATUS:new profileID %s is seen in sslprofile list", time.Now().String(), testProfile) | ||
| } |
Contributor
There was a problem hiding this comment.
This suggestion addresses two important issues:
- Command Injection Prevention: Updates the call to
setup_service.TestdataMakeCleanupto pass the executable and arguments separately, avoiding shell execution. - Redundant Timestamps: Removes the manual
time.Now().String()prefix from all log and fatal messages. The Go testing framework and test runners already automatically handle timestamps for test logs, so manually prefixing them is redundant and clutters the output.
dut := ondatra.DUT(t, "dut")
serverAddr = dut.Name() //returns the device name.
if err := binding.DUTAs(dut.RawAPIs().BindingDUT(), &creds); err != nil {
t.Fatalf("STATUS:Failed to get DUT credentials using binding.DUTAs: %v. The binding for %s must implement the DUTCredentialer interface.", err, dut.Name())
}
username := creds.RPCUsername()
password := creds.RPCPassword()
t.Log("STATUS:Validation of all services that are using gRPC before certz rotation.")
gnmiClient, gnsiC := setup_service.PreInitCheck(context.Background(), t, dut)
//Generate testdata certificates.
t.Log("Creation of test data.")
// Execute mk_cas.sh to generate certificates
t.Log("STATUS:Generation of testdata certificates begins.")
if err := setup_service.TestdataMakeCleanup(t, dirPath, *certsTimeout, "./mk_cas.sh", *certsList); err != nil {
t.Fatalf("STATUS:Generation of testdata certificates failed!: %v", err)
}
//Create a certz client.
ctx := context.Background()
certzClient := gnsiC.Certz()
t.Log("STATUS:Precheck:checking baseline sslprofile list.")
//Get sslprofile list.
if getResp := setup_service.GetSslProfilelist(ctx, t, certzClient, &certzpb.GetProfileListRequest{}); slices.Contains(getResp.SslProfileIds, testProfile) {
t.Fatalf("STATUS:profileID %s already exists.", testProfile)
}
//Add new sslprofileID.
t.Logf("Adding new empty sslprofile ID %s.", testProfile)
if addProfileResponse, err := certzClient.AddProfile(ctx, &certzpb.AddProfileRequest{SslProfileId: testProfile}); err != nil {
t.Fatalf("STATUS:Add profile request failed with %v! ", err)
} else {
t.Logf("STATUS:Received the AddProfileResponse %v.", addProfileResponse)
}
//Get sslprofile list after new sslprofile addition.
if getResp := setup_service.GetSslProfilelist(ctx, t, certzClient, &certzpb.GetProfileListRequest{}); !slices.Contains(getResp.SslProfileIds, testProfile) {
t.Fatal("STATUS:newly added profileID is not seen.")
} else {
t.Logf("STATUS:new profileID %s is seen in sslprofile list", testProfile)
}
Contributor
There was a problem hiding this comment.
Addressed this comment.
Comment on lines
289
to
291
| if err := setup_service.TestdataMakeCleanup(t, dirPath, certsTimeOutVar(), "./cleanup.sh"); err != nil { | ||
| t.Logf("%s:STATUS:Cleanup of testdata certificates failed!: %v", time.Now().String(), err) | ||
| } |
Contributor
There was a problem hiding this comment.
Update the cleanup call to match the new TestdataMakeCleanup signature and directly use *certsTimeout.
Suggested change
| if err := setup_service.TestdataMakeCleanup(t, dirPath, certsTimeOutVar(), "./cleanup.sh"); err != nil { | |
| t.Logf("%s:STATUS:Cleanup of testdata certificates failed!: %v", time.Now().String(), err) | |
| } | |
| if err := setup_service.TestdataMakeCleanup(t, dirPath, *certsTimeout, "./cleanup.sh"); err != nil { | |
| t.Logf("%s:STATUS:Cleanup of testdata certificates failed!: %v", time.Now().String(), err) | |
| } |
Contributor
There was a problem hiding this comment.
Addressed this comment.
Comment on lines
+53
to
+61
| certsList = flag.String("certsList", "01,02,10,1000", "Number of Certificate Sets to generate for this test. Comma separated string") | ||
| certsTimeout = flag.Duration("certsTimeout", 10*time.Minute, "Time duration for cert generation and cleanup. Increase if more certs are to be generated") | ||
|
|
||
| certsString = func() string { | ||
| return *certsList | ||
| } | ||
| certsTimeOutVar = func() time.Duration { | ||
| return *certsTimeout | ||
| } |
Contributor
There was a problem hiding this comment.
Defining package-level anonymous functions (certsString and certsTimeOutVar) just to dereference command-line flags is non-idiomatic in Go and adds unnecessary complexity. You can directly dereference the flag pointers (*certsList and *certsTimeout) where they are used in the test.
Suggested change
| certsList = flag.String("certsList", "01,02,10,1000", "Number of Certificate Sets to generate for this test. Comma separated string") | |
| certsTimeout = flag.Duration("certsTimeout", 10*time.Minute, "Time duration for cert generation and cleanup. Increase if more certs are to be generated") | |
| certsString = func() string { | |
| return *certsList | |
| } | |
| certsTimeOutVar = func() time.Duration { | |
| return *certsTimeout | |
| } | |
| certsList = flag.String("certsList", "01,02,10,1000", "Number of Certificate Sets to generate for this test. Comma separated string") | |
| certsTimeout = flag.Duration("certsTimeout", 10*time.Minute, "Time duration for cert generation and cleanup. Increase if more certs are to be generated") |
References
- Follow Google Go Style Guide for writing readable Go code with a consistent look and feel. (link)
Contributor
There was a problem hiding this comment.
Addressed this comment.
Contributor
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces command-line flags (certsList and certsTimeout) to customize the certificate generation process in the trust bundle tests. It updates the helper function TestdataMakeCleanup to accept variadic arguments for executing commands, and modifies the shell script mk_cas.sh to support a custom list of directories passed as an argument. Additionally, it removes the redundant logTime timestamp prefix from test log messages. I have no feedback to provide as there are no review comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
modified timeout value in trust_bundle.go to wait for the mk_cas.sh to complete.