Authz: General Authz (1-4) tests - Fixed authz test issue mentioned in #2770 #3117
Conversation
Eeramma
commented
Jun 19, 2024
Eeramma
commented
- Added code to handle empty Authz Get() operation in TestAuthz1 & TestAuthz2.
- Added code to handle wait-timeout for bidirectional stream Recv API in rpcexec.go & authz.go
- Corrected test admin infra id from cafyauto to valid SPIFFE ID.
- Added Setup Baseline post reboot to populate policy details.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Pull Request Functional Test Report for #3117 / 2e7a552Virtual Devices
Hardware Devices
|
| t.Logf("Authz Policy of the Device %s before the Rotate Trigger is %s", dut.Name(), policyBefore.PrettyPrint(t)) | ||
| defer policyBefore.Rotate(t, dut, uint64(time.Now().Unix()), fmt.Sprintf("v0.%v", (time.Now().UnixNano())), false) | ||
| statusmsg, policyBefore := authz.Get(t, dut) | ||
| //t.Logf("Message for first authz get %s", msg) |
There was a problem hiding this comment.
random debug log left over?
| statusmsg, policyBefore := authz.Get(t, dut) | ||
| //t.Logf("Message for first authz get %s", msg) | ||
| if statusmsg == nil { | ||
| t.Logf("Expected error FAILED_PRECONDITION seen for authz Get Request.") |
There was a problem hiding this comment.
should this be a t.Fatal ? (why continue if you get zero policy on an existing device you expect policy?)
| //t.Logf("Message for first authz get %s", msg) | ||
| if statusmsg == nil { | ||
| t.Logf("Expected error FAILED_PRECONDITION seen for authz Get Request.") | ||
| } else { |
There was a problem hiding this comment.
what benefit does this else provide?
(genreal advice in golang is to just not else, really ever)
| t.Logf("Expected error FAILED_PRECONDITION seen for authz Get Request.") | ||
| } else { | ||
| t.Logf("Authz Policy of the Device %s before the Rotate Trigger is %s", dut.Name(), policyBefore.PrettyPrint(t)) | ||
| defer policyBefore.Rotate(t, dut, uint64(time.Now().Unix()), fmt.Sprintf("v0.%v", (time.Now().UnixNano())), false) |
| @@ -46,7 +46,7 @@ const ( | |||
| type UsersMap map[string]authz.Spiffe | |||
|
|
|||
| var ( | |||
| testInfraID = flag.String("test_infra_id", "cafyauto", "SPIFFE-ID used by test Infra ID user for authz operation") | |||
| testInfraID = flag.String("test_infra_id", "spiffe://test-abc.foo.bar/xyz/cafyauto", "SPIFFE-ID used by test Infra ID user for authz operation") | |||
There was a problem hiding this comment.
this user is not in the usersMap - is that intentional? perhaps a comment as to why?
| } | ||
| if resp.GetCreatedOn() > uint64(time.Now().UnixMicro()) { | ||
| t.Errorf("CreatedOn value can not be larger than current time") | ||
| statusError, _ := status.FromError(err) |
There was a problem hiding this comment.
looks like you are throwing away an err, why?
(probably a comment like; "ignoring error here for " is good)
| if err != nil { | ||
| if strings.Contains(err.Error(), "invalid policy") || status.Code(err) == codes.InvalidArgument || strings.Contains(err.Error(), "InvalidArgument") { | ||
| return nil | ||
| } else { |
| } else { | ||
| return err | ||
| } | ||
| } else if err == io.EOF { |
| return nil | ||
| for { | ||
| _, err := gnsiCStream.Recv() | ||
| if err != nil { |
There was a problem hiding this comment.
could this be a swith err {} instead of this chain or else/if/etc?
| msg, err := mStream.Recv() | ||
| if err == io.EOF { | ||
| return nil | ||
| } else if err != nil { |
There was a problem hiding this comment.
why else? why not just switch on err ?
chains of tihs if/else/elseif/etc are toilsome to think though, when all you are relaly doing is a switch statement.
(also see previous comment about adminishment in golang for use of else generally)
Pull Request Test Coverage Report for Build 11359918104Details
💛 - Coveralls |
