Add test case for SQLInstance SSL configuration #2653
Conversation
| ipConfiguration: | ||
| sslMode: ENCRYPTED_ONLY | ||
| # When `sslMode=ENCRYPTED_ONLY`, the legacy `requireSsl` flag must be false or cleared. | ||
| requireSsl: false |
There was a problem hiding this comment.
What happens if
ssLmode: ENCRYPTED_ONLYandrequireSsl: true, will the SQL server returns a reasonable error?ssLmode: ENCRYPTED_ONLYandrequireSslunset (not configured inspec), will KCC or the SQL server returns any message? I think we shall allow this case, that better aligns with the "cleared" concept and shall be covered by this test.
There was a problem hiding this comment.
What happens if
ssLmode: ENCRYPTED_ONLYandrequireSsl: true, will the SQL server returns a reasonable error?
Yeah, the server returns this error:
"""
failed to create instance sqlinstance-ssl-test: googleapi: Error 400: Invalid request: For a Postgres instance, sslMode value ENCRYPTED_ONLY and requireSsl value true are conflicting. When sslMode=ENCRYPTED_ONLY, requireSsl must be false. When requireSsl=true, sslMode must be TRUSTED_CLIENT_CERTIFICATE_REQUIRED. It's recommended that you only set sslMode.
"""
ssLmode: ENCRYPTED_ONLYandrequireSslunset (not configured inspec), will KCC or the SQL server returns any message? I think we shall allow this case, that better aligns with the "cleared" concept and shall be covered by this test.
We do allow this case; it works as expected. The limitation is from the GCP API, not from direct controller / KCC validation. The reason I added this test case (with legacy field requireSsl: false) was to highlight this potential mistake a user could make. The API quirks are documented here: https://cloud.google.com/sql/docs/mysql/admin-api/rest/v1/instances#ipconfiguration
Also just updated the test case to remove sslMode from create.yaml, and remove requireSsl from update.yaml.
There was a problem hiding this comment.
Cool.
For unset, I think what we want to verify is that create.yaml has requireSsl: true and ssLmode: <valid value> , the then update.yaml unset the requireSsl, this should pass because the update "unset" the requireSsl rather than "unchange" it. Does it make sense?
jasonvigil
force-pushed
the
sqlinstance-ssl
branch
from
5effb5f to
0fe0ced
Compare
jasonvigil
force-pushed
the
sqlinstance-ssl
branch
from
0fe0ced to
2110857
Compare
jasonvigil
force-pushed
the
sqlinstance-ssl
branch
from
2110857 to
ef9eb2a
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
google-oss-prow
bot
merged commit 8266a1c
into
GoogleCloudPlatform:master
No description provided.