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

[ADD] Feature to expose database #479

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Conversation

josep-tecnativa
Copy link
Contributor

@josep-tecnativa josep-tecnativa commented Jul 3, 2024

Since Traefik 3 is avaliable on Doodba Copier Template, we can configure with our template, a postgres exposed database. This is only working on Traefik 3.

  • Correct when serveral domains, HostSNI duplicated map key traefik label.

@pedrobaeza
Copy link
Member

pedrobaeza commented Jul 3, 2024

Can you check CI?

prod.yaml.jinja Outdated Show resolved Hide resolved
Copy link
Contributor

@celm1990 celm1990 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for improving this feature

@josep-tecnativa
Copy link
Contributor Author

ping @pedrobaeza

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

As this doesn't work in Traefik 2-, you can put for them the port direct mapping instead of limiting the question to Traefik 3

prod.yaml.jinja Outdated Show resolved Hide resolved
@josep-tecnativa josep-tecnativa force-pushed the add-external-db-access-feature branch 4 times, most recently from 380708d to f25fc7b Compare July 26, 2024 11:19
@pedrobaeza
Copy link
Member

It seems OK in code review, but you should try to generate a scaffolding on each type to be sure, or add them to tests.

@josep-tecnativa josep-tecnativa force-pushed the add-external-db-access-feature branch 2 times, most recently from 9173d19 to d962bbb Compare August 6, 2024 05:52
Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Sorry, I don't mean to sound rude, but I have the feeling all this is wrong... 😅

  • TCP routers don't have a concept of "path".
  • Are you sure TLS termination can be done in Traefik for a TCP router?
  • Are you seriously exposing the database always, by default and with no opt-out option?

@josep-tecnativa
Copy link
Contributor Author

Sorry, I don't mean to sound rude, but I have the feeling all this is wrong... 😅

* TCP routers don't have a concept of "path".

* Are you sure TLS termination _can_ be done in Traefik for a TCP router?

* Are you seriously exposing the database _always, by default and with no opt-out option_?

First of all, thanks for pointing out that TCP routers don't have the concept of a path. I have already corrected that.

Secondly, I would say that Traefik 3 can handle TLS termination for TCP routers. This functionality is documented in the official Traefik documentation here. But please correct me if I'm wrong, although I believe it is correct.

Lastly, no, by default I do not expose the database. It is only exposed if the option to expose it is chosen, as you can see here:

{%- if postgres_exposed %}

Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Thanks for attending the previous comment! Yes, it seems I was wrong in those last 2 points.

I see this also would need some changes on the traefik deployment, so you probably need to update the docs, as now there must exist a postgres-entrypoint.

Thank you

_traefik3_paths_labels.yml.jinja Outdated Show resolved Hide resolved
@josep-tecnativa
Copy link
Contributor Author

josep-tecnativa commented Aug 7, 2024

This is ready to merge @pedrobaeza . I have generated a scaffolding on each type and works correctly.

@pedrobaeza
Copy link
Member

@yajo is it OK for you?

Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

One minor comment.

One bigger concern is that there are no tests for this. I think it deserves one.

_traefik3_paths_labels.yml.jinja Show resolved Hide resolved
prod.yaml.jinja Show resolved Hide resolved
prod.yaml.jinja Show resolved Hide resolved
@josep-tecnativa josep-tecnativa force-pushed the add-external-db-access-feature branch 3 times, most recently from 21f9e1f to 222ded6 Compare October 4, 2024 08:23
@josep-tecnativa
Copy link
Contributor Author

@ap-wtioit Do you have any idea why I can't connect to the database in the new test?

https://github.com/Tecnativa/doodba-copier-template/actions/runs/11326688958/job/31496138294#step:16:279

@ap-wtioit
Copy link
Contributor

@josep-tecnativa i'm not 100% sure. One issue we had on our systems was this one: moby/moby#47145 where starting with docker 25 it was no longer possible to access container ports directly from the host for internal networks.

Another issue could be test tested postgres image. If the postgres stopped after the test started it there could also be a connection issue

To be sure i would add the following debugging info to the failed test:

  • docker compose ps info (showing containers running/stopped and open ports)
  • docker inspect ... | grep IPAddress showing the configured IPs per container
  • ip route showing package routing info (did docker add the route to the internal network / ip)

@josep-tecnativa josep-tecnativa force-pushed the add-external-db-access-feature branch 20 times, most recently from 54c1956 to 7f53322 Compare October 16, 2024 09:29
@josep-tecnativa
Copy link
Contributor Author

Since Docker does not allow connections through ports on the same host starting from version 25, it is currently not possible to create a test. The functionality of this PR has been tested by both @celm1990 and me. Therefore, I believe it is ready to be merged (once CI finishes), and if in the future I find a way to create the test, I will implement it.

ping @pedrobaeza

@pedrobaeza pedrobaeza merged commit f3df4ad into main Oct 17, 2024
87 checks passed
@pedrobaeza pedrobaeza deleted the add-external-db-access-feature branch October 17, 2024 07:34
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.

5 participants