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

added validation for source and target #85

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

AshwinKul28
Copy link
Contributor

  • Fixed a bug where the user can create a reverse proxy even if the host or target is empty

Related issue: #54

- Fixed a bug where user can create a reverseproxy even if host or target is empty
Copy link
Member

@quentinguidee quentinguidee left a comment

Choose a reason for hiding this comment

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

Thank you! Here are some request changes that need to be addressed before merging:

apps/reverseproxy/handler/proxy.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@quentinguidee
Copy link
Member

Sorry for the confusion; I think the documentation should be more explicit about this.

In fact, both versions were half-right. This new version check the errors at the correct place, but doesn't returns to the client why the request has failed.

  • The service should return, for example, an ErrSourceInvalid or ErrTargetInvalid depending on the error;
  • Then, the handler should receive the error given by the service (eg. if errors.Is(err, ErrSourceInvalid) {...}) and returns an HTTP response corresponding to the error.

Take for example the feature to delete a container: https://github.com/vertex-center/vertex/blob/dev/apps/containers/handler/container.go#L120:L135. If the service says that the service is still running, the handler returns a response that is adapted to that.

So: the service makes the verification and returns why it fails, and the http handler generates an HTTP response for this error.

But appart from this I think you got it!

For the empty string, this is optional for this PR but you can also use strings.TrimSpace() to also invalidate sources/targets like " " (with only spaces).

@quentinguidee
Copy link
Member

quentinguidee commented Oct 19, 2023

(Also don't hesitate to say what should be improved in the documentation about the architecture, the doc is pretty new!)

@AshwinKul28
Copy link
Contributor Author

AshwinKul28 commented Oct 19, 2023

Thanks @quentinguidee for the explanation. Actually, my both solutions were half-baked at their own places based on what actually this repo expects.

@AshwinKul28
Copy link
Contributor Author

fixing the lint errors.

@quentinguidee
Copy link
Member

Looks good to me! Thank you for your contribution 🚀

@quentinguidee quentinguidee merged commit 654acc1 into vertex-center:dev Oct 19, 2023
14 checks passed
@quentinguidee quentinguidee added type: Enhancement New feature or request app: Reverse Proxy type: Bug Something isn't working and removed type: Enhancement New feature or request labels Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants