-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Add stream-snippet as a ConfigMap and Annotation option #8029
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @Eun! |
Hi @Eun. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Eun
/triage accepted
/priority important-longterm
/kind feature |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see go sec linting, that failed
@iamNoah1 I already checked the linter output, however this is not related to this pull request since the same errors also appear on all other commits: https://github.com/kubernetes/ingress-nginx/commits/main Please advice how I should proceed. |
/assign IamNoah1 |
(I rebased, this fixed the security issue) |
ginkgo.It("should add value of stream-snippet to nginx config", func() { | ||
host := "foo.com" | ||
|
||
snippet := `server {listen 8000; proxy_pass 127.0.0.1:80;}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be used for good, and for evil :)
I'm really worried nowadays about allowing more snippets directives, will need to think a bit better on this.
Spoke with @strongjz and we can merge this. We need tho to keep a good eye on this snippet directives, as they have been our main CVE cases :) As soon as we can ship some privilege separation between control and dataplane I will be more comfortable with this. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Eun, rikatz 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:
Approvers can indicate their approval by writing |
Does this commit clearly handle all the stream directives? |
Well this pr just adds the capability to use the stream snippet, the functionality is dependent on the nginx build. |
What this PR does / why we need it:
This would make it possible to define extra configuration for the stream
context through the ConfigMap or an ingress annotation. e2e tests have been created for the new functionality.
This resolves the issue #4479 and replaces the pr #6404
Types of changes
Which issue/s this PR fixes
fixes #4479
replaces #6404
How Has This Been Tested?
added e2e tests
Checklist: