Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

removing rmbProxy endpoints, RMB, Ygg, and Redis #292

Merged
merged 10 commits into from
Feb 28, 2023

Conversation

sameh-farouk
Copy link
Contributor

@sameh-farouk sameh-farouk commented Feb 14, 2023

Description

as a part of switching to the new RMB implementation, we are refactoring the grid-proxy server to remove the rmb-proxy functionality and any related services such RMB, Ygg and Redis.

Changes

  • Remove rmbproxy endpoints
  • Remove rmb, ygg, and redis
  • Remove all obsolete configuration
  • Update chart, remove RMB Kubernetes service
  • Update swagger docs
  • Update Readme

Removed Services:

  • RMB
  • Ygg
  • Redis

Removed options:

  • redis
  • substrate
  • rmbTimeoutSeconds

Removed Env Vars:

  • MNEMONICS
  • SUBSTRATE
  • REDIS
  • RMB_TIMEOUT

Removed Endpoints:
/twin/*

Related Issues

#291

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings

Copy link
Collaborator

@xmonader xmonader left a comment

Choose a reason for hiding this comment

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

please restore everything related to tls/certs

@sameh-farouk
Copy link
Contributor Author

sameh-farouk commented Feb 15, 2023

please restore everything related to tls/certs

@xmonader thanks for your review, requested changes were done in 0474a2a and 3efaf20

however I have these notes, first, I removed this code because

  1. It was implemented mainly in the rmbproxy pkg, I thought that maybe was planned for something RMB related, I didn't see it used anywhere but maybe I'm wrong.
  2. I understand The helm chart uses Kubernetes certificate management controller, not depend on that code.
  3. I have an impression that the code that serves TLS is not complete, also given that everywhere we start the grid proxy we pass --no-cert wasn't sure that it is still relevant.

now I have a question, why do we need this anyway? regarding any future docker-compose projects wouldn't be better to configure an Nginx container to do TLS Offloading instead or use our gateway?

Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/swagger.json Outdated Show resolved Hide resolved
docs/swagger.yaml Outdated Show resolved Hide resolved
cmds/proxy_server/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xmonader xmonader left a comment

Choose a reason for hiding this comment

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

Add few comments please check

@xmonader
Copy link
Collaborator

please restore everything related to tls/certs

@xmonader thanks for your review, requested changes were done in 0474a2a and 3efaf20

however I have these notes, first, I removed this code because

  1. It was implemented mainly in the rmbproxy pkg, I thought that maybe was planned for something RMB related, I didn't see it used anywhere but maybe I'm wrong.

It was implemented for the gridproxy itself, not related to the rmb proxying features. In general given that code for acme client is already solid in go (thanks to many server applications e.g caddy) we can reuse it in our binaries to depend on less and less, so no need to drag in more components e.g caddy or nginx

  1. I understand The helm chart uses Kubernetes certificate management controller, not depend on that code.

It's true that gridproxy runs on kubernetes -for the time being- which wasn't the case in the beginning where we managed the proxy machines in regular VMs setups, and now we are moving away from that again to docker-compose based setups

  1. I have an impression that the code that serves TLS is not complete, also given that everywhere we start the grid proxy we pass --no-cert wasn't sure that it is still relevant.

It's complete and working, we are only support http-01 flow, also the usage of --no-cert is already to serve use cases where the operator deploys it when kubernetes infrastructure or with a sidecar for TLS.

now I have a question, why do we need this anyway? regarding any future docker-compose projects wouldn't be better to configure an Nginx container to do TLS Offloading instead or use our gateway?

You can have different kinds of setups e.g on a VM, with container(s), within kubernetes. IMHO, that option with so little code, enabled variety of options for deployment, That's why I wouldn't deleted it

@sameh-farouk
Copy link
Contributor Author

sameh-farouk commented Feb 27, 2023

Add few comments please check

@xmonader all non-doc related issues were resolved (except for the server binary name waiting for your reply).
Do you mind handling all the docs-related issues in a separate PR where docs can be focused on and revised? it would be easier also for reviewers. if so I can open an issue for the docs, link it on a new PR and work on it.

@xmonader
Copy link
Collaborator

Let's address the minor docs comments in this PR and do your full review in another issue / PR please

@sameh-farouk
Copy link
Contributor Author

Let's address the minor docs comments in this PR and do your full review in another issue / PR please

@xmonader done in f4854e5, please check.

@xmonader
Copy link
Collaborator

Let's update the branch before merging

@sameh-farouk
Copy link
Contributor Author

Let's update the branch before merging

@xmonader branch updated

@sameh-farouk sameh-farouk merged commit 0ef41af into development Feb 28, 2023
@sameh-farouk sameh-farouk deleted the development_remove_rmb_ygg branch February 28, 2023 15:24
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.

None yet

2 participants