feat(runjob): add support for cloud_sql_instance #1688
Conversation
CyberHippo
changed the title
|
/test all |
|
@CyberHippo: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
|
Hi, @acpana / @anhdle-sso / @barney-s / @cheftako / @diviner524 / @gemmahou / @jasonvigil / @jiefenghe / @jingyih / @justinsb / @maqiuyujoyce / @roycaihw / @yuwenma / @ziyue-101 /@xiaoweim sorry for tagging you directly, could you please take a look at my PR. This would greatly improve the usability of |
|
/ok-to-test |
|
@gemmahou: No presubmit jobs available for GoogleCloudPlatform/k8s-config-connector@master In response to this:
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-sigs/prow repository. |
|
/test all |
|
@gemmahou: No presubmit jobs available for GoogleCloudPlatform/k8s-config-connector@master In response to this:
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-sigs/prow repository. |
|
Hi @CyberHippo would you mind collaborating more on your issue #1307? Could you help us understand the business impact and the priority to support this feature? Is there a KCC/Google internal feature request associate to it? Thanks! |
|
Hi @gemmahou , thank you for taking a look!
Sure, let me know what information you need.
We are deploying compute heavy workloads using Cloud Run Jobs and these workloads need to access our Cloud SQL instances. As of now we deploy these Run Jobs using KCC but the lack of Cloud SQL instance support means that we need to create and use Cloud SQL client certificates instead of relying on the SQL socket. It seems that Cloud Run Jobs already support this feature and that it only needs to be added in the RunJob specs for it to work. Please note that KCC Run Service supports Cloud SQL instances.
Not that I am aware of but I could open one if needed. I'm not familiar with KCC dev cycle. |
|
Friendly ping @gemmahou |
Here's the guide for asking a new feature support: https://g3doc.corp.google.com/cloud/config/configconnector/g3doc/customer-engagement.md#recommended-support-channels We suggest open a GH or Buganizier issue, describe the stakeholder, business impact, and assign to KCC team member or our PM Nic(nslattery). That will help us manage and track feature requests. cc: @yuwenma |
config/servicemappings/run.yaml
Outdated
| - tfField: template.template.volumes.cloud_sql_instance.instances | ||
| description: |- | ||
| The Cloud SQL instance connection names, as can be found in https://console.cloud.google.com/sql/instances. Visit https://cloud.google.com/sql/docs/mysql/connect-run for more information on how to connect Cloud SQL and Cloud Run. Format: {project}:{location}:{instance} | ||
| key: instances |
There was a problem hiding this comment.
Should be instancesRef per the naming convention: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/README.ConfigureResourceReferences.md#configure-reference-resource-in-the-service-mappings, point 3
Edit: Please ignore this comment. Since this is a List field, currently we use fieldName directly.
There was a problem hiding this comment.
I think for a list-typed reference field, we should use name instanceRefs instead.
There was a problem hiding this comment.
Hi, I used instances to be coherent with the instance field of Run Services but I changed it to instanceRefs in 249d217
|
We require test coverage for new field reference: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/README.NewResourceFromTerraform.md#test-your-changes Here are some existing test cases for your reference: None of above tests covered the CloudSQLInstance added in this PR, could you add a new test case(i.e. jobwithsqlinstance), include the field reference(CloudSQLInstance) you add, run the dynamic and samples test to make sure it works as expected? |
|
/test all |
|
@CyberHippo: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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-sigs/prow repository. |
|
@CyberHippo triggered the presubmit test. Could you let us know who is the stakeholder/customer for this feature request? Thanks! |
Hi @gemmahou, thanks for running the presubmit job ! The customer would be the company elmy. |
crds/run_v1beta1_runjob.yaml
Outdated
| for more information on how to connect Cloud SQL and Cloud | ||
| Run. | ||
| properties: | ||
| instances: |
There was a problem hiding this comment.
IIUC this file is generated and field name should be updated to instanceRefs as well?
There was a problem hiding this comment.
Good catch! I missed it running make manifests, fixed with 26dbc18
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gemmahou 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 03a3d7d
into
GoogleCloudPlatform:master
Change description
Add support for
cloud_sql_instancefor Cloud Run Jobs.Fixes #1307
Tests you have done
make ready-prto ensure this PR is ready for review.