Fix redirect mapping regression #4028

Alice-Lilith · 2022-01-20T22:45:16Z

Description

Cherry picking a community contribution and making a small change to the tests so they actually run now so we can get this out in the next release.

Related Issues

Original Contribution: #4004
Related issues:
#3709
#4005

Testing

We already had tests that should have discovered that this regression occurred, but those tests were set to not run. I've re-enabled them and updated the gold test config to account for the fact that the redirects are now showing up in the Envoy config.

Checklist

I made sure to update CHANGELOG.md.

Remember, the CHANGELOG needs to mention:
- Any new features
- Any changes to our included version of Envoy
- Any non-backward-compatible changes
- Any deprecations
This is unlikely to impact how Ambassador performs at scale.

Remember, things that might have an impact at scale include:
- Any significant changes in memory use that might require adjusting the memory limits
- Any significant changes in CPU use that might require adjusting the CPU limits
- Anything that might change how many replicas users should use
- Changes that impact data-plane latency/scalability
My change is adequately tested.

Remember when considering testing:
- Your change needs to be specifically covered by tests.
  - Tests need to cover all the states where your change is relevant: for example, if you add a behavior that can be enabled or disabled, you'll need tests that cover the enabled case and tests that cover the disabled case. It's not sufficient just to test with the behavior enabled.
- You also need to make sure that the entire area being changed has adequate test coverage.
  - If existing tests don't actually cover the entire area being changed, add tests.
  - This applies even for aspects of the area that you're not changing – check the test coverage, and improve it if needed!
- We should lean on the bulk of code being covered by unit tests, but...
- ... an end-to-end test should cover the integration points
I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.

1. The ldflags thing is a sham, we must always get it from the ambassador.version file. + And therefore we should not set -ldflags in Dockerfile or builder.sh. And because the 'version' build-arg is now unused, remove that too. 2. Use go-shellquote instead of our own janky de-quoting. 3. Simplify by using os.ReadFile. 4. Don't use global variables. 5. Emphasize that the version-parsing code must be kept in-sync with VERSION.py Signed-off-by: Luke Shumaker <[email protected]> (cherry picked from commit 02a83ee)

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Flynn <[email protected]>

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>

This turned out to be an upstream bug in k8s.io/kube-openapi. Upgrading that package fixes the bug. kubernetes/kube-openapi#230 Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Flynn <[email protected]>

Signed-off-by: Gabriel Féron <[email protected]> Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: AliceProxy <[email protected]>

LukeShu · 2022-01-21T06:14:21Z

Does this need to be on top of shared/2.1.2-fixes? I ask because that PR (#4026) already needs to be rebased on to newer #4025 (which also has some changes requested on it). If there's not a reason it needs to be on top of shared/2.1.2-fixes, it would probably be simpler to just target release/v2.1 directly.

LukeShu · 2022-01-21T06:14:53Z

Oops, I accidentally clicked "Close with comment" instead of "Comment". 😬

LukeShu · 2022-01-21T06:16:35Z

python/ambassador/ir/irhost.py

@@ -383,7 +383,8 @@ def matches_httpgroup(self, group: 'IRHTTPMappingGroup') -> bool:
        else:
            # It is NOT A TYPO that we use group.get("host") here -- whether the Mapping supplies
            # "hostname" or "host", the Mapping code normalizes to "host" internally.
-            group_glob = group.get('host') or None  # NOT A TYPO: see above.
+            redirect_glob = group.get('host_redirect', {}).get('host') if group.get('host_redirect', {}) else None


I'm confused, isn't this equivalent to just

redirect_glob = group.get('host_redirect', {}).get('host')

?

The main issue with why this looks so gross is that group.get('host_redirect', {}) is sometimes None meaning that a group.get('host_redirect', {}).get('host') will throw an error because you cant NoneType.get('host') thats why I first check if anything is returned by group.get('host_redirect', {}) before doing group.get('host_redirect', {}).get('host') that said, there is still probably a better way to write that out.

I would probably do

# It's possible for group.host_redirect to be None instead of missing, and it's also # conceivably possible for group.host_redirect.host to be "", which we'd rather be # None. Hence we do this two-line dance to massage the various cases. host_redirect = group.get('host_redirect') or {} redirect_glob = host_redirect.get('host') or None

This might not be "better" but at least there's room for the explanation...

LukeShu · 2022-01-21T06:17:38Z

python/ambassador/ir/irhost.py

@@ -383,7 +383,8 @@ def matches_httpgroup(self, group: 'IRHTTPMappingGroup') -> bool:
        else:
            # It is NOT A TYPO that we use group.get("host") here -- whether the Mapping supplies
            # "hostname" or "host", the Mapping code normalizes to "host" internally.
-            group_glob = group.get('host') or None  # NOT A TYPO: see above.
+            redirect_glob = group.get('host_redirect', {}).get('host') if group.get('host_redirect', {}) else None
+            group_glob = group.get('host') or redirect_glob or None  # NOT A TYPO: see above.


If redirect_glob is falsey, doesn't that mean it's None? Isn't the or None superfluous?

This is true, I can definitely change that

LukeShu · 2022-01-21T06:18:58Z

I would encourage you to squash this in to just 1 commit (and add a bit to the commit message explaining what's different from @gferon's original commit).

kflynn

I agree with @LukeShu that this could be collapsed into one commit, and that some of the Python can be cleaned up a bit. Thanks for taking this one on!

(Oh, and I think it can go directly onto release/v2.1, too.)

LukeShu · 2022-01-21T23:18:23Z

closing in favor of #4034

LukeShu and others added 14 commits January 19, 2022 23:09

Mount the Go build cache for Docker builds.

d373683

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Force BuildKit in builder.mk

98b35ff

Signed-off-by: Flynn <[email protected]>

Disable fail-fast in the pytest matrix

780a55f

Signed-off-by: Flynn <[email protected]>

This should've been an ASCII space, not \u00A0 (non-breaking-space)

e393cfa

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

pkg/kates/validation.go: Use modern error packages

55fac51

Signed-off-by: Luke Shumaker <[email protected]>

Route: Correctly whitelist fields for the CORS Envoy config

767241c

Signed-off-by: Luke Shumaker <[email protected]>

Upgrade k8s.io/kube-openapi to fix support for rewrite: ""

4c8528b

This turned out to be an upstream bug in k8s.io/kube-openapi. Upgrading that package fixes the bug. kubernetes/kube-openapi#230 Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Log inputs & outputs in apiext if debugging is enabled

501086b

Signed-off-by: Flynn <[email protected]> Signed-off-by: Luke Shumaker <[email protected]>

Use the correct mapping weight when generating Envoy routes(!!)

a3ef899

Signed-off-by: Flynn <[email protected]>

annotations_test: drop extraneous newline

15feb5f

Signed-off-by: Flynn <[email protected]>

Fix matching a Mapping with host_redirect a Host

28f58ed

Signed-off-by: Gabriel Féron <[email protected]> Signed-off-by: AliceProxy <[email protected]>

fix redirect mappings thanks to @gferon

cdf74ba

Signed-off-by: AliceProxy <[email protected]>

update gold-test snapshot

e2ec610

Signed-off-by: AliceProxy <[email protected]>

Alice-Lilith requested review from kflynn and LukeShu January 20, 2022 23:21

LukeShu closed this Jan 21, 2022

LukeShu reopened this Jan 21, 2022

LukeShu reviewed Jan 21, 2022

View reviewed changes

kflynn requested changes Jan 21, 2022

View reviewed changes

kflynn force-pushed the shared/2.1.2-fixes branch 2 times, most recently from 348b2e7 to dd18207 Compare January 21, 2022 18:35

Base automatically changed from shared/2.1.2-fixes to release/v2.1 January 21, 2022 19:02

LukeShu closed this Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redirect mapping regression #4028

Fix redirect mapping regression #4028

Alice-Lilith commented Jan 20, 2022

LukeShu commented Jan 21, 2022

LukeShu commented Jan 21, 2022 •

edited

Loading

LukeShu Jan 21, 2022

Alice-Lilith Jan 21, 2022

kflynn Jan 21, 2022

LukeShu Jan 21, 2022

Alice-Lilith Jan 21, 2022

LukeShu commented Jan 21, 2022

kflynn left a comment

LukeShu commented Jan 21, 2022

Fix redirect mapping regression #4028

Fix redirect mapping regression #4028

Conversation

Alice-Lilith commented Jan 20, 2022

Description

Related Issues

Testing

Checklist

LukeShu commented Jan 21, 2022

LukeShu commented Jan 21, 2022 • edited Loading

LukeShu Jan 21, 2022

Choose a reason for hiding this comment

Alice-Lilith Jan 21, 2022

Choose a reason for hiding this comment

kflynn Jan 21, 2022

Choose a reason for hiding this comment

LukeShu Jan 21, 2022

Choose a reason for hiding this comment

Alice-Lilith Jan 21, 2022

Choose a reason for hiding this comment

LukeShu commented Jan 21, 2022

kflynn left a comment

Choose a reason for hiding this comment

LukeShu commented Jan 21, 2022

LukeShu commented Jan 21, 2022 •

edited

Loading