8000 Allow installing IC without creating a new ingress class by haywoodsh · Pull Request #4333 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]
Skip to content

Allow installing IC without creating a new ingress class #4333

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
Merged
merged 6 commits into from
Sep 20, 2023
Merged

Allow installing IC without creating a new ingress class #4333

merged 6 commits into from
Sep 20, 2023

Conversation

haywoodsh
Copy link
Contributor
@haywoodsh haywoodsh commented Sep 7, 2023 •
edited
Loading

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 •
edited
Loading

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
ADubhlaoich
ADubhlaoich approved these changes Sep 11, 2023
View reviewed changes
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 •
edited
Loading

@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?

@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 •
edited
Loading

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 8000 create: false in new helm-chart so helm deleted the old one.

lucacome
lucacome reviewed Sep 15, 2023
View reviewed changes
@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
@haywoodsh
Allow installing IC without creating a new ingress class
4561e40 
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@haywoodsh
add docs for helm upgrade
76c5d30 
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
8000
@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 reviewed Sep 20, 2023
View reviewed changes
@vepatel vepatel self-requested a review September 20, 2023 11:12
vepatel
vepatel requested changes Sep 20, 2023
View reviewed changes
Copy link
Contributor
@vepatel vepatel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

requesting some changes

8000
vepatel
vepatel reviewed Sep 20, 2023
View reviewed changes
vepatel
vepatel reviewed Sep 20, 2023
View reviewed changes
@haywoodsh @vepatel
Update deployments/helm-chart/README.md
eef1c14 
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
@haywoodsh
update docs
780dacb 
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
@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
Reviewers

@danielnginx danielnginx danielnginx left review comments

@ADubhlaoich ADubhlaoich ADubhlaoich approved these changes

@vepatel vepatel vepatel approved these changes

+2 more reviewers

@lucacome lucacome lucacome left review comments

@shaun-nx shaun-nx shaun-nx approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Enhance Helm with an option to not generate a new IngressClass object
7 participants
@haywoodsh @vepatel @lucacome @ADubhlaoich @shaun-nx @danielnginx @brianehlert
0