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

Remove port from endpoint (if added) #154

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

Conversation

tavsec
Copy link

@tavsec tavsec commented Sep 14, 2023

Description of your changes

Today I tried using provider-sql with AWS RDS provider, but RDS provider outputs the endpoint secret with port included (localhost:3306 for example, instead of just localhost, as provider-sql expects). Therefore, crossplane can't access the database, as the formatted DSN, in this case, would be localhost:3306:3306.

This PR will remove the port from the endpoint string, if it exists.

I have:

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

How has this code been tested

I ran the tests, and tried creating new Database on the RDS instance (with MySQL engine).

alereca and others added 5 commits September 14, 2023 16:21
* Set sql_log_bin = 0 before execs in user reconciler

Signed-off-by: Alejandro Recalde <[email protected]>

* Set sql_log_bin = 0 before execs in user reconciler

Signed-off-by: Alejandro Recalde <[email protected]>

* Set sql_log_bin = 0 before execs in grant reconciler

Signed-off-by: Alejandro Recalde <[email protected]>

* Refactor error handling in create, update and delete grant reconciliation methods

Signed-off-by: Alejandro Recalde <[email protected]>

* Fix const `errSetSqlLogBin` should be `errSetSQLLogBin` (golint)

Signed-off-by: Alejandro Recalde <[email protected]>

* Update grant tests

Signed-off-by: Alejandro Recalde <[email protected]>

* Update user controller tests and reduce cyclotomic complexity

Signed-off-by: Alejandro Recalde <[email protected]>

* Update user controller update test

Signed-off-by: Alejandro Recalde <[email protected]>

* Add BinLog field to Database, Grant and User specs

Signed-off-by: Alejandro Recalde <[email protected]>

* Only set sql_log_bin to 0 if binlog spec field is false

Signed-off-by: Alejandro Recalde <[email protected]>

* Fix BinLog spec field comment

Signed-off-by: Alejandro Recalde <[email protected]>

* Update mysql Database resource sample to include forProvider field

Signed-off-by: Alejandro Recalde <[email protected]>

* Rename replicate crd field to binlog (Database, Grant and User specs)

Signed-off-by: Alejandro Recalde <[email protected]>

* Remove AuthPlugin and UsePassword fields from user type

Signed-off-by: Alejandro Recalde <[email protected]>

* Create an xsql.DB.Exec() wrapper in pkg/clients/mysql that allows disabling binlogs before the query and flushing privileges after the query

Signed-off-by: Alejandro Recalde <[email protected]>

---------

Signed-off-by: Alejandro Recalde <[email protected]>
Signed-off-by: Timotej Avsec <[email protected]>
)

* postgres: Expand grant privileges where required

Inside postgresql, some privileges are altered[0] when issuing a `GRANT`
- for example `ALL` is expanded to `CREATE, TEMPORARY, CONNECT`.

When we observe the grant, we query privileges on the database to see if
the ones in the grant exist. Given that they are expanded, we can't
simply query for what the user gave us. Expand the privileges which need
expanding before we make the query.

Closes: crossplane-contrib#92

[0]: https://www.postgresql.org/docs/15/ddl-priv.html

Signed-off-by: Iain Lane <[email protected]>

* Sort the grants privileges before comparing them

When we use them in the observation the order doesn't matter, but in the
test we are generating a diff where it does, so compare them in the
test.

Signed-off-by: Iain Lane <[email protected]>

---------

Signed-off-by: Iain Lane <[email protected]>
Signed-off-by: Iain Lane <[email protected]>
Signed-off-by: Timotej Avsec <[email protected]>
… (crossplane-contrib#148)

* Support mysql grant option by updating grantRegex and parseGrants function

Signed-off-by: Alejandro Recalde <[email protected]>
Signed-off-by: Duologic <[email protected]>

* When GRANT OPTION is present append WITH GRANT OPTION to query

Signed-off-by: Alejandro Recalde <[email protected]>
Signed-off-by: Duologic <[email protected]>

* fix: tests

Signed-off-by: Duologic <[email protected]>

---------

Signed-off-by: Alejandro Recalde <[email protected]>
Signed-off-by: Duologic <[email protected]>
Co-authored-by: Alejandro Recalde <[email protected]>
Co-authored-by: Duologic <[email protected]>
Signed-off-by: Timotej Avsec <[email protected]>
* fix: correctly set GRANT OPTION privilege

Signed-off-by: Duologic <[email protected]>

* chore: introduce createRevokeQuery function

Signed-off-by: Javier Palomo <[email protected]>

* fix: use createRevokeQuery in Delete

Signed-off-by: Javier Palomo <[email protected]>

* fix: dont split user and host

Signed-off-by: Javier Palomo <[email protected]>

* fix: unnecessary type conversion in getPrivilegesString

Signed-off-by: Javier Palomo <[email protected]>

* fix(createRevokeQuery): use right MySQL syntax

Signed-off-by: Javier Palomo <[email protected]>

---------

Signed-off-by: Duologic <[email protected]>
Signed-off-by: Javier Palomo <[email protected]>
Co-authored-by: Duologic <[email protected]>
Signed-off-by: Timotej Avsec <[email protected]>
@Duologic
Copy link
Member

In our composition for RDS we have this:

connectionDetails:
    - fromConnectionSecretKey: username
      name: username
    - fromConnectionSecretKey: attribute.password
      name: password
    - fromConnectionSecretKey: address
      name: endpoint
    - fromConnectionSecretKey: port
      name: port

The address key was added in crossplane-contrib/provider-upjet-aws#530

@tavsec
Copy link
Author

tavsec commented Sep 15, 2023

Hey @Duologic , I'm not entirely sure if I am doing something wrong, but I don't think this is working as it should. This is my RDS manifest:

apiVersion: rds.aws.upbound.io/v1beta1
kind: Instance
metadata:
  annotations:
  name: my-database
spec:
  forProvider:
    allocatedStorage: 20
    autoMinorVersionUpgrade: true
    engine: mysql
    engineVersion: "8.0"
    instanceClass: db.t3.micro
    name: my-database
    publiclyAccessible: true
    region: eu-central-1
    skipFinalSnapshot: true
    storageEncrypted: false
    storageType: gp2
    username: admin
    passwordSecretRef:
      key: password
      name: rds-initial-password
      namespace: default

  providerConfigRef:
    name: default
  writeConnectionSecretToRef:
    name: very-secure-credentials-for-rds
    namespace: default

The secret very-secure-credentials-for-rds is created, and has 7 values (address, attribute.password, endpoint, host, password, port and username) - and they have the "right" value (I can connect to the DB from MySQL client).

But when I create ProviderConfig for sql-provider, which is as folows:

---
apiVersion: mysql.sql.crossplane.io/v1alpha1
kind: ProviderConfig
metadata:
  name: sql-config
spec:
  credentials:
    source: MySQLConnectionSecret
    connectionSecretRef:
      namespace: default
      name: very-secure-credentials-for-rds

and create DB resource

apiVersion: mysql.sql.crossplane.io/v1alpha1
kind: Database
metadata:
  labels: 
    type: database
  name: my-table
spec:
  deletionPolicy: Orphan
  providerConfigRef:
    name: sql-config
  forProvider: {}

I get the following error when describing the database resource: observe failed: cannot select database: dial tcp: lookup mydatabase.some-random-characters.eu-central-1.rds.amazonaws.com:3306:3306: no such host

Maybe, in this case, this should be moved to issues?

@Duologic
Copy link
Member

We make use of compositions rather then directly dealing with managed resources, for example they can create/convert secrets to another format.

@Smana
Copy link

Smana commented Nov 28, 2023

I have an issue regarding the fact that the endpoint from an RDS instance actually contains the port.

status:
  atProvider: {}
  conditions:
  - lastTransitionTime: "2023-11-28T08:29:14Z"
    message: 'observe failed: cannot select role: dial tcp: lookup xplane-harbor-5jxnh-9rb9t.cymnaynfchjt.eu-west-3.rds.amazonaws.com:5432:5432:
...

I tried to define them here as you mentioned @Duologic . However these changes are applied to the secret being generated by the composition:

kubectl get secrets -n crossplane-system 23172561-479b-4a39-a0f0-609562b8f2e3 -o yaml
apiVersion: v1
data:
  endpoint: xxx
  password: xx
  username: xx
...

In my case I want to have a predictable secret name in order to use it with this provider.
The secret at the resource level is indeed created but without the transformation removing the tcp port.

kubectl get secrets -n harbor xplane-harbor-rds -o yaml
apiVersion: v1
data:
  address: xxx
  attribute.password: UmRzI2F6ZXJ0eTQy
  endpoint: xx (contains host:5432)
  host: xx
  password: xxx
  port: xx
  username: xx

Could you please give me a hand? should I open an issue? Why is the endpoint format different in this provider and the rds managed resource?

@Duologic
Copy link
Member

@olvesh
Copy link

olvesh commented Nov 8, 2024

What is blocking fixing this?

I am currently evaluating crossplane for an organisation, and ran into this issue when following tutorials from @vfarcic using AWS RDS:

status:
  conditions:
  - lastTransitionTime: "2024-11-08T14:17:03Z"
    message: 'observe failed: cannot select database: dial tcp: lookup <snip>.eu-north-1.rds.amazonaws.com:5432:5432: 
      no such host'
    reason: ReconcileError
    status: "False"
    type: Synced

As described above, I guess this looks up the endpoint from the my-db secret (as I am following the tutorial):

address: <snip>.eu-north-1.rds.amazonaws.com
attribute.password: postgres
endpoint: <snip>.eu-north-1.rds.amazonaws.com:5432
host: <snip>.eu-north-1.rds.amazonaws.com
password: postgres
port: "5432"
username: masteruser

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.

7 participants