Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

update to Go 1.15 #752

Closed
pohly opened this issue Sep 28, 2020 · 4 comments
Closed

update to Go 1.15 #752

pohly opened this issue Sep 28, 2020 · 4 comments
Labels
0.8 needs to be fixed in 0.8.x

Comments

@pohly
Copy link
Contributor

pohly commented Sep 28, 2020

We should do the next upcoming release (= 0.8) with a Go version that'll be supported for a while. Currently we use 1.13, which is getting old.

@pohly pohly added the 0.8 needs to be fixed in 0.8.x label Sep 28, 2020
@pohly
Copy link
Contributor Author

pohly commented Sep 28, 2020

The problem is that Go 1.15 strengthens the certificate validation and no longer accepts just CN. make test fails: validate CA certificates: x509: certificate relies on legacy Common Name field, use SANs or temporarily enable Common Name matching with GODEBUG=x509ignoreCN=0

Here's a naive attempt to fix this ("naive" as in "I have no idea how to specify SANs for cfssl"), but it didn't work:

diff --git a/Dockerfile b/Dockerfile
index 17275063..8d11277f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -5,7 +5,7 @@ ARG LINUX_BASE=debian:buster-slim
 FROM ${LINUX_BASE} AS build
 ARG APT_GET="env DEBIAN_FRONTEND=noninteractive apt-get"
 
-ARG GO_VERSION="1.13.4"
+ARG GO_VERSION="1.15.2"
 
 # CACHEBUST is set by the CI when building releases to ensure that apt-get really gets
 # run instead of just using some older, cached result.
diff --git a/pkg/pmem-csi-operator/controller/deployment/controller_driver.go b/pkg/pmem-csi-operator/controller/deployment/controller_driver.go
index 92e4b634..8f545eb4 100644
--- a/pkg/pmem-csi-operator/controller/deployment/controller_driver.go
+++ b/pkg/pmem-csi-operator/controller/deployment/controller_driver.go
@@ -402,11 +402,11 @@ func (d *PmemCSIDriver) getSecrets() ([]apiruntime.Object, error) {
 			return nil, err
 		}
 		ncCert = pmemtls.EncodeCert(cert)
-	} else {
-		// check if the provided certificates are valid
-		if err := validateCertificates(caCert, registryPrKey, registryCert, ncPrKey, ncCert); err != nil {
-			return nil, fmt.Errorf("validate CA certificates: %v", err)
-		}
+	}
+
+	// Check if the provided or generated certificates are valid.
+	if err := validateCertificates(caCert, registryPrKey, registryCert, ncPrKey, ncCert); err != nil {
+		return nil, fmt.Errorf("validate CA certificates: %v", err)
 	}
 
 	// Instead of waiting for next GC cycle, initiate garbage collector manually
diff --git a/pkg/registryserver/registryserver.go b/pkg/registryserver/registryserver.go
index 49fcbd6d..76ec4d0b 100644
--- a/pkg/registryserver/registryserver.go
+++ b/pkg/registryserver/registryserver.go
@@ -9,7 +9,6 @@ import (
 	pmemgrpc "github.com/intel/pmem-csi/pkg/pmem-grpc"
 	registry "github.com/intel/pmem-csi/pkg/pmem-registry"
 	"github.com/kubernetes-csi/csi-lib-utils/metrics"
-	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
@@ -185,7 +184,7 @@ func (rs *RegistryServer) RegisterController(ctx context.Context, req *registry.
 				delete(rs.nodeClients, req.NodeId)
 				pmemNodes.Set(float64(len(rs.nodeClients)))
 				rs.mutex.Unlock()
-				return nil, errors.Wrap(err, "failed to register node")
+				return nil, fmt.Errorf("failed to register node: %w", err)
 			}
 		}
 	}
diff --git a/runtime-deps.csv b/runtime-deps.csv
index 88b17b13..7eb0ddf1 100644
--- a/runtime-deps.csv
+++ b/runtime-deps.csv
@@ -6,4 +6,3 @@ grpc-go,https://github.com/grpc/grpc-go,
 kubernetes,https://github.com/kubernetes/kubernetes,9641
 kubernetes-sigs/controller-runtime,https://github.com/kubernetes-sigs/controller-runtime,
 operator-sdk,https://github.com/operator-framework/operator-sdk,
-pkg/errors,https://github.com/pkg/errors,
diff --git a/test/setup-ca.sh b/test/setup-ca.sh
index 77addc71..24a79b12 100755
--- a/test/setup-ca.sh
+++ b/test/setup-ca.sh
@@ -18,6 +18,7 @@ fi
 <<EOF cfssl -loglevel=3 gencert -initca - | cfssljson -bare ca
 {
     "CN": "$CA",
+    "SAN": "$CA",
     "key": {
         "algo": "rsa",
         "size": 2048
@@ -32,6 +33,7 @@ for name in ${CNS}; do
   <<EOF cfssl -loglevel=3 gencert -ca=ca.pem -ca-key=ca-key.pem - | cfssljson -bare $name
 {
     "CN": "$name",
+    "SAN": "$name",
     "hosts": [
         $(if [ "$name" = "pmem-registry" ]; then
              # Some extra names needed for scheduler extender and webhook.

Note that this also enables validation of the certificates generated by the operator. Otherwise "make test" only fails when using the cfssl certificates, but the operator code also needs to be updated.

@pohly
Copy link
Contributor Author

pohly commented Sep 28, 2020

We probably need to migrate to the new behavior in stages:

  • in PMEM-CSI 0.8: generate certificates with SAN, but still accept those without, and declare the old approach as deprecated
  • in PMEM-CSI 0.9: stop accepting the old certificates and update the operator so that it renews the certificates during an update (a variant of issue operator shall renew certificates generated on expiry #594)

pohly added a commit to pohly/pmem-CSI that referenced this issue Sep 28, 2020
Certificates with just CN and no SAN are not accepted anymore by
default by the Go 1.15
runtime (https://golang.org/doc/go1.15#commonname).
Before we update to Go 1.15, we need to restore support via the
GODEBUG variable because otherwise upgrading to the upcoming PMEM-CSI
release will break existing installations.

This can be reverted once we are sure that all installations use
certificates with SAN.

Related-to: intel#752
pohly added a commit to pohly/pmem-CSI that referenced this issue Sep 29, 2020
Certificates with just CN and no SAN are not accepted anymore by
default by the Go 1.15
runtime (https://golang.org/doc/go1.15#commonname).
Before we update to Go 1.15, we need to restore support via the
GODEBUG variable because otherwise upgrading to the upcoming PMEM-CSI
release will break existing installations.

This can be reverted once we are sure that all installations use
certificates with SAN.

Related-to: intel#752
pohly added a commit to pohly/pmem-CSI that referenced this issue Sep 29, 2020
Certificates with just CN and no SAN are not accepted anymore by
default by the Go 1.15
runtime (https://golang.org/doc/go1.15#commonname).
Before we update to Go 1.15, we need to restore support via the
GODEBUG variable because otherwise upgrading to the upcoming PMEM-CSI
release will break existing installations.

This can be reverted once we are sure that all installations use
certificates with SAN.

Related-to: intel#752
@pohly
Copy link
Contributor Author

pohly commented Sep 29, 2020

An update to Go 1.15 without certificate creation changes is in: #753

pohly added a commit to pohly/pmem-CSI that referenced this issue Sep 29, 2020
Certificates with just CN and no SAN are not accepted anymore by
default by the Go 1.15
runtime (https://golang.org/doc/go1.15#commonname).
Before we update to Go 1.15, we need to restore support via the
GODEBUG variable because otherwise upgrading to the upcoming PMEM-CSI
release will break existing installations.

This can be reverted once we are sure that all installations use
certificates with SAN.

Related-to: intel#752
pohly added a commit to pohly/pmem-CSI that referenced this issue Sep 30, 2020
Certificates with just CN and no SAN are not accepted anymore by
default by the Go 1.15
runtime (https://golang.org/doc/go1.15#commonname).
Before we update to Go 1.15, we need to restore support via the
GODEBUG variable because otherwise upgrading to the upcoming PMEM-CSI
release will break existing installations.

This can be reverted once we are sure that all installations use
certificates with SAN.

Related-to: intel#752
@pohly
Copy link
Contributor Author

pohly commented Oct 2, 2020

And certificate creation was fixed in PR #755

@pohly pohly closed this as completed Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.8 needs to be fixed in 0.8.x
Projects
None yet
Development

No branches or pull requests

1 participant