Read PostgreSQL admin password from connection secret #284

ulucinar · 2021-08-18T20:22:00Z

Description of your changes

This PR proposes a fix for the wrong password in the generated connection secret issue discussed in #230. The proposed behavior is backwards-compatible and in case multiple Creates are called and if a connection secret is defined, we make sure that the password initially used to make the Azure PostgreSQLServer create call is reused, effectively preventing multiple erroneous password generations and the issue described in #230.

Fixes #230

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

I have written a test script that does the following in a loop:

Apply the claim defined below to provision a PostgreSQLServer
Waits until the PostgreSQLServer MR becomes ready
Invokes az postgres server firewall-rule create against this newly provisioned instance to allow connections from my workstation
Invokes psql using the credentials and the connection info stored in the connection secret.
Deletes the claim
Waits until PostgreSQLServer MR is actually removed

With the proposed fix, issue #230 has not been observed in 10 iterations of the above loop. However, running the experiment with the head of the master branch, out of 10 trials, 8 were successful and 2 have failed with the issue reported in #230 (psql: error: FATAL: password authentication failed for user "myadmin").

Manifests used for the experiments are as follows. These are some stripped down versions of the manifests given here by @haraldatbmw:

---
apiVersion: foo.bar/v1alpha1
kind: PostgreSQLInstance
metadata:
  namespace: default
  name: example
spec:
  parameters:
    location: westeurope
    storageGB: 5
    version: "11"
    virtualNetworkSubnetId: /subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Network/virtualNetworks/xxx/subnets/xxx
    dbName: example-db
  compositionSelector:
    matchLabels:
      provider: azure
  writeConnectionSecretToRef:
    name: example-db-connection

---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: compositepostgresqlinstances.azure.foo.bar
  labels:
    provider: azure
spec:
  writeConnectionSecretsToNamespace: crossplane-system
  compositeTypeRef:
    apiVersion: foo.bar/v1alpha1
    kind: CompositePostgreSQLInstance
  patchSets:
  - name: Metadata
    patches:
    - fromFieldPath: metadata.labels
    - fromFieldPath: metadata.annotations[crossplane.io/external-name]
  resources:
  # resource-group where the db-server lives in
  - name: resourcegroup
    base:
      apiVersion: azure.crossplane.io/v1alpha3
      kind: ResourceGroup
      spec: {}
    patches:
    - type: PatchSet
      patchSetName: Metadata
    - fromFieldPath: "spec.parameters.location"
      toFieldPath: "spec.location"
  # db-server
  - name: postgresqlserver
    base:
      apiVersion: database.azure.crossplane.io/v1beta1
      kind: PostgreSQLServer
      spec:
        forProvider:
          createMode: Default
          administratorLogin: myadmin
          resourceGroupNameSelector:
            matchControllerRef: true
          minimalTlsVersion: TLS1_2
          sslEnforcement: Enabled
          sku:
            tier: GeneralPurpose
            capacity: 2
            family: Gen5  
          storageProfile:
            backupRetentionDays: 35
            geoRedundantBackup: Disabled
            storageAutogrow: Disabled
        writeConnectionSecretToRef:
          namespace: crossplane-system
        providerConfigRef:
          name: default
    patches:
    - type: PatchSet
      patchSetName: Metadata
    - fromFieldPath: "metadata.uid"
      toFieldPath: "spec.writeConnectionSecretToRef.name"
      transforms:
      - type: string
        string:
          fmt: "postgresqlserver-admin-%s"
    - fromFieldPath: "spec.parameters.version"
      toFieldPath: "spec.forProvider.version"
    - fromFieldPath: "spec.parameters.location"
      toFieldPath: "spec.forProvider.location"
    - fromFieldPath: "spec.parameters.storageGB"
      toFieldPath: "spec.forProvider.storageProfile.storageMB"
      transforms:
        - type: math
          math:
            multiply: 1024
  # db-server vnet-rule for subnet where AKS lives in
  - name: vnetrule
    base:
      apiVersion: database.azure.crossplane.io/v1alpha3
      kind: PostgreSQLServerVirtualNetworkRule
      spec:
        providerConfigRef:
          name: default
        resourceGroupNameSelector:
          matchControllerRef: true
        serverNameSelector:
          matchControllerRef: true
    patches:
    - type: PatchSet
      patchSetName: Metadata
    - fromFieldPath: "spec.parameters.virtualNetworkSubnetId"
      toFieldPath: "spec.properties.virtualNetworkSubnetId"

---
apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: compositepostgresqlinstances.foo.bar
spec:
  connectionSecretKeys:
  - username
  - password
  - hostname
  - port
  - database
  group: foo.bar
  names:
    kind: CompositePostgreSQLInstance
    plural: compositepostgresqlinstances
  claimNames:
    kind: PostgreSQLInstance
    plural: postgresqlinstances
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              parameters:
                type: object
                properties:
                  version:
                    description: PostgreSQL engine version
                    type: string
                    enum: ["9.5", "9.6", "10", "11"]
                  storageGB:
                    type: integer
                  location:
                    description: Geographic location of this PostgreSQL server.
                    type: string
                  virtualNetworkSubnetId:
                    description: Subnet-Id of your AKS cluster
                    type: string
                  dbName:
                    description: The name of the database
                    type: string
                required:
                - version
                - storageGB
                - location
                - virtualNetworkSubnetId
                - dbName
            required:
            - parameters

- Fixes crossplane-contrib#230 Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar · 2021-08-19T10:14:02Z

We also need the CI pipeline fixes implemented here:
crossplane-contrib/provider-aws#782

muvaf · 2021-08-19T11:03:03Z

pkg/controller/database/postgresqlserver/managed.go

@@ -146,6 +149,23 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	return o, nil
 }

+func (e *external) getPassword(ctx context.Context, cr *v1beta1.PostgreSQLServer) (string, error) {
+	if cr == nil || cr.Spec.WriteConnectionSecretToReference == nil ||


Suggested change

if cr == nil || cr.Spec.WriteConnectionSecretToReference == nil ||

if cr.Spec.WriteConnectionSecretToReference == nil ||

nitpick: we'd need to place nil guard to just too many spaces if we were to check existence of the CR. So, we usually assume it's there after the classic cr, ok := mg.(*v1beta1.PostgreSQLServer) statement.

muvaf · 2021-08-19T11:04:26Z

pkg/controller/database/postgresqlserver/managed.go

+	if pw == "" {
+		pw, err = e.newPasswordFn()
+	}
 	if err != nil {
 		return managed.ExternalCreation{}, errors.Wrap(err, errGenPassword)
 	}


Suggested change

if pw == "" {

pw, err = e.newPasswordFn()

}

if err != nil {

return managed.ExternalCreation{}, errors.Wrap(err, errGenPassword)

}

if pw == "" {

pw, err = e.newPasswordFn()

if err != nil {

return managed.ExternalCreation{}, errors.Wrap(err, errGenPassword)

}

}

I was worrying that reviewers would not like the nested ifs :)
Done.

muvaf · 2021-08-19T11:05:13Z

pkg/controller/database/postgresqlserver/managed.go

+	pw, err := e.getPassword(ctx, cr)
+	if err != nil {
+		return managed.ExternalCreation{}, err
+	}


I thought you said it'd save the password to the connection secret before triggering the creation. Is that not the case anymore?

Nope, it was never the case (also not the case when I ran the above set of experiments). We still leave it to the managed reconciler.

Got it. So, we save the password after a successful CreateServer call via the managed reconciler and if CreateServer is ever called again, we'd be getting the password from connection secret.

- Make kind version used in e2e tests configurable - Update kind version to 0.11.1 - Update kind node image version to 1.19.11 - These are needed to get e2e tests running. Please see: crossplane-contrib/provider-aws#782 (comment) Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf

LGTM! As we discussed, normally we have secret refs for password input that we use to get the password and if we do want to autogenerate it, so far, we've written the generated password to that input secret and then treated it like user input in the subsequent operations. But since some parts of this would require change in the behavior, it's fine to use connection details but it'd be great if we have an issue opened to add input secret ref field.

Read PostgreSQL admin password from connection secret

d7e7249

- Fixes crossplane-contrib#230 Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar mentioned this pull request Aug 19, 2021

Wrong password in generated connection-secret for PostgreSQLServer #230

Closed

ulucinar requested a review from muvaf August 19, 2021 09:25

ulucinar force-pushed the fix-230 branch from f5cfc09 to 6bab3d0 Compare August 19, 2021 10:12

muvaf reviewed Aug 19, 2021

View reviewed changes

ulucinar force-pushed the fix-230 branch from 6bab3d0 to 031156b Compare August 19, 2021 11:15

muvaf approved these changes Aug 19, 2021

View reviewed changes

ulucinar merged commit 56d101f into crossplane-contrib:master Aug 19, 2021

ulucinar deleted the fix-230 branch August 19, 2021 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read PostgreSQL admin password from connection secret #284

Read PostgreSQL admin password from connection secret #284

ulucinar commented Aug 18, 2021 •

edited

Loading

ulucinar commented Aug 19, 2021

muvaf Aug 19, 2021

ulucinar Aug 19, 2021

muvaf Aug 19, 2021

ulucinar Aug 19, 2021

muvaf Aug 19, 2021

ulucinar Aug 19, 2021

muvaf Aug 19, 2021

muvaf left a comment

	if cr == nil \|\| cr.Spec.WriteConnectionSecretToReference == nil \|\|
	if cr.Spec.WriteConnectionSecretToReference == nil \|\|

Read PostgreSQL admin password from connection secret #284

Read PostgreSQL admin password from connection secret #284

Conversation

ulucinar commented Aug 18, 2021 • edited Loading

Description of your changes

How has this code been tested

ulucinar commented Aug 19, 2021

muvaf Aug 19, 2021

Choose a reason for hiding this comment

ulucinar Aug 19, 2021

Choose a reason for hiding this comment

muvaf Aug 19, 2021

Choose a reason for hiding this comment

ulucinar Aug 19, 2021

Choose a reason for hiding this comment

muvaf Aug 19, 2021

Choose a reason for hiding this comment

ulucinar Aug 19, 2021

Choose a reason for hiding this comment

muvaf Aug 19, 2021

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

ulucinar commented Aug 18, 2021 •

edited

Loading