Merge pull request #2090 from ardaguclu/ocpbugs-52936

OCPBUGS-52936: oc adm policy: Only initialize UserClient if built-in OAuth is enabled

Author: openshift-merge-bot[bot]

pkg/cli/admin/policy/modify_roles.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"strings"
 
+	userv1 "github.com/openshift/api/user/v1"
 	ocmdhelpers "github.com/openshift/oc/pkg/helpers/cmd"
 	"github.com/spf13/cobra"
 	"k8s.io/client-go/discovery"
@@ -345,19 +346,40 @@ func (o *RoleModificationOptions) innerComplete(f kcmdutil.Factory, cmd *cobra.C
 	if err != nil {
 		return err
 	}
-	o.RbacClient, err = rbacv1client.NewForConfig(clientConfig)
+	o.DiscoveryClient, err = f.ToDiscoveryClient()
 	if err != nil {
 		return err
 	}
-	o.UserClient, err = userv1client.NewForConfig(clientConfig)
+	o.RbacClient, err = rbacv1client.NewForConfig(clientConfig)
 	if err != nil {
 		return err
 	}
-	o.ServiceAccountClient, err = corev1client.NewForConfig(clientConfig)
+
+	userAPIAvailable := false
+	groupList, err := o.DiscoveryClient.ServerGroups()
+	if discovery.IsGroupDiscoveryFailedError(err) {
+		// proceed with partial results; treat missing groups as "not found"
+		err = nil
+	}
 	if err != nil {
 		return err
 	}
-	o.DiscoveryClient, err = f.ToDiscoveryClient()
+	for _, group := range groupList.Groups {
+		// We try to determine that built-in OAuth is enabled or not to initialize UserClient.
+		// Because we don't need UserClient, if external OIDC is used.
+		// Simply checking the existence of userv1 in the discovery can signal us whether built-in OAuth is functioning or not.
+		if group.PreferredVersion.GroupVersion == userv1.GroupVersion.String() {
+			userAPIAvailable = true
+			break
+		}
+	}
+	if userAPIAvailable {
+		o.UserClient, err = userv1client.NewForConfig(clientConfig)
+		if err != nil {
+			return err
+		}
+	}
+	o.ServiceAccountClient, err = corev1client.NewForConfig(clientConfig)
 	if err != nil {
 		return err
 	}

pkg/cli/admin/policy/modify_roles_test.go

@@ -6,16 +6,21 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	diffutil "k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
+	"k8s.io/cli-runtime/pkg/genericiooptions"
 	"k8s.io/cli-runtime/pkg/printers"
 	fakeclient "k8s.io/client-go/kubernetes/fake"
 	rbacv1client "k8s.io/client-go/kubernetes/typed/rbac/v1"
+	cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
+	kcmdutil "k8s.io/kubectl/pkg/cmd/util"
+	"k8s.io/utils/ptr"
 
 	userv1 "github.com/openshift/api/user/v1"
 	fakeuserclient "github.com/openshift/client-go/user/clientset/versioned/fake"
@@ -589,6 +594,87 @@ func TestModifyNamedLocalRoleBinding(t *testing.T) {
 		modifyRoleAndCheck(t, o, tcName, tc.action, tc.expectedRoleBindingName, tc.expectedSubjects, tc.expectedRoleBindingList)
 	}
 }
+
+func TestRoleModificationCompleteNonExistUser(t *testing.T) {
+	ioStreams, _, _, _ := genericiooptions.NewTestIOStreams()
+
+	dc := cmdtesting.NewFakeCachedDiscoveryClient()
+	dc.Groups = []*metav1.APIGroup{
+		{
+			Name: "",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "v1",
+			},
+		},
+		{
+			Name: "foo",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "foo/v1beta1",
+			},
+		},
+		{
+			Name: "bar",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "bar/v1",
+			},
+		},
+	}
+
+	tf := cmdtesting.NewTestFactory().WithNamespace("test").WithDiscoveryClient(dc)
+	defer tf.Cleanup()
+
+	opt := NewRoleModificationOptions(ioStreams)
+	cmd := &cobra.Command{}
+	kcmdutil.AddDryRunFlag(cmd)
+	err := opt.Complete(tf, cmd, []string{"test", "test"}, ptr.To([]string{}), "")
+	if err != nil {
+		t.Fatalf("unexpected error: %s", err)
+	}
+	if opt.UserClient != nil {
+		t.Fatal("expected UserClient to be nil")
+	}
+}
+
+func TestRoleModificationCompleteExistUser(t *testing.T) {
+	ioStreams, _, _, _ := genericiooptions.NewTestIOStreams()
+
+	dc := cmdtesting.NewFakeCachedDiscoveryClient()
+	dc.Groups = []*metav1.APIGroup{
+		{
+			Name: "",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "v1",
+			},
+		},
+		{
+			Name: "foo",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "foo/v1beta1",
+			},
+		},
+		{
+			Name: "user.openshift.io",
+			PreferredVersion: metav1.GroupVersionForDiscovery{
+				GroupVersion: "user.openshift.io/v1",
+			},
+		},
+	}
+
+	tf := cmdtesting.NewTestFactory().WithNamespace("test").WithDiscoveryClient(dc)
+	defer tf.Cleanup()
+
+	opt := NewRoleModificationOptions(ioStreams)
+	cmd := &cobra.Command{}
+	kcmdutil.AddDryRunFlag(cmd)
+	err := opt.Complete(tf, cmd, []string{"test", "test"}, ptr.To([]string{}), "")
+	if err != nil {
+		t.Fatalf("unexpected error: %s", err)
+	}
+	if opt.UserClient == nil {
+		t.Fatal("expected UserClient not to be nil")
+	}
+}
+
 func TestModifyRoleBindingWarnings(t *testing.T) {
 	type clusterState struct {
 		roles               *rbacv1.RoleList