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

Configure Authorino using command-line flags #103

Merged
merged 4 commits into from
Dec 15, 2022
Merged

Conversation

guicassolato
Copy link
Collaborator

@guicassolato guicassolato commented Dec 14, 2022

  • Configure Authorino using command-line flags.
  • Add support for the Authorino healthz/readiness probe addr flag
  • Stops reconciling env vars in the Authorino deployment – users are now free to modify them directly by editing the deployment, as support for them shall be dropped soon from the Operator.

The Operator tries to detect the version of Authorino being deployed (based on the image tag). Whenever it is a version known to be configurable only via env vars (<= 0.10.0), it uses the deprecated way to inject the settings, thus keeping backward compatibility; otherwise, it uses command-line flags. The detection is not perfect, only working when the image tag follows the pattern v0\.\d+\..+. When an "old" version is not detected, the Operator defaults to command-line args.

Closes #101.


Verification steps

kind create cluster
make docker-build OPERATOR_IMAGE=authorino-operator:local
kind load docker-image authorino-operator:local
sed -e 's/quay.io\/kuadrant\/authorino-operator:latest/authorino-operator:local/' config/deploy/manifests.yaml | kubectl apply -f -

With a new Authorino version - configurable using command-line args:

kubectl apply -f -<<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
  logLevel: debug
  replicas: 2
EOF
kubectl get deployment/authorino -o jsonpath='{.spec.template.spec.containers[0].env}'
#
kubectl get deployment/authorino -o jsonpath='{.spec.template.spec.containers[0].args}'
# ["--watch-namespace=default","--log-level=debug","--enable-leader-election"]

With an old Authorino version - configurable using env vars:

kubectl apply -f -<<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  image: quay.io/kuadrant/authorino:v0.10.0
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
  logLevel: debug
  replicas: 2
EOF
kubectl get deployment/authorino -o jsonpath='{.spec.template.spec.containers[0].env}'
# [{"name":"WATCH_NAMESPACE","value":"default"},{"name":"LOG_LEVEL","value":"debug"}]
kubectl get deployment/authorino -o jsonpath='{.spec.template.spec.containers[0].args}'
# ["--enable-leader-election"]

kind delete cluster

@guicassolato guicassolato self-assigned this Dec 14, 2022
@guicassolato guicassolato requested a review from a team December 14, 2022 14:11
@@ -128,6 +128,7 @@ Each [`Authorino`](https://github.com/Kuadrant/authorino-operator/tree/main/conf
| listener | [Listener](#listener) | Specification of the authorization service (gRPC interface). | Required |
| oidcServer | [OIDCServer](#oidcserver) | Specification of the OIDC service. | Required |
| metrics | [Metrics](#metrics) | Configuration of the metrics server (port, level). | Optional |
| healthz | [Healthz](#healthz) | Configuration of the health/readiness probe (port). | Optional |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the intention behind exposing the health check port to the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's related to this issue: Kuadrant/authorino#355

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I still do not get it after reading that. I will try to be more clear. Why expose the port to the user? what is the value added? The operator can enable healthcheck in authorino and use the default port for it. Maybe the healthcheck port is wanted to be monitored externally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for the principle more than anything else. This is a configuration option in the operand. As we don't want users touching the deployment directly, we expose it in the API – in this case, with some type-checking added.

The same is done for other configuration options that arguably might be otherwise more rigid, e.g. the metrics endpoint port number (spec.metrics.port).

If this should be configurable in the operand in the first place, maybe it's a question we should ask in https://github.com/kuadrant/authorino.

@eguzki
Copy link
Contributor

eguzki commented Dec 14, 2022

Looks good to me regarding the command line args.

exposing the healthcheck port in the CRD should have been done in another PR. They do not seem to be related anyhow.. But we are agile :)

@guicassolato
Copy link
Collaborator Author

guicassolato commented Dec 14, 2022

@eguzki

exposing the healthcheck port in the CRD should have been done in another PR. They do not seem to be related anyhow.. But we are agile :)

Only thing linking these two changes together is the fact that the health check config in Authorino was introduced after it had moved to command-line args. Since this was a pending setting option of the operand yet to be exposed at the level of the operator (in the Authorino CR) and we knew the Operator had to move to command-line args anyway, I didn't want to introduce this field in the API while the Operator was still and mostly focused on the env vars.

That said, you're not wrong. The two changes could be have been presented separately. Either the health-probe-addr flag could have been added first together with the other 2 flags (metrics-addr and enable-leader-election) and then fall naturally into this as part of the refactoring; or we moved to command-line args first (i.e. this PR minus health-probe-addr) and then introduced health-probe-addr. Either way, these things would come in a pack anyway.

@guicassolato guicassolato merged commit 5db1a8a into main Dec 15, 2022
@guicassolato guicassolato deleted the cmd-line-flags branch December 15, 2022 10:40
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.

Configure Authorino using command-line flags
2 participants