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

Mysql default port #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

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 <[email protected]>
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.

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
3 participants