aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

Allow setting a baseurl for the swagger urls

Open tbruecke opened this issue 3 years ago • 10 comments

When deploying different agents on same gateway url in service mesh there occures some problem cause static swagger path und swagger.json has no unique uri. This PR enables a baseurl to be set for the swagger urls mentioned via the environment variable ACA_PY_BASE_URL: Until now, the static swagger path and swagger.json path is always the default value set in setup_aiohttp_apispec (static/swagger for 'static_path' and /api/docs/swagger.json for 'url'). By setting the ACA_PY_BASE_URL, the path can now be adjusted so that the urls can be set to{ACA_PY_BASE_URL }/api/docs/swagger.json. The urls can then be easily resolved in the virtual service via unique urls.

tbruecke avatar Jun 22 '22 19:06 tbruecke

Codecov Report

Merging #1837 (0522706) into main (f0b2719) will decrease coverage by 0.00%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #1837      +/-   ##
==========================================
- Coverage   93.72%   93.72%   -0.01%     
==========================================
  Files         536      536              
  Lines       34015    34022       +7     
==========================================
+ Hits        31881    31887       +6     
- Misses       2134     2135       +1     

codecov-commenter avatar Jun 22 '22 21:06 codecov-commenter

The PR got moved (thanks @tbruecke ) -- @jcourt562 -- could you please weigh in on this one?

Thanks!

swcurran avatar Jun 22 '22 21:06 swcurran

I am a little confused by the need for this ? If I run multiple cloud agents on my local docker environment or anywhere else that uses the same base URL, I just change the ACAPY_ADMIN and ACAPY_ENDPOINT ports to be unique for each agent. This allows them to be distinguished. I must be missing something subtle that is trying to be achieved here ?

jcourt562 avatar Jun 22 '22 22:06 jcourt562

@jcourt562 Have you already tried to deploy multiple agents within an Openshift Service Mesh (same gateway url) ? This is the use case where this PR becomes interesting. Let's say the gateway url is https://ssi-entw-gateway-url and the argocd app metadata name is aca-py-xy. This results in the following route with which the aca-py can be accessed: https://ssi-entw-gateway-url/aca-py-xy/*. This as a prerequisite.

This link https://ssi-entw-gateway-url/aca-py-xy/* is required to access the Swagger documentation: https://ssi-entw-gateway-url/aca-py-xy/api/doc. If we now take a closer look at the index.html, we see that the swagger src to the scripts is, as expected, structured like this: https://ssi-entw-gateway-url/static/swagger/* (Without the path /aca-py-xy within the url). If we now deploy another Aca-py, this also has the same url set in the swagger src in the index.html: https://ssi-entw-gateway-url/static/swagger/..... Now the problem arises that we don't have a clear route for this src within the service mesh to assign it to a service within the VirtualService. This is why it is necessary to set a baseurl for swagger. Otherwise this url cannot be resolved individually for each service. Maybe I'm missing a solution. Up until now my problem could only be solved by setting the swagger baseUrl.

tbruecke avatar Jun 23 '22 09:06 tbruecke

I am going to be the first to acknowledge I know nothing much about OpenShift. I have a reasonable understanding of the concept of Virtual Web servers and a pretty good kernel internals understanding of routing and IP. It is pretty fundamental to be able to route to Virtual Servers via a port. A very cursory look at OpenShift suggests this can be done by tieing a Gateway to a Virtual Route, I think it is explained here: ossm-traffic-manage

Now that may not be the simplest solution and if others think changing the code to support this is a better solution then you will need to address a couple of things in your PR :

  • ACA_PY_BASE_URL doesn't conform to the other env variable formats. Shouldn't it be ACAPY_BASE_URL or more explicitly ACAPY_SWAGGER_PREFIX
  • What tests have been written to ensure this works as expected ? I don't see any in the PR.
  • Documentation/help updates to describe the new environment variable

I guess I would prefer to see an external solution to this problem exhausted before adding what looks like a trivial change but would need extra tests written to check it doesn't have an undesired effect.

jcourt562 avatar Jun 23 '22 22:06 jcourt562

@swcurran - you may want someone who has real OpenShift deployment background take a quick look at this. In AWS CDK the loadbalancer configuration takes care of this issue by allowing specification of a port for the application so it's not an issue.

I also went back and refreshed my knowledge on Virtual Web servers. It seems to me that the root cause of your problem is that you would normally be using a different host name for each of your ACA-py instances if you were running them as Virtual Web sites. That means that accessing your example of https://ssi-entw-gateway-url/aca-py-xy/* would usually be done with a different host name that resolves to the same physical web server. So the "ssi-entw-gateway" part would have multiple values that resolve to the same IP address like "ssi-entw-gateway1", "ssi-entw-gateway2". No need for the ACA-py process to know anything about where it is located in the filesystem as the Webserver locates the document root based on the virtual web server name. I am guessing there is something fundamentally different to the way OpenShift works if this isn't the case ?

Again I think someone with OpenShift experience would be better to identify if this change is needed as it should really not be required.

jcourt562 avatar Jun 23 '22 23:06 jcourt562

@WadeBarnes has an awful lot of OpenShift experience. Wade, could you please review the conversation on this PR and weigh in? I know that we have a ton of ACA-Py instances on our OpenShift platform, and this has not been a concern.

Assuming you find a solution -- could you add a note on how this should be managed in OpenShift so that we can add a note to the docs?

Thanks!

swcurran avatar Jun 24 '22 19:06 swcurran

Addendum: I spoke to someone with Openshift knowledge today and he also said that you need a unique url within the service mesh for the corresponding gateway (the host for this would be https://ssi-entw-gateway-url/). The solution via port matching does not help here. This also speaks in favor of setting an additional subdirectory (/aca-py-xy) in server.py, so that unique urls in index.html for swagger would have to be set for the serviceMesh. Apart from this topic, without setting the additional subdirectory (as a prefix in server.py) - if several aca-pys are deployed - when executing the interfaces via the SwaggerUI, no unique path for the service mesh would be set in the curl command, what again leads to the same topic as already described.

tbruecke avatar Jun 30 '22 08:06 tbruecke

I'm working with @tbruecke on this and just wanted to step in and pop this PR up for @WadeBarnes . What are your thoughts on this? Our Cloud Department is convinced, there's no OpenShift configuration available to fix this issue. "This must be solved by the application." Do you have any idea?

pstuermlinger avatar Jul 06 '22 10:07 pstuermlinger

@WadeBarnes -- another one for your backlog. Would definitely like your opinion on this change.

swcurran avatar Jul 18 '22 17:07 swcurran

This PR is a bit old, is it still required?

My 0.02 is that it shouldn't be necessary to configure this in aca-py (you should be able to do the configuration in openshift to provide separate url's for multiple agents) however it wouldn't hurt either ...

ianco avatar Jan 05 '23 21:01 ianco

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarqubecloud[bot] avatar Jan 05 '23 21:01 sonarqubecloud[bot]

First off, sorry for taking so long to get to this. I'm not sure how I missed this after being mentioned a few times.

I too am a little confused as to why this should be needed. The ACA-Py instance should be unaware of it's URL other than to publish the correct endpoint to the ledger in the case it's a public instance.

It would seem to me in the described scenario the service mesh is not doing a proper job of proxying and/or routing the requests to the ACA-Py instances. The URL used to access the ACA-Py instances through the service mesh should be completely transparent to the ACA-Py instance themselves. Perhaps the service mesh is not routing the requests, or is not setting the X-FORWARDED headers appropriately.

I'm also curious as to the use case around deploying ACA-Py instance into a service mesh. Was this a way to achieve a multi-tenant environment? I'm wondering if there is a different way to implement the same use case.

That said I have seen something similar to this when hosting web applications on a sub-path (as opposed to the root path), and it typically only affects the static content as described. In these cases it has been necessary to inform the application of (configure it with) it's expected sub-path. Is the service mesh simply working as a web server serving out applications (ACA-Py instances) on separate sub-paths?

If this is no longer an issue and/or there is a different way to accomplish the desired results I'd say close this PR.

WadeBarnes avatar Aug 08 '23 18:08 WadeBarnes

Given the age and many updates since — closing. If needed, a replacement PR can be submitted.

swcurran avatar Aug 08 '23 19:08 swcurran