Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Augment proxy rewrite #303

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Augment proxy rewrite #303

merged 5 commits into from
Oct 24, 2023

Conversation

waTeim
Copy link

@waTeim waTeim commented Oct 24, 2023

Add to tycho the capability of setting the rewrite target to tycho while preserving backwards compatibility.

old:

proxy-rewrite-rule: boolean

new:

proxy-rewrite:
  enabled: boolean
  target: string

In the new implementation, proxy-rewrite-rule is still allowed, which is used to set proxy-rewrite.enabled in this instance target defaults to None. If a value of None is encountered, then the old rules apply. Both enabled and proxy-rewrite-rule default to False.

Additionally, if target is not None then an additional ambassador directive is included to recover the original URL.

add_response_headers:
    X-Original-Path: <same as the value of prefix>

@frostyfan109
Copy link

Do we know what proxy_rewrite_rule is supposed to do currently (like before this change)? I'm still unclear on its purpose.

@waTeim
Copy link
Author

waTeim commented Oct 24, 2023

As am I, GPT claims all it did previously was set rewrite to the same value as prefix which accomplished nothing.

@frostyfan109
Copy link

I'm pretty sure you said that none of the apps currently (besides RStudio) are using this anyways? So I don't really think there's any reason to hold off on merging. But if that's wrong and apps are using this we should probably figure out what it does.

I know in the tycho code it appears like this rule is completely redundant, so I'm curious why enabling it changed the behavior of RStudio?

@waTeim
Copy link
Author

waTeim commented Oct 24, 2023

It is on for Jupyter, and I did verify that its behavior is unchanged. I now half-suspect enabling proxy-rewrite-rule for R last week didn't actually fix R, but something else was going on (which I found out during testing).

@frostyfan109
Copy link

When I modified the service mapping for RStudio, it affected its behavior? So even if it didn't fix it a couple weeks ago, it still changed something about how Ambassador was proxying its requests, which puzzles me having looked at the Tycho code.

@waTeim
Copy link
Author

waTeim commented Oct 24, 2023

Heisenbug (mild) is what I'm thinking currently.

@frostyfan109
Copy link

It's not because when I changed the service spec back the behavior went back to expected, so I think it must change something.

@waTeim waTeim merged commit 9e3ee18 into develop Oct 24, 2023
2 checks passed
@waTeim waTeim deleted the proxy_rewrite branch October 24, 2023 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants