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

Switch to Centos 8 Stream base image #268

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

elfosardo
Copy link
Member

@elfosardo elfosardo commented May 25, 2021

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 25, 2021
@elfosardo
Copy link
Member Author

/test-integration

Copy link
Member

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot
Copy link
Contributor

@fmuyassarov: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dtantsur
Copy link
Member

/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021
@elfosardo
Copy link
Member Author

related to metal3-io/metal3-dev-env#671

@namnx228
Copy link
Member

/test-integration
/test-centos-integration

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, namnx228

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@furkatgofurov7
Copy link
Member

That is weird, both tests have run exactly for 2h52min, whereas timeout set for integration tests is 1h30min. Re-triggering to see what went wrong.

/test-integration
/test-centos-integration

@elfosardo
Copy link
Member Author

/test-centos-integration

@elfosardo
Copy link
Member Author

giving it another try, locally seems working
/test-centos-integration

@elfosardo
Copy link
Member Author

elfosardo commented Jun 21, 2021

it seems we're moving back to centos 8.3 for the time being metal3-io/metal3-dev-env#671
/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2021
@furkatgofurov7
Copy link
Member

@elfosardo I think we can move forward with this change, since we took back CentOS Stream again into use in metal3-dev-env?
metal3-dev-env#707

@elfosardo
Copy link
Member Author

/unhold

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021
@elfosardo
Copy link
Member Author

/test-centos-integration

1 similar comment
@elfosardo
Copy link
Member Author

/test-centos-integration

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

@elfosardo hi! This seems to be failing even though we switched to Stream in m3-dev-env, also a question, should we update vbmc Dockerfile too?

@elfosardo
Copy link
Member Author

@elfosardo hi! This seems to be failing even though we switched to Stream in m3-dev-env, also a question, should we update vbmc Dockerfile too?

I can't reproduce the failure locally :/
yes, we should change the vbmc dockerfile too, I thought to do that in a different patch though

@elfosardo
Copy link
Member Author

the issue seems related to the connection to mariadb
this is from the ironic-api logs
[Wed Jul 07 10:44:53.011685 2021] [wsgi:error] [pid 112:tid 140611176912640] [remote 172.17.0.16:55960] 2021-07-07 10:44:53.009 112 WARNING oslo_db.sqlalchemy.engines [req-aa2c23c9-eee8-42bc-ac2a-42a7b3fbf51a - - - - -] SQL connection failed. 10 attempts left.: oslo_db.exception.DBConnectionError: (pymysql.err.OperationalError) (2003, "Can't connect to MySQL server on '127.0.0.1' ([SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:897))")\x1b[00m

@elfosardo
Copy link
Member Author

elfosardo commented Jul 8, 2021

seems like we're using different versions of oslo-db between ubuntu and centos integration tests
in the centos integration test we use a more recent one (9.1.0) which probably introduces the issue with the db connection

@elfosardo
Copy link
Member Author

/test-integration

1 similar comment
@elfosardo
Copy link
Member Author

/test-integration

@elfosardo
Copy link
Member Author

/test-centos-integration

@elfosardo
Copy link
Member Author

/test-integration

@namnx228
Copy link
Member

/test-centos-integration

@elfosardo
Copy link
Member Author

After some troubleshooting (thanks @fmuyassarov for setting up the environment) it looks like the issue is related to the certificate used to connect to mariadb.
When connecting to mariadb we use ssl, but the cert used to connect, which is /certs/ca/mariadb/tls.crt, doesn't have any Common Name specified.
The Issuer and the Subject fields are empty.
Not sure where we get those certs from.

[root@minikube mariadb]# openssl x509 -in tls.crt -noout -text Certificate: Data: Version: 3 (0x2) Serial Number: 3f:78:00:f4:43:83:85:70:83:00:74:42:fb:21:11:13 Signature Algorithm: sha256WithRSAEncryption Issuer: Validity Not Before: Aug 5 15:10:25 2021 GMT Not After : Nov 3 15:10:25 2021 GMT Subject: Subject Public Key Info:

Even trying with the normal mysql client we get the same error:
[root@minikube mariadb]# mysql -uironic -pchangeme -hlocalhost --ssl_ca=/certs/ca/mariadb/tls.crt ERROR 2026 (HY000): SSL connection error: self signed certificate

Although the certs generated in metal3-dev-env look fine:
[centos@feruz-dev-ci certs]$ pwd /opt/metal3-dev-env/certs [centos@feruz-dev-ci certs]$ openssl x509 -in mariadb.crt -noout -text Certificate: Data: Version: 3 (0x2) Serial Number: 6a:aa:75:25:bf:32:cd:5f:cd:51:74:da:21:b4:5d:78:7a:8a:ff:63 Signature Algorithm: sha256WithRSAEncryption Issuer: CN = ironic CA Validity Not Before: Aug 4 14:02:01 2021 GMT Not After : Nov 7 14:02:01 2023 GMT Subject: CN = mariaDB

@elfosardo
Copy link
Member Author

/test-integration

@namnx228
Copy link
Member

@elfosardo /certs/ca/mariadb/tls.crt is a self-signed certificate, and it is used as a CA certificate.
The other cert that was generated in @fmuyassarov dev-env is the certificate issued by the CA, and it should looks different from the CA certificate. So, mariadb uses the second cert for the TLS handshake, and ironic or other clients use the first cert to verify if they are talking to the right mariadb. In my opinion, the self-signed certificate should be fine without CN as long as the client can trust and use its public key for verification.

@elfosardo
Copy link
Member Author

@namnx228 it doesn't look like that cert is used as CA according to mariadb configuration:
--ssl=on --ssl_cert=/certs/mariadb/tls.crt --ssl_key=/certs/mariadb/tls.key
the correct option for the CA cert should be ssl_ca

@namnx228
Copy link
Member

namnx228 commented Aug 12, 2021

@elfosardo The one used for --ssl_cert is /certs/mariadb/tls.crt (without ca) while the self-signed one is /certs/ca/mariadb/tls.crt (with ca). The one without ca should have CN.

@elfosardo
Copy link
Member Author

@elfosardo The one used for --ssl_cert is /certs/mariadb/tls.crt (without ca) while the self-signed one is /certs/ca/mariadb/tls.crt (with ca). The one without ca should have CN.

@namnx228 what I mean is that the CA cert is not defined anywhere in mariadb, or at least I can't see it
the problem here is that ironic is using /certs/ca/mariadb/tls.crt to try to establish an ssl connection to mariadb and it's failing because the cert is self-signed and because it doesn't have any Common Name
the ERROR 2026 (HY000): SSL connection error: self signed certificate message looks clear enough

@namnx228
Copy link
Member

@elfosardo Yes, I agree that the culprit here is that there is a issue with the CA certificate of Mariadb. I just wonder why we didn't see it in Centos before.
Actually, I also saw a problem with TLS CA certificate before in the CI when we switch from Centos to Centos stream in metal3-dev-env, and the solution was to disable SELinux which is enforced by default. Maybe it can also solve this problem?

@namnx228
Copy link
Member

CommonName has been added to Ironic and Mariadb certs in this PR: metal3-io/baremetal-operator#951
Let's see if it can solve the issue of this PR.
/test-integration
/test-centos-integration

@namnx228
Copy link
Member

/test-integration
/test-centos-integration

@namnx228
Copy link
Member

/test-integration

3 similar comments
@fmuyassarov
Copy link
Member

/test-integration

@fmuyassarov
Copy link
Member

/test-integration

@elfosardo
Copy link
Member Author

/test-integration

@elfosardo
Copy link
Member Author

/test-centos-integration

@elfosardo
Copy link
Member Author

@namnx228 thanks for taking care of that! Let's see if it fixes the issue
I haven't checked but it could be due to an upgrade in mariadb packaged version in centos stream

@metal3-io-bot metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 18, 2021
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 18, 2021
@elfosardo
Copy link
Member Author

/test-integration

@elfosardo
Copy link
Member Author

/test-centos-integration

@namnx228
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2021
@metal3-io-bot metal3-io-bot merged commit abc12be into metal3-io:master Aug 18, 2021
elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Apr 22, 2022
[OCP 4.11 only] Backport the kernel params patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants