-
Notifications
You must be signed in to change notification settings - Fork 147
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
CERTZ2.1 server_certificates #3313
base: main
Are you sure you want to change the base?
Conversation
Pull Request Functional Test Report for #3313 / d4c3a84Virtual Devices
Hardware Devices
|
Pull Request Test Coverage Report for Build 11444866423Details
💛 - Coveralls |
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Outdated
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Outdated
Show resolved
Hide resolved
} | ||
rotateResponse := &certzpb.RotateCertificateResponse{} | ||
for i := 0; i < 20; i++ { | ||
rotateResponse, err = rotateRequestClient.Recv() |
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.
Recv
here is blocking -- so you don't need to do this. If you're wanting something like a maximum wait time here you need to do something like having a goroutine that sits calling Recv
, and one that subsequently cancels it after a timeout. An example is shown below.
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.
Did this comment make sense, please let me know if you'd like to discuss it further.
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Outdated
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Outdated
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/internal/setup_service/setup_service.go
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/server_certificates/server_certificates_test.go
Outdated
Show resolved
Hide resolved
feature/security/gnsi/certz/tests/server_certificates/server_certificates_test.go
Outdated
Show resolved
Hide resolved
Thanks for this test case -- it looks to cover the |
/gcbrun |
"encoding/pem" | ||
"fmt" | ||
|
||
//"io" |
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.
Remove stale unused import?
*creds.UserPass | ||
} | ||
|
||
func (r *rpcCredentials) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) { |
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.
func (r *rpcCredentials) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) { | |
func (r *rpcCredentials) GetRequestMetadata(_ context.Context, _ ...string) (map[string]string, error) { |
//a valid check for trust not empty | ||
if len(trust) == 0 { | ||
return &certzpb.CertificateChain{} | ||
} else { |
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.
Per the static check, you can just change this to:
if len(trust) == 0 {
return &certzpb.CertificateChain{}
}
<rest of code>
since the else
is not adding any new condition.
} | ||
|
||
// CertzRotate function to request the client certificate rotation and returns true on successful rotation. | ||
func CertzRotate(t *testing.T, caCert *x509.CertPool, certzClient certzpb.CertzClient, cert tls.Certificate, san, serverAddr, profileID string, entities ...*certzpb.Entity) bool { |
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.
There are unused arguments for this function -- since this isn't meeting a signature defined elsewhere, please remove the arguments.
} | ||
rotateResponse := &certzpb.RotateCertificateResponse{} | ||
for i := 0; i < 20; i++ { | ||
rotateResponse, err = rotateRequestClient.Recv() |
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.
Did this comment make sense, please let me know if you'd like to discuss it further.
CERTZ2.1 server_certificates testcase
CERTZ2.2 server_certificates testcase