Reset network status when CSR is deleted

The signer controller is reflecting CSR approval status into network operator
status during signing process, but when CSR is removed it's not getting network
status back into original state because CSR is no longer available. When CSR
signing reattempt happens, signer controller is not updating existing
CertificateFailed condition type, instead it tries to add another
CertificateFailed condition and leads to Duplicate value: "Failed" error, so
fixing it just by updating existing CertificateFailed condition.

Signed-off-by: Periyasamy Palanisamy <[email protected]>

Author: pperiyasamy

pkg/controller/signer/signer-controller.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"time"
 
 	cnoclient "github.com/openshift/cluster-network-operator/pkg/client"
 	"github.com/openshift/cluster-network-operator/pkg/controller/statusmanager"
@@ -88,6 +89,8 @@ func (r *ReconcileCSR) Reconcile(ctx context.Context, request reconcile.Request)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			// Request object not found, could have been deleted after reconcile request.
+			// restore network status when CSR is deleted.
+			r.status.SetNotDegraded(statusmanager.CertificateSigner)
 			// Return and don't requeue as the CSR has been deleted.
 			return reconcile.Result{}, nil
 		}
@@ -227,7 +230,7 @@ func signerFailure(r *ReconcileCSR, csr *csrv1.CertificateSigningRequest, reason
 
 // Update the status conditions on the CSR object
 func updateCSRStatusConditions(r *ReconcileCSR, csr *csrv1.CertificateSigningRequest, reason string, message string) {
-	csr.Status.Conditions = append(csr.Status.Conditions, csrv1.CertificateSigningRequestCondition{
+	setCertificateSigningRequestCondition(&csr.Status.Conditions, csrv1.CertificateSigningRequestCondition{
 		Type:    csrv1.CertificateFailed,
 		Status:  "True",
 		Reason:  reason,
@@ -240,3 +243,26 @@ func updateCSRStatusConditions(r *ReconcileCSR, csr *csrv1.CertificateSigningReq
 			fmt.Sprintf("Unable to update csr: %v", err))
 	}
 }
+
+func setCertificateSigningRequestCondition(conditions *[]csrv1.CertificateSigningRequestCondition, newCondition csrv1.CertificateSigningRequestCondition) {
+	if conditions == nil {
+		conditions = &[]csrv1.CertificateSigningRequestCondition{}
+	}
+	var existingCondition *csrv1.CertificateSigningRequestCondition
+	for i := range *conditions {
+		if (*conditions)[i].Type == newCondition.Type {
+			existingCondition = &(*conditions)[i]
+		}
+	}
+	if existingCondition == nil {
+		newCondition.LastTransitionTime = metav1.NewTime(time.Now())
+		*conditions = append(*conditions, newCondition)
+		return
+	}
+	if existingCondition.Status != newCondition.Status {
+		existingCondition.Status = newCondition.Status
+		existingCondition.LastTransitionTime = metav1.NewTime(time.Now())
+	}
+	existingCondition.Reason = newCondition.Reason
+	existingCondition.Message = newCondition.Message
+}

pkg/controller/signer/signer_test.go

@@ -0,0 +1,214 @@
+package signer
+
+import (
+	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"encoding/pem"
+	"fmt"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	configv1 "github.com/openshift/api/config/v1"
+	operv1 "github.com/openshift/api/operator/v1"
+	cnoclient "github.com/openshift/cluster-network-operator/pkg/client"
+	"github.com/openshift/cluster-network-operator/pkg/client/fake"
+	"github.com/openshift/cluster-network-operator/pkg/controller/statusmanager"
+	"github.com/openshift/cluster-network-operator/pkg/names"
+	certificatev1 "k8s.io/api/certificates/v1"
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+)
+
+var (
+	csrName  = "ipsec-csr"
+	nodeName = "testnode"
+	coName   = "testing"
+)
+
+//nolint:errcheck
+func init() {
+	certificatev1.AddToScheme(scheme.Scheme)
+	corev1.AddToScheme(scheme.Scheme)
+}
+
+func TestSigner_DegradedCondition(t *testing.T) {
+	g := NewGomegaWithT(t)
+	client := fake.NewFakeClient()
+	status := statusmanager.New(client, coName, names.StandAloneClusterName)
+	signer := ReconcileCSR{client: client, status: status}
+
+	co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: coName}}
+	setCO(t, client, co)
+	no := &operv1.Network{ObjectMeta: metav1.ObjectMeta{Name: names.OPERATOR_CONFIG}}
+	setOC(t, client, no)
+
+	csr, err := generateCSR()
+	g.Expect(err).NotTo(HaveOccurred())
+	csrObj := &certificatev1.CertificateSigningRequest{}
+	csrObj.Name = csrName
+	csrObj.Spec.Request = []byte(csr)
+	csrObj.Spec.SignerName = signerName
+	csrObj.Spec.Usages = []certificatev1.KeyUsage{"ipsec tunnel"}
+	csrObj.Spec.Username = fmt.Sprintf("system:ovn-node:%s", nodeName)
+	csrObj.Status.Conditions = append(csrObj.Status.Conditions, certificatev1.CertificateSigningRequestCondition{
+		Type:    certificatev1.CertificateApproved,
+		Status:  "True",
+		Reason:  "AutoApproved",
+		Message: "Automatically approved by " + signerName})
+
+	err = client.Default().CRClient().Create(context.TODO(), csrObj)
+	g.Expect(err).NotTo(HaveOccurred())
+	_, err = client.Default().Kubernetes().CertificatesV1().CertificateSigningRequests().Create(context.TODO(), csrObj, metav1.CreateOptions{})
+	g.Expect(err).NotTo(HaveOccurred())
+
+	node := &corev1.Node{}
+	node.Name = nodeName
+	_, err = client.Default().Kubernetes().CoreV1().Nodes().Create(context.TODO(), node, metav1.CreateOptions{})
+	g.Expect(err).NotTo(HaveOccurred())
+
+	randomByteArray := []byte("blahblahblah")
+	caSecret := &corev1.Secret{}
+	caSecret.Name = "signer-ca"
+	caSecret.Namespace = "openshift-ovn-kubernetes"
+	caSecret.Data = make(map[string][]byte)
+	caSecret.Data["tls.crt"] = randomByteArray
+	caSecret.Data["tls.key"] = randomByteArray
+	err = client.Default().CRClient().Create(context.TODO(), caSecret)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	for range 3 {
+		_, err = signer.Reconcile(context.TODO(),
+			reconcile.Request{NamespacedName: types.NamespacedName{Name: csrName}})
+		g.Expect(err).NotTo(HaveOccurred())
+
+		err = client.Default().CRClient().Get(context.TODO(), types.NamespacedName{Name: csrName}, csrObj)
+		g.Expect(err).NotTo(HaveOccurred())
+		g.Expect(csrObj.Status.Certificate).Should(BeEmpty())
+
+		co, _, err = getStatuses(client, "testing")
+		if err != nil {
+			t.Fatalf("error getting network.operator: %v", err)
+		}
+		g.Expect(err).NotTo(HaveOccurred())
+		g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
+			{
+				Type:   configv1.OperatorDegraded,
+				Status: configv1.ConditionTrue,
+			},
+		})).To(BeTrue())
+		g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
+			{
+				Type:   configv1.OperatorUpgradeable,
+				Status: configv1.ConditionTrue,
+			},
+		})).To(BeTrue())
+	}
+
+	err = client.Default().CRClient().Delete(context.TODO(), csrObj)
+	g.Expect(err).NotTo(HaveOccurred())
+	_, err = signer.Reconcile(context.TODO(),
+		reconcile.Request{NamespacedName: types.NamespacedName{Name: csrName}})
+	g.Expect(err).NotTo(HaveOccurred())
+
+	co, _, err = getStatuses(client, "testing")
+	if err != nil {
+		t.Fatalf("error getting network.operator: %v", err)
+	}
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
+		{
+			Type:   configv1.OperatorDegraded,
+			Status: configv1.ConditionFalse,
+		},
+	})).To(BeTrue())
+	g.Expect(conditionsInclude(co.Status.Conditions, []configv1.ClusterOperatorStatusCondition{
+		{
+			Type:   configv1.OperatorUpgradeable,
+			Status: configv1.ConditionTrue,
+		},
+	})).To(BeTrue())
+
+}
+
+func generateCSR() (string, error) {
+	// Create private key.
+	csrKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		return "", fmt.Errorf("failed to generate private key: %v", err)
+	}
+	// Create CSR with private key.
+	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{}, csrKey)
+	if err != nil {
+		return "", err
+	}
+	// Encode CSR in PEM format.
+	csrPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes})
+	if csrPEM == nil {
+		return "", fmt.Errorf("failed to encode CSR in PEM format")
+	}
+	return string(csrPEM), nil
+}
+
+func setOC(t *testing.T, client cnoclient.Client, oc *operv1.Network) {
+	t.Helper()
+	g := NewGomegaWithT(t)
+	_, err := client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Update(context.TODO(), oc, metav1.UpdateOptions{})
+	if apierrors.IsNotFound(err) {
+		_, err = client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Create(context.TODO(), oc, metav1.CreateOptions{})
+	}
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
+func setCO(t *testing.T, client cnoclient.Client, co *configv1.ClusterOperator) {
+	t.Helper()
+	g := NewGomegaWithT(t)
+	err := client.Default().CRClient().Update(context.TODO(), co)
+	if apierrors.IsNotFound(err) {
+		err = client.Default().CRClient().Create(context.TODO(), co)
+	}
+	g.Expect(err).NotTo(HaveOccurred())
+}
+
+func getStatuses(client cnoclient.Client, name string) (*configv1.ClusterOperator, *operv1.Network, error) {
+	co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: name}}
+	err := client.ClientFor("").CRClient().Get(context.TODO(), types.NamespacedName{Name: name}, co)
+	if err != nil {
+		return nil, nil, err
+	}
+	oc, err := client.Default().OpenshiftOperatorClient().OperatorV1().Networks().Get(context.TODO(), names.OPERATOR_CONFIG, metav1.GetOptions{})
+	return co, oc, err
+}
+
+// Tests that the parts of newConditions that are set match what's in oldConditions (but
+// doesn't look at anything else in oldConditions)
+func conditionsInclude(oldConditions, newConditions []configv1.ClusterOperatorStatusCondition) bool {
+	for _, newCondition := range newConditions {
+		foundMatchingCondition := false
+
+		for _, oldCondition := range oldConditions {
+			if newCondition.Type != oldCondition.Type || newCondition.Status != oldCondition.Status {
+				continue
+			}
+			if newCondition.Reason != "" && newCondition.Reason != oldCondition.Reason {
+				return false
+			}
+			if newCondition.Message != "" && newCondition.Message != oldCondition.Message {
+				return false
+			}
+			foundMatchingCondition = true
+			break
+		}
+
+		if !foundMatchingCondition {
+			return false
+		}
+	}
+
+	return true
+}