helm: allow removing port 80 from the cilium-ingress service by bitflicker64 · Pull Request #48870 · cilium/cilium · GitHub
Skip to content

helm: allow removing port 80 from the cilium-ingress service - #48870

Merged
joestringer merged 1 commit into
cilium:mainfrom
bitflicker64:helm-ingress-optional-http-port
Oct 22, 2026
Merged

joestringer merged 1 commit into
cilium:mainfrom
bitflicker64:helm-ingress-optional-http-port

Conversation

@bitflicker64

Copy link
Copy Markdown
Contributor

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer's Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Disclose use of machine learning models (including LLMs and other generative AI)
    in accordance with the Cilium AI Policy, and indicate the rating using
    AI Influence Level.
    This PR was prepared with AIL:3. I personally reviewed the change, checked the
    rendered Service for every combination of the new values, and ran the chart lint
    and the values and schema checks.
  • Thanks for contributing!

Before: the shared cilium-ingress Service always carries port 80 next to port 443, and no Helm value removes it. On a cloud provider that means the load balancer always comes up with a plain HTTP listener. Where an organization's policy forbids HTTP listeners, that load balancer gets deleted by an automated sweep, so Cilium Ingress is unusable without a mutating admission policy that patches the Service after Helm has written it. That is what the issue reporter ended up doing.

After: two new values, ingressController.service.insecurePortEnabled and ingressController.service.securePortEnabled, decide whether each port is listed on the shared Service. Both default to true, so an existing install renders exactly the Service it rendered before. With insecurePortEnabled: false only the HTTPS port is left and the load balancer comes up without an HTTP listener.

How: the two port entries in cilium-ingress-service.yaml are guarded by the matching value, and validate.yaml rejects an install that turns both off, since a Service needs at least one port. values.yaml, values.schema.json, the chart README and the Helm reference are regenerated with the repo tooling.

On naming, since this is filed as a CFP: insecurePortEnabled and securePortEnabled follow insecureNodePort and secureNodePort, which already sit next to them under ingressController.service. I am happy to rename them if you prefer a different shape, for example a single list of ports.

Scope: this covers shared mode, which is the Service the issue is about. Dedicated mode Services are built by the operator, and getService in operator/pkg/model/translation/ingress/dedicated_ingress.go already omits port 80 for a TLS only Ingress.

Verified on this branch:

  • helm template with ingressController.enabled=true and no other flags renders the same Service and EndpointSlice as main. Diffing the full chart output before and after the change shows no difference outside the generated certificates.
  • insecurePortEnabled=false renders only the https port, securePortEnabled=false renders only the http port, and both together fail with the message from validate.yaml.
  • type=NodePort with insecurePortEnabled=false and secureNodePort=30443 still puts the node port on the remaining entry.
  • hostNetwork.enabled=true with insecurePortEnabled=false renders a ClusterIP Service with only the https port.
  • make -C install/kubernetes lint and make -C install/kubernetes check-values-yaml both pass, and rerunning helm-docs, helm-schema and the Documentation update-helm-values target produces no further diff.

Fixes: #48108

Add ingressController.service.insecurePortEnabled and ingressController.service.securePortEnabled to control whether the shared Cilium Ingress Service exposes port 80 and port 443.

@bitflicker64
bitflicker64 requested a review from a team as a code owner September 19, 2026 18:56
@bitflicker64
bitflicker64 requested a review from squeed September 19, 2026 18:56
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 19, 2026
@github-actions github-actions Bot added the kind/community-contribution This was a contribution made by a community member. label Sep 19, 2026
@joestringer joestringer added the ail:3 AI Created, Human Full Structure. Generated content with editing done by a human. label Sep 23, 2026
@joestringer joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 21, 2026
@joestringer

Copy link
Copy Markdown
Member

The shared Ingress Service always lists port 80 next to 443, so a cloud
load balancer always ends up with a plain HTTP listener. Some
organizations run policies that delete any load balancer exposing one,
which leaves the Ingress controller unusable even when every Ingress it
serves is TLS only.

Add ingressController.service.insecurePortEnabled and
ingressController.service.securePortEnabled. Both default to true, so the
rendered Service is unchanged for existing users. Setting either to false
drops the matching port from the Service. Turning both off is rejected in
validate.yaml, since a Service needs at least one port.

Dedicated load balancer mode already omits port 80 for a TLS only
Ingress, so this brings the shared Service in line with it.

Fixes: cilium#48108

Signed-off-by: Himanshu Verma <himnshuverma10152006@gmail.com>
@bitflicker64
bitflicker64 force-pushed the helm-ingress-optional-http-port branch from a8812a5 to 9c545ea Compare October 21, 2026 20:01
@bitflicker64

Copy link
Copy Markdown
Contributor Author

Rebased onto main: the branch is now a single commit on top of 910d938, head 9c545eaece, and CI is running on the new head. The diff is unchanged from the reviewed version.

@joestringer could you drop dont-merge/needs-rebase and add a release-note/* label when you get a chance? Mergeability is currently failing only on those two labels. The release-note blurb is in the PR description.

@joestringer

This comment has been minimized.

@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Oct 21, 2026
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 21, 2026
@joestringer

Copy link
Copy Markdown
Member

@bitflicker64 done and triggered the tests. Could you confirm if you tried out this change in a local environment and confirmed that by following the steps in the parent issue that you no longer observe the problem outlined in the issue report?

@bitflicker64

Copy link
Copy Markdown
Contributor Author

@joestringer
joestringer enabled auto-merge October 22, 2026 21:12
@joestringer
joestringer added this pull request to the merge queue Oct 22, 2026
Merged via the queue into cilium:main with commit 4f1d126 Oct 22, 2026
81 of 82 checks passed
@maintainer-s-little-helper maintainer-s-little-helper Bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ail:3 AI Created, Human Full Structure. Generated content with editing done by a human. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm chart support for removing port 80 in cilium-ingress

3 participants