Merge pull request #434 from Kuadrant/host-collision-switch

Enable/disable host name collision prevention for strict host subsets

controllers/auth_config_controller.go

@@ -58,12 +58,13 @@ const (
 // AuthConfigReconciler reconciles an AuthConfig object
 type AuthConfigReconciler struct {
 	client.Client
-	Logger        logr.Logger
-	Scheme        *runtime.Scheme
-	Index         index.Index
-	StatusReport  *StatusReportMap
-	LabelSelector labels.Selector
-	Namespace     string
+	Logger                      logr.Logger
+	Scheme                      *runtime.Scheme
+	Index                       index.Index
+	AllowSupersedingHostSubsets bool
+	StatusReport                *StatusReportMap
+	LabelSelector               labels.Selector
+	Namespace                   string
 
 	indexBootstrap sync.Mutex
 }
@@ -608,7 +609,7 @@ func (r *AuthConfigReconciler) addToIndex(ctx context.Context, resourceNamespace
 
 	for _, host := range hosts {
 		// check for host name collision between resources
-		if indexedResourceId, found := r.Index.FindId(host); found && indexedResourceId != resourceId {
+		if r.hostTaken(host, resourceId) {
 			looseHosts = append(looseHosts, host)
 			logger.Info("host already taken", "host", host)
 			continue
@@ -625,6 +626,15 @@ func (r *AuthConfigReconciler) addToIndex(ctx context.Context, resourceNamespace
 	return
 }
 
+func (r *AuthConfigReconciler) hostTaken(host, resourceId string) bool {
+	indexedResourceId, found := r.Index.FindId(host)
+	return found && indexedResourceId != resourceId && !r.supersedeHostSubset(host, indexedResourceId)
+}
+
+func (r *AuthConfigReconciler) supersedeHostSubset(host, supersetResourceId string) bool {
+	return r.AllowSupersedingHostSubsets && !utils.SliceContains(r.Index.FindKeys(supersetResourceId), host)
+}
+
 func (r *AuthConfigReconciler) bootstrapIndex(ctx context.Context) error {
 	r.indexBootstrap.Lock()
 	defer r.indexBootstrap.Unlock()

controllers/auth_config_controller_test.go

@@ -230,7 +230,7 @@ func TestTranslateAuthConfig(t *testing.T) {
 	// TODO
 }
 
-func TestPreventHostCollision(t *testing.T) {
+func TestPreventHostCollisionExactMatches(t *testing.T) {
 	mockController := gomock.NewController(t)
 	defer mockController.Finish()
 	indexMock := mock_index.NewMockIndex(mockController)
@@ -242,19 +242,67 @@ func TestPreventHostCollision(t *testing.T) {
 	client := newTestK8sClient(&authConfig, &secret)
 	reconciler := newTestAuthConfigReconciler(client, indexMock)
 
-	indexMock.EXPECT().Empty().Return(false)
-	indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes()
-	indexMock.EXPECT().FindId("echo-api").Return("other-namespace/other-auth-config-with-same-host", true)
-	indexMock.EXPECT().FindId("other.io").Return("", false)
-	indexMock.EXPECT().FindId("yet-another.io").Return(fmt.Sprintf("%s/other-auth-config-same-ns", authConfig.Namespace), true)
-	indexMock.EXPECT().Set(authConfigName.String(), "other.io", gomock.Any(), true)
+	indexMock.EXPECT().Empty().Return(false)                                                                              // simulate index not empty, so it skips bootstraping
+	indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes()                                    // simulate no prexisting hosts linked to the authconfig to be reconciled
+	indexMock.EXPECT().FindId("echo-api").Return("other-namespace/other-auth-config-with-same-host", true)                // simulate other existing authconfig with conflicting host, in a different namespace
+	indexMock.EXPECT().FindId("other.io").Return(fmt.Sprintf("%s/other-auth-config-same-ns", authConfig.Namespace), true) // simulate other existing authconfig with conflicting host, in the same namespace
+	indexMock.EXPECT().FindId("yet-another.io").Return("", false)                                                         // simulate no other existing authconfig with conflicting host
+
+	indexMock.EXPECT().Set(authConfigName.String(), "yet-another.io", gomock.Any(), true) // expect only the new host to be indexed
 
 	result, err := reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
 
 	assert.DeepEqual(t, result, ctrl.Result{})
 	assert.NilError(t, err)
 }
 
+func TestPreventHostCollisionAllowSupersedingHostSubsets(t *testing.T) {
+	mockController := gomock.NewController(t)
+	defer mockController.Finish()
+	indexMock := mock_index.NewMockIndex(mockController)
+
+	authConfig := newTestAuthConfig(map[string]string{})
+	authConfig.Spec.Hosts = []string{"echo-api.io"}
+	authConfigName := types.NamespacedName{Name: authConfig.Name, Namespace: authConfig.Namespace}
+
+	secret := newTestOAuthClientSecret()
+	client := newTestK8sClient(&authConfig, &secret)
+	reconciler := newTestAuthConfigReconciler(client, indexMock)
+
+	indexMock.EXPECT().Empty().Return(false).AnyTimes()                                // simulate index not empty, so it skips bootstraping
+	indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes() // simulate no prexisting hosts linked to the authconfig to be reconciled
+
+	// allow superseding host subsets = false
+	indexMock.EXPECT().FindId("echo-api.io").Return("other/other", true) // simulate other existing authconfig with conflicting host
+
+	result, err := reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
+
+	assert.DeepEqual(t, result, ctrl.Result{})
+	assert.NilError(t, err)
+
+	// allow superseding host subsets = true, conflicting host found and the new one is NOT a strict subset of the one found
+	reconciler.AllowSupersedingHostSubsets = true
+	indexMock.EXPECT().FindId("echo-api.io").Return("other/other-1", true)       // simulate other existing authconfig with conflicting host
+	indexMock.EXPECT().FindKeys("other/other-1").Return([]string{"echo-api.io"}) // simulate identical host found linked to other authconfig (i.e. not a strict subset)
+
+	result, err = reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
+
+	assert.DeepEqual(t, result, ctrl.Result{})
+	assert.NilError(t, err)
+
+	// allow superseding host subsets = true, conflicting host found but the new one is a strict subset of the one found
+	reconciler.AllowSupersedingHostSubsets = true
+	indexMock.EXPECT().FindId("echo-api.io").Return("other/other-2", true) // simulate other existing authconfig with conflicting host
+	indexMock.EXPECT().FindKeys("other/other-2").Return([]string{"*.io"})  // simulate superset host found linked to other authconfig
+
+	indexMock.EXPECT().Set(authConfigName.String(), "echo-api.io", gomock.Any(), true) // expect only the new host to be indexed
+
+	result, err = reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
+
+	assert.DeepEqual(t, result, ctrl.Result{})
+	assert.NilError(t, err)
+}
+
 func TestMissingWatchedAuthConfigLabels(t *testing.T) {
 	mockController := gomock.NewController(t)
 	defer mockController.Finish()

docs/architecture.md

@@ -238,6 +238,8 @@ Authorino tries to prevent host name collision between `AuthConfig`s by rejectin
 
 When wildcards are involved, a host name that matches a host wildcard already linked in the index to another `AuthConfig` will be considered taken, and therefore the newest `AuthConfig` will be rejected to be linked to that host.
 
+This behavior can be disabled to allow `AuthConfig`s to partially supersede each others' host names (limited to strict host subsets), by supplying the `--allow-superseding-host-subsets` command-line flag when running the Authorino instance.
+
 ## The Authorization JSON
 
 On every Auth Pipeline, Authorino builds the **Authorization JSON**, a "working-memory" data structure composed of `context` (information about the request, as supplied by the Envoy proxy to Authorino) and `auth` (objects resolved in phases (i) to (v) of the pipeline). The evaluators of each phase can read from the Authorization JSON and implement dynamic properties and decisions based on its values.

main.go

@@ -104,6 +104,7 @@ type authServerOptions struct {
 	watchNamespace                 string
 	watchedAuthConfigLabelSelector string
 	watchedSecretLabelSelector     string
+	allowSupersedingHostSubsets    bool
 	timeout                        int
 	extAuthGRPCPort                int
 	extAuthHTTPPort                int
@@ -165,6 +166,7 @@ func authServerCmd(opts *authServerOptions) *cobra.Command {
 	cmd.PersistentFlags().StringVar(&opts.watchNamespace, "watch-namespace", utils.EnvVar("WATCH_NAMESPACE", ""), "Kubernetes namespace to watch")
 	cmd.PersistentFlags().StringVar(&opts.watchedAuthConfigLabelSelector, "auth-config-label-selector", utils.EnvVar("AUTH_CONFIG_LABEL_SELECTOR", ""), "Kubernetes label selector to filter AuthConfig resources to watch")
 	cmd.PersistentFlags().StringVar(&opts.watchedSecretLabelSelector, "secret-label-selector", utils.EnvVar("SECRET_LABEL_SELECTOR", "authorino.kuadrant.io/managed-by=authorino"), "Kubernetes label selector to filter Secret resources to watch")
+	cmd.PersistentFlags().BoolVar(&opts.allowSupersedingHostSubsets, "allow-superseding-host-subsets", false, "Enable AuthConfigs to supersede strict host subsets of supersets already taken")
 	cmd.PersistentFlags().IntVar(&opts.timeout, "timeout", utils.EnvVar("TIMEOUT", 0), "Server timeout - in milliseconds")
 	cmd.PersistentFlags().IntVar(&opts.extAuthGRPCPort, "ext-auth-grpc-port", utils.EnvVar("EXT_AUTH_GRPC_PORT", 50051), "Port number of authorization server - gRPC interface")
 	cmd.PersistentFlags().IntVar(&opts.extAuthHTTPPort, "ext-auth-http-port", utils.EnvVar("EXT_AUTH_HTTP_PORT", 5001), "Port number of authorization server - raw HTTP interface")
@@ -256,13 +258,14 @@ func runAuthorizationServer(cmd *cobra.Command, _ []string) {
 
 	// sets up the authconfig reconciler
 	authConfigReconciler := &controllers.AuthConfigReconciler{
-		Client:        mgr.GetClient(),
-		Index:         index,
-		StatusReport:  statusReport,
-		Logger:        controllerLogger.WithName("authconfig"),
-		Scheme:        mgr.GetScheme(),
-		LabelSelector: controllers.ToLabelSelector(opts.watchedAuthConfigLabelSelector),
-		Namespace:     opts.watchNamespace,
+		Client:                      mgr.GetClient(),
+		Index:                       index,
+		AllowSupersedingHostSubsets: opts.allowSupersedingHostSubsets,
+		StatusReport:                statusReport,
+		Logger:                      controllerLogger.WithName("authconfig"),
+		Scheme:                      mgr.GetScheme(),
+		LabelSelector:               controllers.ToLabelSelector(opts.watchedAuthConfigLabelSelector),
+		Namespace:                   opts.watchNamespace,
 	}
 	if err = authConfigReconciler.SetupWithManager(mgr); err != nil {
 		logger.Error(err, "failed to setup controller", "controller", "authconfig")

pkg/index/index_test.go

@@ -13,34 +13,28 @@ import (
 
 // TestAuthConfigTree tests operations to build and modify the following index tree:
 //
-//	                 ┌───┐
-//	       ┌─────────┤ . ├──────────┐
-//	       │         └───┘          │
-//	       │                        │
-//	       │                        │
-//	    ┌──┴─┐                   ┌──┴──┐
-//	┌───┤ io ├───┐           ┌───┤ com ├───┐
-//	│   └────┘   │           │   └─────┘   │
-//	│            │           │             │
-//	│            │           │             │
-//	│            │           │             │
-//
-// ┌─┴─┐       ┌──┴──┐    ┌───┴──┐      ┌───┴──┐
-// │ * │       │ nip │    │ pets │    ┌─┤ acme ├─┐
-// └───┘       └──┬──┘    └───┬──┘    │ └──────┘ │
-//
-//	▲            │           │       │          │
-//	│            │           │       │          │
-//	│            │           │       │          │
-//	│      ┌─────┴──────┐  ┌─┴─┐  ┌──┴──┐     ┌─┴─┐
-//
-// auth-1   │ talker-api │  │ * │  │ api │     │ * │
-//
-//	└────────────┘  └───┘  └─────┘     └───┘
-//	      ▲           ▲       ▲          ▲
-//	      │           │       │          │
-//	      │           │       │          │
-//	      └───auth-2──┘     auth-3     auth-4
+//	                   ┌───┐
+//	         ┌─────────┤ . ├────────┐
+//	         │         └───┘        │
+//	         │                      │
+//	         │                      │
+//	      ┌──┴─┐                 ┌──┴──┐
+//	  ┌───┤ io ├──┐          ┌───┤ com ├───┐
+//	  │   └────┘  │          │   └─────┘   │
+//	  │           │          │             │
+//	  │           │          │             │
+//	┌─┴─┐      ┌──┴──┐    ┌──┴───┐     ┌───┴──┐
+//	│ * │      │ nip │    │ pets │   ┌─┤ acme ├─┐
+//	└───┘      └──┬──┘    └──┬───┘   │ └──────┘ │
+//	  ▲           │          │       │          │
+//	  │           │          │       │          │
+//	  │    ┌──────┴─────┐  ┌─┴─┐  ┌──┴──┐     ┌─┴─┐
+//	auth-1 │ talker-api │  │ * │  │ api │     │ * │
+//	       └────────────┘  └───┘  └─────┘     └───┘
+//	              ▲          ▲       ▲          ▲
+//	              │          │       │          │
+//	              │          │       │          │
+//	              └──auth-2──┘     auth-3     auth-4
 func TestAuthConfigTree(t *testing.T) {
 	c := newAuthConfigTree()
 
@@ -50,22 +44,26 @@ func TestAuthConfigTree(t *testing.T) {
 	authConfig4 := buildTestAuthConfig()
 
 	// Build the index
+	// Set the more generic host first
 	if err := c.Set("auth-1", "*.io", authConfig1, false); err != nil {
 		t.Error(err)
 	}
 
-	if err := c.Set("auth-2", "*.pets.com", authConfig2, false); err != nil {
+	// ...and then the more specific one
+	if err := c.Set("auth-2", "talker-api.nip.io", authConfig2, false); err != nil {
 		t.Error(err)
 	}
 
-	if err := c.Set("auth-2", "talker-api.nip.io", authConfig2, false); err != nil {
+	if err := c.Set("auth-2", "*.pets.com", authConfig2, false); err != nil {
 		t.Error(err)
 	}
 
+	// Set the more specific host first
 	if err := c.Set("auth-3", "api.acme.com", authConfig3, false); err != nil {
 		t.Error(err)
 	}
 
+	// ...and then the more generic one
 	if err := c.Set("auth-4", "*.acme.com", authConfig4, false); err != nil {
 		t.Error(err)
 	}
@@ -130,15 +128,15 @@ func TestAuthConfigTree(t *testing.T) {
 	assert.Check(t, config == nil)
 
 	config = c.Get("talker-api.nip.io")
-	assert.DeepEqual(t, *config, authConfig1) // because `*.io -> auth-1` is still in the tree
+	assert.DeepEqual(t, *config, authConfig1) // because `*.io <- auth-1` is still in the tree
 
 	config = c.Get("api.acme.com")
 	assert.DeepEqual(t, *config, authConfig3)
 
 	c.Delete("auth-3")
 
 	config = c.Get("api.acme.com")
-	assert.DeepEqual(t, *config, authConfig4) // because `*.acme.com -> auth-4` is still in the tree
+	assert.DeepEqual(t, *config, authConfig4) // because `*.acme.com <- auth-4` is still in the tree
 }
 
 type bogusIdentity struct{}