Skip to content

Commit 60600fe

Browse files
guicassolatoguicassolato
committed
Enable/disable host name collision prevention for strict host subsets
Adds a new command-line flag `--allow-superseding-host-subsets` that disables the host name collision prevention for strict subsets of hosts attempted to be linked after a superset already taken. Closes #433 under a more conservative approach where the switch for the host name collision mechanism does not come in levels, but as a simple ON/OFF switch, whose semantics is limited to strict subsets. I reckon this is a reasonable compromise as there are only few use cases I can think of for fully superseding the auth scheme a given host is linked to, based only on the order of creation of the resources. Moreover, this approach exempts us from having to update the status of the (partially) superseded AuthConfigs, consistently with the current behaviour, when the order of creation goes: more specific first, then more generic host. The status of the more generic (wider) AuthConfig has never reflected that a subset of its hosts was actually under another auth scheme declared first.
1 parent eecd82e commit 60600fe

File tree

5 files changed

+114
-53
lines changed

5 files changed

+114
-53
lines changed

‎controllers/auth_config_controller.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,13 @@ const (
5858
// AuthConfigReconciler reconciles an AuthConfig object
5959
type AuthConfigReconciler struct {
6060
client.Client
61-
Logger logr.Logger
62-
Scheme *runtime.Scheme
63-
Index index.Index
64-
StatusReport *StatusReportMap
65-
LabelSelector labels.Selector
66-
Namespace string
61+
Logger logr.Logger
62+
Scheme *runtime.Scheme
63+
Index index.Index
64+
AllowSupersedingHostSubsets bool
65+
StatusReport *StatusReportMap
66+
LabelSelector labels.Selector
67+
Namespace string
6768

6869
indexBootstrap sync.Mutex
6970
}
@@ -608,7 +609,7 @@ func (r *AuthConfigReconciler) addToIndex(ctx context.Context, resourceNamespace
608609

609610
for _, host := range hosts {
610611
// check for host name collision between resources
611-
if indexedResourceId, found := r.Index.FindId(host); found && indexedResourceId != resourceId {
612+
if r.hostTaken(host, resourceId) {
612613
looseHosts = append(looseHosts, host)
613614
logger.Info("host already taken", "host", host)
614615
continue
@@ -625,6 +626,15 @@ func (r *AuthConfigReconciler) addToIndex(ctx context.Context, resourceNamespace
625626
return
626627
}
627628

629+
func (r *AuthConfigReconciler) hostTaken(host, resourceId string) bool {
630+
indexedResourceId, found := r.Index.FindId(host)
631+
return found && indexedResourceId != resourceId && !r.supersedeHostSubset(host, indexedResourceId)
632+
}
633+
634+
func (r *AuthConfigReconciler) supersedeHostSubset(host, supersetResourceId string) bool {
635+
return r.AllowSupersedingHostSubsets && !utils.SliceContains(r.Index.FindKeys(supersetResourceId), host)
636+
}
637+
628638
func (r *AuthConfigReconciler) bootstrapIndex(ctx context.Context) error {
629639
r.indexBootstrap.Lock()
630640
defer r.indexBootstrap.Unlock()

‎controllers/auth_config_controller_test.go‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ func TestTranslateAuthConfig(t *testing.T) {
230230
// TODO
231231
}
232232

233-
func TestPreventHostCollision(t *testing.T) {
233+
func TestPreventHostCollisionExactMatches(t *testing.T) {
234234
mockController := gomock.NewController(t)
235235
defer mockController.Finish()
236236
indexMock := mock_index.NewMockIndex(mockController)
@@ -242,19 +242,67 @@ func TestPreventHostCollision(t *testing.T) {
242242
client := newTestK8sClient(&authConfig, &secret)
243243
reconciler := newTestAuthConfigReconciler(client, indexMock)
244244

245-
indexMock.EXPECT().Empty().Return(false)
246-
indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes()
247-
indexMock.EXPECT().FindId("echo-api").Return("other-namespace/other-auth-config-with-same-host", true)
248-
indexMock.EXPECT().FindId("other.io").Return("", false)
249-
indexMock.EXPECT().FindId("yet-another.io").Return(fmt.Sprintf("%s/other-auth-config-same-ns", authConfig.Namespace), true)
250-
indexMock.EXPECT().Set(authConfigName.String(), "other.io", gomock.Any(), true)
245+
indexMock.EXPECT().Empty().Return(false) // simulate index not empty, so it skips bootstraping
246+
indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes() // simulate no prexisting hosts linked to the authconfig to be reconciled
247+
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
248+
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
249+
indexMock.EXPECT().FindId("yet-another.io").Return("", false) // simulate no other existing authconfig with conflicting host
250+
251+
indexMock.EXPECT().Set(authConfigName.String(), "yet-another.io", gomock.Any(), true) // expect only the new host to be indexed
251252

252253
result, err := reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
253254

254255
assert.DeepEqual(t, result, ctrl.Result{})
255256
assert.NilError(t, err)
256257
}
257258

259+
func TestPreventHostCollisionAllowSupersedingHostSubsets(t *testing.T) {
260+
mockController := gomock.NewController(t)
261+
defer mockController.Finish()
262+
indexMock := mock_index.NewMockIndex(mockController)
263+
264+
authConfig := newTestAuthConfig(map[string]string{})
265+
authConfig.Spec.Hosts = []string{"echo-api.io"}
266+
authConfigName := types.NamespacedName{Name: authConfig.Name, Namespace: authConfig.Namespace}
267+
268+
secret := newTestOAuthClientSecret()
269+
client := newTestK8sClient(&authConfig, &secret)
270+
reconciler := newTestAuthConfigReconciler(client, indexMock)
271+
272+
indexMock.EXPECT().Empty().Return(false).AnyTimes() // simulate index not empty, so it skips bootstraping
273+
indexMock.EXPECT().FindKeys(authConfigName.String()).Return([]string{}).AnyTimes() // simulate no prexisting hosts linked to the authconfig to be reconciled
274+
275+
// allow superseding host subsets = false
276+
indexMock.EXPECT().FindId("echo-api.io").Return("other/other", true) // simulate other existing authconfig with conflicting host
277+
278+
result, err := reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
279+
280+
assert.DeepEqual(t, result, ctrl.Result{})
281+
assert.NilError(t, err)
282+
283+
// allow superseding host subsets = true, conflicting host found and the new one is NOT a strict subset of the one found
284+
reconciler.AllowSupersedingHostSubsets = true
285+
indexMock.EXPECT().FindId("echo-api.io").Return("other/other-1", true) // simulate other existing authconfig with conflicting host
286+
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)
287+
288+
result, err = reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
289+
290+
assert.DeepEqual(t, result, ctrl.Result{})
291+
assert.NilError(t, err)
292+
293+
// allow superseding host subsets = true, conflicting host found but the new one is a strict subset of the one found
294+
reconciler.AllowSupersedingHostSubsets = true
295+
indexMock.EXPECT().FindId("echo-api.io").Return("other/other-2", true) // simulate other existing authconfig with conflicting host
296+
indexMock.EXPECT().FindKeys("other/other-2").Return([]string{"*.io"}) // simulate superset host found linked to other authconfig
297+
298+
indexMock.EXPECT().Set(authConfigName.String(), "echo-api.io", gomock.Any(), true) // expect only the new host to be indexed
299+
300+
result, err = reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: authConfigName})
301+
302+
assert.DeepEqual(t, result, ctrl.Result{})
303+
assert.NilError(t, err)
304+
}
305+
258306
func TestMissingWatchedAuthConfigLabels(t *testing.T) {
259307
mockController := gomock.NewController(t)
260308
defer mockController.Finish()

‎docs/architecture.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,8 @@ Authorino tries to prevent host name collision between `AuthConfig`s by rejectin
238238

239239
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.
240240

241+
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.
242+
241243
## The Authorization JSON
242244

243245
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‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ type authServerOptions struct {
104104
watchNamespace string
105105
watchedAuthConfigLabelSelector string
106106
watchedSecretLabelSelector string
107+
allowSupersedingHostSubsets bool
107108
timeout int
108109
extAuthGRPCPort int
109110
extAuthHTTPPort int
@@ -165,6 +166,7 @@ func authServerCmd(opts *authServerOptions) *cobra.Command {
165166
cmd.PersistentFlags().StringVar(&opts.watchNamespace, "watch-namespace", utils.EnvVar("WATCH_NAMESPACE", ""), "Kubernetes namespace to watch")
166167
cmd.PersistentFlags().StringVar(&opts.watchedAuthConfigLabelSelector, "auth-config-label-selector", utils.EnvVar("AUTH_CONFIG_LABEL_SELECTOR", ""), "Kubernetes label selector to filter AuthConfig resources to watch")
167168
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")
169+
cmd.PersistentFlags().BoolVar(&opts.allowSupersedingHostSubsets, "allow-superseding-host-subsets", false, "Enable AuthConfigs to supersede strict host subsets of supersets already taken")
168170
cmd.PersistentFlags().IntVar(&opts.timeout, "timeout", utils.EnvVar("TIMEOUT", 0), "Server timeout - in milliseconds")
169171
cmd.PersistentFlags().IntVar(&opts.extAuthGRPCPort, "ext-auth-grpc-port", utils.EnvVar("EXT_AUTH_GRPC_PORT", 50051), "Port number of authorization server - gRPC interface")
170172
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) {
256258

257259
// sets up the authconfig reconciler
258260
authConfigReconciler := &controllers.AuthConfigReconciler{
259-
Client: mgr.GetClient(),
260-
Index: index,
261-
StatusReport: statusReport,
262-
Logger: controllerLogger.WithName("authconfig"),
263-
Scheme: mgr.GetScheme(),
264-
LabelSelector: controllers.ToLabelSelector(opts.watchedAuthConfigLabelSelector),
265-
Namespace: opts.watchNamespace,
261+
Client: mgr.GetClient(),
262+
Index: index,
263+
AllowSupersedingHostSubsets: opts.allowSupersedingHostSubsets,
264+
StatusReport: statusReport,
265+
Logger: controllerLogger.WithName("authconfig"),
266+
Scheme: mgr.GetScheme(),
267+
LabelSelector: controllers.ToLabelSelector(opts.watchedAuthConfigLabelSelector),
268+
Namespace: opts.watchNamespace,
266269
}
267270
if err = authConfigReconciler.SetupWithManager(mgr); err != nil {
268271
logger.Error(err, "failed to setup controller", "controller", "authconfig")

‎pkg/index/index_test.go‎

Lines changed: 30 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -13,34 +13,28 @@ import (
1313

1414
// TestAuthConfigTree tests operations to build and modify the following index tree:
1515
//
16-
// ┌───┐
17-
// ┌─────────┤ . ├──────────┐
18-
// │ └───┘ │
19-
// │ │
20-
// │ │
21-
// ┌──┴─┐ ┌──┴──┐
22-
// ┌───┤ io ├───┐ ┌───┤ com ├───┐
23-
// │ └────┘ │ │ └─────┘ │
24-
// │ │ │ │
25-
// │ │ │ │
26-
// │ │ │ │
27-
//
28-
// ┌─┴─┐ ┌──┴──┐ ┌───┴──┐ ┌───┴──┐
29-
// │ * │ │ nip │ │ pets │ ┌─┤ acme ├─┐
30-
// └───┘ └──┬──┘ └───┬──┘ │ └──────┘ │
31-
//
32-
// ▲ │ │ │ │
33-
// │ │ │ │ │
34-
// │ │ │ │ │
35-
// │ ┌─────┴──────┐ ┌─┴─┐ ┌──┴──┐ ┌─┴─┐
36-
//
37-
// auth-1 │ talker-api │ │ * │ │ api │ │ * │
38-
//
39-
// └────────────┘ └───┘ └─────┘ └───┘
40-
// ▲ ▲ ▲ ▲
41-
// │ │ │ │
42-
// │ │ │ │
43-
// └───auth-2──┘ auth-3 auth-4
16+
// ┌───┐
17+
// ┌─────────┤ . ├────────┐
18+
// │ └───┘ │
19+
// │ │
20+
// │ │
21+
// ┌──┴─┐ ┌──┴──┐
22+
// ┌───┤ io ├──┐ ┌───┤ com ├───┐
23+
// │ └────┘ │ │ └─────┘ │
24+
// │ │ │ │
25+
// │ │ │ │
26+
// ┌─┴─┐ ┌──┴──┐ ┌──┴───┐ ┌───┴──┐
27+
// │ * │ │ nip │ │ pets │ ┌─┤ acme ├─┐
28+
// └───┘ └──┬──┘ └──┬───┘ │ └──────┘ │
29+
// ▲ │ │ │ │
30+
// │ │ │ │ │
31+
// │ ┌──────┴─────┐ ┌─┴─┐ ┌──┴──┐ ┌─┴─┐
32+
// auth-1 │ talker-api │ │ * │ │ api │ │ * │
33+
// └────────────┘ └───┘ └─────┘ └───┘
34+
// ▲ ▲ ▲ ▲
35+
// │ │ │ │
36+
// │ │ │ │
37+
// └──auth-2──┘ auth-3 auth-4
4438
func TestAuthConfigTree(t *testing.T) {
4539
c := newAuthConfigTree()
4640

@@ -50,22 +44,26 @@ func TestAuthConfigTree(t *testing.T) {
5044
authConfig4 := buildTestAuthConfig()
5145

5246
// Build the index
47+
// Set the more generic host first
5348
if err := c.Set("auth-1", "*.io", authConfig1, false); err != nil {
5449
t.Error(err)
5550
}
5651

57-
if err := c.Set("auth-2", "*.pets.com", authConfig2, false); err != nil {
52+
// ...and then the more specific one
53+
if err := c.Set("auth-2", "talker-api.nip.io", authConfig2, false); err != nil {
5854
t.Error(err)
5955
}
6056

61-
if err := c.Set("auth-2", "talker-api.nip.io", authConfig2, false); err != nil {
57+
if err := c.Set("auth-2", "*.pets.com", authConfig2, false); err != nil {
6258
t.Error(err)
6359
}
6460

61+
// Set the more specific host first
6562
if err := c.Set("auth-3", "api.acme.com", authConfig3, false); err != nil {
6663
t.Error(err)
6764
}
6865

66+
// ...and then the more generic one
6967
if err := c.Set("auth-4", "*.acme.com", authConfig4, false); err != nil {
7068
t.Error(err)
7169
}
@@ -130,15 +128,15 @@ func TestAuthConfigTree(t *testing.T) {
130128
assert.Check(t, config == nil)
131129

132130
config = c.Get("talker-api.nip.io")
133-
assert.DeepEqual(t, *config, authConfig1) // because `*.io -> auth-1` is still in the tree
131+
assert.DeepEqual(t, *config, authConfig1) // because `*.io <- auth-1` is still in the tree
134132

135133
config = c.Get("api.acme.com")
136134
assert.DeepEqual(t, *config, authConfig3)
137135

138136
c.Delete("auth-3")
139137

140138
config = c.Get("api.acme.com")
141-
assert.DeepEqual(t, *config, authConfig4) // because `*.acme.com -> auth-4` is still in the tree
139+
assert.DeepEqual(t, *config, authConfig4) // because `*.acme.com <- auth-4` is still in the tree
142140
}
143141

144142
type bogusIdentity struct{}

0 commit comments

Comments
 (0)