diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index 0ab6a45f2..1aa4a380e 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -67,7 +67,7 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ if delResErr == nil { delResErr = err } - return r.reconcileStatus(ctx, ap, common.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) + return r.reconcileStatus(ctx, ap, targetNetworkObject, common.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -103,7 +103,7 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ specErr := r.reconcileResources(ctx, ap, targetNetworkObject) // reconcile authpolicy status - statusResult, statusErr := r.reconcileStatus(ctx, ap, specErr) + statusResult, statusErr := r.reconcileStatus(ctx, ap, targetNetworkObject, specErr) if specErr != nil { return ctrl.Result{}, specErr diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 582c23d65..c44d36018 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -310,58 +310,6 @@ var _ = Describe("AuthPolicy controller", func() { Expect(authConfig.Spec.Conditions[0].Any[0].Any[0].All[1].Value).To(Equal("/.*")) }) - It("Attaches policy to the Gateway while having other policies attached to all HTTPRoutes", func() { - routePolicy := policyFactory() - - err := k8sClient.Create(context.Background(), routePolicy) - logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) - Expect(err).ToNot(HaveOccurred()) - - // check policy status - Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) - - // attach policy to the gatewaay - gwPolicy := policyFactory(func(policy *api.AuthPolicy) { - policy.Name = "gw-auth" - policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" - policy.Spec.TargetRef.Kind = "Gateway" - policy.Spec.TargetRef.Name = testGatewayName - }) - - err = k8sClient.Create(context.Background(), gwPolicy) - logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gwPolicy).String(), "error", err) - Expect(err).ToNot(HaveOccurred()) - - // check policy status - Eventually(func() bool { - existingPolicy := &api.AuthPolicy{} - err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(gwPolicy), existingPolicy) - if err != nil { - return false - } - condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(common.PolicyConditionEnforced)) - return condition != nil && - condition.Reason == string(common.PolicyReasonUnknown) && - condition.Message == "AuthPolicy has encountered some issues: AuthScheme is not ready yet" - }, 30*time.Second, 5*time.Second).Should(BeTrue()) - - // check istio authorizationpolicy - iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, gwPolicy.Spec.TargetRef), Namespace: testNamespace} - Eventually(func() bool { - err := k8sClient.Get(context.Background(), iapKey, &secv1beta1resources.AuthorizationPolicy{}) - logf.Log.V(1).Info("Fetching Istio's AuthorizationPolicy", "key", iapKey.String(), "error", err) - return apierrors.IsNotFound(err) - }, 2*time.Minute, 5*time.Second).Should(BeTrue()) - - // check authorino authconfig - authConfigKey := types.NamespacedName{Name: authConfigName(client.ObjectKeyFromObject(gwPolicy)), Namespace: testNamespace} - Eventually(func() bool { - err := k8sClient.Get(context.Background(), authConfigKey, &authorinoapi.AuthConfig{}) - return apierrors.IsNotFound(err) - }, 30*time.Second, 5*time.Second).Should(BeTrue()) - }) - It("Rejects policy with only unmatching top-level route selectors while trying to configure the gateway", func() { policy := policyFactory(func(policy *api.AuthPolicy) { policy.Spec.RouteSelectors = []api.RouteSelector{ @@ -1253,6 +1201,52 @@ var _ = Describe("AuthPolicy controller", func() { Eventually(assertAcceptedCondTrueAndEnforcedCond(policy, metav1.ConditionFalse, string(common.PolicyReasonUnknown), "AuthPolicy has encountered some issues: AuthScheme is not ready yet"), 30*time.Second, 5*time.Second).Should(BeTrue()) }) + + It("Overridden reason - Attaches policy to the Gateway while having other policies attached to all HTTPRoutes", func() { + routePolicy := policyFactory() + + err := k8sClient.Create(context.Background(), routePolicy) + logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) + Expect(err).ToNot(HaveOccurred()) + + // check route policy status + Eventually(isAuthPolicyAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(isAuthPolicyEnforced(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + // attach policy to the gatewaay + gwPolicy := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "gw-auth" + policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = testGatewayName + }) + + err = k8sClient.Create(context.Background(), gwPolicy) + logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gwPolicy).String(), "error", err) + Expect(err).ToNot(HaveOccurred()) + + // check policy status + Eventually(isAuthPolicyAccepted(gwPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually( + assertAcceptedCondTrueAndEnforcedCond(gwPolicy, metav1.ConditionFalse, string(common.PolicyReasonOverridden), + fmt.Sprintf("AuthPolicy is overridden by [{\"Namespace\":\"%s\",\"Name\":\"%s\"}]", testNamespace, routePolicy.Name)), + 30*time.Second, 5*time.Second).Should(BeTrue()) + + // check istio authorizationpolicy + iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, gwPolicy.Spec.TargetRef), Namespace: testNamespace} + Eventually(func() bool { + err := k8sClient.Get(context.Background(), iapKey, &secv1beta1resources.AuthorizationPolicy{}) + logf.Log.V(1).Info("Fetching Istio's AuthorizationPolicy", "key", iapKey.String(), "error", err) + return apierrors.IsNotFound(err) + }, 2*time.Minute, 5*time.Second).Should(BeTrue()) + + // check authorino authconfig + authConfigKey := types.NamespacedName{Name: authConfigName(client.ObjectKeyFromObject(gwPolicy)), Namespace: testNamespace} + Eventually(func() bool { + err := k8sClient.Get(context.Background(), authConfigKey, &authorinoapi.AuthConfig{}) + return apierrors.IsNotFound(err) + }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }) }) }) diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 9e1018c0b..5bbda3ece 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -2,6 +2,8 @@ package controllers import ( "context" + "encoding/json" + "errors" "fmt" "slices" @@ -11,6 +13,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + v1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" @@ -19,32 +22,15 @@ import ( ) // reconcileStatus makes sure status block of AuthPolicy is up-to-date. -func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.AuthPolicy, specErr error) (ctrl.Result, error) { +func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object, specErr error) (ctrl.Result, error) { logger, _ := logr.FromContext(ctx) logger.V(1).Info("Reconciling AuthPolicy status", "spec error", specErr) - // fetch the AuthConfig and check if it's ready. - isAuthConfigReady := true - if specErr == nil { // skip fetching authconfig if we already have a reconciliation error. - apKey := client.ObjectKeyFromObject(ap) - authConfigKey := client.ObjectKey{ - Namespace: ap.Namespace, - Name: authConfigName(apKey), - } - authConfig := &authorinoapi.AuthConfig{} - if err := r.GetResource(ctx, authConfigKey, authConfig); err != nil && !apierrors.IsNotFound(err) { - return ctrl.Result{}, err - } - - isAuthConfigReady = authConfig.Status.Ready() - } - - newStatus := r.calculateStatus(ap, specErr, isAuthConfigReady) + newStatus := r.calculateStatus(ctx, ap, targetNetworkObject, specErr) equalStatus := ap.Status.Equals(newStatus, logger) logger.V(1).Info("Status", "status is different", !equalStatus) logger.V(1).Info("Status", "generation is different", ap.Generation != ap.Status.ObservedGeneration) - logger.V(1).Info("Status", "AuthConfig is ready", isAuthConfigReady) if equalStatus && ap.Generation == ap.Status.ObservedGeneration { logger.V(1).Info("Status up-to-date. No changes required.") return ctrl.Result{}, nil @@ -72,7 +58,7 @@ func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.Auth return ctrl.Result{}, nil } -func (r *AuthPolicyReconciler) calculateStatus(ap *api.AuthPolicy, specErr error, authConfigReady bool) *api.AuthPolicyStatus { +func (r *AuthPolicyReconciler) calculateStatus(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object, specErr error) *api.AuthPolicyStatus { newStatus := &api.AuthPolicyStatus{ Conditions: slices.Clone(ap.Status.Conditions), ObservedGeneration: ap.Status.ObservedGeneration, @@ -86,27 +72,69 @@ func (r *AuthPolicyReconciler) calculateStatus(ap *api.AuthPolicy, specErr error return newStatus } - enforcedCond := r.enforcedCondition(ap, authConfigReady) + enforcedCond := r.enforcedCondition(ctx, ap, targetNetworkObject) meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) return newStatus } func (r *AuthPolicyReconciler) acceptedCondition(policy common.KuadrantPolicy, specErr error) *metav1.Condition { - cond := common.AcceptedCondition(policy, specErr) - - return cond + return common.AcceptedCondition(policy, specErr) } -func (r *AuthPolicyReconciler) enforcedCondition(policy common.KuadrantPolicy, authConfigReady bool) *metav1.Condition { - var err common.PolicyError +// enforcedCondition checks if the provided AuthPolicy is enforced based on the status of the associated AuthConfig and Gateway. +func (r *AuthPolicyReconciler) enforcedCondition(ctx context.Context, policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { + logger, _ := logr.FromContext(ctx) + authConfigReady, gwPolicyOverridden, err := r.checkAuthConfigAndGateway(ctx, policy) + if err != nil { + logger.Error(err, "Failed to check AuthConfig and Gateway") + return common.EnforcedCondition(policy, common.NewErrUnknown(policy.Kind(), err)) + } + if !authConfigReady { - err = common.NewErrUnknown(policy.Kind(), fmt.Errorf("AuthScheme is not ready yet")) + logger.V(1).Info("AuthConfig is not ready") + return common.EnforcedCondition(policy, common.NewErrUnknown(policy.Kind(), errors.New("AuthScheme is not ready yet"))) } - // TODO: Implement 'Overridden' Reason if AuthPolicy supports Inherited Policy Attachment + if gwPolicyOverridden { + logger.V(1).Info("Gateway Policy is overridden") + return r.handleGatewayPolicyOverride(logger, policy, targetNetworkObject) + } + + logger.V(1).Info("AuthPolicy is enforced") + return common.EnforcedCondition(policy, nil) +} - cond := common.EnforcedCondition(policy, err) +// checkAuthConfigAndGateway checks if the AuthConfig is ready and if the Gateway Policy is overridden. +func (r *AuthPolicyReconciler) checkAuthConfigAndGateway(ctx context.Context, policy *api.AuthPolicy) (bool, bool, error) { + apKey := client.ObjectKeyFromObject(policy) + authConfigKey := client.ObjectKey{ + Namespace: policy.Namespace, + Name: authConfigName(apKey), + } + authConfig := &authorinoapi.AuthConfig{} + err := r.GetResource(ctx, authConfigKey, authConfig) + if err != nil { + if !apierrors.IsNotFound(err) { + return false, false, fmt.Errorf("failed to get AuthConfig: %w", err) + } + return true, common.IsTargetRefGateway(policy.GetTargetRef()), nil + } + return authConfig.Status.Ready(), false, nil +} - return cond +// handleGatewayPolicyOverride handles the case where the Gateway Policy is overridden. +func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { + obj := targetNetworkObject.(*v1.Gateway) + gatewayWrapper := common.GatewayWrapper{Gateway: obj, PolicyRefsConfig: &common.KuadrantAuthPolicyRefsConfig{}} + refs := gatewayWrapper.PolicyRefs() + filteredRef := common.Filter(refs, func(key client.ObjectKey) bool { + return key != client.ObjectKeyFromObject(policy) + }) + jsonData, err := json.Marshal(filteredRef) + if err != nil { + logger.Error(err, "Failed to marshal filtered references") + return common.EnforcedCondition(policy, common.NewErrUnknown(policy.Kind(), err)) + } + return common.EnforcedCondition(policy, common.NewErrOverridden(policy.Kind(), string(jsonData))) } diff --git a/pkg/common/apimachinery_status_conditions.go b/pkg/common/apimachinery_status_conditions.go index 960d96ed1..ee5364a1b 100644 --- a/pkg/common/apimachinery_status_conditions.go +++ b/pkg/common/apimachinery_status_conditions.go @@ -14,8 +14,9 @@ import ( const ( PolicyConditionEnforced gatewayapiv1alpha2.PolicyConditionType = "Enforced" - PolicyReasonEnforced gatewayapiv1alpha2.PolicyConditionReason = "Enforced" - PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" + PolicyReasonEnforced gatewayapiv1alpha2.PolicyConditionReason = "Enforced" + PolicyReasonOverridden gatewayapiv1alpha2.PolicyConditionReason = "Overridden" + PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" ) // ConditionMarshal marshals the set of conditions as a JSON array, sorted by condition type. diff --git a/pkg/common/apimachinery_status_conditions_test.go b/pkg/common/apimachinery_status_conditions_test.go index 8ce5004f7..91bae0c68 100644 --- a/pkg/common/apimachinery_status_conditions_test.go +++ b/pkg/common/apimachinery_status_conditions_test.go @@ -3,13 +3,13 @@ package common import ( - "fmt" + "errors" "reflect" "testing" "time" goCmp "github.com/google/go-cmp/cmp" - "k8s.io/apimachinery/pkg/api/errors" + apiErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -149,7 +149,7 @@ func TestAcceptedCondition(t *testing.T) { Group: "gateway.networking.k8s.io", Kind: "HTTPRoute", Name: "my-target-ref", - }, errors.NewNotFound(schema.GroupResource{}, "my-target-ref")), + }, apiErrors.NewNotFound(schema.GroupResource{}, "my-target-ref")), }, want: &metav1.Condition{ Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), @@ -166,7 +166,7 @@ func TestAcceptedCondition(t *testing.T) { Group: "gateway.networking.k8s.io", Kind: "HTTPRoute", Name: "my-target-ref", - }, fmt.Errorf("deletion err")), + }, errors.New("deletion err")), }, want: &metav1.Condition{ Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), @@ -179,7 +179,7 @@ func TestAcceptedCondition(t *testing.T) { name: "invalid reason", args: args{ policy: &FakePolicy{}, - err: NewErrInvalid("FakePolicy", fmt.Errorf("invalid err")), + err: NewErrInvalid("FakePolicy", errors.New("invalid err")), }, want: &metav1.Condition{ Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), @@ -192,7 +192,7 @@ func TestAcceptedCondition(t *testing.T) { name: "conflicted reason", args: args{ policy: &FakePolicy{}, - err: NewErrConflict("FakePolicy", "testNs/policy1", fmt.Errorf("conflict err")), + err: NewErrConflict("FakePolicy", "testNs/policy1", errors.New("conflict err")), }, want: &metav1.Condition{ Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), @@ -205,7 +205,7 @@ func TestAcceptedCondition(t *testing.T) { name: "unknown error reason", args: args{ policy: &FakePolicy{}, - err: fmt.Errorf("reconcile err"), + err: errors.New("reconcile err"), }, want: &metav1.Condition{ Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), @@ -251,7 +251,7 @@ func TestEnforcedCondition(t *testing.T) { name: "enforced false", args: args{ policy: &FakePolicy{}, - err: NewErrUnknown(policy.Kind(), fmt.Errorf("unknown err")), + err: NewErrUnknown(policy.Kind(), errors.New("unknown err")), }, want: &metav1.Condition{ Type: string(PolicyConditionEnforced), diff --git a/pkg/common/errors.go b/pkg/common/errors.go index 1adc691a6..938e80c8d 100644 --- a/pkg/common/errors.go +++ b/pkg/common/errors.go @@ -107,3 +107,25 @@ func NewErrUnknown(kind string, err error) ErrUnknown { Err: err, } } + +var _ PolicyError = ErrOverridden{} + +type ErrOverridden struct { + Kind string + OverridingPolicies string +} + +func (e ErrOverridden) Error() string { + return fmt.Sprintf("%s is overridden by %s", e.Kind, e.OverridingPolicies) +} + +func (e ErrOverridden) Reason() gatewayapiv1alpha2.PolicyConditionReason { + return PolicyReasonOverridden +} + +func NewErrOverridden(kind, overridingPolicies string) ErrOverridden { + return ErrOverridden{ + Kind: kind, + OverridingPolicies: overridingPolicies, + } +}