universal-resolver icon indicating copy to clipboard operation
universal-resolver copied to clipboard

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

Open ankurdotb opened this issue 1 year ago • 2 comments

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 avatar Oct 14 '22 18:10 ankurdotb

@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?

ankurdotb avatar Oct 14 '22 18:10 ankurdotb

@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.

peacekeeper avatar Oct 16 '22 13:10 peacekeeper

@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/[email protected]/command.go:876 +0x60e
github.com/spf13/cobra.(*Command).ExecuteC(0xc000cd4000)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:990 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:918
main.main()
	/builder/main.go:30 +0x90

BernhardFuchs avatar Oct 17 '22 09:10 BernhardFuchs

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

ankurdotb avatar Oct 18 '22 12:10 ankurdotb

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 avatar Oct 21 '22 04:10 ankurdotb

Issue is fixed now and ready for review, btw

ankurdotb avatar Oct 21 '22 04:10 ankurdotb

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.

BernhardFuchs avatar Oct 21 '22 10:10 BernhardFuchs

@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?

ankurdotb avatar Oct 21 '22 14:10 ankurdotb