Fixed bug where ingress table printer did not check to see if HTTP was enabled before including port 80 in the output #90658

brianpursley · 2020-04-30T22:43:08Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
The ingress table printer was assuming ports to be 80 and 443 if TLS is configured. But this is not always true. You can set the kubernetes.io/ingress.allow-http annotation to false and that will disable HTTP and so in this case the ingress table printer should output only 443. This PR fixes this bug.

Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#500

Special notes for your reviewer:
The "HTTP and HTTPS ports" unit test case is the original unit test (pre-PR). I added four additional cases. My goal was not to change the test, but just add more cases.

Does this PR introduce a user-facing change?:

Fixed bug where kubectl get ingress displayed port 80 in the output even if HTTP is disabled.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

brianpursley · 2020-04-30T22:43:36Z

/sig cli
/priority important-soon

brianpursley · 2020-05-01T01:10:51Z

/retest

brianpursley · 2020-05-01T15:21:52Z

/assign @janetkuo

pkg/printers/internalversion/printers_test.go

pkg/printers/internalversion/printers.go

brianpursley · 2020-05-28T00:03:06Z

/unassign @janetkuo
/assign @soltysh

Feel free to reassign if you want...

soltysh

Nit, which can be fixed in the next PR.
/lgtm
/approve

pkg/printers/internalversion/printers.go

k8s-ci-robot · 2020-06-04T13:59:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/printers/OWNERS~~ [soltysh]

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

…s enabled before including port 80 in the output

brianpursley · 2020-06-05T18:05:54Z

@soltysh I resolved your comment, but then after taking a second look, I reverted back.

I think there is actually an invalid config that could occur by mistake if the user does not set TLS, and explicitly sets the kubernetes.io/ingress.allow-http annotation to false. I added a comment in the code explaining this.

If you still think I should make the change to default to "80" even if the kubernetes.io/ingress.allow-http is false, let me know and I'll make the change. My concern was that it might be misleading to show port 80 if they have done something by mistake that disables it in ingress config, but I'm not sure if that is a real thing that could happen or not.

soltysh

/lgtm

brianpursley · 2020-06-05T19:07:53Z

/retest

liggitt · 2020-06-05T20:28:00Z

pkg/printers/internalversion/printers.go

-		return "80, 443"
+func formatPorts(obj *networking.Ingress) string {
+	allowHTTP := true
+	if annotationValue, exists := obj.Annotations["kubernetes.io/ingress.allow-http"]; exists {


is this officially part of the ingress API?

Good call, I guess someone from networking can weigh in.

The current code just hard codes the assumption that if you have TLS, ports are "443, 80" and otherwise just "80".

From what I can tell, kubernetes.io/ingress.allow-http is valid at least for ingress-gce:
https://github.com/kubernetes/ingress-gce/blob/c7c2a08366ed113009b60f88aca8760f43034c93/pkg/annotations/ingress.go#L30-L35.

To be honest, and probably this is what you're getting at, it seems like the port numbers should not even be determined within the printer.

Seems like the ports should be reported out as status from the various ingress controllers, because in theory it would not even have to be 443 and 80.

liggitt · 2020-06-05T20:28:41Z

/hold

would like an ack from networking folks that this is part of the API we should modify the API server to honor
/assign @bowei

liggitt · 2020-06-05T20:29:39Z

/remove-sig cli
/sig network
/kind api-change

brianpursley · 2020-07-12T16:37:59Z

To @liggitt 's point

AWS has its own annotations that affect this

It seems like the problem is kubectl cannot know, without checking ingress provider-specific annotations, which ports are ACTUALLY being used.

I don't see this being merged as it is right now, so I'm going to go ahead and close this PR. I'll make a note in the issue of what the challenge is in doing this.

k8s-ci-robot · 2020-07-12T16:38:06Z

@brianpursley: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot requested review from deads2k and zhouya0 April 30, 2020 22:43

k8s-ci-robot assigned janetkuo May 1, 2020

brianpursley commented May 14, 2020

View reviewed changes

pkg/printers/internalversion/printers_test.go Show resolved Hide resolved

zhouya0 reviewed May 22, 2020

View reviewed changes

pkg/printers/internalversion/printers.go Show resolved Hide resolved

k8s-ci-robot assigned soltysh and unassigned janetkuo May 28, 2020

soltysh approved these changes Jun 4, 2020

View reviewed changes

pkg/printers/internalversion/printers.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2020

brianpursley force-pushed the kubectl-500 branch from 4e51807 to 4a4697b Compare June 5, 2020 17:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2020

brianpursley force-pushed the kubectl-500 branch 2 times, most recently from 39df2f3 to c9874e3 Compare June 5, 2020 17:53

Fixed bug where ingress table printer did not check to see if HTTP wa…

62400a3

…s enabled before including port 80 in the output

brianpursley force-pushed the kubectl-500 branch from c9874e3 to 62400a3 Compare June 5, 2020 18:01

soltysh approved these changes Jun 5, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2020

liggitt reviewed Jun 5, 2020

View reviewed changes

k8s-ci-robot assigned bowei Jun 5, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Jun 5, 2020

brianpursley closed this Jul 12, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2020

brianpursley mentioned this pull request Jul 12, 2020

"get ingress" still shows port 80 even though it is HTTPS only. kubernetes/kubectl#500

Closed

brianpursley deleted the kubectl-500 branch February 2, 2023 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug where ingress table printer did not check to see if HTTP was enabled before including port 80 in the output #90658

Fixed bug where ingress table printer did not check to see if HTTP was enabled before including port 80 in the output #90658

brianpursley commented Apr 30, 2020 •

edited

Loading

brianpursley commented Apr 30, 2020

brianpursley commented May 1, 2020

brianpursley commented May 1, 2020

brianpursley commented May 28, 2020

soltysh left a comment

k8s-ci-robot commented Jun 4, 2020

brianpursley commented Jun 5, 2020 •

edited

Loading

soltysh left a comment

brianpursley commented Jun 5, 2020

liggitt Jun 5, 2020

brianpursley Jun 5, 2020

brianpursley Jun 5, 2020

liggitt commented Jun 5, 2020

liggitt commented Jun 5, 2020

brianpursley commented Jul 12, 2020

k8s-ci-robot commented Jul 12, 2020

Fixed bug where ingress table printer did not check to see if HTTP was enabled before including port 80 in the output #90658

Fixed bug where ingress table printer did not check to see if HTTP was enabled before including port 80 in the output #90658

Conversation

brianpursley commented Apr 30, 2020 • edited Loading

brianpursley commented Apr 30, 2020

brianpursley commented May 1, 2020

brianpursley commented May 1, 2020

brianpursley commented May 28, 2020

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 4, 2020

brianpursley commented Jun 5, 2020 • edited Loading

soltysh left a comment

Choose a reason for hiding this comment

brianpursley commented Jun 5, 2020

liggitt Jun 5, 2020

Choose a reason for hiding this comment

brianpursley Jun 5, 2020

Choose a reason for hiding this comment

brianpursley Jun 5, 2020

Choose a reason for hiding this comment

liggitt commented Jun 5, 2020

liggitt commented Jun 5, 2020

brianpursley commented Jul 12, 2020

k8s-ci-robot commented Jul 12, 2020

brianpursley commented Apr 30, 2020 •

edited

Loading

brianpursley commented Jun 5, 2020 •

edited

Loading