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

Pend invalid brats test (external db tls) #2567

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Conversation

aramprice
Copy link
Member

These tests have been invalid since their creation. From the beginning they have passed certificate material in the director deployment manifest, however the director itself has (always?) expected to be passed paths to the certificates.

Original tests here:
f97c556#diff-ab90421945d114536b0c024f3cbaee7f4566c79a592ba9d46b395f0771745b42R293-R306

and

Director config here:

certificate_paths = tls_options.fetch('cert')
db_ca_path = certificate_paths.fetch('ca')
db_client_cert_path = certificate_paths.fetch('certificate')
db_client_private_key_path = certificate_paths.fetch('private_key')
db_ca_provided = tls_options.fetch('bosh_internal').fetch('ca_provided')
mutual_tls_enabled = tls_options.fetch('bosh_internal').fetch('mutual_tls_enabled')
case connection_config['adapter']
when 'mysql2'
# http://sequel.jeremyevans.net/rdoc/files/doc/opening_databases_rdoc.html#label-mysql+
connection_config['ssl_mode'] = tls_options.fetch('skip_host_verify', false) ? 'verify_ca' : 'verify_identity'
connection_config['sslverify'] = true
connection_config['sslca'] = db_ca_path if db_ca_provided
connection_config['sslcert'] = db_client_cert_path if mutual_tls_enabled
connection_config['sslkey'] = db_client_private_key_path if mutual_tls_enabled

@aramprice
Copy link
Member Author

@selzoc
Copy link
Member

selzoc commented Sep 23, 2024

This branch seems to have a lot more than just pending the brats test - is that intentional?

@aramprice
Copy link
Member Author

aramprice commented Sep 23, 2024

Yeah, I suppose that could be split out

The first set of commits were exploratory for understanding the problem (and adding some facilities for debugging this sort of thing in the future).

I am/was hesitant to simply pend the specs without supporting info/evidence.

That being said there are 3-4 commits which are purely cleanup that could likely be pushed separately.

¯_(ツ)_/¯

I collapsed some of the Golang fixups for clarity.

@aramprice aramprice force-pushed the pend-invalid-brats branch 4 times, most recently from 55c031b to 0bec3a0 Compare September 23, 2024 20:26
Previously the brats tests were running against MySQL 5.7.

Per the mysql2 docs the `sslverify` option is not supported as of MySQL
8.0 so this option is removed from the DB configuration.
This can be helpful for debugging / manually runnings specs.
- use camelCase
- rename variable for clarity (golang)
- remove unnecessary conversion (golang)
- remove unused var (golang)
- extract `var`'s rather than defining in each context (golang)
- brats spec contexts use consistent order
- brats utils uses common SQL connection logic
- remove whitespace from gcp_mysql.yml
This matches BOSH's expectations that the values be file names and not
the file contents in the deployment manifest.
These tests have been invalid since their creation. From the beginning
they have passed certificate material in the director deployment
manifest, however the director itself has (always?) expected to be passed
_paths_ to the certificates.

Original tests here:
f97c556#diff-ab90421945d114536b0c024f3cbaee7f4566c79a592ba9d46b395f0771745b42R293-R306

and

Director config here:
https://github.com/cloudfoundry/bosh/blob/c095b06c91f14ef51b5a8bcea20c3ec3651590b8/src/bosh-director/lib/bosh/director/config.rb#L303-L318
@aramprice aramprice merged commit 010655a into main Sep 24, 2024
24 checks passed
@aramprice aramprice deleted the pend-invalid-brats branch September 24, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants