safe-global / safe-infrastructure

One `docker-compose.yml` file to rule them all
MIT License
56 stars 92 forks source link

Add new Client Gateway #109

Closed hectorgomezv closed 1 year ago

hectorgomezv commented 1 year ago

Closes #99

This PR:

Uxio0 commented 1 year ago

@hectorgomezv Sorry I introduced a conflict when merging a previous PR.

Also, could you update README information for the hooks on https://github.com/safe-global/safe-infrastructure/blob/main/docs/running_locally.md#step-4-add-your-webhooks ?

hectorgomezv commented 1 year ago

@hectorgomezv Sorry I introduced a conflict when merging a previous PR.

Also, could you update README information for the hooks on https://github.com/safe-global/safe-infrastructure/blob/main/docs/running_locally.md#step-4-add-your-webhooks ?

Thank you @Uxio0! The conflict was solved. I've also included some changes in the runnning_locally.sh file regarding the environment variable naming change in the new CGW (AUTH_TOKEN instead of WEBHOOK_TOKEN).

Uxio0 commented 1 year ago

Perfect @hectorgomezv

@mshakeg This new client gateway is ready for arm64 so it should work out the box in your environment

hectorgomezv commented 1 year ago

I've detected a problem with the new Client Gateway validation when dealing with a local setup. When uploading a logo image for a chain, the Config Service stores something like:

      "nativeCurrency": {
        "name": "Matic",
        "symbol": "MATIC",
        "decimals": 18,
        "logoUri": "/media/chains/137/currency_logo.png"
      },

This doesn't pass the new CGW validation because the logoUri is not considered a valid URL:

...
 message":{
  "arguments":[],
  "code":42,
  "detail": [{
    "instancePath":"/nativeCurrency/logoUri",
    "message":"must match format \"uri\"",
    "schemaPath":"native-  currency.json/properties/logoUri/format",
  }]
"message":"Validation failed"}
...

What's your take on this? (cc. @fmrsabino @Uxio0) Should we relax the CGW validation checks for this field?

Uxio0 commented 1 year ago

Not a strong opinion here, this was done to allow people without a DNS or S3 to upload and manage images, let's see @fmrsabino take on this. The most important thing is to find a way for this to work locally.

hectorgomezv commented 1 year ago

I have an update on this, I tried to use the incoming request data to compose an absolute URL in the Config Service response. This is the branch with the change: https://github.com/safe-global/safe-config-service/compare/main...return-native-currency-absolute-uri But I need to investigate this further because I'm not sure this could be a feasible option for the production setup (due to the request origins inside Kubernetes clusters).

But this also leads to unwanted effects when using Docker, because the request origin is the Nginx container, so the value that the Config Service container sees as origin is http://nginx/, and the composed value get converted to http://nginx/media/chains/137/currency_logo.png. This passes the CGW validation, but the URL is not valid outside the Docker network.

Another workaround might be accessing the correct path (http://localhost:8000/cfg/...) by using environment variables inside the container, but I suspect this path could lead us to code custom logic for running the service with/without Docker, which would add too much complexity and modify the production code to adapt it to the local setup.

Any thoughts? I think the easier way right now is to relax the CGW validation, but I don't have a strong opinion on it. Do you think another path is preferable @Uxio0 @fmrsabino ?

Uxio0 commented 1 year ago

Setting MEDIA_URL to http://localhost:8000/ will not work? (just guessing, never tried that)

hectorgomezv commented 1 year ago

Setting MEDIA_URL to http://localhost:8000/ will not work? (just guessing, never tried that)

I was also planning to try that one (thanks for the suggestion!). But I saw this also breaks links to the static content, but I'm checking and that behavior was already the case in the Docker setup before this PR, at least for me 🤔

But I agree it's the simplest solution. If I'm not wrong we'd need to make the MEDIA_URL configurable in the Config Service (I've created a branch for it: https://github.com/safe-global/safe-config-service/compare/make-media-url-configurable?expand=1), and then include in cfg.env the following:

MEDIA_URL = "http://localhost:8000/cfg/media/"

If it sounds good to you, @Uxio0 , I can do the changes on both sides, but I guess this PR would get blocked until the next release of the Config Service including that change. (edit: but we could build the latest image tag, which is the one referenced in this project's env.sample, to speed up things)

Uxio0 commented 1 year ago

If it sounds good to you, @Uxio0 , I can do the changes on both sides, but I guess this PR would get blocked until the next release of the Config Service including that change. (edit: but we could build the latest image tag, which is the one referenced in this project's env.sample, to speed up things)

Sounds good to me, no hurries. We could also improve /cfg/media access by sharing the folder on docker and exposing it on nginx (so Django doesn't need to handle static files), but that's not required for this PR

hectorgomezv commented 1 year ago

@Uxio0 I've added the MEDIA_URL environment variable to the cfg.env file inAdd MEDIA_URL to cfg.env, update README example versions. We've updated the Config Service latest image including this optional setting: https://github.com/safe-global/safe-config-service/pull/912

So with this change, we avoid the problems with the validation and all the containers and I think we're good to go.