Move co-located containers onto shared foreman-core-network - #728
Move co-located containers onto shared foreman-core-network#728Alleny244 wants to merge 1 commit into
Conversation
c6c2b37 to
de1fe85
Compare
6bf8eb0 to
a7edb41
Compare
| ansible.builtin.include_role: | ||
| name: iop_kafka | ||
|
|
||
| # Kafka is the first container on iop-core-network, so the gateway IP |
There was a problem hiding this comment.
This, as you are discovering, is tricky and the same issue I ran into when I tried to do this before. This creates a rather fragile experience.
|
Here's two other ideas:
The
|
I’ll go with PostgreSQL on both networks here. Putting all containers on the same network would require broader changes, so a separate PR would be right? |
3b4d9d7 to
6f86341
Compare
Yes and no, you are ultimately trying to solve this broader problem of running only what's needed on the public interface. I'd rather us solve the architecture question than patch it and change it again right after. I was reading on how |
Yeah, makes sense. I’ll look into the single network approach and see how we can handle the startup ordering. |
Should I reuse the existing iop-core-network or create a new general purpose network ( if naming conventions are being looked into) for the containers |
New general purpose network name that is more descriptive. |
| foreman_development_database_host: "localhost" | ||
| # Prefer 127.0.0.1 over localhost: Postgres is published on IPv4 only, and | ||
| # "localhost" often tries ::1 first (connection refused). | ||
| foreman_development_database_host: "127.0.0.1" |
There was a problem hiding this comment.
The commit title says:
Restrict PostgreSQL listen_addresses to localhost and IOP gateway
But I don't see any change in the listen_addresses. Please update the commit message & PR title/description to reflect the current state of the PR.
There was a problem hiding this comment.
Yeah, sure. I’ll update it. Also, the current implementation was based on putting PostgreSQL on both networks. Based on the discussion above, we decided to move forward with creating a new network and placing all the containers on it instead. I’ve been working on that implementation and will push the changes shortly.
| # Optional parameters - can use defaults. | ||
| # Use 127.0.0.1: Postgres is published on IPv4 only (not ::1), and TCP | ||
| # requires a password now that it is no longer on host networking. | ||
| iop_fdw_database_host: "127.0.0.1" |
There was a problem hiding this comment.
The iop.md architecture diagram (mermaid) still draws Postgres under "Host System", outside the iop-core-network subgraph, while the prose now says Postgres is reached via postgresql:5432 on that network.
Should we update that as well?
There was a problem hiding this comment.
Yeah, it's updated now
There was a problem hiding this comment.
Why do some places use 127.0.0.1 and others use postgresql ?
| name: postgres_fdw | ||
| login_db: "{{ iop_fdw_database_name }}" | ||
| login_user: postgres | ||
| login_password: "{{ iop_fdw_login_password }}" |
There was a problem hiding this comment.
This will log the password value in the logs, especially when using -vvv. Should we use no_log: true or something similar (secrets?) to mask the value of the password?
There was a problem hiding this comment.
Yes, added no_log property here
|
|
||
| iop_database_host: host.containers.internal | ||
| # Co-located Postgres joins iop-core-network; IOP reaches it by container name. | ||
| iop_database_host: postgresql |
There was a problem hiding this comment.
Suggestion from AI:
Pre-existing (not introduced by this PR) architectural gap:
iop_database_hostinsrc/vars/database.ymlis unconditionally set topostgresql(a container name only resolvable on iop-core-network), with no branching on database_mode. Combining --database-mode external with the iop feature was already questionable before this PR (nothing in features.yaml or obsah metadata blocks that combination), and this PR doesn't change that — just flagging it's still an open gap, not something to necessarily fix in this PR.
| password: "{{ iop_vulnerability_database_password }}" | ||
| feature: iop | ||
|
|
||
|
|
6f86341 to
d65b220
Compare
5c5760a to
456b6e3
Compare
@ehelms @ekohl we need a decision on which way (PR) we want to go. |
| - name: Issue server certificate | ||
| when: | ||
| - (certificates_source != 'custom_server') or (certificates_hostname == 'localhost') | ||
| # localhost/candlepin are always issued (service-name TLS on the shared network). |
There was a problem hiding this comment.
I don't understand why these changes are necessary for this PR. Can you please explain?
There was a problem hiding this comment.
Since the services are now on the same network, they communicate using the container names instead of localhost. So we need to add those names to the certificate SANs as well, else TLS hostname verification will fail, right?
There was a problem hiding this comment.
Why does this role need to know this though? Just like localhost, can candlepin not be specified in the list of hosts in the playbook to generate certificates for?
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh yes, it's updated now
|
I'd like to avoid introducing additional networks at this point. It's a major architectural change and currently I'm much more interested in actually having feature parity with our existing installation. That will allow us to deprecate the package-based installations, which in turn unlocks dropping Ruby 3.1 support, which in turn allows upgrading Ruby on Rails to at least Rails 7.2. That is needed to get CVE fixes. At this point in the release cycle even this change is IMHO very risky. If we're going to introduce networks, I think we should do it properly and well thought out. IMHO neither this nor #403 do what I would expect: have 1 network per service. When I say service, I mean:
But then there are more complexities, like do you run 1 instance of Valkey for both Foreman and Pulp or isolate them from each other? PostgreSQL is in a similar boat. |
|
On "1 network per service" — I think the tricky part is exactly the Valkey/Postgres question you raised. If Foreman, Candlepin and Pulp all need the same Postgres, there are really only two ways to handle it: hook Postgres up to all three networks, or give each service its own Postgres. The multi-homing option doesn't end up buying much isolation though — Postgres is reachable from all three networks either way, so "who can reach the DB" looks basically the same as just having one Separate Postgres/Valkey per service would give real isolation, but that feels like a much bigger change than what we're actually trying to fix (containers listening on That's the thinking behind going with networks per workflow rather than per named service: That proxy split feels like the boundary that matters most. The rest ends up as one network because those services are already tightly coupled, and splitting them further wouldn't really stop anything from reaching anything — just add more bridges for the same result. |
#611 would disagree with you. |
|
I'll give both PRs a re-read, but the thing I am missing is why or what value individual networks per service gives us. Networks aren't for free, they will require our users to ensure that address space is available and the more introduce the more we potentially clash with any existing configuration they might have. We should be thoughtful about this trade-off when considering what additional networks bring us in value given most of our services run locally. The biggest reason I pushed for a single, custom podman network is to allow easy container-to-container DNS name resolution as this is not available on the default podman network. |
My initial thought was that you would have a good isolation. However, given all the cross-service talk it's questionable how much benefit we would actually get.
If you make the network IPv6-only then the address space is less of a concern. Also note that with systemd socket activation for Foreman and Pulp they don't have any ingress, only egress. But sure, it's also additional complexity that can break. |
| iop_advisor_database_user: advisor_user | ||
| iop_advisor_database_password: "{{ undef(hint='Set a secure database password') }}" | ||
| iop_advisor_database_host: host.containers.internal | ||
| iop_advisor_database_host: "{{ iop_database_host | default('postgresql') }}" |
There was a problem hiding this comment.
| iop_advisor_database_host: "{{ iop_database_host | default('postgresql') }}" | |
| iop_advisor_database_host: postgresql |
I think you get the pattern by now. While I get the temptation to use a variable, the whole point of defaults is to default to something that is meaningful. And not to be reliant on some variable to exist somewhere external.
There was a problem hiding this comment.
yeah agreed, have updated all to use default postgresql
| postgresql_networks: | ||
| - "{{ foreman_core_network_name | default('foreman-core-network') }}" | ||
| postgresql_publish: | ||
| - "127.0.0.1:5432:5432" |
There was a problem hiding this comment.
Does this need to be published if everything is on the network that needs it?
There was a problem hiding this comment.
For host side consumers, they still require port publication right?
There was a problem hiding this comment.
What host side consumers are you expecting to exist?
There was a problem hiding this comment.
Mainly for deploy-dev workflow which runs forman on the host itself
There was a problem hiding this comment.
Just a doubt here, please correct me if I’m wrong, since the Ansible task community.postgresql.postgresql_user uses login_host: 127.0.0.1, doesn't PostgreSQL need to have its port published on the host for this task to work?
There was a problem hiding this comment.
Fair point, I suppose that is the best we can do right now.
|
This raises the question of:
These are both services that are only internally communicated with by other services on the network aka the foreman container. We currently do not have TLS between any of the other services (e.g. iop-*, postgresql, redis). |
TLS here is mainly because Candlepin is currently configured to use HTTPS, and the IOP gateway is used as a Smart Proxy, which Foreman expects to access over HTTPS right?. It’s not required by the shared network itself |
Fair point on the IOP gateway. The dedicated network does offer some security enhancements. |
yeah maybe we could leave TLS posure as a follow up? |
0a1546e to
0e55232
Compare
|
Can you introduce a new document that lays out the architecture of the network? Take a look at #771 as I'd like it to go into that folder structure. |
Sure I ll take a look into that and formulate one |
0e55232 to
4c367a1
Compare
|
One thing that I noticed was , TLS issue affects both Candlepin and IOP: their certs are issued for candlepin / iop-core-gateway, but deploy-dev Foreman connects via localhost. Currently we are using those hostnames instead of localhost so hostname verification succeeds for production. For the host Foreman, I guess we’d also need to add both names to /etc/hosts pointing to 127.0.0.1 for TLS |
4c367a1 to
ea97817
Compare
Problem Statement
Embedded PostgreSQL defaults to
listen_addresses = '*', exposing port5432on public interfaces when containers use host networking.https://redhat.atlassian.net/browse/SAT-48503
Changes
foreman-core-network(10.130.0.0/24) via theforeman_core_networkrole, created early in deploy.postgresql,valkey,candlepin,iop-core-gateway).127.0.0.1:5432,127.0.0.1:6379, Candlepin/IOP gateway ports) so ports are not exposed on0.0.0.0/ public interfaces.iop_network/iop-core-network; IOP usesforeman-core-network.foreman_proxyon host networking (edge/DHCP/DNS/TFTP/REX).0.0.0.0:5432/[::]:5432(and equivalent for Valkey).Tests
Checklist
docs/iop.md)