Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migrate to new dnpm:dip node #251

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

feat: migrate to new dnpm:dip node #251

wants to merge 4 commits into from

Conversation

Threated
Copy link
Member

@Threated Threated commented Nov 14, 2024

The files under ccp/ are only copies of the files under minimal/.

I removed the /api path from traefik as it was conflicting with dnpms backend path.
I have never seen anyone of us actually use the traefik frontend but I think /dashboard is a better name anyway but if we want to keep it that way we might be able to do a bit of traefik path rewriting to make it work without this change.

Maybe we can even backport this to the dnpm-extra-broker branch?

@patrickskowronekdkfz
Copy link
Contributor

I removed the /api path from traefik as it was conflicting with dnpms backend path. I have never seen anyone of us actually use the traefik frontend but I think /dashboard is a better name anyway but if we want to keep it that way we might be able to do a bit of traefik path rewriting to make it work without this change.

This is an old relic, I used it to debug traffic ages ago. I think we should have removed that long ago.

volumes:
- /etc/bridgehead/dnpm:/bwhc_config:ro
- ${DNPM_DATA_DIR}:/bwhc_data
- dnpm-authup:/usr/src/app/writable
Copy link
Member

Choose a reason for hiding this comment

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

Can we mount that to a folder instead of a volume? Docker volumes are always forgotten

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could mount it to where it was before which is this DNPM_DATA_DIR path that was usually in /etc/bridgehead/dnpm somewhere but I am not a huge fan of that location. Not sure what data is actually stored there if its patient data I would not necessarily store it under /tmp/bridgehead or /var/cache/bridgehead

Copy link
Member

Choose a reason for hiding this comment

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

/var/cache/ is not persistant enough for this kind of data, as it can't be reconstructed in case of a cache clearing. Obviously, /etc/bridgehead/... is a bad solution that was only used to use an already established path. I would propose to create a /var/bridgehead/ directory @lablans @torbrenner ?

MYSQL_ROOT_HOST: "%"
MYSQL_ROOT_PASSWORD: ${DNPM_MYSQL_ROOT_PASSWORD}
volumes:
- dnpm-mysql:/var/lib/mysql
Copy link
Member

Choose a reason for hiding this comment

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

Can we mount that to a folder instead of a volume? Docker volumes are always forgotten

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean I tend to say that random docker volume paths are also often forgotten but yeah lets keep it in the spirit of the bridgehead and map it somewhere. /var/cache/bridgehead/dnpm/mysql?

Comment on lines +49 to +52
- NUXT_API_URL=http://dnpm-backend:9000/
- NUXT_PUBLIC_API_URL=https://${HOST}/api/
- NUXT_AUTHUP_URL=http://dnpm-authup:3000/
- NUXT_PUBLIC_AUTHUP_URL=https://${HOST}/auth/
Copy link
Member

Choose a reason for hiding this comment

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

If I see that correcly, there is still no way to expose the dnpm portal under a non-root path? Currently, it collides with the bridgehead landing page why I included all that override stuff in the dnpm-setup.sh logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I guess we might be able to make it work under a different path with some traefik magic.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced, as we would need to adapt all the links and calls of the frontend as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

But as you can see here the frontend actually lets you configure the public api url and the public auth url and for the calls to the frontend itself we could redirect those with traefiks StripPrefix middlewares I think. But of course we dont have to I just think it would be possible. If we want to actually do it I can try to figure something out

ccp/modules/dnpm-node-compose.yml Outdated Show resolved Hide resolved
ccp/modules/dnpm-node-compose.yml Outdated Show resolved Hide resolved
- AUTHUP_URL=robot://system:${DNPM_AUTHUP_SECRET}@http://dnpm-authup:3000
volumes:
- /etc/bridgehead/dnpm/config:/dnpm_config
- dnpm-backend-data:/dnpm_data
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have persistance as well? Again, maybe bind to a directory

Copy link
Member Author

@Threated Threated Nov 15, 2024

Choose a reason for hiding this comment

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

Yeah I was also surprised by all the docker volumes. I just copied them as is to make it work quickly. This is the dnpm-backend tho which does contain patient data so we might want to have it in a docker volume as with blaze

Comment on lines +76 to +79
- "traefik.enable=true"
- "traefik.http.routers.dnpm-backend.rule=PathPrefix(`/api`)"
- "traefik.http.services.dnpm-backend.loadbalancer.server.port=9000"
- "traefik.http.routers.dnpm-backend.tls=true"
Copy link
Member

Choose a reason for hiding this comment

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

I assume whe have to expose the ETL endpoints as well. I forwarded you the configuration on the 5th of november.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm looking into the mail it seems they made it work with the old bwhc backend. There the endpoint was under /bwhc which now seems to be /api in most cases so they might be able to ask for <BK_URL>/api/etl which would work with this traefik rule?

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping to clamp the ETL-API acces more down by enforcing at least a basic auth. But I have to study the DNPM:DIP API description to give an answer.

ccp/modules/dnpm-node-compose.yml Show resolved Hide resolved
minimal/modules/dnpm-node-compose.yml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants