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

Allow installing IC without creating a new ingress class #4333

Merged
merged 6 commits into from Sep 20, 2023

Conversation

haywoodsh
Copy link
Contributor

@haywoodsh haywoodsh commented Sep 7, 2023

Proposed changes

Add controller.ingressClass.create option to options to create a new ingress class object or use an existing ingress class during deployment

closes #4209

Test

  1. Create an IngressClass manually.
    kubectl create -f - <<EOF
    apiVersion: networking.k8s.io/v1
    kind: IngressClass
    metadata:
      name: test-ingress-class
    spec:
      controller: nginx.org/ingress-controller
    EOF
    
  2. Navigate to the helm chart folder.
    cd deployments/helm-chart
    
  3. Deploy Ingress Controller with helm without creating a new IngressClass
    helm install nginx-ingress-test-ingress-class . --set controller.ingressClass.name=test-ingress-class --set controller.ingressClass.create=False
    NAME: nginx-ingress-test-ingress-class
    LAST DEPLOYED: Fri Sep  8 20:10:36 2023
    NAMESPACE: default
    STATUS: deployed
    REVISION: 1
    TEST SUITE: None
    NOTES:
    The NGINX Ingress Controller has been installed.
    

The IC is successfully installed with an existing IngressClass object.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added enhancement Pull requests for new features/feature enhancements helm_chart Pull requests that update the Helm Chart labels Sep 7, 2023
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #4333 (4462415) into main (3b91d97) will decrease coverage by 0.09%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #4333      +/-   ##
==========================================
- Coverage   52.19%   52.11%   -0.09%     
==========================================
  Files          59       59              
  Lines       16929    16956      +27     
==========================================
  Hits         8836     8836              
- Misses       7796     7823      +27     
  Partials      297      297              
Files Changed Coverage Δ
internal/configs/configurator.go 37.68% <0.00%> (-0.27%) ⬇️
internal/k8s/controller.go 11.93% <0.00%> (-0.08%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch 3 times, most recently from 00367b9 to ec20721 Compare September 8, 2023 18:41
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Sep 8, 2023
@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch 2 times, most recently from ecd2a8e to 7a05db5 Compare September 8, 2023 19:18
@haywoodsh haywoodsh marked this pull request as ready for review September 8, 2023 19:19
@haywoodsh haywoodsh requested review from a team as code owners September 8, 2023 19:19
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM, docs-wise!

@vepatel
Copy link
Contributor

vepatel commented Sep 14, 2023

@haywoodsh upgrading existing installation using:

  • diff ingressClass and setting create: True, eg: name: nginx2 removes the old ingressClass
  • same ingressClass and setting create: False removes the old ingressClass
I0914 11:43:45.163449       1 main.go:236] Kubernetes version: 1.25.13
F0914 11:43:45.167742       1 main.go:251] Error when getting IngressClass nginx: ingressclasses.networking.k8s.io "nginx" not found

Is this expected?

@vepatel
Copy link
Contributor

vepatel commented Sep 14, 2023

If two IC deployments are sharing same ingress class then is it possible to switch between the two for serving the requests, but looks like that the ADDRESS field in Ingress is only updated by last deployed IC.
Not the case with different ingressclass as it gets updated after reload.
cc@brianehlert

@brianehlert brianehlert added this to the v3.3.0 milestone Sep 14, 2023
@haywoodsh
Copy link
Contributor Author

@haywoodsh upgrading existing installation using:

  • diff ingressClass and setting create: True, eg: name: nginx2 removes the old ingressClass
  • same ingressClass and setting create: False removes the old ingressClass
I0914 11:43:45.163449       1 main.go:236] Kubernetes version: 1.25.13
F0914 11:43:45.167742       1 main.go:251] Error when getting IngressClass nginx: ingressclasses.networking.k8s.io "nginx" not found

Is this expected?

Appreciate your testing @vepatel! Create: True is equivalent to deploying the IC without this parameter in current versions. Did you initially install an older IC version without the parameter or create: True, and then use helm upgrade to upgrade with create: False? I didn't expect this PR would handle such a scenario. If the prior IngressClass was installed and managed by helm, I can see Helm deleting it during upgrade when you don't include it in the new release with create: False.
I wonder if there is a way to use schema to enforce a constant value, but at the very least, I should make it clear in the documentation this is not recommended for users to switch the value if they're using helm upgrade.

@vepatel
Copy link
Contributor

vepatel commented Sep 15, 2023

Thanks @haywoodsh there's a doc task linked in #4209 now 👍🏼
I installed 3.2.1 with default parameters (as 3.2.1 helm-chart doesn't have create option) and then upgraded by setting create: false in new helm-chart so helm deleted the old one.

@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch from 4c6d792 to e6a753e Compare September 18, 2023 10:27
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch from 8bc15ff to 76c5d30 Compare September 20, 2023 09:16
@vepatel vepatel self-requested a review September 20, 2023 11:12
Copy link
Contributor

@vepatel vepatel left a comment

Choose a reason for hiding this comment

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

requesting some changes

Co-authored-by: Venktesh Shivam Patel <ve.patel@f5.com>
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch from f8aa38d to 80418ca Compare September 20, 2023 14:04
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@haywoodsh haywoodsh force-pushed the feat/allow-not-to-generate-new-ingress-class-object branch from a7b28b4 to 780dacb Compare September 20, 2023 14:52
@haywoodsh haywoodsh merged commit 1306b31 into main Sep 20, 2023
61 checks passed
@haywoodsh haywoodsh deleted the feat/allow-not-to-generate-new-ingress-class-object branch September 20, 2023 15:58
@lucacome lucacome removed the enhancement Pull requests for new features/feature enhancements label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart
Projects
Status: Done 🚀
Development

Successfully merging this pull request may close these issues.

Enhance Helm with an option to not generate a new IngressClass object
7 participants