Conversation
|
This looks related to #141 but is not an aspect I have incorporated. I was wanting to get in a version of remote database testing and then harden it with more of the options. |
|
It is, @Gauravtalreja1 ran into this when testing ext db stuff |
evgeni
force-pushed
the
ssldb
branch
5 times, most recently
from
f8a1e26 to
1ba7910
Compare
evgeni
force-pushed
the
ssldb
branch
4 times, most recently
from
921e621 to
7355577
Compare
| containers.podman.podman_secret: | ||
| state: present | ||
| name: candlepin-db-ca | ||
| data: "{{ lookup('ansible.builtin.file', candlepin_database_ssl_ca) if candlepin_database_ssl_ca else 'empty' }}" |
There was a problem hiding this comment.
This is creating an empty secret if there is no database SSL cert? Why not use a when conditional on the sercret?
There was a problem hiding this comment.
Because then I need to also conditionally mount it, and that's painful ;)
There was a problem hiding this comment.
I get that, I worry about this being a red herring while debugging.
There was a problem hiding this comment.
what kind of red herring? the file being present?
There was a problem hiding this comment.
Correct. The file / secret being present but empty raises the "should it be empty? or is it accidentally empty?"
There was a problem hiding this comment.
That's why the string is "empty", or should I do "this secret was intentionally left blank"?
|
What else do you think is needed to take it out of draft? |
|
I wanted to write up some tests to validate it. |
evgeni
force-pushed
the
ssldb
branch
2 times, most recently
from
46aeafa to
3a61d77
Compare
.github/workflows/test.yml
Outdated
| - certificate_source: default | ||
| security: none | ||
| database: external | ||
| - certificate_source: default | ||
| security: none | ||
| database: externalssl |
There was a problem hiding this comment.
I personally think that users should never run external db without SSL, but I know that today we document and support that, so I added this as another matrix entry instead of repurposing the external one. But do we really need to test both?
There was a problem hiding this comment.
If you are proposing that we test external database only with TLS - I agree.
There was a problem hiding this comment.
Correct, that's what I wanted to say
There was a problem hiding this comment.
Do we intend to drop support for external DB without SSL altogether? If not, we should make sure both cases are covered by tests
There was a problem hiding this comment.
I see little chance that non-SSL ext-DB breaks while SSL-enabled continues to work.
and you can still verify that in robotello for formal support, I just don't think it needs to happen here
There was a problem hiding this comment.
The internal database scenario should effectively test the non-ssl scenario for us since it's doing the same style connection and handling.
There was a problem hiding this comment.
yeah.
I've now pushed a separate commit, flipping the setup to "only ssl" -- we can still drop it if Gaurav strongly disagrees :)
evgeni
force-pushed
the
ssldb
branch
7 times, most recently
from
481ca09 to
d157f1a
Compare
evgeni
force-pushed
the
ssldb
branch
2 times, most recently
from
8953b69 to
a0572e5
Compare
|
@ehelms look, no draft! |
.github/workflows/test.yml
Outdated
| if: matrix.database != 'internal' | ||
| run: | | ||
| ./forge remote-database | ||
| ./forge remote-database ${{ matrix.database == 'externalssl' && '--database-ssl true' || ''}} |
There was a problem hiding this comment.
Does that mean we introducing a new --database-mode here for externalssl scenario?
There was a problem hiding this comment.
no, this just instructs the forge tool (which we don't ship to users, it's CI/dev only) to deploy a DB with SSL
There was a problem hiding this comment.
same here, externalssl is gone now
| database_ssl: | ||
| type: Boolean |
There was a problem hiding this comment.
Do we really need a new variable here? Could we use the existing --database-ssl-ca, given that it would be only defined for the external SSL scenario, right?
There was a problem hiding this comment.
for forge we don't have --database-ssl-ca as it will generate an own CA for the DB
There was a problem hiding this comment.
now that we only test ssl on the external db, we could drop this new variable again
3 participants
No description provided.