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

RELATED_IMAGE_AUTHORINO env var #229

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Nov 14, 2024

What

Authorino image can be read from RELATED_IMAGE_AUTHORINO env var. Defaults to quay.io/kuadrant/authorino:latest

Required to specify the authorino image from the CSV manifest

The authorino image being used in the deployment is read from different sources with some order of precedence:

  1. Authorino CR's spec.image
  2. RELATED_IMAGE_AUTHORINO env var
  3. Value set at the operator compilation time passed as ldflags -X github.com/kuadrant/authorino-operator/controllers.DefaultAuthorinoImage=VALUE
  4. quay.io/kuadrant/limitador:latest

Verification steps

run cluster

kind create cluster --name authorino-operator-system

install CRDs

make install

run operator locally with specific RELATED_IMAGE_AUTHORINO env var.

RELATED_IMAGE_AUTHORINO=quay.io/kuadrant/authorino:v0.18.0 make run

Create authorino object without spec.image to deploy version from env var.

k apply -f - <<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
EOF

Check authorino's version is quay.io/kuadrant/authorino:v0.18.0

$ kubectl get deployments authorino -o jsonpath='{.spec.template.spec.containers[0].image}'

The response should be

quay.io/kuadrant/authorino:v0.18.0

Clean up

kind delete cluster --name authorino-operator-system

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.54%. Comparing base (66594a6) to head (f0150c1).

Files with missing lines Patch % Lines
controllers/authorino_controller.go 25.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
- Coverage   61.78%   61.54%   -0.24%     
==========================================
  Files           2        2              
  Lines         785      788       +3     
==========================================
  Hits          485      485              
- Misses        249      251       +2     
- Partials       51       52       +1     
Flag Coverage Δ
unit 61.54% <25.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eguzki
Copy link
Contributor Author

eguzki commented Nov 14, 2024

supersedes #174

Comment on lines 202 to 203
// `DefaultAuthorinoImage can be empty string. But image cannot be or deployment will fail
image = "quay.io/kuadrant/authorino:latest"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is dangerous. DefaultAuthorinoImage should never be empty. If we're keeping this build-arg variable as an option to set the default Authorino image, then IMO the variable should be set at every build to a non-empty value.

If this variable can be empty, I think I'd rather break the operator than make it silently reset all Authorino instances in the cluster to latest if the RELATED_IMAGE_AUTHORINO is removed from the deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, panicking if it is empty

Copy link
Member

@adam-cattermole adam-cattermole left a comment

Choose a reason for hiding this comment

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

Assuming @guicassolato is happy with the proposed solution, I've verified this locally with the various different settings for DEFAULT_AUTHORINO_IMAGE, .spec.image and RELATED_IMAGE_AUTHORINO and it all seems to be working well (and following intended precedence)

@eguzki
Copy link
Contributor Author

eguzki commented Nov 14, 2024

he requested changes, so I cannot shoud not merge even with one approval.

Copy link
Collaborator

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

I'm happy with panicking if it goes all the way down and DefaultAuthorinoImage is empty. Thanks for addressing it!

I also like the explanation about the order of precedence in the description of the PR. We should probably add it to the last paragraph of https://github.com/Kuadrant/authorino-operator/blob/main/RELEASE.md (otherwise RELATED_IMAGE_AUTHORINO in undocumented).

But I'll approve the PR now, in light that it has been tested and it works. Docs can be added later.

Thanks, @eguzki and @adam-cattermole!

Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
@eguzki eguzki force-pushed the authorino-version-from-env-var branch from 76f7331 to f0150c1 Compare November 14, 2024 14:14
@eguzki
Copy link
Contributor Author

eguzki commented Nov 14, 2024

Signed the commits

@eguzki eguzki merged commit ee36d38 into main Nov 14, 2024
11 checks passed
@eguzki eguzki deleted the authorino-version-from-env-var branch November 14, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants