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

Regression: Ambassador 1.7.2+ routes the / path even when redirection is requested #2996

Closed
kflynn opened this issue Oct 1, 2020 · 1 comment
Closed

Regression: Ambassador 1.7.2+ routes the / path even when redirection is requested #2996

kflynn opened this issue Oct 1, 2020 · 1 comment
Assignees
@LukeShu
Labels
t:bug Something isn't working
Milestone
2020 Cycle 6

Comments

@kflynn
Copy link
Member

kflynn commented Oct 1, 2020 •
edited
Loading

This is a regression in 1.7.2. A Host specifying

requestPolicy:
  insecure:
    action: Redirect

(which is the default) will still route requests to /.
@kflynn kflynn mentioned this issue Oct 1, 2020
Closed
@khussey khussey added the t:bug Something isn't working label Oct 1, 2020
@khussey khussey added this to the 2020 Cycle 6 milestone Oct 1, 2020
@kflynn kflynn changed the title Regression: Ambassador 1.7.3 routes the / path even when redirection is requested Regression: Ambassador 1.7.2+ routes the / path even when redirection is requested Oct 1, 2020
@khussey
Copy link
Contributor

khussey commented Oct 6, 2020

This has been fixed in 1.7.4 by reverting the changes for #2888 that shipped in 1.7.0.

@khussey khussey closed this as completed Oct 6, 2020
kflynn pushed a commit that referenced this issue Oct 8, 2020
@LukeShu
(from AES) Add (failing) tests
bb72b22 
There are 3 toggles for known bugs

    bug_single_insecure_action = False # Do all Hosts have to have the same insecure.action?
    bug_forced_star = False            # Do we erroneously send replies in cleartext instead of TLS for unknown hosts?
    bug_404_routes = False             # Do we erroneously send 404 responses directly instead of redirect-to-tls first?

These toggles are all set to "False" in this commit; they exist to
make it possible for these tests to be used against 1.6.2/1.7.4-rc.1
where those bugs have "historical privilege" and don't count as
regressions.

The results of the added tests as of this commit:

  | HostCRDRootRedirectCongratulations  | pass                 |
  | HostCRDRootRedirectSlashMapping     | fail                 |
  | HostCRDRootRedirectRE2Mapping       | fail                 |
  | HostCRDRootRedirectECMARegexMapping | fail                 |
  | HostCRDForcedStar                   | pass                 |
  | TLSInheritFromModule                | doesn't become ready |

None of the errors have to do with the `bug_*` toggles; setting any of
the toggles to "True" wouldn't make a difference.

This should cover the issues:
  #2957
  #2996

I didn't write automated tests for
#2984 since any test I
can think of writing that might pass for 1.6.2 and 1.7.4-rc.1 is just
testing for buggy behavior.  I think we need to have a conversation
about what the correct behavior is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LukeShu LukeShu

Labels
t:bug Something isn't working
Projects
None yet
Milestone
2020 Cycle 6
Development

No branches or pull requests
3 participants
@LukeShu @khussey @kflynn