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

Jump to bottom

Update make uninstall #191

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Update make uninstall #191

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a865238
Update uninstall
Boomatang Jul 8, 2024
f5e773c
Refactor deployment commands to have `<action>` and `un<action>`
Boomatang Jul 22, 2024
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
10 changes: 10 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ install: manifests kustomize install-authorino ## Install CRDs into the K8s clus

uninstall: manifests kustomize ## Uninstall CRDs from the K8s cluster specified in ~/.kube/config.
kubectl delete -f $(OPERATOR_MANIFESTS)
$(MAKE) uninstall-authorino
Copy link
Collaborator

Choose a reason for hiding this comment

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

make install does not install Authorino CRDs. I wonder if make uninstall should indeed try to undo what its counterpart did not do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The make install does install the Authorino CRD, it is part of OPERATOR_MANIFESTS. The adding of the make uninstall-authorino is to mirror how the make install calls install-authorino. This change aligns closer with what its counterpart does.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My confusion. OPERATOR_MANIFESTS points to this file, which does not include Authorino CRDs. However, install depends on install-authorino. Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I am missing something. The very first file in that manifest is the authorino CRD. What am I missing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Authorino CRD belongs to the operator. AuthConfig CRD is the one that belongs to Authorino.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I get you now when you said Authorio CRDs you meant Authorino as in Authorino the product and not the CRD kind. Happy that confusion was cleared up.
So to be sure, are you asking for anything to be changed here?


deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
cd config/manager && $(KUSTOMIZE) edit set image controller=${OPERATOR_IMAGE}
Expand All @@ -222,12 +223,21 @@ undeploy: ## Undeploy controller from the K8s cluster specified in ~/.kube/confi
$(KUSTOMIZE) build config/default | kubectl delete -f -

install-authorino: install-cert-manager ## install RBAC and CRD for authorino
kubectl create namespace authorino-operator
$(KUSTOMIZE) build config/authorino | kubectl apply -f -

uninstall-authorino: ## uninstall RBAC and CRD for authorino
$(KUSTOMIZE) build config/authorino | kubectl delete -f -
kubectl delete namespace authorino-operator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deleting the operator's namespace seems a bit harsh. Effectively, this will decommission the operator instance. IWO, it's almost like a "undeploy" target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya the namespace thing was something that kept biting me when doing this. As the create of the namespace is documented in the README I would be happy the remove the changes I made around the namespace.

$(MAKE) uninstall-cert-manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we install cert-manager as part make install-authorino. We probably shouldn't uninstall it here either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The make install-authorino does call install-cert-manager. This is aligning the install and uninstall targets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that now.

Tricky one. Installing when it's already present causes less harm than uninstalling when you aren't supposed to. Not sure what best here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I see your point. The question I find my self asking is, what is the purpose of the make targets? Are they how we recommend the user installs the operator in production? If so how do we handle upgrades? Or is the purpose to aid developers in the day to day develop of the operator? In which case it would be better to revert all changes to a cluster? I am also not sure which is best but I do lean to the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the purpose of the make targets?

Mainly dev and testing workflows, I'd say.

Are they how we recommend the user installs the operator in production?

No.

Or is the purpose to aid developers in the day to day develop of the operator?

Yes.

it would be better to revert all changes to a cluster? I am also not sure which is best but I do lean to the latter.

I was thinking... These make targets are useful for people who clone the repo, maybe they make changes, and want to try them out locally. They may no know exactly which YAMLs to apply for each step to of the workflow and therefore the targets provide them with a bit of guidance/documentation.

We cannot predict the exact scenario that development/test work is going on. E.g. maybe it's for a change in the controller's code, maybe it's a change in the Authorino CRD, maybe it's because of a change in Authorino itself that requires some tweak in the operator to be tested, maybe it's running in a fresh newly created cluster, maybe it's running on a preexisting cluster with preexisting stuff installed, maybe cert-manager is already running (installed for something else), maybe it's the first time the operator is installed, maybe it's a re-install, maybe it's to test a new version of CRDs on a preexisting install, etc.

With that in mind, perhaps a reasonable approach is providing very granular targets, for very specific operations, alongside with fewer more opinionated ones. For example,

  • install: Installs Operator CRD/RBAC, plus Authorino CRD/RBAC

  • install-operator: Installs Operator CRD/RBAC

  • install-authorino: Installs Authorino CRD/RBAC

  • install-cert-manager: Installs cert-manager

  • uninstall: Uninstalls Operator CRD/RBAC, plus Authorino CRD/RBAC

  • uninstall-operator: Uninstalls Operator CRD/RBAC

  • uninstall-authorino: Uninstalls Authorino CRD/RBAC

  • uninstall-cert-manager: Uninstalls cert-manager

  • namespace: Creates authorino-operator namespace

  • delete-namespace: Deletes authorino-operator namespace

  • deploy: Creates Deployments of the Operator and webhooks; depends on install, cert-manager, namespace

  • deploy-webhooks: Creates Deployment of the webhook; depends on install-authorino, cert-manager, namespace

  • undeploy: Deletes Deployments of the Operator and webhooks

  • undeploy-webhooks: Deletes Deployment of the webhook

  • run: Runs the operator locally (with go run); depends on install

I guess the pattern being:

  • <action> = runs deps + action itself; safe to run if deps already present
  • un<action> = undo the action only

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, it is this idea of easy of use. The idea that <action> installs the required dependencies makes for a very easy of use. But not having the same easy of use for the un<action> seems wrong or at least creates a double standard. At the same time that issue of undoing something that was existing pre <action> is far worse.

While I think this is a compromise and contradiction in usability, it is also possible the correct solution for now. Change would need some docs update also and the scope of some current targets would need to be reduced. The example off the top of my head is how the install target in a nest way also calls install-cert-manager. Which I think is not what your expecting to happen.


install-cert-manager: ## install the cert manager need for the web hooks
kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v${CERT_MANAGER_VERSION}/cert-manager.yaml
kubectl -n cert-manager wait --timeout=300s --for=condition=Available deployments --all

uninstall-cert-manager: ## uninstall the cert manager need for the web hooks
kubectl delete -f https://github.com/cert-manager/cert-manager/releases/download/v${CERT_MANAGER_VERSION}/cert-manager.yaml

# go-get-tool will 'go install' any package $2 and install it to $1.
PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
define go-get-tool
Expand Down
Loading