Merge pull request #30040 from everettraven/oidc/fix-testing

CNTRLPLANE-945: oidc: fix test issues

Author: openshift-merge-bot[bot]

test/extended/authentication/keycloak_client.go

@@ -104,7 +104,7 @@ func (kc *keycloakClient) CreateUser(username, password string, groups ...string
 		Groups:        groups,
 		Credentials: []credential{
 			{
-				Temporary: true,
+				Temporary: false,
 				Type:      credentialTypePassword,
 				Value:     password,
 			},
@@ -131,8 +131,10 @@ func (kc *keycloakClient) CreateUser(username, password string, groups ...string
 }
 
 type authenticationResponse struct {
-	AccessToken string `json:"access_token"`
-	IDToken     string `json:"id_token"`
+	AccessToken      string `json:"access_token"`
+	IDToken          string `json:"id_token"`
+	Error            string `json:"error,omitempty"`
+	ErrorDescription string `json:"error_description,omitempty"`
 }
 
 func (kc *keycloakClient) Authenticate(clientID, username, password string) error {
@@ -159,6 +161,10 @@ func (kc *keycloakClient) Authenticate(clientID, username, password string) erro
 		return fmt.Errorf("unmarshalling response data: %w", err)
 	}
 
+	if respBody.Error != "" {
+		return fmt.Errorf("%s: %s", respBody.Error, respBody.ErrorDescription)
+	}
+
 	kc.accessToken = respBody.AccessToken
 	kc.idToken = respBody.IDToken
 

test/extended/authentication/keycloak_helpers.go

@@ -351,7 +351,7 @@ func createKeycloakRoute(ctx context.Context, service *corev1.Service, client ty
 }
 
 func waitForKeycloakAvailable(ctx context.Context, client *exutil.CLI, namespace string) error {
-	timeoutCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute))
+	timeoutCtx, cancel := context.WithDeadline(ctx, time.Now().Add(10*time.Minute))
 	defer cancel()
 	err := wait.PollUntilContextCancel(timeoutCtx, 10*time.Second, true, func(ctx context.Context) (done bool, err error) {
 		deploy, err := client.AdminKubeClient().AppsV1().Deployments(namespace).Get(ctx, keycloakResourceName, metav1.GetOptions{})
@@ -365,6 +365,8 @@ func waitForKeycloakAvailable(ctx context.Context, client *exutil.CLI, namespace
 			}
 		}
 
+		fmt.Println("keycloak deployment is not yet available. status: ", deploy.Status)
+
 		return false, nil
 	})
 

test/extended/authentication/oidc.go

@@ -15,6 +15,7 @@ import (
 	exutil "github.com/openshift/origin/test/extended/util"
 	authnv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/errors"
 
@@ -150,7 +151,8 @@ var _ = g.Describe("[sig-auth][Suite:openshift/auth/external-oidc][Serial][Slow]
 					gomega.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error authenticating as keycloak user")
 
 					copiedOC := *oc
-					tokenOC := copiedOC.WithToken(keycloakCli.AccessToken())
+					token := keycloakCli.AccessToken()
+					tokenOC := copiedOC.WithToken(token)
 					ssr, err := tokenOC.KubeClient().AuthenticationV1().SelfSubjectReviews().Create(ctx, &authnv1.SelfSubjectReview{
 						ObjectMeta: metav1.ObjectMeta{
 							Name: fmt.Sprintf("%s-info", username),
@@ -183,10 +185,12 @@ var _ = g.Describe("[sig-auth][Suite:openshift/auth/external-oidc][Serial][Slow]
 					gomega.Expect(err).NotTo(o.HaveOccurred(), "should be able to create a SelfSubjectReview")
 				}).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed())
 
-				err := resetAuthentication(ctx, oc, originalAuth)
+				err, modified := resetAuthentication(ctx, oc, originalAuth)
 				o.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error reverting authentication to original state")
 
-				waitForRollout(ctx, oc)
+				if modified {
+					waitForRollout(ctx, oc)
+				}
 			})
 
 			g.It("should rollout configuration on the kube-apiserver successfully", func() {
@@ -360,13 +364,16 @@ var _ = g.Describe("[sig-auth][Suite:openshift/auth/external-oidc][Serial][Slow]
 	})
 
 	g.AfterAll(func() {
-		err := removeResources(ctx, cleanups...)
-		o.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error cleaning up keycloak resources")
-
-		err = resetAuthentication(ctx, oc, originalAuth)
+		err, modified := resetAuthentication(ctx, oc, originalAuth)
 		o.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error reverting authentication to original state")
 
-		waitForRollout(ctx, oc)
+		// Only if we modified the Authentication resource during the reset should we wait for a rollout
+		if modified {
+			waitForRollout(ctx, oc)
+		}
+
+		err = removeResources(ctx, cleanups...)
+		o.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error cleaning up keycloak resources")
 	})
 })
 
@@ -477,11 +484,12 @@ func admittedURLForRoute(ctx context.Context, client *exutil.CLI, routeName, nam
 	return fmt.Sprintf("https://%s", admittedURL), err
 }
 
-func resetAuthentication(ctx context.Context, client *exutil.CLI, original *configv1.Authentication) error {
+func resetAuthentication(ctx context.Context, client *exutil.CLI, original *configv1.Authentication) (error, bool) {
 	if original == nil {
-		return nil
+		return nil, false
 	}
 
+	modified := false
 	timeoutCtx, cancel := context.WithDeadline(ctx, time.Now().Add(5*time.Minute))
 	defer cancel()
 	cli := client.AdminConfigClient().ConfigV1().Authentications()
@@ -491,17 +499,24 @@ func resetAuthentication(ctx context.Context, client *exutil.CLI, original *conf
 			return false, fmt.Errorf("getting the current authentications.config.openshift.io/cluster: %w", err)
 		}
 
+		if equality.Semantic.DeepEqual(current.Spec, original.Spec) {
+			return true, nil
+		}
+
 		current.Spec = original.Spec
+		modified = true
 
 		_, err = cli.Update(ctx, current, metav1.UpdateOptions{})
 		if err != nil {
-			return false, err
+			// Only log the error so we continue to retry until the context has timed out
+			fmt.Println("updating authentication resource:", err)
+			return false, nil
 		}
 
 		return true, nil
 	})
 
-	return err
+	return err, modified
 }
 
 func waitForRollout(ctx context.Context, client *exutil.CLI) {
@@ -523,8 +538,8 @@ func waitForRollout(ctx context.Context, client *exutil.CLI) {
 		}
 
 		gomega.Expect(found).To(o.BeTrue(), "should have found the NodeInstallerProgressing condition")
-		gomega.Expect(nipCond.Status).To(o.Equal(operatorv1.ConditionTrue), "NodeInstallerProgressing condition should be True")
-	}).WithTimeout(5*time.Minute).WithPolling(10*time.Second).Should(o.Succeed(), "should eventually begin rolling out a new revision")
+		gomega.Expect(nipCond.Status).To(o.Equal(operatorv1.ConditionTrue), "NodeInstallerProgressing condition should be True", nipCond)
+	}).WithTimeout(10*time.Minute).WithPolling(20*time.Second).Should(o.Succeed(), "should eventually begin rolling out a new revision")
 
 	// Then wait for it to flip back
 	o.Eventually(func(gomega o.Gomega) {
@@ -542,6 +557,6 @@ func waitForRollout(ctx context.Context, client *exutil.CLI) {
 		}
 
 		gomega.Expect(found).To(o.BeTrue(), "should have found the NodeInstallerProgressing condition")
-		gomega.Expect(nipCond.Status).To(o.Equal(operatorv1.ConditionFalse), "NodeInstallerProgressing condition should be True")
+		gomega.Expect(nipCond.Status).To(o.Equal(operatorv1.ConditionFalse), "NodeInstallerProgressing condition should be False", nipCond)
 	}).WithTimeout(30*time.Minute).WithPolling(30*time.Second).Should(o.Succeed(), "should eventually rollout out a new revision successfully")
 }