decentralized-identity / universal-resolver

Universal Resolver implementation and drivers.
https://uniresolver.io/
Apache License 2.0
529 stars 233 forks source link

feat: Switch cheqd DID driver to use local Docker service path #331

Closed ankurdotb closed 1 year ago

ankurdotb commented 1 year ago

Previously, the DID URL mapping for did:cheqd incorrectly targetted a remote endpoint. This change remaps this to the local Docker container URI path.

Additionally, the previous tag on Github Container Registry was invalid. The version number has been bumped to v1.5.2 to make this accessible.

ankurdotb commented 1 year ago

@BernhardFuchs Is there a staging environment this can be tested in? I've taken your comment from #329 and fixed them here. One question is the correct way to do a greedy match beyond the /1.0/identifiers/<id> path. In our case, we support custom paths such as /1.0/identifiers/<did>/resources/<resource-id>. As far as I understand, when it's passed from the router to our container, only the first path is passed, i.e., <did> = $1. Should this be defined as a different regex to match anything and be passed correctly from the router container?

peacekeeper commented 1 year ago

@ankurdotb At the moment the open-source Universal Resolver only supports resolving DIDs, not dereferencing DID URLs. So currently there is no way how additional paths or query strings or fragments can be passed down to the driver container. Until recently, since Cheqd has been pioneering interesting ways to use DID URLs to identify various resources, there really hasn't been a lot of adoption of DID URLs.

It's on our roadmap to add support for this as well in the Universal Resolver, I'll let you know once this is available. I have also already discussed this with Renata a bit.

BernhardFuchs commented 1 year ago

@ankurdotb There seems to be an issue with configuration. The container fails at startup with the following error:

{"level":"info","time":"2022-10-17T08:46:51Z","message":"Loading configuration"}
panic: invalid config parameter, Endpoint config for mainnet is invalid:

goroutine 1 [running]:
github.com/cheqd/did-resolver/cmd.serve()
    /builder/cmd/serve.go:30 +0xb05
github.com/cheqd/did-resolver/cmd.getServeCmd.func1(0xc000cd4280, {0xeff705, 0x0, 0x0})
    /builder/cmd/serve.go:21 +0x17
github.com/spf13/cobra.(*Command).execute(0xc000cd4280, {0x18c2008, 0x0, 0x0})
    /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:876 +0x60e
github.com/spf13/cobra.(*Command).ExecuteC(0xc000cd4000)
    /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:990 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
    /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:918
main.main()
    /builder/main.go:30 +0x90
ankurdotb commented 1 year ago

@BernhardFuchs Need to look into this, the obvious fix didn't work. I'm marking this as draft until resolved

ankurdotb commented 1 year ago

Hey @BernhardFuchs or @peacekeeper, I've got a question for you: in #329 Bernhard said (my emphasis in bold):

Change the url in the application.yml to http://cheqd-did-driver:**8080**

In the docker-compose.yml, our port mapping is 8131:8080. So, shouldn't the value above be 8131, which is the port published/exposed by our container? If it's 8080, then it goes through the top level servlet. I assume that's the intended behaviour?

ankurdotb commented 1 year ago

Issue is fixed now and ready for review, btw

BernhardFuchs commented 1 year ago

Thx for the fix, the deployment works now and I merge it with this branch https://github.com/decentralized-identity/universal-resolver/tree/test-driver-did-cheqd because I had to add the env variables manually https://github.com/decentralized-identity/universal-resolver/blob/test-driver-did-cheqd/ci/deploy-k8s-aws/scripts/driver-config.yaml

Historically we used docker-compose to host the application and you added them at the correct place but atm we don't parse the env variables from docker-compose.yml. Most contributors add default values in their Dockerfiles and so it is not a major issue, but we are aware that the current behaviour is confusing and needs some rework.

Ad port numbers: For the docker-compose setup we needed distinct ports on the host machine and that's where the current port mapping in docker-compose.yml is originated. We are using K8s now and have a custom CD pipeline which parses the container port. So the host port can be basically ignored unless some wants to use docker-compose.

ankurdotb commented 1 year ago

@BernhardFuchs When I did a docker-compose...up it correctly read the environment variables. So if I understand correctly, you're not picking them up because you use k8's and that skips the file, is that right?

We can definitely add those parameters as default ARGs/ENVs, if that helps you in not having to keep a custom CI injection. For anyone using Docker Compose instead of K8s, the compose up statement will override the default args. I can do that in a separate PR if you want?