Skip to content
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

Label selectors for the AuthConfig #179

Merged
merged 5 commits into from
Nov 3, 2021
Merged

Label selectors for the AuthConfig #179

merged 5 commits into from
Nov 3, 2021

Conversation

guicassolato
Copy link
Collaborator

@guicassolato guicassolato commented Oct 28, 2021

Filters AuthConfig-related events by labels of the resource.

Labels selectors for the instance can be set via newly introduced AUTH_CONFIG_LABEL_SELECTOR environment variable, in the format of zero or more expected-label=expected-value key-value pairs, separated by anything that qualifies as a space in Unicode (i.e. \t, \n, \v, \f, \r, , 0x85, 0xA0) parseable Kubernetes label selectors.

All the following are valid examples of AuthConfig label selector filters:

AUTH_CONFIG_LABEL_SELECTOR="authorino.3scale.net/managed-by=authorino"
AUTH_CONFIG_LABEL_SELECTOR="authorino.3scale.net/managed-by=authorino,other-label=other-value"
AUTH_CONFIG_LABEL_SELECTOR="authorino.3scale.net/managed-by in (authorino,kuadrant)"
AUTH_CONFIG_LABEL_SELECTOR="authorino.3scale.net/managed-by!=authorino-v0.4"
AUTH_CONFIG_LABEL_SELECTOR="!disabled"

By default, no label is required in the AuthConfigs for them to be watched by the Authorino controller – i.e. AUTH_CONFIG_LABEL_SELECTOR is empty and therefore the controller will reconcile all AuthConfigs in the watched space.

Breaking changes:
The environment variable AUTHORINO_SECRET_LABEL_KEY, used to specify labels required in Secrets to be watched by Authorino, was renamed to SECRET_LABEL_SELECTOR and now also expects the same format of zero or more key-value pairs as AUTH_CONFIG_LABEL_SELECTOR.

The name of ConfigMap created to handle the leader election is now determined by the value of AUTH_CONFIG_LABEL_SELECTOR.

Bug fixes:
The PR also fixes a bug in the label filter of the Secret controller that caused removed required labels from existing Secrets not to trigger the reconciliation of the Secret and related AuthConfig.

The PR also fixes a bug in the selection of AuthConfigs related to Secrets reconciliated, to trigger the reconciliation of the AuthConfigs. Now the sets of labels expected in the Secret and matching spec.identity.apiKey.labelSelectors do not have to be identical (checked before with reflect.DeepEqual), but to match as in regular label selector matches performed by Kubernetes, like when the Secrets are listed and added to cache of valid credentials of an identity config:

if err := apiKey.k8sClient.List(ctx, secretList, matchingLabels); err != nil {

As as consequence of the above, the default label selectors used to filter Secret-related events to the reconciler no longer has to be mentioned in spec.identity.apiKey.labelSelectors.

Logging:
Adds a new debug log entry to print instance configuration options during bootstrap, and fixes other log entries related to the reconciliation of AuthConfigs, Secrets and AuthConfig status updates, especially when skipped due to non-watched resource.

Doc updates:
Description of the API key feature in the examples and Architecture page updated to reflect the default label selectors used to filter Secret-related events to the reconciler no longer needed to be mentioned in spec.identity.apiKey.labelSelectors.


Closes #174

@guicassolato guicassolato self-assigned this Oct 28, 2021
controllers/auth_config_controller.go Outdated Show resolved Hide resolved
controllers/auth_config_status_updater.go Outdated Show resolved Hide resolved
controllers/label_selector.go Outdated Show resolved Hide resolved
@eguzki
Copy link
Collaborator

eguzki commented Nov 2, 2021

So, let me explain what I understand to check if I understood correctly.

Atuhorino will have a deployment time env var called SECRET_LABEL_SELECTOR and all secrets matching the label selector will be monitored and added to the cache. Eventually, this SECRET_LABEL_SELECTOR will be configurable in the spec of the authorino CRD from the operator.

When I (as a authorino client) create an AuthConfig for my API KEY based authN, I have basically two options.

A) create the secret with labels matching SECRET_LABEL_SELECTOR selector, then the authconfig looks like this

spec:
  identity:
  - apiKey: {}
    credentials:
      in: authorization_header
      keySelector: APIKEY
    name: MyAPIKey

Note that I do not specify the label selector in the apiKey object.

B) create the secret with my own labels, i.e. MYAPP=MYLABEL, then create authconfig like this:

spec:
  identity:
  - apiKey:
      labelSelectors:
        MYAPP: MYLABEL
    credentials:
      in: authorization_header
      keySelector: APIKEY
    name: MyAPIKey

With option A, any change done in the secret will be reconciled and authorino will enforce new api keys (provided the value in the secret changed).
With option B, the secret will not be reconciled. So authorino will read initially the secret, but changes will not be reconciled. So, the recommended option would be A).

Am I right?

@eguzki
Copy link
Collaborator

eguzki commented Nov 2, 2021

The code LGTM

@guicassolato
Copy link
Collaborator Author

guicassolato commented Nov 3, 2021

Am I right?

Almost 100%, but not quite 🙂 Let me try to explain...

Atuhorino will have a deployment time env var called SECRET_LABEL_SELECTOR and all secrets matching the label selector will be monitored

Correct.

[...] and added to the cache.

Incorrect.

Eventually, this SECRET_LABEL_SELECTOR will be configurable in the spec of the authorino CRD from the operator.

Correct. It is right here: https://github.com/Kuadrant/authorino-operator/blob/6bd2a13104ecb99841dbdf9f040717d29d669fd6/api/v1beta1/authorino_types.go#L78


The SECRET_LABEL_SELECTOR env var already exists, except that right now it is called AUTHORINO_SECRET_LABEL_KEY and it only expects a label key and no label value. This PR changes its name and format, but it doesn't change its meaning.

SECRET_LABEL_SELECTOR is (and will continue to be) used to filter the events related to Secrets that reach the reconciler. Period.

As before, we should use SECRET_LABEL_SELECTOR to avoid the reconciler being overwhelmed with lots of events that are completely irrelevant to the AuthConfigs being managed. (Imagine a cluster-wide deployment of Authorino watching for every single Secret-related event across the entire cluster. This would be neither efficient, nor safe.)


A priori, spec.identity.apiKey.labelSelectors has nothing to do with SECRET_LABEL_SELECTOR. To understand that, we need to understand what each of Authorino's reconcilers do regarding the reconciliation and caching of API keys:

  • AuthConfigReconciler selects all the Secrets that are supposed to be represent API keys. identity.APIKey.GetCredentialsFromCluster(...) lists all Secrets whose labels match AuthConfig.spec.identity.apiKey.labelSelectors and caches them as API keys.

  • SecretReconciler goes through the entire cache of AuthConfigs and, for every AuthConfig whose spec.identity.apiKey.labelSelectors matches the labels of the Secret being reconciled, it triggers the reconciliation of the AuthConfig, calling AuthConfigReconciler.Reconcile(...). It is then AuthConfigReconciler that will reconcile the AuthConfig and refresh the cache of API keys; SecretReconciler does not write to the cache.


What happens if I have SECRET_LABEL_SELECTOR=x and an AuthConfig where spec.identity.apiKey.labelSelectors=y?

  • Whenever the AuthConfig is reconciled, all Secrets where y ⊆ metadata.labels will be cached.
  • Only events related to Secrets where x ⊆ metadata.labels will be watched by the SecretReconciler; among those, only when y ⊆ metadata.labels the AuthConfig will be reconciled and therefore the event reflected in the cache of API keys of the AuthConfig
    • If the x ⊈ metadata.labels but y ⊆ metadata.labels for a given Secret, then events related to this Secret will not be watched and the potentially impacted cache of AuthConfig will only be updated by other means (e.g. if something touches the AuthConfig itself, rather than the Secret)
    • If the x ⊆ metadata.labels but y ⊈ metadata.labels for a given Secret, then SecretReconciler will watch the event, but it won't identify the AuthConfig whose spec.identity.apiKey.labelSelectors=y as potentially impacted and therefore it won't trigger the reconciliation of that AuthConfig, causing its cache of API keys to remain as-is
  • If you want to ensure an AuthConfig to cache only Secrets whose events will be watched by SecretReconciler and yet give it, in the labels of its related Secrets, something that allows to distinguish this AuthConfig from others, set spec.identity.apiKey.labelSelectors=x ∪ y and, of course, make sure x ∪ y ⊆ metadata.labels for every related Secret.
    • If you have an instance of Authorino forever meant to watch only one AuthConfig, set SECRET_LABEL_SELECTOR=spec.identity.apiKey.labelSelectors=x and, of course, x ⊆ metadata.labels should be true for every related Secret.

When I (as a authorino client) create an AuthConfig for my API KEY based authN, I have basically two options.

A) create the secret with labels matching SECRET_LABEL_SELECTOR selector, then the authconfig looks like this

spec:
  identity:
  - apiKey: {}
    credentials:
      in: authorization_header
      keySelector: APIKEY
    name: MyAPIKey

Note that I do not specify the label selector in the apiKey object.

[...] any change done in the secret will be reconciled and authorino will enforce new api keys (provided the value in the secret changed).

Incorrect.

  • In this case, all Secrets in the entire space watched by Authorino are potential API keys valid to authenticate to this service. AuthConfigReconciler will make sure to cache them all.
  • However, only events related to the Secrets where SECRET_LABEL_SELECTOR ⊆ metadata.labels will trigger the reconciliation of the AuthConfig
    • If you have a Secret where SECRET_LABEL_SELECTOR ⊈ metadata.labels and that Secret is updated, the update will not be immediately reflected in the cache of the AuthConfig; only when some other Secret where SECRET_LABEL_SELECTOR ⊆ metadata.labels is updated the cache will be refreshed (entirely)

B) create the secret with my own labels, i.e. MYAPP=MYLABEL, then create authconfig like this:

spec:
  identity:
  - apiKey:
      labelSelectors:
        MYAPP: MYLABEL
    credentials:
      in: authorization_header
      keySelector: APIKEY
    name: MyAPIKey

[...] the secret will not be reconciled. So authorino will read initially the secret, but changes will not be reconciled.

Correct.

So, the recommended option would be A).

The recommended option is always a mix of option A and option B. As long as you understand what SECRET_LABEL_SELECTOR does (limits the scope within the reconciliation space) and what AuthConfig.spec.identity.apiKey.labelSelectors does (selects Secrets as valid API keys whenever the AuthConfig is reconciled), you're good.

To be safe, I'd look for the following configuration:

SECRET_LABEL_SELECTOR ⊆ AuthConfig.spec.identity.apiKey.labelSelectors ⊆ Secret.metadata.labels

To be even safer and have more flexibility with the sets of AuthConfigs and Secrets:

SECRET_LABEL_SELECTOR ⊂ AuthConfig.spec.identity.apiKey.labelSelectors ⊂ Secret.metadata.labels

@eguzki
Copy link
Collaborator

eguzki commented Nov 3, 2021

Super clear, thanks.

I was thinking then that this statement should be in documentation (maybe it is and I missed it):

SECRET_LABEL_SELECTOR ⊂ AuthConfig.spec.identity.apiKey.labelSelectors ⊂ Secret.metadata.labels

I will make sure that kuadrant-controller works accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label selectors for AuthConfigs
2 participants