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 check endpoints that do not mirror status code #853

Merged
merged 6 commits into from
Jun 15, 2022

Conversation

zepatrik
Copy link
Member

closes #851

Naming should be improved, /check-sdk is probably not as helpful.

@zepatrik zepatrik requested a review from aeneasr March 11, 2022 13:35
@@ -70,7 +73,7 @@ type getCheckRequest struct {
MaxDepth int `json:"max-depth"`
}

// swagger:route GET /check read getCheck
// swagger:route GET /check-sdk read getCheck
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use a query parameter instead? For example alwaysSend200=true? And I would suggest to keep the response payload the same between the two variants.

Alternatively it could be /check/<method> or /allowed/<method> where <method> could be for example openapi or envoy, traefik, ...

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively it could be /check/ or /allowed/ where could be for example openapi or envoy, traefik, ...

Makes sense 👍
The problem with query parameter is that no status should be default for SDK, but not default for others.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense yeah. Another option is to use the user-agent, however I think that is not strict enough for security purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you had a good point there 😉 #851 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed to check/openapi.

@zepatrik zepatrik self-assigned this May 10, 2022
@hperl hperl self-assigned this Jun 13, 2022
@hperl hperl force-pushed the fix/sdk-check-errors branch from 4ce02ef to d52c3bb Compare June 13, 2022 13:54
@zepatrik zepatrik force-pushed the fix/sdk-check-errors branch 2 times, most recently from 70788b5 to 422a8ec Compare June 15, 2022 15:00
@zepatrik zepatrik force-pushed the fix/sdk-check-errors branch from 422a8ec to 9b89f20 Compare June 15, 2022 15:23
@zepatrik
Copy link
Member Author

For now we have both check variants as part of the openapi spec. Ideally we would expose both on the docs page, but exclude the status mirroring endpoints from the SDK clients. I am not sure yet where to do that.

@zepatrik zepatrik merged commit 07d0fbd into master Jun 15, 2022
@zepatrik zepatrik deleted the fix/sdk-check-errors branch June 15, 2022 15:33
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.

.NET SDK throws exception when GetCheckAsync returns allowed: false
3 participants