-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable/disable host name collision prevention for strict host subsets #434
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really one minor comment...
@@ -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 indexedResourceId, found := r.Index.FindId(host); found && indexedResourceId != resourceId && (!r.AllowSupersedingHostSubsets || utils.SliceContains(r.Index.FindKeys(indexedResourceId), host)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd maybe extract that one bool condition into a method of the reconciler... really a minor thing, but I find it makes it really hard to read when &&
and ||
are mixed, and when there is a !
in there just to screws with my brain ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
ee7448e
to
60600fe
Compare
Exposes Authorino's `--allow-superseding-host-subsets` command-line flag (Kuadrant/authorino#434) as a new API option `spec.supersedingHostSubsets: Boolean` (default: `false`)
…143) Exposes Authorino's `--allow-superseding-host-subsets` command-line flag (Kuadrant/authorino#434) as a new API option `spec.supersedingHostSubsets: Boolean` (default: `false`)
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 the reconciler 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.
Verification steps
① Setup
② Activate the switch
Avoid the Authorino Operator to reconcile the changes back:
Modify the Authorino deployment activating the switch:
Add the
--allow-superseding-host-subsets
command-line flag to the list ofargs
of the pod.③ Create AuthConfigs in an order that otherwise would result in one of them being rejected
More generic:
More specific:
Verify both AuthConfigs have been accepted:
④ Create another AuthConfig that tries to fully supersede a hostname already taken
Verify all the right AuthConfigs are accepted and rejected:
⑤ Send requests to the sample API and confirm the right AuthConfigs are being enforced
Expose the proxy:
More specific auth scheme:
curl http://talker-api.127.0.0.1.nip.io:8000 -i # HTTP/1.1 401 Unauthorized
More generic auth scheme:
curl http://other.127.0.0.1.nip.io:8000 -i # HTTP/1.1 403 Forbidden
⑥ (Optional) Verify that bootstrapping the index with preexisting resources keeps the state consistsent