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.

Sign up for GitHub

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

Set default Authorino image via env var #174

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Set default Authorino image via env var #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ COPY controllers/ controllers/
COPY pkg/ pkg/

ARG VERSION=latest
ARG DEFAULT_AUTHORINO_IMAGE=quay.io/kuadrant/authorino:latest
RUN CGO_ENABLED=0 GO111MODULE=on go build -a -ldflags "-X main.version=${VERSION} -X github.com/kuadrant/authorino-operator/controllers.DefaultAuthorinoImage=${DEFAULT_AUTHORINO_IMAGE}" -o manager main.go
RUN CGO_ENABLED=0 GO111MODULE=on go build -a -ldflags "-X main.version=${VERSION}" -o manager main.go

# Use Red Hat minimal base image to package the binary
# https://catalog.redhat.com/software/containers/ubi9-minimal
Expand Down
12 changes: 8 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,18 +174,19 @@ endif
# Run the tests
test: manifests generate fmt vet setup-envtest
echo $(SETUP_ENVTEST)
KUBEBUILDER_ASSETS='$(strip $(shell $(SETUP_ENVTEST) --arch=amd64 use -p path 1.22.x))' go test -ldflags="-X github.com/kuadrant/authorino-operator/controllers.DefaultAuthorinoImage=$(DEFAULT_AUTHORINO_IMAGE)" ./... -coverprofile cover.out
KUBEBUILDER_ASSETS='$(strip $(shell $(SETUP_ENVTEST) --arch=amd64 use -p path 1.22.x))' go test ./... -coverprofile cover.out

##@ Build

build: generate fmt vet ## Build manager binary.
go build -ldflags "-X main.version=$(VERSION) -X github.com/kuadrant/authorino-operator/controllers.DefaultAuthorinoImage=$(DEFAULT_AUTHORINO_IMAGE)" -o bin/manager main.go
go build -ldflags "-X main.version=$(VERSION)" -o bin/manager main.go

run: export DEFAULT_AUTHORINO_IMAGE := $(DEFAULT_AUTHORINO_IMAGE)
run: manifests generate fmt vet ## Run a controller from your host.
go run -ldflags "-X main.version=$(VERSION) -X github.com/kuadrant/authorino-operator/controllers.DefaultAuthorinoImage=$(DEFAULT_AUTHORINO_IMAGE)" ./main.go
go run -ldflags "-X main.version=$(VERSION)" ./main.go

docker-build: ## Build docker image with the manager.
docker build --build-arg VERSION=$(VERSION) --build-arg DEFAULT_AUTHORINO_IMAGE=$(DEFAULT_AUTHORINO_IMAGE) -t $(OPERATOR_IMAGE) .
docker build --build-arg VERSION=$(VERSION) -t $(OPERATOR_IMAGE) .

docker-push: ## Push docker image with the manager.
docker push ${OPERATOR_IMAGE}
Expand Down Expand Up @@ -234,6 +235,7 @@ DEPLOYMENT_FILE = $(DEPLOYMENT_DIR)/manifests.yaml
deploy-manifest:
mkdir -p $(DEPLOYMENT_DIR)
cd $(PROJECT_DIR)/config/manager && $(KUSTOMIZE) edit set image controller=$(OPERATOR_IMAGE) ;\
echo "DEFAULT_AUTHORINO_IMAGE=$(DEFAULT_AUTHORINO_IMAGE)" > $(PROJECT_DIR)/config/manager/env-vars.env
cd $(PROJECT_DIR) && $(KUSTOMIZE) build config/deploy > $(DEPLOYMENT_FILE)
# clean up
cd $(PROJECT_DIR)/config/manager && $(KUSTOMIZE) edit set image controller=${DEFAULT_OPERATOR_IMAGE}
Expand Down Expand Up @@ -325,6 +327,7 @@ verify-manifests: manifests $(YQ) ## Verify manifests update.
[ -z "$$(git ls-files --other --exclude-standard --directory --no-empty-directory ./config)" ]
$(YQ) ea -e 'select([.][].kind == "Deployment") | select([.][].metadata.name == "authorino-operator").spec.template.spec.containers[0].image | . == "$(OPERATOR_IMAGE)"' config/deploy/manifests.yaml
$(YQ) ea -e 'select([.][].kind == "Deployment") | select([.][].metadata.name == "authorino-webhooks").spec.template.spec.containers[0].image | . == "$(EXPECTED_DEFAULT_AUTHORINO_IMAGE)"' config/deploy/manifests.yaml
$(YQ) ea -e 'select([.][].kind == "ConfigMap") | select([.][].metadata.name == "authorino-operator-env").data.DEFAULT_AUTHORINO_IMAGE | . == "$(EXPECTED_DEFAULT_AUTHORINO_IMAGE)"' config/deploy/manifests.yaml
$(YQ) e -e '.metadata.annotations.containerImage == "$(OPERATOR_IMAGE)"' config/manifests/bases/authorino-operator.clusterserviceversion.yaml

.PHONY: verify-bundle
Expand All @@ -334,6 +337,7 @@ verify-bundle: bundle $(YQ) ## Verify bundle update.
$(YQ) e -e '.metadata.annotations.containerImage == "$(OPERATOR_IMAGE)"' $(BUNDLE_CSV)
$(YQ) e -e '.spec.install.spec.deployments[0].spec.template.spec.containers[0].image == "$(OPERATOR_IMAGE)"' $(BUNDLE_CSV)
$(YQ) e -e '.spec.install.spec.deployments[1].spec.template.spec.containers[0].image == "$(EXPECTED_DEFAULT_AUTHORINO_IMAGE)"' $(BUNDLE_CSV)
$(YQ) e -e '.data.DEFAULT_AUTHORINO_IMAGE == "$(EXPECTED_DEFAULT_AUTHORINO_IMAGE)"' ./bundle/manifests/authorino-operator-env_v1_configmap.yaml

.PHONY: verify-fmt
verify-fmt: fmt ## Verify fmt update.
Expand Down
17 changes: 8 additions & 9 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,24 @@ to match the latest <git-ref>.
* Tons of Authorino docs and examples rely on this
* The `manifests` make target is used to generate the compiled version of all manifests (operator’s and operand’s).
It accepts as parameters:

* `VERSION`: sets the version of the manifests of the operator generated by the operator-sdk command and image tag of
the deployment added to `config/deploy/manifests.yaml` (default: latest)
* `AUTHORINO_VERSION`: sets the operand’s branch that sources the manifests (CRD, role definitions) that are appended
to `config/deploy/manifests.yaml` (default: latest)

* Changes in the operand’s CRD/role definitions out date the `config/deploy/manifests.yaml` file, thus requiring a PR to
the operator repo ([example](https://github.com/Kuadrant/authorino-operator/pull/68))

* The `verify-manifests` make target is used to verify consistency of the manifests
* Triggered in the CI tests
* Causes all PRs in the Operator repo to break whenever there’s a change in the operand’s API committed to the main
branch → requires immediate update of the copy of operand’s manifests in the main branch of the operator and rebase
of all PRs

* Apart from the operand’s manifests pinned to a specific or latest version in the copy committed to
`config/deploy/manifests.yaml`, the Operator’s code contains hard-coded references to the default version of the operand
to install:
* Set at compilation time ([`controllers.DefaultAuthorinoImage`](https://github.com/Kuadrant/authorino-operator/blob/03b42633627337bc7d908e36e579340a0d8e12fb/controllers/constants.go#L95))

* Each build of the operator is matched to a default Auhtorino image:
* Controlled by the value of the `DEFAULT_AUTHORINO_IMAGE` environment variable set in the operator's deployment
* Should match the version of the `authorino-webhook` deployment
* Configured in the `build.yaml` file ([example](https://github.com/Kuadrant/authorino-operator/blob/release-v0.10.0/build.yaml))
* It can be overwritten in the Authorino CR → good for dev/test/staging workflows
* Set to "latest" in the main branch
* For individual Authorino instances, it can be overwritten in the Authorino CR → good for dev/test/staging workflows
6 changes: 6 additions & 0 deletions bundle/manifests/authorino-operator-env_v1_configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: v1
data:
DEFAULT_AUTHORINO_IMAGE: quay.io/kuadrant/authorino:latest
kind: ConfigMap
metadata:
name: authorino-operator-env
6 changes: 6 additions & 0 deletions bundle/manifests/authorino-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,12 @@ spec:
- --leader-elect
command:
- /manager
env:
- name: DEFAULT_AUTHORINO_IMAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

RELATED_IMAGE_AUTHORINO

In the operator bundle building pipeline, there is some logic to catch RELATED_IMAGE_* env vars to get dependency images and build dependency tree used for some verification steps. I can try to find the docs out there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is about automatic rebuild of the bundle when there is a rebuild of one of the dependencies to catch the latest builds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the specific case of Authorino, I worry that RELATED_IMAGE_ could be bit misleading, because of the override in the Authorino CR.

Not that changing would be a big deal for me. I just didn't want to convey that it means the same thing as it does in Limitador.

Copy link
Contributor

@eguzki eguzki Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I do not follow. The usual order of preference is:

  1. Authorino CR -> image name
  2. CSV -> env var RELATED_IMAGE_AUTHORINO of the container (also useful for dev/testing)
  3. Default image hardcoded in some const default variable (usually latest)

But, your call!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Authorino CR -> image name
  2. CSV -> env var RELATED_IMAGE_AUTHORINO of the container (also useful for dev/testing)

Exactly how you described, but notice that Limitador is not the same, right? (Or maybe it is and I missed that in the Limitador CR.)

Last time I checked, in Authorino Operator, the related Authorino image is a default value that can be overridden in the Authorino CR. Whereas in Limitador Operator, the related Limitador image is NOT a default value that can be overridden anywhere; rather, if set, the value of the env var is final.

IOW, Limitador Operator has a "related operand image"; Authorino Operator has a "default related operand image", if that makes sense.

  1. Default image hardcoded in some const default variable (usually latest)

Another difference between Limitador and Authorino operators here.

Notice that, for Authorino Operator, we did NOT hard code a default image value for the case where the env var is missing. I'd argue that breaking the deployments if the env var is missing is better than hiding a silent potential problem that could happen by bumping all operands to latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly how you described, but notice that Limitador is not the same, right? (Or maybe it is and I missed that in the Limitador CR.)

In the limitador operator the order of preference is as I described.

  1. If the image is set in the CR, then use that here
image := GetLimitadorImageVersion()
if limitador.Spec.Version != nil {
        image = fmt.Sprintf("%s:%s", LimitadorRepository, *limitador.Spec.Version)
}
  1. CSV -> env var RELATED_IMAGE_LIMITADOR here
func GetLimitadorImageVersion() string {
	return env.GetString("RELATED_IMAGE_LIMITADOR", defaultImageVersion)
}

3.- Default image hardcoded in some const default variable (usually latest)

var (
	defaultImageVersion = fmt.Sprintf("%s:%s", LimitadorRepository, "latest")
)

Notice that, for Authorino Operator, we did NOT hard code a default image value for the case where the env var is missing. I'd argue that breaking the deployments if the env var is missing is better than hiding a silent potential problem that could happen by bumping all operands to latest.

Not a bad idea. But in practice, the CSV will always have a related image (many pipeline verification steps ensure that).
Having a default hardcoded image in case it is not set in the CR nor env var is handy for dev/testing. Anyway, I also like your idea to force some env var but not a big deal and never had issues with that.

Copy link
Contributor

@eguzki eguzki Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is one caveat in the limitador CR that I was not approved to implement: from the CR you can only especify the docker tag and the registry is hardcoded. I need to write RFC to fix that and blah blah blah... and in the end, the image set in the limitador CR is not a supported thing, just useful for dev/testing and a way out in case the user wants to use their own image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL: one can override the env var in the Limitador CR. Well, at least the image tag part. Thanks, @eguzki!

valueFrom:
configMapKeyRef:
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 rationale to get the image url from the config map? why not a plain env var? The CSV will be the source of truth.

new authorino image -> requires new operator bundle -> new CSV.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so we can rely on kustomize tooling to set the variable value dynamically when we run make manifests (also triggered on make bundle.) There's a configMapGenerator that picks up the dynamic value.

Alternative approaches we employ for other customisations are:

  1. using envsubst, or
  2. directly setting the value in the config/manager/manager.yaml with yq.

Happy to change if you prefer any of these other options.

Copy link
Contributor

Choose a reason for hiding this comment

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

The config map is yet another resource of the bundle. When there is a new image, even though the CSV does not change (not entirely true, check my next comment), a new bundle is needed anyway because the config map changes.

Your call. Just keep in mind that the image of authorino URL is also referenced in the CSV in relatedImages section, so the CSV needs to change anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like adding the ConfigMap either. I'll refactor this.

key: DEFAULT_AUTHORINO_IMAGE
name: authorino-operator-env
image: quay.io/kuadrant/authorino-operator:latest
livenessProbe:
httpGet:
Expand Down
14 changes: 14 additions & 0 deletions config/deploy/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6095,6 +6095,14 @@ subjects:
namespace: authorino-operator
---
apiVersion: v1
data:
DEFAULT_AUTHORINO_IMAGE: quay.io/kuadrant/authorino:latest
kind: ConfigMap
metadata:
name: authorino-operator-env
namespace: authorino-operator
---
apiVersion: v1
data:
controller_manager_config.yaml: |
apiVersion: controller-runtime.sigs.k8s.io/v1alpha1
Expand Down Expand Up @@ -6152,6 +6160,12 @@ spec:
- --leader-elect
command:
- /manager
env:
- name: DEFAULT_AUTHORINO_IMAGE
valueFrom:
configMapKeyRef:
key: DEFAULT_AUTHORINO_IMAGE
name: authorino-operator-env
image: quay.io/kuadrant/authorino-operator:latest
livenessProbe:
httpGet:
Expand Down
1 change: 1 addition & 0 deletions config/manager/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
env-vars.env
9 changes: 7 additions & 2 deletions config/manager/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- manager.yaml

Expand All @@ -8,8 +11,10 @@ configMapGenerator:
- files:
- controller_manager_config.yaml
name: manager-config
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
- envs:
- env-vars.env
name: authorino-operator-env

images:
- name: controller
newName: quay.io/kuadrant/authorino-operator
Expand Down
6 changes: 6 additions & 0 deletions config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ spec:
- /manager
args:
- --leader-elect
env:
- name: DEFAULT_AUTHORINO_IMAGE
valueFrom:
configMapKeyRef:
name: authorino-operator-env
key: DEFAULT_AUTHORINO_IMAGE
image: controller:latest
name: manager
securityContext:
Expand Down
4 changes: 3 additions & 1 deletion controllers/authorino_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ type AuthorinoReconciler struct {
client.Client
Log logr.Logger
Scheme *runtime.Scheme

DefaultAuthorinoImage string
}

//+kubebuilder:rbac:groups=operator.authorino.kuadrant.io,resources=authorinos,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -192,7 +194,7 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
var saName = authorino.Name + "-authorino"

if authorino.Spec.Image == "" {
authorino.Spec.Image = DefaultAuthorinoImage
authorino.Spec.Image = r.DefaultAuthorinoImage
}

var volumes []k8score.Volume
Expand Down
6 changes: 2 additions & 4 deletions controllers/authorino_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,13 @@ var _ = Describe("Authorino controller", func() {
}, testTimeout, testInterval).Should(BeTrue())

replicas := int32(testAuthorinoReplicas)
image := DefaultAuthorinoImage
existContainer := false

Expect(deployment.Spec.Replicas).Should(Equal(&replicas))
Expect(deployment.Labels).Should(Equal(map[string]string{"thisLabel": "willPropagate"}))
for _, container := range deployment.Spec.Template.Spec.Containers {
if container.Name == authorinoContainerName {
Expect(container.Image).Should(Equal(image))
Expect(container.Image).Should(Equal("authorino:latest"))
Expect(container.ImagePullPolicy).Should(Equal(k8score.PullAlways))
checkAuthorinoArgs(authorinoInstance, container.Args)
Expect(len(container.Env)).Should(Equal(0))
Expand Down Expand Up @@ -238,7 +237,6 @@ func newExtServerConfigMap() *k8score.ConfigMap {

func newFullAuthorinoInstance() *api.Authorino {
name := "a" + string(uuid.NewUUID())
image := DefaultAuthorinoImage
replicas := int32(testAuthorinoReplicas)
tslEnable := true
tlsDisabled := false
Expand All @@ -254,7 +252,7 @@ func newFullAuthorinoInstance() *api.Authorino {
Labels: map[string]string{"thisLabel": "willPropagate"},
},
Spec: api.AuthorinoSpec{
Image: image,
Image: "authorino:latest",
Replicas: &replicas,
ImagePullPolicy: k8score.PullAlways,
Volumes: api.VolumesSpec{
Expand Down
3 changes: 0 additions & 3 deletions controllers/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,3 @@ const (
statusUnableToUpdateDeployment = "UnableToUpdateDeployment"
statusDeploymentNotReady = "DeploymentNotReady"
)

// ldflags
var DefaultAuthorinoImage string
7 changes: 4 additions & 3 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ var _ = BeforeSuite(func() {
Expect(k8sClient.Create(context.TODO(), newCertSecret())).Should(Succeed())

err = (&AuthorinoReconciler{
Client: k8sClient,
Log: ctrl.Log.WithName("authorino-operator").WithName("controller").WithName("Authorino"),
Scheme: mgr.GetScheme(),
Client: k8sClient,
Log: ctrl.Log.WithName("authorino-operator").WithName("controller").WithName("Authorino"),
Scheme: mgr.GetScheme(),
DefaultAuthorinoImage: "authorino:latest",
}).SetupWithManager(mgr)
Expect(err).ToNot(HaveOccurred())

Expand Down
18 changes: 10 additions & 8 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ import (
)

var (
scheme = runtime.NewScheme()
setupLog = ctrl.Log.WithName("setup")
logger log.Logger
version string // value injected in compilation-time
scheme = runtime.NewScheme()
setupLog = ctrl.Log.WithName("setup")
logger log.Logger
version string // value injected at compilation-time
defaultAuthorinoImage = os.Getenv("DEFAULT_AUTHORINO_IMAGE")
)

func init() {
Expand Down Expand Up @@ -89,7 +90,7 @@ func main() {

ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))

setupLog.Info("booting up authorino operator", "version", version, "default authorino image", controllers.DefaultAuthorinoImage)
setupLog.Info("booting up authorino operator", "version", version, "default authorino image", defaultAuthorinoImage)

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
Expand All @@ -105,9 +106,10 @@ func main() {
}

if err = (&controllers.AuthorinoReconciler{
Client: mgr.GetClient(),
Log: logger,
Scheme: mgr.GetScheme(),
Client: mgr.GetClient(),
Log: logger,
Scheme: mgr.GetScheme(),
DefaultAuthorinoImage: defaultAuthorinoImage,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Authorino")
os.Exit(1)
Expand Down
Loading