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: add websockets configuration #20814

Merged
merged 1 commit into from
Aug 10, 2022
Merged

ingress: add websockets configuration #20814

merged 1 commit into from
Aug 10, 2022

Conversation

nikhiljha
Copy link
Contributor

@nikhiljha nikhiljha commented Aug 7, 2022 •
edited
Loading

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible. -> Does this need to be tested? I feel like this is an Envoy thing.
  • 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.
  • 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.
  • Thanks for contributing!

Some popular web software (e.x. Jupyterhub, Home Assistant) requires websocket support from the ingress. This commit provides an annotation to enable this support in Envoy. It is disabled by default.

Fixes: #20427

@nikhiljha nikhiljha requested a review from a team as a code owner August 7, 2022 16:46
@nikhiljha nikhiljha requested a review from nebril August 7, 2022 16:47
@maintainer-s-little-helper
Copy link

Commit 8807faf68930634b627b690860b44dfe3fc4da69 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 7, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 7, 2022
@sayboras sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 8, 2022
@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 Aug 8, 2022
@sayboras sayboras added area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 8, 2022
@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 Aug 8, 2022
sayboras
sayboras reviewed Aug 8, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, I have below two small comments, let me know if it's making sense.

Will kick off the Ingress Conformance test later.

operator/pkg/ingress/annotations/annotations.go Outdated Show resolved Hide resolved
operator/pkg/ingress/annotations/annotations.go Outdated Show resolved Hide resolved
@nikhiljha nikhiljha force-pushed the websockets branch 2 times, most recently from 6e7e554 to 61b5461 Compare August 8, 2022 00:49
sayboras
sayboras reviewed Aug 8, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Looks good to me, one small comment on unit tests.

operator/pkg/ingress/envoy_config.go Show resolved Hide resolved
operator/pkg/ingress/envoy_config.go Show resolved Hide resolved
@nikhiljha nikhiljha force-pushed the websockets branch 3 times, most recently from a8e032d to cd0f52c Compare August 10, 2022 01:47
@nikhiljha
ingress: add websockets configuration
65c943a 
Some popular web software (e.x. Jupyterhub, Home Assistant) requires websocket support from the ingress. This commit provides an annotation to enable this support in Envoy. It is enabled by default to be in-line with other popular Ingress implementations (Traefik, ingress-nginx).

Fixes: cilium#20427

Signed-off-by: Nikhil Jha <hi@nikhiljha.com>
sayboras
sayboras approved these changes Aug 10, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

thanks and lgtm 💯

@sayboras
Copy link
Member

I don't think full CI is required, as Ingress related codepath are not covered by jenkins tests.

As long as IngressConformance is successful, we can merge this PR.

https://github.com/cilium/cilium/runs/7760353023?check_suite_focus=true

@sayboras
Copy link
Member

sayboras commented Aug 10, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sVerifier Runs the kernel verifier against Cilium's BPF datapath

Failure Output

FAIL: Failed to load BPF program bpf_host with datapath configuration:

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

@sayboras sayboras added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. needs-backport/1.12 labels Aug 10, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.1 Aug 10, 2022
@nbusseneau
Copy link
Member

Thanks for your contribution @nikhiljha.

@nbusseneau nbusseneau merged commit 234f9a8 into cilium:master Aug 10, 2022
@nikhiljha nikhiljha deleted the websockets branch August 10, 2022 15:40
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.12 in 1.12.1 Aug 11, 2022
@tklauser tklauser mentioned this pull request Aug 11, 2022
Merged
@joestringer
Copy link
Member

Hi @nikhiljha , would you be interested in submitting a PR to add an agent flag to make this optional? It would be nice to know that it's possible to disable this at runtime in case of any issues arising from the use of websocket support.

@joestringer joestringer added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Aug 12, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.1 Aug 12, 2022
@nikhiljha
Copy link
Contributor Author

@joestringer Sure! Although I don't understand what you mean. If you don't add the annotation to your ingress the envoy crd doesn't change from the previous release of Cilium, so you can disable websocket support at runtime simply by removing the annotation.

i.e. in this pr, you must explicitly opt in to websocket support, and you can disable it by not doing that

@nikhiljha
Copy link
Contributor Author

nikhiljha commented Aug 12, 2022 •
edited
Loading

Ah, I see what happened. I didn't update the commit message / PR description after deciding to make it not enabled by default 😓 as was requested in a code review.

@sayboras
Copy link
Member

👋

I overlooked the commit message as well, I think it's still fine, as we have the history of conversation in this PR.

@joestringer
Copy link
Member

@nikhiljha Ah okay, I think that should work fine. 👍

@joestringer joestringer mentioned this pull request Aug 15, 2022
Merged
@chancez
Copy link
Contributor

chancez commented Aug 17, 2022

Is there a reason this isn't automatically enabled based on the client's request, instead of having to explicitly configure it in the ingress config? With ingress-nginx for example, it's automatic. Basically, in nginx you can just only add the Upgrade header if the client sends an upgrade header, so then it's "works" regardless, without having to mark anything as "websockets".

This was referenced Aug 17, 2022
Open
Closed
@nikhiljha nikhiljha mentioned this pull request Dec 28, 2022
Open
10 tasks
@akstron
Copy link
Contributor

akstron commented Jun 25, 2023

So, I was working on this issue #22887 and hopped on this pr to gather more insights. It seems like the changes made by this pr is not fully added in the main branch. Particularly, I can't find the websocket config update code in the current main branch. Are the files reorganized? Can anyone help me regarding the organization, particularly about operator/pkg. Thanks.
cc: @nikhiljha @sayboras

@akstron
Copy link
Contributor

akstron commented Jul 4, 2023

So, I was working on this issue #22887 and hopped on this pr to gather more insights. It seems like the changes made by this pr is not fully added in the main branch. Particularly, I can't find the websocket config update code in the current main branch. Are the files reorganized? Can anyone help me regarding the organization, particularly about operator/pkg. Thanks. cc: @nikhiljha @sayboras

@joestringer requesting your suggestions on this. Thanks.

@judge-red judge-red mentioned this pull request Jan 5, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. 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
No open projects
1.12.1
Backport done to v1.12
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ingress: Persistent Envoy filters
8 participants
@nikhiljha @sayboras @nbusseneau @joestringer @chancez @akstron @nebril @tklauser