-
Notifications
You must be signed in to change notification settings - Fork 21
Update google_sql_database_instance config to output connection details #48
Conversation
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
config/sql/config.go
Outdated
if a, ok := certattrs["common_name"].(string); ok { | ||
conn["commonName"] = []byte(a) | ||
} | ||
if a, ok := certattrs["create_time"].(string); ok { | ||
conn["createTime"] = []byte(a) | ||
} | ||
if a, ok := certattrs["expiration_time"].(string); ok { | ||
conn["expirationTime"] = []byte(a) | ||
} | ||
if a, ok := certattrs["sha1_fingerprint"].(string); ok { | ||
conn["sha1Fingerprint"] = []byte(a) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these in connection details secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't need them for my current connection details. However, I did add these to keep them consistent with what exists today in provider-gcp https://github.com/crossplane/provider-gcp/blob/master/pkg/controller/database/cloudsql.go#L218-L221.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, should we use the same keys there in that case? https://github.com/crossplane/provider-gcp/blob/42bb10f07f49419b08af6734b81d91ea0f1b9c4a/apis/database/v1beta1/cloudsql_instance_types.go#L35-L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 I'll update them 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up creating a new const
block in config.go versus importing the package from provider-gcp just to get the static strings (that just seemed heavy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right call 👍
Thanks @tnthornton, looking good, just left couple of minor comments 👍 |
update cert key name to serverCaCert Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Thanks for the review @turkenh !. I've made the requested adjustments and verified we get the expected details in the new secret: data:
commonName: <REDACTED>
connectionName: <REDACTED>
createTime: MjAyMi0wMy0wOVQxNjo1ODozNy4yNTha
expirationTime: MjAzMi0wMy0wNlQxNjo1OTozNy4yNTha
privateIpAddress: ""
publicIpAddress: <REDACTED>
serverCaCert: <REDACTED>
sha1Fingerprint: <REDACTED> |
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Updated keys:
|
Thanks @tnthornton 🙌 |
Description of your changes
Prior to this change, the connection details secret from
google_sql_database_instance
included no exported attributes as seen in #47.For this change the ResourceConfigurator for
google_sql_database_instance
was extended to include transferring details from the exported attributes to the connection details secret that is inline with behavior we see in provider-gcp.Fixes #47
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
kubectl apply -f examples/sql/instance.yaml