Skip to content

CNTRLPLANE-945: oidc: fix test issues #30040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 11, 2025
Merged

CNTRLPLANE-945: oidc: fix test issues #30040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
489cf9d
oidc: extend keycloak availability check timeout
everettraven Jul 31, 2025
856479f
keycloakclient: fix user creation so that authentication as the creat…
everettraven Jul 31, 2025
e920064
oidc: only wait for KAS rollout if we actually modified the Authentic…
everettraven Aug 4, 2025
b1ee547
oidc: only remove keycloak resources after switching back from OIDC a…
everettraven Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions test/extended/authentication/keycloak_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (kc *keycloakClient) CreateUser(username, password string, groups ...string
Groups: groups,
Credentials: []credential{
{
Temporary: true,
Temporary: false,
Copy link
Contributor Author

@everettraven everettraven Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing locally, this looked like it was causing issues where the users we created were considered to be "incomplete" because they needed to set a real password.

Type: credentialTypePassword,
Value: password,
},
Expand All @@ -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 {
Expand All @@ -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

Expand Down
4 changes: 3 additions & 1 deletion test/extended/authentication/keycloak_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand All @@ -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
})

Expand Down
45 changes: 30 additions & 15 deletions test/extended/authentication/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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")
})
})

Expand Down Expand Up @@ -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()
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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")
}