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

Create connection secret with desired fields #73

Merged
merged 1 commit into from Dec 12, 2020

Conversation

turkenh
Copy link
Collaborator

@turkenh turkenh commented Dec 11, 2020

Signed-off-by: Hasan Turken turkenh@gmail.com

Description of your changes

Closes #56

I have:

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

@@ -31,5 +31,28 @@ spec:
# name: svals
# namespace: wordpress
# optional: false
# connectionDetails:
Copy link
Contributor

@lukeweber lukeweber Dec 12, 2020

Choose a reason for hiding this comment

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

I feel connectionDetails isn't the perfect name as it's really general purpose data sync from a Release.

observation:
    - items ...
    writeObservationSecretToRef:
    - ...

Copy link
Collaborator Author

@turkenh turkenh Dec 12, 2020

Choose a reason for hiding this comment

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

I see what you mean and actually gave it a try to change it in that direction, however ended up with current solution because of following reasons:

  • writeConnectionSecretToRef is part of crossplane managed resource spec, there are existing machinery coming with crossplane runtime hence this field is common across all providers. I preferred to reuse this pattern/machinery here.
  • connectionDetails is also something crossplane users should be familiar with, it exists in composite resource api (example) with similar purposes, where you select what you want to be included in the connection secret that composite resource creates.

@@ -31,6 +40,7 @@ const (
errReleaseInfoNilInObservedRelease = "release info is nil in observed helm release"
errChartNilInObservedRelease = "chart field is nil in observed helm release"
errChartMetaNilInObservedRelease = "chart metadata field is nil in observed helm release"
errObjectNotPartOfRelease = "object is not part of release: %v"
Copy link
Contributor

Choose a reason for hiding this comment

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

Really happy with where this landed for security reasons and minimizing unexpected behavior.

I could imagine someone might install a few helm charts and create a final observation, but this puts some rails that no one mis-configures and fetches something from a different helm chart.

The downside of course as discussed is that maybe an operator is installed and simply creates an object that isn't part of the release explicitly. If this ends up being a use case, they could provide an empty secret in the chart, or we could consider in the future an escape config to let them fetch arbitrary resources, but this seems like a great first step.

Copy link

Choose a reason for hiding this comment

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

I'm interested in the scenario where I want to observe and provision connection details from a Secret created by an operator. In this case I want to retrieve the kubeconfig from a vcluster Release. I'm not confident that the controller could handle an existing secret without a patch either. Do I have a way to prevent object is not part of release error without forking the charts and creating a dummy secret? It seems like an inelegant and operationally expensive solution.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@turkenh changing this to v1beta1 should cause integration tests to start passing :)

apiVersion: pkg.crossplane.io/v1alpha1

Closes crossplane-contrib#56

Signed-off-by: Hasan Turken <turkenh@gmail.com>
Comment on lines +48 to +53
# - apiVersion: v1
# kind: Secret
# name: wordpress-example
# namespace: wordpress
# fieldPath: data.wordpress-password
# toConnectionSecretKey: password
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a ServiceAccountKey.cloudplatform.gcp.upbound.io/v1beta1 inside a helm chart, which creates a connection secret at <uid>-serviceaccountkey in the crossplane namespace.

So I made a list entry like this on my Release:

            - apiVersion: v1
              kind: Secret
              name: ""
              namespace: crossplane

And then I'm patching the name like so:

        - type: CombineFromComposite
          combine:
            variables:
              - fromFieldPath: metadata.uid
            strategy: string
            string:
              fmt: "%s-serviceaccountkey"
          toFieldPath: spec.connectionDetails[0].name
          policy:
            fromFieldPath: Required

Now the issue is that on the Release I get an event that says:

│   Warning  CannotObserveExternalResource  31s (x5 over 3m34s)    managed/release.helm.crossplane.io  cannot get connection details: object is not part of release: {Secret crossplane 8578b338-7520-44e8-a3e7-a1555abfd257-serviceaccountkey  v1  }

Which is of course true, because the connection secret is not part of that release, it's just a result of it.
But the connection secret exists:

╰─ kg secret -n crossplane 8578b338-7520-44e8-a3e7-a1555abfd257-serviceaccountkey
NAME                                                     TYPE                                DATA   AGE
8578b338-7520-44e8-a3e7-a1555abfd257-serviceaccountkey   connection.crossplane.io/v1alpha1   2      7m33s

How can I propagate the secret of that Secret resource outside of the release? I thought it should be possible since I'm just referring to an arbitrary secret by name and namespace that the controller has access to...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we discussed offline, skipPartOfReleaseCheck: true should do the trick.

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.

Create Connection Secrets
5 participants