chore: cleanup the logic related to installCRDs

tao12345666333 commented 10 months ago

What this PR does / why we need it:

We have already removed the installCRDs configuration item from the values.yaml file.[1]

If the user passes --set ingressController.installCRDs as a parameter, this logic will install all CRDs twice.

However, if it's a fresh installation, nothing will happen.

If there are already installed CRDs, the logic will attempt to find annotations for them. However, our CRDs don't have any release-name annotations

➜  kubectl get crds kongplugins.configuration.konghq.com -oyaml  
apiVersion: apiextensions.k8s.io/v1                                                            
kind: CustomResourceDefinition                                                                                                                                                                 
metadata:                                                                                      
  annotations:                                                                                 
    controller-gen.kubebuilder.io/version: v0.13.0                                             
  creationTimestamp: "2023-11-16T18:30:37Z"                                                                                                                                                    
  generation: 1                                                                                
  name: kongplugins.configuration.konghq.com                                                                                                                                                   
  resourceVersion: "3228"                     
  uid: 658f72b7-f65e-4a01-922e-045a1b87a1a3                                                    
spec:                                                                                          
  ...

so this part of the logic can be safely removed.

1: https://github.com/Kong/charts/commit/bf98a5cbae44af1f30c72f0d0d4f1910a38a0d8c

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #948

Special notes for your reviewer:

The CRDs will be installed by default, but users can skip their installation by passing --skip-crds to the helm install command.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

[x] PR is based off the current tip of the main branch.
[ ] ~Changes are documented under the "Unreleased" header in CHANGELOG.md~
[x] New or modified sections of values.yaml are documented in the README.md
[x] Commits follow the Kong commit message guidelines

CLAassistant commented 10 months ago

All committers have signed the CLA.

rainest commented 10 months ago

We have this in place for ancient installs for which there is unfortunately no good migration path.

Helm 2 did not have Helm 3's CRD installation system and used this template as the only means of installing CRDs. If you installed back then and we remove this template, upgrading will actually delete your CRDs (and any CRs associated with them). Helm didn't suggest a migration path to abandon the CRDs and leave them in place, so we kept the template around.

We added additional logic after removing the values.yaml key that essentially pretends installCRDs is present and enabled in values.yaml when chart-owned CRDs are present.

helm.sh/resource-policy: keep may actually work for removing the CRDs from chart-owned resources while leaving them in place (we weren't familiar with it back in the day), but we hadn't explored it, since the lookup-based system does work as intended and doesn't need much attention. That'd probably be the approach if we wanted to remove the template entirely, but it would pose a minor footgun where skipping over a version that adds the keep policy and going to a version that actually removes the template would still delete the CRDs.

Either way, we can't remove the template immediately--we would need some intermediate step that we've tested to successfully abandon the CRDs and leave them in place from a chart 1.x+Helm 2 install that enabled the template.

tao12345666333 commented 9 months ago

Helm v2 was completely discontinued in November 2020. https://helm.sh/blog/helm-v2-deprecation-timeline/ The latest version of Kubernetes at that time was v1.20.

I also checked the latest version of our Helm chart v1.x, which is v1.15.2 and was released in April 2021 on GitHub.

However, the chart v1.15.2 uses the following resource groups:

apiextensions.k8s.io/v1beta1
rbac.authorization.k8s.io/v1beta1

These two resource groups no longer work in Kubernetes v1.22.

Therefore, Helm chart v1 can only work before version v1.22 (excluding v1.22), which was released in August 2020 https://github.com/kubernetes/kubernetes/releases/tag/v1.22.0

Since this commit, the Helm chart is no longer supported by Helm v2. ( chart kong-2.8.0) https://github.com/Kong/charts/commit/2e89647d86ec7c8fa2a2a1697a325e6679b18d08#diff-466edb10b903c1c9f9019fd0128824ba889bbe1bdff3da186cf698e3a5703af8R1

So my point of view is:

Our Chart cannot be used with Helm v2 starting from version 2.8, users must upgrade to Helm v3 (from that time);
The new version of Chart can only be used with Helm v3 in the new version of Kubernetes. We don't need to maintain this logic.
We do not have any complete tests covering this scenario (perhaps there was a similar issue before, but now this kind of scenario no longer exists).

pmalek commented 9 months ago

As discussed on team sync: we could potentially introduce this as a breaking change and release in kong v3 milestone. WDYT @rainest ?

rainest commented 9 months ago

The legacy logic is not for installs on Helm 2, it is for installs that started on Helm 2 (and some that started on Helm 3 but still used the templated CRDs). If you installed with installCRDs=true at any point, you're basically stuck with it forever, regardless of whatever versions (chart or Helm) you upgraded to after.

You cannot introduce it as a breaking change as proposed because there is no action users can reasonably take to avoid it deleting CRDs if the template is simply absent. AFAIK this would require manually editing the Helm release Secret data to remove whatever resource tracking data it holds there. The format in there isn't documented and doesn't easily lend itself to inspection (kubectl get secret sh.helm.release.v1.ana.v1 -ojson | jq -r .data.release |base64 -d yields some other encoded format), and I don't think I'd recommend that approach even if its contents were more obvious.

You may be able to remove it safely by first using an intermediate step with a resource policy addition, but that's not what was proposed here. I've never really bothered because keeping the template around as-is hasn't incurred much maintenance cost: we've touched it all of twice in two years, once to add a comment and once to add a setting for the rare user that lacks list CRD permissions.

Testing it in CI would indeed require a dedicated workflow for this alone, where you initially install with installCRD=true, add some custom resource, apply an upgrade with installCRDs unset, and then confirm that CRDs/custom resources are still present. Between how much that differs from our existing workflows and how seldom we touch this, we've just used manual testing at PR time, e.g. what's in https://github.com/Kong/charts/pull/565

To actually remove this would require demonstrating a complete sequence of steps that demonstrates you can install with installCRDs=true and upgrade through (presumably several) different versions onto a version that removes the template without it deleting your resources, with reasonable confidence that you wouldn't be able to accidentally skip whatever intermediate version makes the final upgrade safe. That probably requires some combination of a resource policy and a detection lookup that invokes a fail on the version that removes the template if you haven't first passed through the resource policy addition.

Kong / charts