Support for SSL BackendSet certificate #243

MadalinaPatrichi · 2018-09-10T09:17:32Z

Added support for supplying SSL certificates for LoadBalancers backend sets:

Added support for new type of generic Kubernetes secret to read from, which has the following data fields:

ca.crt - Base64 encoded CA certificate
tls.crt - Base64 encoded Public certificate
tls.key - Base64 encoded Private key
passphrase - Base64 encoded passphrase

Modified Health Check to support both TCP and HTTP protocols. The choice between protocols is driven by whether there exists an SSL certificate associated to the port for which the health check is being done. Both the SSL certificates and the SSL enabled ports are specified in the annotations of the Load Balancer configuration file
Added Ginkgo test to check connectivity for SSL enabled backend sets

owainlewis · 2018-09-11T10:14:23Z

pkg/oci/load_balancer.go

-	secretString, ok := svc.Annotations[ServiceAnnotationLoadBalancerTLSSecret]
-	if !ok {
-		return "", "", errors.Errorf("no %q annotation found", ServiceAnnotationLoadBalancerTLSSecret)
+func (cp *CloudProvider) readSSLSecret(secretType string, svc *v1.Service) (string, string, string, string, error) {


The return type here is pretty hard to understand and might be prone to assignment error. Suggest refactoring into a struct or similar.

func (cp *CloudProvider) readSSLSecret(secretType string, svc *v1.Service) (*TLSCertificate, error)

You're right, thanks!

owainlewis · 2018-09-12T11:24:10Z

Minor typo in commit message: Add Backendset SSL cerrtificates => Add Backendset SSL certificates

owainlewis · 2018-09-13T13:56:18Z

pkg/oci/load_balancer.go

 	if !exists {
 		return cp.createLoadBalancer(ctx, spec)
 	}

+	// for _, v := range spec.BackendSets {


Let's remove this

prydie

👍

prydie · 2018-09-13T14:52:00Z

docs/tutorial-ssl-backendset.md

+    targetPort: 80
+```
+
+First the required secret needs to be created in Kubernetes. For the purposes


s/secret/Secret/

prydie · 2018-09-13T14:53:56Z

docs/tutorial-ssl-backendset.md

+production you would most likely use a public certificate signed by a
+certificate authority.
+
+Below is an example of a secret configuration file required to be uploaded as a Kubernetes


s/secret/Secret/

prydie · 2018-09-13T14:54:00Z

docs/tutorial-ssl-backendset.md

+certificate authority.
+
+Below is an example of a secret configuration file required to be uploaded as a Kubernetes
+generic secret. The CA certificate, the public certificate and the private key need to be base64 encoded :


s/secret/Secret/

Space before :

prydie · 2018-09-13T14:56:34Z

docs/tutorial-ssl-backendset.md

+  tls.key: LS0tLS1CRUdJTi(...)
+```
+
+Self-signed certificates can be generated using the following GitHub repository: https://github.com/square/certstrap


Maybe cfssl (or god forbid even openssl) would be a more standard recommendation?

Haha, true, I'll just remove it, Google can help if needed

prydie · 2018-09-13T14:57:56Z

pkg/oci/load_balancer.go

+	// ServiceAnnotationLoadBalancerTLSBackendSecret is a Service annotation for
+	// specifying the TLS secret to install on the load balancer listeners which
+	// have SSL enabled.
+	// See: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls


This probably needs updating to reflect that we aren't using TLS Secrets.

pkg/oci/load_balancer_spec.go

prydie · 2018-09-13T15:44:03Z

pkg/oci/load_balancer.go

@@ -230,7 +249,8 @@ func (cp *CloudProvider) ensureSSLCertificate(ctx context.Context, lb *loadbalan
 	}

 	// Although we iterate here only one certificate is supported at the moment.
-	certs, err := spec.Certificates()
+	certs := make(map[string]loadbalancer.CertificateDetails)


This is a strange pattern. Any reason you're not allocating the map in the function and returning?

It's because I wanted to hold all certificates (both for listeners and backend sets) within the same map. Therefore, I am creating a map of certificates and I add to it step by step. I guess this saves me from having to make a union of the map :) for which go doesn't have a nice implementation of (unlike python)

Ah, that makes sense. I think perhaps then that extending SSLConfig to include both BackendSet and Listener SSL configuration would likely result in cleaner code. Thoughts?

Yeah, you're right, Or maybe have a separate struct to hold both

prydie · 2018-09-13T15:47:05Z

pkg/oci/load_balancer.go

+	// specifying the TLS secret to install on the load balancer listeners which
+	// have SSL enabled.
+	// See: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls
+	ServiceAnnotationLoadBalancerTLSBackendSecret = "service.beta.kubernetes.io/oci-load-balancer-tls-backend-secret"


Do we also need to support something along the lines of ServiceAnnotationLoadBalancerSSLPorts but for backends at all?

I thought of this too, but @owainlewis was mentioning that we can assume that both the Listener certificate and the Backend set certificates can be uniformly applied for the same ports. The closest thing to what OCI offers would be to have a one-to-one mapping between one port and one certificate, which is not the case now, even with Listeners. Maybe we should have a discussion about this and address this separately

prydie · 2018-09-13T15:48:11Z

pkg/oci/load_balancer.go

-
-	return string(cert), string(key), nil
+	if key, ok = secret.Data[SSLPrivateKeyFileName]; !ok {
+		return &SSLK8SSecret{}, errors.Errorf("%s not found in secret %s/%s", SSLPrivateKeyFileName, ns, name)


return nil, errors.Errorf("...

prydie · 2018-09-13T15:50:22Z

pkg/oci/load_balancer_util.go

@@ -29,8 +29,14 @@ import (
 )

 const (
-	sslCertificateFileName = "tls.crt"
-	sslPrivateKeyFileName  = "tls.key"
+	// SSLCAFileName - Key name for ca data in the secrets config


Docstring comments should read as a sentence:

// SSLCAFileName is the key name for ca data in the secrets config.

prydie · 2018-09-17T14:03:34Z

test/e2e/framework/util.go

@@ -349,6 +434,23 @@ func GetReadySchedulableNodesOrDie(c clientset.Interface) (nodes *v1.NodeList) {
 	return nodes
 }

+// NewSecretData returns the default v1.Secret template for this jig, but
+// does not actually create the Secret.
+func NewSecretData(ns, generateName string, caCert, cert, privateKey, passphrase []byte) *v1.Secret {


Appears unused.

prydie · 2018-09-17T14:06:53Z

pkg/oci/load_balancer_spec.go

-	path, port := apiservice.GetServiceHealthCheckPathPort(svc)
-	if path != "" {
+func getHealthChecker(cfg *SSLConfig, port int, svc *v1.Service) *loadbalancer.HealthCheckerDetails {
+	var protocol string


// If the health-check has SSL enabled use TCP rather than HTTP. protocol := lbNodesHealthCheckProtoHTTP if cfg != nil && cfg.Ports.Has(port) { protocol = lbNodesHealthCheckProtoTCP }

prydie · 2018-09-17T14:34:24Z

pkg/oci/load_balancer_util.go

@@ -29,8 +29,14 @@ import (
 )

 const (
-	sslCertificateFileName = "tls.crt"
-	sslPrivateKeyFileName  = "tls.key"
+	// SSLCAFileName is a key name for ca data in the secrets config


As docstrings are expected to be sentences (for nice godocs) they should end with a full-stop.

prydie · 2018-09-17T14:34:48Z

pkg/oci/load_balancer_util.go

+	SSLCAFileName = "ca.crt"
+	// SSLCertificateFileName is a key name for certificate data in the secrets config
+	SSLCertificateFileName = "tls.crt"
+	// SSLPrivateKeyFileName - is a key name for cartificate private key in the secrets config


s/-// (sentences)

prydie · 2018-09-17T14:36:21Z

test/e2e/framework/networking_util.go


 	ipPort := net.JoinHostPort(ip, strconv.Itoa(port))
-	url := fmt.Sprintf("http://%s%s", ipPort, request)


url := fmt.Sprintf("http://%s%s", ipPort, request) if secure { url = fmt.Sprintf("https://%s%s", ipPort, request) }

prydie

Couple of nits and queries but otherwise looking awesome 👍

prydie · 2018-09-19T15:11:18Z

test/e2e/load_balancer.go

+		ns := f.Namespace.Name
+
+		jig := framework.NewServiceTestJig(f.ClientSet, serviceName)
+		//nodeIP := framework.PickNodeIP(jig.Client) // for later


Should probably clean up commented out code?

prydie · 2018-09-19T15:12:06Z

test/e2e/framework/networking_util.go


 	ipPort := net.JoinHostPort(ip, strconv.Itoa(port))
-	url := fmt.Sprintf("http://%s%s", ipPort, request)
+	var url string


url := fmt.Sprintf("http://%s%s", ipPort, request)

(no need for separate declare and define)

prydie · 2018-09-19T15:14:41Z

pkg/oci/client/load_balancer.go

 	if !c.rateLimiter.Writer.TryAccept() {
 		return "", RateLimitError(true, "CreateCertificate")
 	}

-	// TODO(apryde): We currently don't have a mechanism for supplying
-	// CreateCertificateDetails.CaCertificate.
 	resp, err := c.loadbalancer.CreateCertificate(ctx, loadbalancer.CreateCertificateRequest{
 		LoadBalancerId: &lbID,
 		CreateCertificateDetails: loadbalancer.CreateCertificateDetails{


Could you not just do CreateCertificateDetails: cert, here?

No, one is CreateCertificateDetails type and one is CertificateDetails. Unless there's some magic to make that happen? :)

please say there is to make go cooler for me

🤦‍♂️

Is that a facepalm because it should have been obvious for me on how to cast it from one type to the other?

Goodness no! Facepalm that I didn't notice they were different types 😄

prydie · 2018-09-19T15:15:22Z

pkg/oci/load_balancer.go

-		return "", "", errors.Errorf("%s not found in secret %s/%s", sslCertificateFileName, ns, name)
+	var ok bool
+	var cacert, cert, key, pass []byte
+	cacert, _ = secret.Data[SSLCAFileName]


cacert = secret.Data[SSLCAFileName]

prydie · 2018-09-19T15:17:31Z

pkg/oci/load_balancer.go

 	if requiresCertificate(service) {
 		ports, err := getSSLEnabledPorts(service)
 		if err != nil {
 			return nil, err
 		}
-		ssl = NewSSLConfig(lbName, ports, cp)
+		secretListenerString, _ := service.Annotations[ServiceAnnotationLoadBalancerTLSSecret]


prydie · 2018-09-19T15:17:37Z

pkg/oci/load_balancer.go

 	if requiresCertificate(service) {
 		ports, err := getSSLEnabledPorts(service)
 		if err != nil {
 			return nil, err
 		}
-		ssl = NewSSLConfig(lbName, ports, cp)
+		secretListenerString, _ := service.Annotations[ServiceAnnotationLoadBalancerTLSSecret]
+		secretBackendSetString, _ := service.Annotations[ServiceAnnotationLoadBalancerBackendSetSecret]


prydie · 2018-09-19T15:37:23Z

pkg/oci/load_balancer_spec.go

 	}
-	if cert == "" || key == "" {
+	if len(sslSecret.PublicCert) == 0 || len(sslSecret.PrivateKey) == 0 {


I'm not clear on how this works in the case that there's BackendSet certs but not Listener?

LMAATCFT (let me add a test case for that) good spot

prydie · 2018-09-19T15:40:20Z

pkg/oci/load_balancer_spec.go

+			secretName = sslCfg.BackendSetSSLSecretName
+		}
+		sslConfig := getSSLConfiguration(sslCfg, secretName, port)
+		if sslConfig != nil {


No need for if statement. loadbalancer.BackendSetDetails{SslConfiguration: sslConfig} when sslConfig == nil is equivalent to not specifying the property as the default value for pointers is nil.

prydie · 2018-09-19T16:59:04Z

pkg/oci/load_balancer_spec.go

-	if len(sslSecret.PublicCert) == 0 || len(sslSecret.PrivateKey) == 0 {
-		return certs, nil
-	}
+	if len(s.SSLConfig.ListenerSSLSecretName) != 0 {


We can support ca certificates and passphrases on listener certs and implement as follows I believe:

secrets := make([]string, 0, 2) if s.SSLConfig.ListenerSSLSecretName != "" { secrets = append(secrets, s.SSLConfig.ListenerSSLSecretName) } if s.SSLConfig.BackendSetSSLSecretName != "" { secrets = append(secrets, s.SSLConfig.BackendSetSSLSecretName) } for _, name := range secrets { cert, err := s.SSLConfig.readSSLSecret(s.service.Namespace, name) if err != nil { return nil, errors.Wrap(err, "reading SSL BackendSet Secret") } certs[name] = loadbalancer.CertificateDetails{ CertificateName: &s.SSLConfig.BackendSetSSLSecretName, CaCertificate: common.String(string(cert.CACert)), PublicCertificate: common.String(string(cert.PublicCert)), PrivateKey: common.String(string(cert.PrivateKey)), Passphrase: common.String(string(cert.Passphrase)), } }

owainlewis

Couple of minor comments to address from previous reviews. Overall looks good to me.

owainlewis · 2018-09-17T09:26:21Z

docs/tutorial-ssl-backendset.md

+  annotations:
+    service.beta.kubernetes.io/oci-load-balancer-ssl-ports: "443"
+    service.beta.kubernetes.io/oci-load-balancer-tls-secret: ssl-certificate-secret
+    service.beta.kubernetes.io/oci-load-balancer-backendset-secret: ssl-certificate-secret


To be consistent let's change to service.beta.kubernetes.io/oci-load-balancer-tls-backendset-secret

owainlewis · 2018-09-17T09:34:01Z

test/e2e/load_balancer.go

+		tcpIngressIP := framework.GetIngressPoint(&tcpService.Status.LoadBalancer.Ingress[0])
+		framework.Logf("TCP load balancer: %s", tcpIngressIP)
+
+		// By("hitting the TCP service's NodePort")


Let's remove unused code

owainlewis · 2018-09-20T14:55:11Z

pkg/oci/load_balancer.go

+	// specifying the generic secret to install on the load balancer listeners which
+	// have SSL enabled.
+	// See: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls
+	ServiceAnnotationLoadBalancerBackendSetSecret = "service.beta.kubernetes.io/oci-load-balancer-backendset-secret"


service.beta.kubernetes.io/oci-load-balancer-backendset-secret => service.beta.kubernetes.io/oci-load-balancer-backendset-tls-secret to be consistent?

owainlewis · 2018-09-20T14:56:36Z

pkg/oci/load_balancer.go

-	var cert, key []byte
-	if cert, ok = secret.Data[sslCertificateFileName]; !ok {
-		return "", "", errors.Errorf("%s not found in secret %s/%s", sslCertificateFileName, ns, name)
+	var ok bool


Is this (var ok bool) needed?

if cert, ok := secret.Data[SSLCertificateFileName]; !ok {

yes, I think it is. It's because we need to declare cert and key beforehand.

owainlewis · 2018-09-20T14:58:39Z

pkg/oci/load_balancer.go

 	certs, err := spec.Certificates()
 	if err != nil {
 		return err
 	}
+
+	var ok bool


Is this needed in outer scope vs if _, ok := lb.Certificates[*cert.CertificateName]; !ok {

owainlewis · 2018-09-20T15:09:37Z

test/e2e/load_balancer.go

+		ns := f.Namespace.Name
+
+		jig := framework.NewServiceTestJig(f.ClientSet, serviceName)
+		//nodeIP := framework.PickNodeIP(jig.Client) // for later


Remove unused code

owainlewis · 2018-09-20T15:10:17Z

test/e2e/load_balancer.go

+		jig := framework.NewServiceTestJig(f.ClientSet, serviceName)
+		//nodeIP := framework.PickNodeIP(jig.Client) // for later
+
+		sslSecretNameL := "ssl-certificate-secret-listener"


Minor: Would generally prefer a more readable name like sslListenerSecretName

owainlewis · 2018-09-20T15:11:11Z

test/e2e/load_balancer.go

+				cloudprovider.SSLPassphrase:          []byte(framework.SSLPassphrase),
+			},
+		})
+		sslSecretNameB := "ssl-certificate-secret-backendset"


Minor: Would prefer sslBackendSetSecretName vs sslSecretNameB

owainlewis · 2018-09-20T15:11:22Z

test/e2e/load_balancer.go

+			loadBalancerCreateTimeout = framework.LoadBalancerCreateTimeoutLarge
+		}
+
+		// TODO(apryde): Test that LoadBalancers can receive static IP addresses


owainlewis · 2018-09-20T15:12:12Z

test/e2e/load_balancer.go

+			loadBalancerCreateTimeout = framework.LoadBalancerCreateTimeoutLarge
+		}
+
+		// TODO(apryde): Test that LoadBalancers can receive static IP addresses


Copy paste from previous test. Probably not worth duplicating again.

owainlewis

Looks good. Once additional reviews are +1 we can merge.

prydie

Awesome work! 🎉

owainlewis requested review from owainlewis, simonlord and prydie September 11, 2018 10:08

owainlewis reviewed Sep 11, 2018

View reviewed changes

owainlewis added enhancement feature labels Sep 11, 2018

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 3 times, most recently from b2ffd66 to 079b329 Compare September 13, 2018 08:49

MadalinaPatrichi changed the title ~~[WIP] Support for SSL backend certificate~~ Support for SSL backend certificate Sep 13, 2018

MadalinaPatrichi changed the title ~~Support for SSL backend certificate~~ Support for SSL BackendSet certificate Sep 13, 2018

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 2 times, most recently from cdf2d44 to 30084bb Compare September 13, 2018 09:22

owainlewis removed the request for review from simonlord September 13, 2018 13:55

owainlewis self-assigned this Sep 13, 2018

owainlewis reviewed Sep 13, 2018

View reviewed changes

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 2 times, most recently from 429affc to f14a7e8 Compare September 13, 2018 14:20

prydie suggested changes Sep 13, 2018

View reviewed changes

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 5 times, most recently from 952a2c1 to b7efcb9 Compare September 17, 2018 13:45

prydie reviewed Sep 17, 2018

View reviewed changes

prydie suggested changes Sep 19, 2018

View reviewed changes

MadalinaPatrichi force-pushed the mp/TLSImplementation branch from 7adec2e to 8b07710 Compare September 19, 2018 16:46

prydie reviewed Sep 19, 2018

View reviewed changes

MadalinaPatrichi force-pushed the mp/TLSImplementation branch from 8b07710 to 71a0110 Compare September 20, 2018 12:51

MadalinaPatrichi added 2 commits September 20, 2018 13:52

Add Backendset SSL certificates

89656d0

Refactor SSL Certificates

7e7d9f0

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 5 times, most recently from ef38a27 to 5d97908 Compare September 20, 2018 13:31

Third round of comments

22bb891

MadalinaPatrichi force-pushed the mp/TLSImplementation branch from 5d97908 to 22bb891 Compare September 20, 2018 14:06

owainlewis suggested changes Sep 20, 2018

View reviewed changes

Minor name changes

3167fd9

MadalinaPatrichi force-pushed the mp/TLSImplementation branch from 8c9c044 to 3167fd9 Compare September 20, 2018 15:45

owainlewis approved these changes Sep 20, 2018

View reviewed changes

MadalinaPatrichi mentioned this pull request Sep 21, 2018

Support setting CA Certificate bundle #149

Closed

MadalinaPatrichi force-pushed the mp/TLSImplementation branch 4 times, most recently from 8e313ba to b64268e Compare September 21, 2018 15:03

Rename e2e tests

0d3625f

MadalinaPatrichi force-pushed the mp/TLSImplementation branch from b64268e to 0d3625f Compare September 21, 2018 15:59

prydie approved these changes Sep 21, 2018

View reviewed changes

prydie merged commit 1da23ec into master Sep 21, 2018

prydie deleted the mp/TLSImplementation branch September 21, 2018 16:25

prydie added this to the 0.7.0 milestone Sep 21, 2018

prydie mentioned this pull request Sep 21, 2018

Implement support for end to end TLS #235

Closed

ayushverma14 pushed a commit to ayushverma14/oci-cloud-controller-manager that referenced this pull request Jul 18, 2022

Support assigning SSL certs to BackendSets (oracle#243)

06c4926


		ipPort := net.JoinHostPort(ip, strconv.Itoa(port))
		url := fmt.Sprintf("http://%s%s", ipPort, request)

Support for SSL BackendSet certificate #243

Support for SSL BackendSet certificate #243

Conversation

MadalinaPatrichi commented Sep 10, 2018 • edited Loading

owainlewis Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadalinaPatrichi Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadalinaPatrichi Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis left a comment

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

MadalinaPatrichi commented Sep 10, 2018 •

edited

Loading

owainlewis Sep 11, 2018 •

edited

Loading

MadalinaPatrichi Sep 14, 2018 •

edited

Loading

MadalinaPatrichi Sep 19, 2018 •

edited

Loading