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

Prevent IngressClass from being deleted/recreated #518

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

dcarley
Copy link
Contributor

@dcarley dcarley commented Dec 15, 2021

What this PR does / why we need it:

This conditional is intended to not create the IngressClass if it has
already been created outside of the chart, e.g. manually prior to it
being included in v2.3.0

The previous implementation that used lookup to check if the
IngressClass existed caused the template to be omitted from the chart
and Helm would delete it on the second release, then it would be
recreated on the third release and so on.

Fix this by checking whether the existing IngressClass was installed
by this chart based on Helm's annotations. The conditionals have been
refactored to set a variable (like the CRDs template) since the
conditions are now more complex and Helm makes it hard to safely lookup
nested values.

Which issue this PR fixes

Special notes for your reviewer:

I can't see any automated regression tests that cover subtle problems
like this and I'm not sure you'd want to script them, so here's the
manual testing that I've performed.

Doesn't create when there is an existing IngressClass:

$ kubectl apply -f- <<EOS
apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  name: kong
spec:
  controller: konghq.com/ingress-controller
EOS

ingressclass.networking.k8s.io/kong created
$ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
$ helm get manifest kong | grep IngressClass
$ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
$ helm get manifest kong | grep IngressClass

Does create and doesn't delete when there is no existing IngressClass:

$ kubectl delete IngressClass/kong
ingressclass.networking.k8s.io "kong" deleted
$ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
$ helm get manifest kong | grep IngressClass
kind: IngressClass
$ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
$ helm get manifest kong | grep IngressClass
kind: IngressClass

Checklist

This conditional is intended to not create the `IngressClass` if it has
already been created outside of the chart, e.g. manually prior to it
being included in v2.3.0

The previous implementation that used `lookup` to check if the
`IngressClass` existed caused the template to be omitted from the chart
and Helm would delete it on the second release, then it would be
recreated on the third release and so on.

Fix this by checking whether the existing `IngressClass` was installed
by this chart based on Helm's annotations. The conditionals have been
refactored to set a variable (like the CRDs template) since the
conditions are now more complex and Helm makes it hard to safely lookup
nested values.

I can't see any automated regression tests that cover subtle problems
like this and I'm not sure you'd want to script them, so here's the
manual testing that I've performed.

Doesn't create when there is an existing `IngressClass`:

    $ kubectl apply -f- <<EOS
    apiVersion: networking.k8s.io/v1
    kind: IngressClass
    metadata:
      name: kong
    spec:
      controller: konghq.com/ingress-controller
    EOS

    ingressclass.networking.k8s.io/kong created
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass

Does create and doesn't delete when there is no existing `IngressClass`:

    $ kubectl delete IngressClass/kong
    ingressclass.networking.k8s.io "kong" deleted
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    kind: IngressClass
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    kind: IngressClass
@CLAassistant
Copy link

CLAassistant commented Dec 15, 2021

CLA assistant check
All committers have signed the CLA.

@rainest
Copy link
Contributor

rainest commented Dec 15, 2021

Upgrade testing has been something we've wanted to add for a while now; absent that we do manual verification at the time of the PR like you've done here.

@dcarley LGTM, we can go ahead and merge once you've signed the CLA.

@dcarley
Copy link
Contributor Author

dcarley commented Dec 22, 2021

Just waiting for the CLA to be signed on behalf of my employer @circleci first. Shouldn't be too long.

@rainest
Copy link
Contributor

rainest commented Jan 7, 2022

@dcarley just checking in on this, had you heard back from your employer yet?

@z00b
Copy link

z00b commented Jan 10, 2022

hi @rainest i'd be happy to sign on behalf of CircleCI, but i don't seem to be able to do this on this PR? cla assistant shows that i have authorized the CLA but either that's pointing to just me personally or @dcarley 's PR isn't associated with CircleCI. any thoughts on how to proceed?

@dcarley
Copy link
Contributor Author

dcarley commented Jan 11, 2022

I signed it after you, based on the process for organisations that was discussed in Kong/kong#5070, so it looks like we're good to go now. I'll let the rest of our team know that they can/should sign the CLA individually in future 🚀

@shaneutt shaneutt merged commit e1c4768 into Kong:next Jan 11, 2022
ubergesundheit pushed a commit to giantswarm/kong-charts-upstream that referenced this pull request Feb 9, 2022
This conditional is intended to not create the `IngressClass` if it has
already been created outside of the chart, e.g. manually prior to it
being included in v2.3.0

The previous implementation that used `lookup` to check if the
`IngressClass` existed caused the template to be omitted from the chart
and Helm would delete it on the second release, then it would be
recreated on the third release and so on.

Fix this by checking whether the existing `IngressClass` was installed
by this chart based on Helm's annotations. The conditionals have been
refactored to set a variable (like the CRDs template) since the
conditions are now more complex and Helm makes it hard to safely lookup
nested values.

I can't see any automated regression tests that cover subtle problems
like this and I'm not sure you'd want to script them, so here's the
manual testing that I've performed.

Doesn't create when there is an existing `IngressClass`:

    $ kubectl apply -f- <<EOS
    apiVersion: networking.k8s.io/v1
    kind: IngressClass
    metadata:
      name: kong
    spec:
      controller: konghq.com/ingress-controller
    EOS

    ingressclass.networking.k8s.io/kong created
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass

Does create and doesn't delete when there is no existing `IngressClass`:

    $ kubectl delete IngressClass/kong
    ingressclass.networking.k8s.io "kong" deleted
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    kind: IngressClass
    $ helm upgrade kong charts/kong/ --install --set ingressController.installCRDs=false >/dev/null
    $ helm get manifest kong | grep IngressClass
    kind: IngressClass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants