feat: make HUB_CERTIFICATE_AUTHORITY optional to allow agent use OS's root CA #149
Conversation
…certificate authority
Codecov Report
@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 78.62% 78.86% +0.24%
==========================================
Files 20 20
Lines 2737 2735 -2
==========================================
+ Hits 2152 2157 +5
+ Misses 466 460 -6
+ Partials 119 118 -1
|
| if err != nil { | ||
| klog.ErrorS(err, "Cannot decode hub cluster certificate authority data") | ||
| return nil, err | ||
| if err == nil { |
There was a problem hiding this comment.
why is that the err != nil not checked?
There was a problem hiding this comment.
the env.Lookup() is customized function in this module which wrap the os.Envlookup(). err == nil mean the environment be found.
Actually I do feel this wrap function is a kind of confusing, I use it only because of keeping consistent with other codes.
There was a problem hiding this comment.
| if err == nil { | |
| // use the agent OS's root CA when the hubCA env is not set | |
| if err == nil { // check if hubCA env is not set |
Please add the comment here to improve the readability as in normal case we check the err != nil and need some signal here to indicate why we check err == nil here.
| // - the input object namespace is a valid RFC 1123 DNS label; and | ||
| // - the input object name follows one of the three formats used in Kubernetes (RFC 1123 DNS subdomain, | ||
| // RFC 1123 DNS label, RFC 1035 DNS label). |
There was a problem hiding this comment.
why are those in this PR?
There was a problem hiding this comment.
Those changes was made by "make reviewable"
| } | ||
| hubConfig = &rest.Config{ | ||
| BearerTokenFile: tokenFilePath, | ||
| Host: hubURL, | ||
| TLSClientConfig: rest.TLSClientConfig{ | ||
| Insecure: tlsClientInsecure, | ||
| CAData: decodedClusterCaCertificate, | ||
| CAData: caData, |
There was a problem hiding this comment.
could you please clarify that, if the hubCAEnvKey is not set, the caData will be empty, it will use the OS's root CA?
Have you manually tested the agent using OS's root CA to talk to hub api server?
There was a problem hiding this comment.
could you please clarify that, if the hubCAEnvKey is not set, the caData will be empty, it will use the OS's root CA?
Yes. that is the purpose of this PR.
Have you manually tested the agent using OS's root CA to talk to hub api server?
Yes, I did. I tested it manually.
What type of PR is this?
/kind feature
What this PR does / why we need it:
When network controllers use secure TLS to talk the hub, a CA data configuration is mandatory today. This changes to make it optional so that allows network controller to use OS's root certificate to talk hub cluster.
Which issue(s) this PR fixes:
Fixes #
Requirements:
make reviewablefor basic local testHow has this code been tested
Special notes for your reviewer