-
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
Annotation: Make proxy-ssl-secret
optional.
#11490
base: main
Are you sure you want to change the base?
Conversation
… usage of other proxy-ssl-* annotations
Hi @g1franc. 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-sigs/prow repository. |
✅ Deploy Preview for kubernetes-ingress-nginx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: g1franc 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 |
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.
/retitle Annotation: Make proxy-ssl-secret
optional.
/triage accepted
/kind bug
/priority backlog
Assuming that you only changed the indentation of most of the lines: Could you please follow the linter recommendation and revert the change in indentation?
Apart from that your change looks good.
proxy-ssl-secret
optional.
/cherry-pick release-1.10 |
@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you. In response to this:
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-sigs/prow repository. |
Oh and please also rebase your code on top of |
The failing |
/ok-to-test |
@g1franc why would you set proxy-ssl-protocols if you are not configuring a secrert ? @Gacko why would we accept setting TLS-Protocols optionally and that too without a secret for a certificate to begin with ? Its possible I am completely lost here. @g1franc so is there any chance that you can actually create a kind/minikube cluster and do the setting of the proxy-protocol and then send TLS traffic all the way through from a client outside the cluster to the upstream inside clusrter. If you copy paste all the |
@longwuyuan I'm configuring proxy-ssl-protocol because the backend is using TLS (HTTPS) but I don't need to provide a secret for the TLS client certificate as the backend don't use one to authentificate the client. Error when trying to use TLSv1.2 instead of 1.3 is
|
@g1franc thank you for explaining. Makes sense now. But controller does not try TLS v1.3 then should we not check why controller does not try TLS v1.3 ? The choice to make the existence of the TLS certificate optional, seems completely NOT related to the version of the protocol being used for TLS. I am not a developer so I am taking this at face value. I will check the code changes now and see if I can make any sense of it. But my concern comes from arbitrarily deciding to change something that is not at all broken. |
@g1franc thank for sharing screen on meet.jit.si and showing the problem and the use-case. I have thought about it and I see that the CA related information is also needed in that secret (this data below screenshot) so I am very confused about this because you said you don't want to use a client-certificate but only want to use So is this potentially making it easy to use |
I think part of the confusion here is mixing several different behaviours, specifically in the proxy case (i.e. when nginx is acting as a client to an upstream):
(3)(i) and (3)(ii) do not require any trust information about a CA. (3)(iii) and (3)(iv) require at least that the CA can be verified. Public CAs aren't an option for many private clusters, especially given the cost of public CA certificates and no external route for automated verification. (3)(iii) and (3)(iv) can also use the installed host-level CA verification as a baseline. Adding additional configuration should only be necessary if those cannot be used or if further restriction is needed. But I think the biggest factor here is that (1) and (3) have been conflated. To be very precise: a certificate does not need to be a secret. It's only a secret if it contains a private key. And that case only occurs with client authentication. |
@g1franc I did a test and I can't reproduce the problem you mentioned
So what am I missing. Please help me understand why you think that controller is using TLS v1.2 |
If you are interested, I can join another meeting on jitsi to do tpdump of traffic from controller to backend. |
Hello @longwuyuan, Maybe I misunderstood your test, but it seems that you tested NGINX serving TLSv1.3 while the issue is about the Operator configuring NGINX to proxify to an TLSv1.3 backend. So to make the test relevant, you can keep your container to act as backend TLSv1.3 only + you need a second nginx to proxify to this first container, using proxy-ssl-protocol property. |
@g1franc thank you very much for your comment because it adds more light to the use case and proves that my assumption of the use case is not accurate. I need your help to get the description of your use-case in much much more clear and very accurate details. I am going to write some description below. Please correct it in your response. client laptop <--HTTPS --> LB <--HTTPS--> ingress-controller <-- HTTPS --> backend |
Something like this needs to be documented on why or not a proxy ssl secret is needed. |
Hi @g1franc , Could you kindly review my test one more time.
If my config is wrong, please let me know and I will change and retest. Also note the nginx docs link that says default TLS protocol version is v1.3 since Nginx v1.24.x and not v1.2. The controler uses Nginx v1.25.x internally. So do you think we need to check your cluster & controller to understand why its using default TLS protocol as v1.2. |
@g1franc I am also adding some proof that my backend can only serve TLSv1.3 and not TLSv1.2
|
This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach |
The This bot removes
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /remove-lifecycle frozen |
Update main.go to make proxy-ssl-secret annotation optional usage of other proxy-ssl-* annotations.
What this PR does / why we need it:
Currently, if the annotation proxy-ssl-secret is not provided, it is not possible to use other proxy-ssl-* annotations. For example, it prevent to set only proxy-ssl-protocols to specify the TLS protocol to connect to the upstream backend.
This change aim to make the annotation proxy-ssl-secret optional by preventing an exit from the method in case of it not being specified. I don't alter the current behavior in the regard that if that an invalid value if specified, no other proxy-ssl-* annotations will be processed.
Types of changes
Which issue/s this PR fixes
fixes #10264
How Has This Been Tested?
Tested in local dev cluster and also with tested regression with
FOCUS='proxyssl' make kind-e2e-test
Checklist: