advancedTLS: Rename get root certs related pieces #7207
Conversation
| // If users want to reload the root trust certificate, it is required to return | ||
| // the proper TrustCerts in GetRootCAs. | ||
| type GetRootCAsResults struct { | ||
| type RootCertificates struct { |
There was a problem hiding this comment.
Since it's just a wrapper and used as an output of a single function - how about RootCertificatesResults?
There was a problem hiding this comment.
I think Results is just noise - this struct fundamentally is root certificates, and it's a given that it's a Result when it's the output of a function. To me this would be sort of like naming the function GetRootCAsFunction
security/advancedtls/advancedtls.go
Outdated
| @@ -479,7 +495,7 @@ func (o *Options) serverConfig() (*tls.Config, error) { | |||
| type advancedTLSCreds struct { | |||
| config *tls.Config | |||
| verifyFunc PostHandshakeVerificationFunc | |||
| getRootCAs func(params *GetRootCAsParams) (*GetRootCAsResults, error) | |||
| getRootCAs func(params *ConnectionInfo) (*RootCertificates, error) | |||
There was a problem hiding this comment.
Shouldn't it be renamed to GetRootCertificates in this struct as well?
There was a problem hiding this comment.
This is non-exported so I wasn't as worried about getting it changed right now
There was a problem hiding this comment.
It seems like if you do want to make this change, now would be a great time to do it. But I don't feel strongly. Same for the other one.
There was a problem hiding this comment.
Went on and made the change in both
| @@ -384,9 +384,14 @@ func (s) TestClientServerHandshake(t *testing.T) { | |||
| if err := cs.LoadCerts(); err != nil { | |||
| t.Fatalf("cs.LoadCerts() failed, err: %v", err) | |||
| } | |||
| getRootCAsForClient := func(params *GetRootCAsParams) (*GetRootCAsResults, error) { | |||
| getRootCAsForClient := func(params *ConnectionInfo) (*RootCertificates, error) { | |||
There was a problem hiding this comment.
Same here (and below) - getRootCertificatesForClient?
There was a problem hiding this comment.
Since this was just tests and also not part of the API I'm trying to keep the diff focused on the API.
I don't mind making the changes here either, WDYT?
There was a problem hiding this comment.
Went on and made the change in both
security/advancedtls/advancedtls.go
Outdated
| @@ -479,7 +495,7 @@ func (o *Options) serverConfig() (*tls.Config, error) { | |||
| type advancedTLSCreds struct { | |||
| config *tls.Config | |||
| verifyFunc PostHandshakeVerificationFunc | |||
| getRootCAs func(params *GetRootCAsParams) (*GetRootCAsResults, error) | |||
| getRootCAs func(params *ConnectionInfo) (*RootCertificates, error) | |||
There was a problem hiding this comment.
It seems like if you do want to make this change, now would be a great time to do it. But I don't feel strongly. Same for the other one.
This PR renames
GetRootCAsParamstoConnectionInfoandGetRootCAsResultstoRootCertificatesto better name what these structs actually represent.RELEASE NOTES: none