Repository: argoproj-labs/argocd-operator
Pull request #681 (2022-06-15 14:30:12)

fix: nil pointer error for keycloak on openshift
**What type of PR is this?**
> Uncomment only one ` /kind` line, and delete the rest.
> For example, `> /kind bug` would simply become: `/kind bug`

 /kind bug
> /kind chore
> /kind cleanup
> /kind failing-test
> /kind enhancement
> /kind documentation
> /kind code-refactoring


**What does this PR do / why we need it**:
adds check for nil pointer error when generating keycloak conflig for openshift 

**Have you updated the necessary documentation?**

* [x] Documentation update is required by this PR.
* [x] Documentation has been updated.

**Which issue(s) this PR fixes**:

Fixes #?

**How to test changes / Special notes to the reviewer**:



Patch:
diff --git a/controllers/argocd/keycloak.go b/controllers/argocd/keycloak.go
index e5bad7e8..7effa186 100644
--- a/controllers/argocd/keycloak.go
+++ b/controllers/argocd/keycloak.go
@@ -819,7 +819,7 @@ func (r *ReconcileArgoCD) prepareKeycloakConfig(cr *argoprojv1a1.ArgoCD) (*keycl
 	}
 
 	// By default TLS Verification should be enabled.
-	if cr.Spec.SSO.VerifyTLS == nil || *cr.Spec.SSO.VerifyTLS || cr.Spec.SSO.Keycloak.VerifyTLS == nil || *cr.Spec.SSO.Keycloak.VerifyTLS {
+	if (cr.Spec.SSO.VerifyTLS == nil || *cr.Spec.SSO.VerifyTLS) && (cr.Spec.SSO.Keycloak == nil || (cr.Spec.SSO.Keycloak.VerifyTLS == nil || *cr.Spec.SSO.Keycloak.VerifyTLS)) {
 		tlsVerification = true
 	}
 
diff --git a/controllers/argocd/sso.go b/controllers/argocd/sso.go
index 543eafa5..e598cdf3 100644
--- a/controllers/argocd/sso.go
+++ b/controllers/argocd/sso.go
@@ -143,7 +143,7 @@ func (r *ReconcileArgoCD) reconcileSSO(cr *argoprojv1a1.ArgoCD) error {
 			} else if cr.Spec.Dex != nil && (cr.Spec.Dex.Image != "" || cr.Spec.Dex.Config != "" || cr.Spec.Dex.Resources != nil || len(cr.Spec.Dex.Groups) != 0 ||
 				cr.Spec.Dex.Version != "" || cr.Spec.Dex.OpenShiftOAuth != cr.Spec.SSO.Dex.OpenShiftOAuth) {
 				// old dex spec fields are expressed when `.spec.sso.provider` is set to dex instead of using new `.spec.sso.dex` ==> conflict
-				errMsg = "cannot specify .spec.Dex fields when dex is configured through .spec.sso"
+				errMsg = "cannot specify .spec.Dex fields when dex is configured through .spec.sso.dex"
 				isError = true
 			} else if cr.Spec.SSO.Image != "" || cr.Spec.SSO.Version != "" || cr.Spec.SSO.VerifyTLS != nil || cr.Spec.SSO.Resources != nil {
 				// old keycloak spec fields expressed when `.spec.sso.provider` is set to dex ==> conflict
@@ -164,13 +164,10 @@ func (r *ReconcileArgoCD) reconcileSSO(cr *argoprojv1a1.ArgoCD) error {
 		if cr.Spec.SSO.Provider == v1alpha1.SSOProviderTypeKeycloak {
 			// Relevant SSO settings at play are `DISABLE_DEX`, `.spec.dex`, `.spec.sso` fields, `.spec.sso.keycloak`, `.spec.sso.dex`
 
-			if (cr.Spec.SSO.Keycloak != nil) && ((cr.Spec.SSO.Image != "" && cr.Spec.SSO.Keycloak.Image != "" && cr.Spec.SSO.Image != cr.Spec.SSO.Keycloak.Image) ||
-				(cr.Spec.SSO.Version != "" && cr.Spec.SSO.Keycloak.Version != "" && cr.Spec.SSO.Version != cr.Spec.SSO.Keycloak.Version) ||
-				(cr.Spec.SSO.VerifyTLS != nil && cr.Spec.SSO.Keycloak.VerifyTLS != nil && cr.Spec.SSO.VerifyTLS != cr.Spec.SSO.Keycloak.VerifyTLS) ||
-				(cr.Spec.SSO.Resources != nil && cr.Spec.SSO.Keycloak.Resources != nil && cr.Spec.SSO.Resources != cr.Spec.SSO.Keycloak.Resources)) {
+			if (cr.Spec.SSO.Keycloak != nil) && (cr.Spec.SSO.Image != "" || cr.Spec.SSO.Version != "" || cr.Spec.SSO.Resources != nil || cr.Spec.SSO.VerifyTLS != cr.Spec.SSO.Keycloak.VerifyTLS) {
 				// Keycloak specs expressed both in old `.spec.sso` fields as well as in `.spec.sso.keycloak` simultaneously and they don't match
 				// ==> conflict
-				errMsg = "cannot supply conflicting configuration in .spec.sso when keycloak is configured through .spec.sso.keycloak"
+				errMsg = "cannot specify keycloak fields in .spec.sso when keycloak is configured through .spec.sso.keycloak"
 				err = errors.New(illegalSSOConfiguration + errMsg)
 				isError = true
 			} else if cr.Spec.SSO.Dex != nil {


Test patch:
diff --git a/controllers/argocd/keycloak_test.go b/controllers/argocd/keycloak_test.go
index a8fa6a23..2341ab3b 100644
--- a/controllers/argocd/keycloak_test.go
+++ b/controllers/argocd/keycloak_test.go
@@ -16,6 +16,7 @@ package argocd
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"testing"
 
@@ -404,6 +405,122 @@ func TestKeycloak_testServerCert(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestKeycloakConfigVerifyTLSForOpenShift(t *testing.T) {
+	tests := []struct {
+		name             string
+		argoCD           *v1alpha1.ArgoCD
+		desiredVerifyTLS bool
+	}{
+		{
+			name: ".spec.sso.verifyTLS & .spec.sso.keycloak.verifyTLS both nil",
+			argoCD: makeTestArgoCD(func(ac *argoprojv1alpha1.ArgoCD) {
+				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
+					Provider: argoappv1.SSOProviderTypeKeycloak,
+				}
+			}),
+			desiredVerifyTLS: true,
+		},
+		{
+			name: ".spec.sso.verifyTLS nil, .spec.sso.keycloak.verifyTLS false",
+			argoCD: makeTestArgoCD(func(ac *argoprojv1alpha1.ArgoCD) {
+				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
+					Provider: argoappv1.SSOProviderTypeKeycloak,
+					Keycloak: &v1alpha1.ArgoCDKeycloakSpec{
+						VerifyTLS: boolPtr(false),
+					},
+				}
+			}),
+			desiredVerifyTLS: false,
+		},
+		{
+			name: ".spec.sso.verifyTLS false, .spec.sso.keycloak.verifyTLS nil",
+			argoCD: makeTestArgoCD(func(ac *argoprojv1alpha1.ArgoCD) {
+				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
+					Provider:  argoappv1.SSOProviderTypeKeycloak,
+					VerifyTLS: boolPtr(false),
+				}
+			}),
+			desiredVerifyTLS: false,
+		},
+		{
+			name: ".spec.sso.verifyTLS nil, .spec.sso.keycloak.verifyTLS true",
+			argoCD: makeTestArgoCD(func(ac *argoprojv1alpha1.ArgoCD) {
+				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
+					Provider: argoappv1.SSOProviderTypeKeycloak,
+					Keycloak: &v1alpha1.ArgoCDKeycloakSpec{
+						VerifyTLS: boolPtr(true),
+					},
+				}
+			}),
+			desiredVerifyTLS: true,
+		},
+		{
+			name: ".spec.sso.verifyTLS true, .spec.sso.keycloak.verifyTLS nil",
+			argoCD: makeTestArgoCD(func(ac *argoprojv1alpha1.ArgoCD) {
+				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
+					Provider:  argoappv1.SSOProviderTypeKeycloak,
+					VerifyTLS: boolPtr(true),
+				}
+			}),
+			desiredVerifyTLS: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+
+			r := makeFakeReconciler(t, test.argoCD)
+
+			keycloakRoute := &routev1.Route{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      defaultKeycloakIdentifier,
+					Namespace: test.argoCD.Namespace,
+				},
+				Spec: routev1.RouteSpec{
+					Host: "test-host",
+				},
+			}
+			r.Client.Create(context.TODO(), keycloakRoute)
+
+			argoCDRoute := &routev1.Route{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fmt.Sprintf("%s-%s", test.argoCD.Name, "server"),
+					Namespace: test.argoCD.Namespace,
+				},
+				Spec: routev1.RouteSpec{
+					Host: "test-argocd-host",
+				},
+			}
+			r.Client.Create(context.TODO(), argoCDRoute)
+
+			keycloakSecret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fmt.Sprintf("%s-%s", defaultKeycloakIdentifier, "secret"),
+					Namespace: test.argoCD.Namespace,
+				},
+				Data: map[string][]byte{"SSO_USERNAME": []byte("username"), "SSO_PASSWORD": []byte("password")},
+			}
+			r.Client.Create(context.TODO(), keycloakSecret)
+
+			sslCertsSecret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      servingCertSecretName,
+					Namespace: test.argoCD.Namespace,
+				},
+				Data: map[string][]byte{
+					"tls.crt": []byte("asdasfsff"),
+				},
+			}
+			r.Client.Create(context.TODO(), sslCertsSecret)
+
+			keyCloakConfig, err := r.prepareKeycloakConfig(test.argoCD)
+			assert.NoError(t, err)
+
+			assert.Equal(t, test.desiredVerifyTLS, keyCloakConfig.VerifyTLS)
+		})
+	}
+}
+
 func TestKeycloak_NodeLabelSelector(t *testing.T) {
 	a := makeTestArgoCDForKeycloak()
 	a.Spec.NodePlacement = &argoappv1.ArgoCDNodePlacementSpec{
diff --git a/controllers/argocd/sso_test.go b/controllers/argocd/sso_test.go
index 9dec4040..37972eb7 100644
--- a/controllers/argocd/sso_test.go
+++ b/controllers/argocd/sso_test.go
@@ -142,7 +142,7 @@ func TestReconcile_illegalSSOConfiguration(t *testing.T) {
 			setEnvVarFunc: nil,
 			envVar:        "",
 			wantErr:       true,
-			Err:           errors.New("illegal SSO configuration: cannot specify .spec.Dex fields when dex is configured through .spec.sso"),
+			Err:           errors.New("illegal SSO configuration: cannot specify .spec.Dex fields when dex is configured through .spec.sso.dex"),
 		},
 		{
 			name: "sso provider dex but no .spec.sso.dex provided",
@@ -244,9 +244,7 @@ func TestReconcile_illegalSSOConfiguration(t *testing.T) {
 				ac.Spec.SSO = &v1alpha1.ArgoCDSSOSpec{
 					Provider: argov1alpha1.SSOProviderTypeKeycloak,
 					Image:    "test-image",
-					Version:  "test-image-version",
 					Keycloak: &v1alpha1.ArgoCDKeycloakSpec{
-						Image:   "test-image-2",
 						Version: "test-image-version-2",
 					},
 				}
@@ -254,7 +252,7 @@ func TestReconcile_illegalSSOConfiguration(t *testing.T) {
 			setEnvVarFunc: nil,
 			envVar:        "",
 			wantErr:       true,
-			Err:           errors.New("illegal SSO configuration: cannot supply conflicting configuration in .spec.sso when keycloak is configured through .spec.sso.keycloak"),
+			Err:           errors.New("illegal SSO configuration: cannot specify keycloak fields in .spec.sso when keycloak is configured through .spec.sso.keycloak"),
 		},
 		{
 			name: "sso provider keycloak + `.spec.sso.dex`",