[1.8] Fix issues around tlsRedirect (#29895) #30134
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/XL
howardjohn
force-pushed
the
18/cp-tls-redirect
branch
from
de7200a
to
43b9870
Compare
* Fix issues around tlsRedirect * Fixes istio#27315 * Fixes istio#27157 This partially reverts istio#24958. This PR did two things - allow using httpsRedirect without VirtualServices (good) and remove all routes when using httpsRedirect (bad). The latter breaks in two cases: when a user has an HTTPS server and they put httpsRedirect, it previously was silently ignored - after the PR, all the routes were dropped. With this change, these routes are added, and users will get a warning at validation time since the config is not useful. The other case is that a user can have an HTTP server but still "bypass" the httpsRedirect with X-Forward-Proto and num_trusted_hops, so removing the routes breaks this. Both of these cases have unit and integration tests. Additionally, the original PR indirectly caused httpsRedirect=true to take precedence during Gateway conflicts. Previously, the last gateway won, which caused weird behavior. I kept the behavior of the original PR here; during conflict we will keep the httpsRedirect. This isn't ideal, since it interferes with cert-manager ACME requests. Longer term we will want to make httpsRedirect per-route most likely. See istio#27643 (comment) for more info. * note * Fixes * fix lint * fix echo (cherry picked from commit 56d740a)
howardjohn
force-pushed
the
18/cp-tls-redirect
branch
from
43b9870
to
f496053
Compare
|
/retest |
howardjohn
changed the title
GregHanson
approved these changes
Jan 22, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix issues around tlsRedirect
Fixes [1.7.x] VirtualService HTTP routes are not picked up when Gateway has httpsRedirect set to true #27315
Fixes Virtual service routes ignored when httpsRedirect: true in port 443 section #27157
This partially reverts #24958. This
PR did two things - allow using httpsRedirect without VirtualServices
(good) and remove all routes when using httpsRedirect (bad). The latter
breaks in two cases: when a user has an HTTPS server and they put
httpsRedirect, it previously was silently ignored - after the PR, all
the routes were dropped. With this change, these routes are added, and
users will get a warning at validation time since the config is not
useful.
The other case is that a user can have an HTTP server but still "bypass"
the httpsRedirect with X-Forward-Proto and num_trusted_hops, so removing
the routes breaks this.
Both of these cases have unit and integration tests.
Additionally, the original PR indirectly caused httpsRedirect=true to
take precedence during Gateway conflicts. Previously, the last gateway
won, which caused weird behavior. I kept the behavior of the original PR
here; during conflict we will keep the httpsRedirect. This isn't ideal,
since it interferes with cert-manager ACME requests. Longer term we will
want to make httpsRedirect per-route most likely. See
#27643 (comment) for
more info.
note
Fixes
fix lint
fix echo
(cherry picked from commit 56d740a)
Please provide a description for what this PR is for.
And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure
Pull Request Attributes
Please check any characteristics that apply to this pull request.
[ ] Does not have any changes that may affect Istio users.