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

/openapi endpoint with forwarded header #433

Merged
merged 4 commits into from
Jun 3, 2021

Conversation

sknot-rh
Copy link
Member

Signed-off-by: Stanislav Knot sknot@redhat.com

If I understood #432 correctly, this should fix it.

If we call /openapi endpoint and the x-Forwarded-Path header is set, its value is projected into basePath in the result openapi spec.

@sknot-rh sknot-rh added this to the 0.17.0 milestone Jun 19, 2020
@ppatierno ppatierno modified the milestones: 0.17.0, 0.18.0 Jun 25, 2020
@sknot-rh sknot-rh requested a review from ppatierno June 30, 2020 06:15
@ppatierno
Copy link
Member

@elakito it would be great to have your feedback on this

@elakito
Copy link
Contributor

elakito commented Jul 7, 2020

Ok. It looks good but I have a few comments.

  1. Regarding the header name, both x-forwarded-prefix and x-forwarded-path headers are used for this purpose and it seems x-forwarded-prefix is more widely used or mentioned (in google search, "x-forwarded-prefix" returns 6.920 hits whereas "x-forwarded-path" returns 793 hits). Therefore, it would be more convenient to look up both names so that it will work out-of-the-box in all environments.

  2. for this path rewriting step, if this header is absent, would it be simpler to just return the original buffer than always doing json-parse-serialize to create another buffer?

  3. another question regarding this path rewriting step, originally I thought one would use a templated original data with a template engine like mustache to set the variables. I think that would probably be more efficient than doing the json-parse-serialize approach but would create a new dependency to another lib or else would need some custom template variable replacing code. So, I am raising this point but no strong opinion about which approach is better.

  4. for this rewriting to be useful, the host property should either be removed from openapiv2.json so that the client can simply assume the host to be the current target host or be replaced to the correct host using x-forwarded-host. I think simply removing this host property from openapi spec is a better choice than adding the host replacing code.

@sknot-rh sknot-rh force-pushed the forwarded-openapi branch 2 times, most recently from 6354901 to 3f2e4e0 Compare July 8, 2020 08:48
Copy link
Contributor

@elakito elakito left a comment

Choose a reason for hiding this comment

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

LGTM.
thanks.

@sknot-rh sknot-rh requested a review from scholzj July 9, 2020 08:09
@ppatierno ppatierno modified the milestones: 0.18.0, 0.19.0 Jul 13, 2020
@ppatierno ppatierno modified the milestones: 0.19.0, 0.20.0 Oct 6, 2020
Base automatically changed from master to main March 25, 2021 19:24
@scholzj
Copy link
Member

scholzj commented Jun 1, 2021

@sknot-rh Could you please have a look at the conflict? @ppatierno Could you please review this?

@scholzj scholzj removed their request for review June 1, 2021 19:34
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@scholzj scholzj merged commit 9964dcc into strimzi:main Jun 3, 2021
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.

None yet

4 participants