Skip to content

Conversation

@sendmars
Copy link

@sendmars sendmars commented Apr 6, 2022

Description of your changes

When connection secret doesn't contain port, this change assumes default port for MySQL, 3306.

This is needed because provider-aws doesn't write port (by default) to the connection secret for Aurora DB cluster. Provider-sql shouldn't accept the empty string as it is an invalid value for port, and assume that a missing value should default to the standard port.

Fixes #76

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

[Testing incomplete]
I ran make run locally, which caused a local reconciler to run in parallel with the one in the kubernetes cluster.

When describing the user.mysql on the kubernetes cluster, I see (messages only, for readability):

cannot select user: dial tcp 10.0.x.x:3306: i/o timeout         # local, patched reconciler
cannot select user: dial tcp 10.0.x.x:0: i/o timeout            # reconciler in the cluster

The change sets the port correctly. The connection to db_host:3306 is open, so I would expect that the user is created. The problem seem not to be related to my change, but I would need some assistance in figuring this out. The debug output contains (newlines added manually):

2022-04-05T23:46:44.471+0100    DEBUG   provider-sql    Cannot observe external resource
{
"controller": "managed/user.mysql.sql.crossplane.io",
"request": "/cluster-test-user",
"uid": "...",
"version": "...",
"external-name": "cluster-test-user",
"error": "cannot select user: dial tcp 10.0.x.x:3306: i/o timeout",
"errorVerbose": "dial tcp 10.0.x.2x:3306: i/o timeout
      cannot select user
      github.com/crossplane-contrib/provider-sql/pkg/controller/mysql/user.(*external).Observe
          /workdir/opensource/provider-sql/pkg/controller/mysql/user/reconciler.go:216 ..."

I checked that the port 3306 is part of c.db in Observe. I suspect that the issue with connection comes from the interaction of the two reconcilers.

sendmars added 2 commits April 6, 2022 02:09
Signed-off-by: Domagoj Marsic <domagoj.marsic@sendoso.com>
If a secret doesn't contain port, the provider won't be able to connect
as it will use an empty string, which then turns to 0. An attempt to
connect times out since this is not a valid port.

This is the case with connection secrets created by provider-aws, for
example for DBCluster. We should assume that the absence of the port
key/value means that connection should happen over the default port,
3306 for MySQL servers.

Signed-off-by: Domagoj Marsic <domagoj.marsic@sendoso.com>
Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

This could work as a stop-gap but has a very limited usecase.

I've touched a similar problem where I wanted to set the endpoint to local pod (cloudsql-proxy on GCP), I managed to workaround it by passing in the endpoint/port through a label on a Composition.

However it can be a bit more generic, perhaps we can extend the ProviderConfigs with overrides for host/port? This can be fairly generic for all 3 database types. WDYT?

@luisdavim
Copy link

This could work as a stop-gap but has a very limited usecase.

I've touched a similar problem where I wanted to set the endpoint to local pod (cloudsql-proxy on GCP), I managed to workaround it by passing in the endpoint/port through a label on a Composition.

However it can be a bit more generic, perhaps we can extend the ProviderConfigs with overrides for host/port? This can be fairly generic for all 3 database types. WDYT?

I think that extending the ProviderConfigs is a good idea but I don't see any issue with the change being proposed here, falling back to the default port when no port is provided makes sense to me.

@Duologic
Copy link
Member

I'd be up for merging this if someone can rebase.

@pierluigilenoci
Copy link

@sendmars please rebase.

@chlunde
Copy link
Collaborator

chlunde commented May 27, 2025

@sendmars could you rebase this PR? Thanks!

Otherwise I'll close it to clean up the PR backlog, but anyone interrested could recreate a similar PR. It would be nice to ensure all supported databases behave the same way, at least if that makes sense for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use default Mysql port if not specified in the connection secret

5 participants