authz rbac fixes

Signed-off-by: Tommy Hughes <tohughes@redhat.com>
feast-dev · dmartinol · Dec 2, 2024 · Nov 19, 2024 · Dec 1, 2024 · Dec 1, 2024
commit d4c322947a98cb92bf6489f455eb4bc5ff11b2db
@@ -33,7 +33,7 @@ const (
 	OnlineStoreReadyType   = "OnlineStore"
 	RegistryReadyType      = "Registry"
 	ReadyType              = "FeatureStore"
-	AuthorizationReadyType = "AuthorizationReadyType"
+	AuthorizationReadyType = "Authorization"
 
 	// Feast condition reasons:
 	ReadyReason                 = "Ready"

@@ -65,6 +65,7 @@ rules:
 - apiGroups:
   - rbac.authorization.k8s.io
   resources:
+  - rolebindings
   - roles
   verbs:
   - create

@@ -2667,6 +2667,7 @@ rules:
 - apiGroups:
   - rbac.authorization.k8s.io
   resources:
+  - rolebindings
   - roles
   verbs:
   - create

@@ -17,35 +17,40 @@ import (
 
 // Deploy the feast authorization
 func (authz *FeastAuthorization) Deploy() error {
-	authzConfig := authz.Handler.FeatureStore.Status.Applied.AuthzConfig
-	if authzConfig != nil {
-		if authzConfig.KubernetesAuthz != nil {
-			if err := authz.deployKubernetesAuth(authzConfig.KubernetesAuthz); err != nil {
-				return err
-			}
-		} else {
-			authz.removeOrphanedRoles()
-			_ = authz.Handler.DeleteOwnedFeastObj(authz.initFeastRole())
-			_ = authz.Handler.DeleteOwnedFeastObj(authz.initFeastRoleBinding())
+	if authz.isKubernetesAuth() {
+		if err := authz.deployKubernetesAuth(); err != nil {
+			return err
 		}
+	} else {
+		authz.removeOrphanedRoles()
+		_ = authz.Handler.DeleteOwnedFeastObj(authz.initFeastRole())
+		_ = authz.Handler.DeleteOwnedFeastObj(authz.initFeastRoleBinding())
+		apimeta.RemoveStatusCondition(&authz.Handler.FeatureStore.Status.Conditions, feastKubernetesAuthConditions[metav1.ConditionTrue].Type)
 	}
 	return nil
 }
 
-func (authz *FeastAuthorization) deployKubernetesAuth(kubernetesAuth *feastdevv1alpha1.KubernetesAuthz) error {
-	authz.removeOrphanedRoles()
+func (authz *FeastAuthorization) isKubernetesAuth() bool {
+	authzConfig := authz.Handler.FeatureStore.Status.Applied.AuthzConfig
+	return authzConfig != nil && authzConfig.KubernetesAuthz != nil
+}
 
-	if err := authz.createFeastRole(); err != nil {
-		return authz.setFeastKubernetesAuthCondition(err)
-	}
-	if err := authz.createFeastRoleBinding(); err != nil {
-		return authz.setFeastKubernetesAuthCondition(err)
-	}
+func (authz *FeastAuthorization) deployKubernetesAuth() error {
+	if authz.isKubernetesAuth() {
+		authz.removeOrphanedRoles()
 
-	for _, roleName := range kubernetesAuth.Roles {
-		if err := authz.createAuthRole(roleName); err != nil {
+		if err := authz.createFeastRole(); err != nil {
+			return authz.setFeastKubernetesAuthCondition(err)
+		}
+		if err := authz.createFeastRoleBinding(); err != nil {
 			return authz.setFeastKubernetesAuthCondition(err)
 		}
+
+		for _, roleName := range authz.Handler.FeatureStore.Status.Applied.AuthzConfig.KubernetesAuthz.Roles {
+			if err := authz.createAuthRole(roleName); err != nil {
+				return authz.setFeastKubernetesAuthCondition(err)
+			}
+		}
 	}
 	return authz.setFeastKubernetesAuthCondition(nil)
 }
@@ -61,7 +66,7 @@ func (authz *FeastAuthorization) removeOrphanedRoles() {
 	}
 
 	desiredRoles := []string{}
-	if authz.Handler.FeatureStore.Status.Applied.AuthzConfig.KubernetesAuthz != nil {
+	if authz.isKubernetesAuth() {
 		desiredRoles = authz.Handler.FeatureStore.Status.Applied.AuthzConfig.KubernetesAuthz.Roles
 	}
 	for _, role := range roleList.Items {

@@ -57,7 +57,7 @@ type FeatureStoreReconciler struct {
 //+kubebuilder:rbac:groups=feast.dev,resources=featurestores/finalizers,verbs=update
 //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;create;update;watch;delete
 //+kubebuilder:rbac:groups=core,resources=services;configmaps;persistentvolumeclaims;serviceaccounts,verbs=get;list;create;update;watch;delete
-//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;create;update;watch;delete
+//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;create;update;watch;delete
 //+kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to

@@ -127,7 +127,7 @@ var _ = Describe("FeatureStore Controller-Ephemeral services", func() {
 			Expect(resource.Status.FeastVersion).To(Equal(feastversion.FeastVersion))
 			Expect(resource.Status.ClientConfigMap).To(Equal(feast.GetFeastServiceName(services.ClientFeastType)))
 			Expect(resource.Status.Applied.FeastProject).To(Equal(resource.Spec.FeastProject))
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 			Expect(resource.Status.Applied.Services).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore.Persistence).NotTo(BeNil())

@@ -122,7 +122,7 @@ var _ = Describe("FeatureStore Controller-Ephemeral services", func() {
 			Expect(resource.Status.FeastVersion).To(Equal(feastversion.FeastVersion))
 			Expect(resource.Status.ClientConfigMap).To(Equal(feast.GetFeastServiceName(services.ClientFeastType)))
 			Expect(resource.Status.Applied.FeastProject).To(Equal(resource.Spec.FeastProject))
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 			Expect(resource.Status.Applied.Services).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore).To(BeNil())
 			Expect(resource.Status.Applied.Services.OnlineStore).To(BeNil())

@@ -153,7 +153,7 @@ var _ = Describe("FeatureStore Controller-Ephemeral services", func() {
 			Expect(resource.Status.FeastVersion).To(Equal(feastversion.FeastVersion))
 			Expect(resource.Status.ClientConfigMap).To(Equal(feast.GetFeastServiceName(services.ClientFeastType)))
 			Expect(resource.Status.Applied.FeastProject).To(Equal(resource.Spec.FeastProject))
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 			Expect(resource.Status.Applied.Services).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore.Persistence).NotTo(BeNil())

@@ -134,7 +134,7 @@ var _ = Describe("FeatureStore Controller", func() {
 			Expect(resource.Status.ServiceHostnames.OnlineStore).To(BeEmpty())
 			Expect(resource.Status.ServiceHostnames.Registry).To(Equal(feast.GetFeastServiceName(services.RegistryFeastType) + "." + resource.Namespace + ".svc.cluster.local:80"))
 			Expect(resource.Status.Applied.FeastProject).To(Equal(resource.Spec.FeastProject))
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 			Expect(resource.Status.Applied.Services).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore).To(BeNil())
 			Expect(resource.Status.Applied.Services.OnlineStore).To(BeNil())
@@ -152,6 +152,8 @@ var _ = Describe("FeatureStore Controller", func() {
 			Expect(cond.Reason).To(Equal(feastdevv1alpha1.ReadyReason))
 			Expect(cond.Type).To(Equal(feastdevv1alpha1.ReadyType))
 			Expect(cond.Message).To(Equal(feastdevv1alpha1.ReadyMessage))
+			cond = apimeta.FindStatusCondition(resource.Status.Conditions, feastdevv1alpha1.AuthorizationReadyType)
+			Expect(cond).To(BeNil())
 
 			cond = apimeta.FindStatusCondition(resource.Status.Conditions, feastdevv1alpha1.RegistryReadyType)
 			Expect(cond).ToNot(BeNil())
@@ -459,7 +461,7 @@ var _ = Describe("FeatureStore Controller", func() {
 			Expect(resource.Status.FeastVersion).To(Equal(feastversion.FeastVersion))
 			Expect(resource.Status.ClientConfigMap).To(Equal(feast.GetFeastServiceName(services.ClientFeastType)))
 			Expect(resource.Status.Applied.FeastProject).To(Equal(resource.Spec.FeastProject))
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 			Expect(resource.Status.Applied.Services).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore).NotTo(BeNil())
 			Expect(resource.Status.Applied.Services.OfflineStore.Persistence).NotTo(BeNil())
@@ -962,6 +964,7 @@ var _ = Describe("FeatureStore Controller", func() {
 				},
 				Spec: feastdevv1alpha1.FeatureStoreSpec{
 					FeastProject: referencedRegistry.Spec.FeastProject,
+					AuthzConfig:  &feastdevv1alpha1.AuthzConfig{},
 					Services: &feastdevv1alpha1.FeatureStoreServices{
 						OnlineStore:  &feastdevv1alpha1.OnlineStore{},
 						OfflineStore: &feastdevv1alpha1.OfflineStore{},

@@ -248,16 +248,11 @@ func getClientRepoConfig(featureStore *feastdevv1alpha1.FeatureStore) RepoConfig
 		}
 	}
 
-	if status.Applied.AuthzConfig.KubernetesAuthz == nil {
-		clientRepoConfig.AuthzConfig = AuthzConfig{
-			Type: NoAuthAuthType,
-		}
-	} else {
-		if status.Applied.AuthzConfig.KubernetesAuthz != nil {
-			clientRepoConfig.AuthzConfig = AuthzConfig{
-				Type: KubernetesAuthType,
-			}
-		}
+	clientRepoConfig.AuthzConfig = AuthzConfig{
+		Type: NoAuthAuthType,
+	}
+	if status.Applied.AuthzConfig != nil && status.Applied.AuthzConfig.KubernetesAuthz != nil {
+		clientRepoConfig.AuthzConfig.Type = KubernetesAuthType
 	}
 	return clientRepoConfig
 }

@@ -62,10 +62,6 @@ func ApplyDefaultsToStatus(cr *feastdevv1alpha1.FeatureStore) {
 	cr.Status.FeastVersion = feastversion.FeastVersion
 	applied := cr.Spec.DeepCopy()
 
-	if applied.AuthzConfig == nil {
-		applied.AuthzConfig = &feastdevv1alpha1.AuthzConfig{}
-	}
-
 	if applied.Services == nil {
 		applied.Services = &feastdevv1alpha1.FeatureStoreServices{}
 	}

@@ -380,8 +380,7 @@ var _ = Describe("FeatureStore API", func() {
 		It("should set the default AuthzConfig", func() {
 			resource := featurestore
 			services.ApplyDefaultsToStatus(resource)
-			Expect(resource.Status.Applied.AuthzConfig).ToNot(BeNil())
-			Expect(resource.Status.Applied.AuthzConfig).To(Equal(&feastdevv1alpha1.AuthzConfig{}))
+			Expect(resource.Status.Applied.AuthzConfig).To(BeNil())
 		})
 	})
 })