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.

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

[ingress-nginx] Enable to fix the nodePort of ingress-nginx service #11310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[ingress-nginx] Enable to fix the nodePort of ingress-nginx service #11310

wants to merge 1 commit into from

Conversation

mochizuki875
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

Auto creation of Service/ingress-nginx was added by #10925.
In this change, we can choose serviceType.
However, we can’t fix nodePort when choose NodePort or LoadBalancer as serviceType.

In this PR, I’ve added ingress_nginx_service_nodeport_http and ingress_nginx_service_nodeport_https properties in addons.yml to fix nodePort.

Which issue(s) this PR fixes:
Fixes

Does this PR introduce a user-facing change?:

User has a possibility to fix nodePort of ingress-nginx service with property in addons.yaml

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mochizuki875
Once this PR has been reviewed and has the lgtm label, please assign ant31 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yankay yankay closed this Jun 20, 2024
@yankay yankay reopened this Jun 20, 2024
@ant31
Copy link
Contributor

ant31 commented Jun 20, 2024

/retest

1 similar comment
@mochizuki875
Copy link
Member Author

/retest

@yankay
Copy link
Member

yankay commented Jun 21, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 21, 2024
ant31
ant31 reviewed Jun 26, 2024
View reviewed changes
roles/kubernetes-apps/ingress_controller/ingress_nginx/defaults/main.yml Outdated
@@ -2,6 +2,8 @@
ingress_nginx_namespace: "ingress-nginx"
ingress_nginx_host_network: false
ingress_nginx_service_type: LoadBalancer
ingress_nginx_service_nodeport_http: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it an empty dict? what the value expected if filled ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ant31
Thank you for your feedback.
The intention is to leave the .spec.ports[].nodePort of the Service unspecified by default as it currently is(Dynamic port will be assigned).
If user specified ingress_nginx_service_nodeport_http or ingress_nginx_service_nodeport_https field in addons.yml, these values will be used as .spec.ports[].nodePort.

As a result of my reconsideration, I think "" is appropriate instead of {} and fixed.
In addition, user may be confused how to use these param, so I've added commented out example to addons.yml like that.

https://github.com/mochizuki875/kubespray/blob/2b24f3f3a5cffbe8eb8eea0eea1eb4042e22422a/inventory/sample/group_vars/k8s_cluster/addons.yml#L103-L104

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you !

@mochizuki875 mochizuki875 force-pushed the add_ingress_controller_nodeport_field branch from cc5d3f0 to 2b24f3f Compare June 27, 2024 02:39
ant31
ant31 reviewed Jun 27, 2024
View reviewed changes
roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/svc-ingress-nginx.yml.j2 Outdated
- name: https
port: 443
targetPort: 443
protocol: TCP
{% if (ingress_nginx_service_type == 'NodePort' or ingress_nginx_service_type == 'LoadBalancer') and ingress_nginx_service_nodeport_https %}
nodePort: {{ingress_nginx_service_nodeport_https}}
Copy link
Contributor

@ant31 ant31 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

{{ingress_nginx_service_nodeport_https|int}}

That checking it's valid int and it easier to pass arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

@ant31
I've fixed.
How about that?

@mochizuki875 mochizuki875 force-pushed the add_ingress_controller_nodeport_field branch from 2b24f3f to 61c9a46 Compare June 27, 2024 07:05
@ant31
Copy link
Contributor

ant31 commented Jun 27, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ant31 ant31 ant31 left review comments

@cyclinder cyclinder Awaiting requested review from cyclinder

@ErikJiang ErikJiang Awaiting requested review from ErikJiang

Assignees

@ant31 ant31

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mochizuki875 @k8s-ci-robot @ant31 @yankay