duplicited CRD in dist/install.yaml

[lukas016]

What broke? What's expected?

current make build-install generate dist/install.yaml from config/crd and config/default.
config/default internally use config/crd too. Result are 2 same definitions of CRD in dist/install.yaml

cat -n dist/install.yaml| grep 'Test'
    10	    kind: Test
    11	    listKind: TestList
    19	        description: Test is the Schema for the tests API
    39	            description: TestSpec defines the desired state of Test
    42	                description: Foo is an example field of Test. Edit test_types.go to
    47	            description: TestStatus defines the observed state of Test
    77	    kind: Test
    78	    listKind: TestList
    86	        description: Test is the Schema for the tests API
   106	            description: TestSpec defines the desired state of Test
   109	                description: Foo is an example field of Test. Edit test_types.go to
   114	            description: TestStatus defines the observed state of Test

Reproducing this issue

1. Use CRD in project
2. Call make build-installer
3. Find duplicity in dist/install.yaml

KubeBuilder (CLI) Version

master

PROJECT version

3

Plugin versions

No response

Other versions

No response

Extra Labels

No response

[lukas016]

@camilamacedo86 can we continue in discussion here because current implementation has problem with duplication of CRDs.

Do you want to generate CRD always independently on config/default?

[camilamacedo86]

Hi @lukas016,

Feel free to check it and provide a solution that works
The goal is to generate the YAML (bundle) with all required/scaffolds to install/distribute the Operator.

[antonosmond]

Just to add to this, if you use webhooks, the duplicated CRDs are actually different.
If you run make build-installer, the make command is creating the first copy of the CRD with:

$(KUSTOMIZE) build config/crd > dist/install.yaml

In this version the webhook.clientConfig won't have the namePrefix or namespace override.

The second copy of the CRD is generated with:

$(KUSTOMIZE) build config/default >> dist/install.yaml

The config/default/kustomization.yaml includes - ../crd but this version does patch the webhook.clientConfig so you end up with 2 conflicting copies of the CRD, one unpatched e.g:

    webhook:
      clientConfig:
        service:
          name: webhook-service
          namespace: system
          path: /convert

and the other patched e.g.

    webhook:
      clientConfig:
        service:
          name: blueprint-controller-webhook-service
          namespace: kloudy-system
          path: /convert

[antonosmond]

Was gonna submit a PR to fix this but looks like someone else is already on it: #3814

[lukas016]

@antonosmond good point and thx, i wanted look on it but i was busy. I wrote you one suggestion.

[lukas016]

@camilamacedo86 his solution is completely identical with deploy job. Do we want to keep it independent? Because we can use dist/install.yaml as input for target deploy and build-installer can be dependency.
During create commit you can decide with git, if you want to commit changes in dist/install.yaml or not.

dist/install.yaml will always update with deploy or manually with build-installer, what can be helpful because i always forget call generate for this project what is for me similar problem.

[mateusoliveira43]

sorry not looking for issues prior to opening PR #3814

I like your idea @lukas016 ! Something like this, right?

 .PHONY: deploy
-deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
+deploy: build-installer ## Deploy controller to the K8s cluster specified in ~/.kube/config.
-	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
-	$(KUSTOMIZE) build config/default | $(KUBECTL) apply -f -
+      $(KUBECTL) apply -f dist/install.yaml

 .PHONY: undeploy
-undeploy: kustomize ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
+undeploy: build-installer ## Undeploy controller from the K8s cluster specified in ~/.kube/config. Call with ignore-not-found=true to ignore resource not found errors during deletion.
-	$(KUSTOMIZE) build config/default | $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f -
+      $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f dist/install.yaml

(can be a follow up PR to not increase size of created PR)

[mateusoliveira43]

Another question related to topic: should we delete install and uninstall commands from generated Makefile?

Basically, deploy and undeploy commands will already install and uninstall CRDs (and as @antonosmond caught the difference between commands, deploy/undeploy seem more correct). Is there a use case that installing just the CRDs is necessary?

[lukas016]

current solution and this new one has one problem. If you call make undeploy, you don't have guarantee order how are resources deleted. I had very often problem with leftover, when i forgot delete resources for CRD before call undeploy. Result was stuck deletion process. Deployment was delete before deleting of resources which were marked for deletion because CRD was marked for deletion with undeploy but deletion of CRD was stuck because resources had finalizer from controller which was removed.

Do we want to somehow solve this dependency chain? If we want to solve it, we will need to keep install/uninstall targets probably and ideally crd won't be deleted with undeploy.

I don't think it is critical issues and another operators have similar issue.

[mateusoliveira43]

yeah, I face this problem in one of the projects I work on. Before running undeploy/uninstall, I have to force delete all CRs (instances of CRDs).

What we do in the project is use this command (example with Restore CR)

cleanup:
    $(KUBECTL) delete restore -n $(NAMESPACE) --all --wait=false
    for restore_name in $(shell $(KUBECTL) get restore -n $(NAMESPACE) -o name);do $(KUBECTL) patch "$$restore_name" -n $(NAMESPACE) -p '{"metadata":{"finalizers":null}}' --type=merge;done

prior to running undeploy/uninstall.

Maybe this can be automatically generated by kubebuilder? Every time a new api is added (kubebuilder create api is called), the new CR is added to this clean up in Makefile

[lukas016]

i prefer cleaner way, maybe replace patch with delete specific resource and let it delete with controller. Because i have operators which create some stuff on host system and i would like delete them properly because i will have leftover on other places.

[mateusoliveira43]

yeah, good point.

But even so, I think we could have trouble. One other project I have worked, the controller also have a CLI. If you run cli delete resource and kubectl delete resource (even with the controller running), you can have different results.

This seems a hard problem. Maybe for now just add more docs? Like

User, before running make undeploy/uninstall, make sure there are no running instances of CRDs. Finalizers in those objects can fail the command

[camilamacedo86]

Hi @antonosmond

We merged the PR: https://github.com/kubernetes-sigs/kubebuilder/pull/3814/files
Looking at the sample testdata it seems right since the sample also scaffolds webhooks.
However, I am looking at your comment #3767 (comment). So, I would like to kindly ask for you check the current approach and let us know if you face an issue.
If so, can you please raise a new issue with the steps for we scaffold a project and reproduce the scenario?
Thank you a lot for your understanding.

Hi @mateusoliveira43

Following some comments inline:

Another question related to topic: should we delete install and uninstall commands from generated Makefile?

No. This options should still.
The option build-installer is optional
And those others are very useful for development purposes

Regards the other comments over cleanup/undeploy, if you see that is required could you please raise an issue about this topic? Is hard to follow up if we keep many scopes in the same one.

thank you for all help and understanding.
I hope that you do not mind, but I am closing this one since the fix is merged.

[mateusoliveira43]

from my side would just check with this would be interesting as follow up? #3767 (comment)