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.

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

feat: add sample for HTTP redirect. #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wsh
Copy link

@wsh wsh commented Jul 31, 2023

Replaces #1 (somehow my fork got diassociated).

@wsh wsh requested review from a team and upeeth as code owners July 31, 2023 03:11
@wsh
Copy link
Author

wsh commented Jul 31, 2023

/assign @leonm1

Copy link
Collaborator

@martijneken martijneken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A few small nits, but otherwise LGTM.

// See the License for the specific language governing permissions and
// limitations under the License.

#include <boost/url/parse.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tags so the recipes are embeddable in docs? (see other samples)

Basically wrap both plugins in:
// [START serviceextensions_http_redirect]
// [END serviceextensions_http_redirect]

return Action::Continue
};
let redirect = match url.path() {
"/index.php" => base,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is reusing base needlessly fragile? I wonder what happens if :path ever includes domain. I also find it somewhat harder to read/understand. Maybe we can use dummy.com above, and example.com below in the 301?

// Create stream context.
auto http_context = TestHttpContext(handle_);

// Expect no-op if no path is specified. (Never expected.)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically 3 different tests here (no headers, root path, redirect path), so let's split them up.

_ => None
};
if let Some(redirect_url) = redirect {
self.send_http_response(301, vec![("Location", redirect_url.as_str())], None);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think we want to return Action::Pause here.

return FilterHeadersStatus::Continue;
}
if (url->path() == "/index.php") {
sendLocalResponse(301, "", "", {{"Location", "http://www.example.com/"}});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extra clarity:
return FilterHeadersStatus::StopAllIterationAndWatermark;

Signed-off-by: Will Hayworth <wsh@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants