installer/pkg/config-generator/tls: add kube + tnc tls certs #22
Conversation
openshift-ci-robot
added
the
size/L
trawler
changed the title
trawler
changed the title
|
/retest |
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
looks good overall. I have some nits
|
|
||
| if _, _, err := generateCert(clusterDir, kubeCAKey, kubeCACert, ingressKeyPath, ingressCertPath, cfg); err != nil { | ||
| return fmt.Errorf("failed to generate ingress CAs: %v", err) | ||
| if _, _, err = generateCert(clusterDir, kubeCAKey, kubeCACert, ingressKeyPath, ingressCertPath, cfg); err != nil { |
There was a problem hiding this comment.
why remove the : from here? we can happily keep it like we do in the rest of the instances above
| ExtKeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, | ||
| Subject: pkix.Name{CommonName: "system:admin", Organization: []string{"system:masters"}}, | ||
| Validity: validityThreeYears, | ||
| IsCA: false} |
There was a problem hiding this comment.
it's convention to terminate the curly brace on a new line
| Validity: validityThreeYears, | ||
| IsCA: false} | ||
|
|
||
| if _, _, err = generateCert(clusterDir, kubeCAKey, kubeCACert, adminKeyPath, adminCertPath, cfg); err != nil { |
| DNSNames: []string{fmt.Sprintf("%s-%s.%s", c.Name, "api", c.BaseDomain), "kubernetes", "kubernetes.default", "kubernetes.default.svc", "kubernetes.default.svc.cluster.local"}, | ||
| Validity: validityThreeYears, | ||
| IPAddresses: []net.IP{net.ParseIP(apiServerAddress)}, | ||
| IsCA: false} |
There was a problem hiding this comment.
same note about the closing curly brace here
| "localhost", "127.0.0.1"}, | ||
| Validity: validityThreeYears, | ||
| IPAddresses: []net.IP{net.ParseIP(apiServerAddress)}, | ||
| IsCA: false} |
| IsCA: false} | ||
|
|
||
| if _, _, err := generateCert(clusterDir, kubeCAKey, kubeCACert, osAPIServerKeyPath, osAPIServerCertPath, cfg); err != nil { | ||
| return fmt.Errorf("failed to generate kube openshift server certificate: %v", err) |
There was a problem hiding this comment.
what is kube openshift server? make it should be openshift API server?
| Validity: validityThreeYears, | ||
| IsCA: false} | ||
|
|
||
| if _, _, err = generateCert(clusterDir, caKey, caCert, tncKeyPath, tncCertPath, cfg); err != nil { |
| DNSNames: []string{tncDomain}, | ||
| Subject: pkix.Name{CommonName: tncDomain}, | ||
| Validity: validityThreeYears, | ||
| IsCA: false} |
There was a problem hiding this comment.
same note about the closing curly brace
| IsCA: false} | ||
|
|
||
| if _, _, err = generateCert(clusterDir, caKey, caCert, tncKeyPath, tncCertPath, cfg); err != nil { | ||
| return fmt.Errorf("failed to generate tnc certs: %v", err) |
There was a problem hiding this comment.
Instead of using %s to format the hard-coded api string, why not include that in the template?
fmt.Sprintf("%s-api.%s", c.Name, c.BaseDomain)For precedent on this approach, see here, here, and likely other places. If the pattern sounds appealing, there are some other fmt.Sprintf calls in your PR that could also be updated.
|
lgtm, can we add a unit for generateCert? |
|
@enxebre we already have unit testing for private key, create certificate and certificate request in installer/pkg/tls (all included in generateCert). What would be the benefit of adding an additional unit test? |
|
I still see value in testing generateCert which integrate those functions behaves as expected. Then you can change the internal implementation details and the consumed functions being confident that you are no breaking the layer on top and the expected output is satisfied |
There was a problem hiding this comment.
hint: We should align formats with the DNSNames field below so either always inline or always multiline, also those could be moved into constants
Both of these lines were touched by ae41b0a (Merge pull request openshift#22 from trawler/kube_tls_certs, 2018-07-09), but there's no need to use Sprintf to inject string literals into the format template. 26930f2 (post-review fixes, 2018-07-07, openshift#22) fixed one such instance, but left these. The * instance dates back to bf61dc9 (installer/pkg/config-generator/tls: generate ingress certs, 2018-07-06, openshift#16).
Latest upstream
Fix the CRD added to /manifests
…-nmstateconfig AGENT-39: Read node0 IP from nmstateconfig
6 participants
fixes INST-1111 & INST-1113