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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion apis/release/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1beta1

import (
"helm.sh/helm/v3/pkg/release"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
runtime "k8s.io/apimachinery/pkg/runtime"

Expand Down Expand Up @@ -85,7 +86,8 @@ type ReleaseObservation struct {
// A ReleaseSpec defines the desired state of a Release.
type ReleaseSpec struct {
runtimev1alpha1.ResourceSpec `json:",inline"`
ForProvider ReleaseParameters `json:"forProvider"`
ConnectionDetails []ConnectionDetail `json:"connectionDetails,omitempty"`
ForProvider ReleaseParameters `json:"forProvider"`
// RollbackRetriesLimit is max number of attempts to retry Helm deployment by rolling back the release.
RollbackRetriesLimit *int32 `json:"rollbackLimit,omitempty"`
}
Expand All @@ -99,6 +101,12 @@ type ReleaseStatus struct {
Synced bool `json:"synced,omitempty"`
}

// ConnectionDetail todo
type ConnectionDetail struct {
v1.ObjectReference `json:",inline"`
ToConnectionSecretKey string `json:"toConnectionSecretKey,omitempty"`
}

// +kubebuilder:object:root=true
// +kubebuilder:storageversion

Expand Down
21 changes: 21 additions & 0 deletions apis/release/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cluster/integration/integration_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ check_pods 2
echo_step "installing ${PROJECT_NAME} into \"${CROSSPLANE_NAMESPACE}\" namespace"

INSTALL_YAML="$( cat <<EOF
apiVersion: pkg.crossplane.io/v1alpha1
apiVersion: pkg.crossplane.io/v1beta1
kind: Provider
metadata:
name: "${PACKAGE_NAME}"
Expand Down
23 changes: 23 additions & 0 deletions examples/sample/release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.

# - apiVersion: v1
# kind: Service
# name: wordpress-example
# namespace: wordpress
# fieldPath: spec.clusterIP
# #fieldPath: status.loadBalancer.ingress[0].ip
# toConnectionSecretKey: ip
# - apiVersion: v1
# kind: Service
# name: wordpress-example
# namespace: wordpress
# fieldPath: spec.ports[0].port
# toConnectionSecretKey: port
# - apiVersion: v1
# kind: Secret
# name: wordpress-example
# namespace: wordpress
# fieldPath: data.wordpress-password
# toConnectionSecretKey: password
Comment on lines +48 to +53
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.

# writeConnectionSecretToRef:
# name: wordpress-credentials
# namespace: crossplane-system
providerConfigRef:
name: helm-provider
29 changes: 29 additions & 0 deletions package/crds/helm.crossplane.io_releases.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,35 @@ spec:
spec:
description: A ReleaseSpec defines the desired state of a Release.
properties:
connectionDetails:
items:
description: ConnectionDetail todo
properties:
apiVersion:
description: API version of the referent.
type: string
fieldPath:
description: 'If referring to a piece of an object instead of an entire object, this string should contain a valid JSON/Go field access statement, such as desiredState.manifest.containers[2]. For example, if the object reference is to a container within a pod, this would take on a value like: "spec.containers{name}" (where "name" refers to the name of the container that triggered the event) or if no container name is specified "spec.containers[2]" (container with index 2 in this pod). This syntax is chosen only to have some well-defined way of referencing a part of an object. TODO: this design is not final and this field is subject to change in the future.'
type: string
kind:
description: 'Kind of the referent. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
name:
description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names'
type: string
namespace:
description: 'Namespace of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/'
type: string
resourceVersion:
description: 'Specific resourceVersion to which this reference is made, if any. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency'
type: string
toConnectionSecretKey:
type: string
uid:
description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
type: string
type: object
type: array
deletionPolicy:
description: DeletionPolicy specifies what will happen to the underlying external when this managed resource is deleted - either "Delete" or "Orphan" the external resource. The "Delete" policy is the default when no policy is specified.
enum:
Expand Down
60 changes: 60 additions & 0 deletions pkg/controller/release/observe.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ package release

import (
"context"
"encoding/base64"
"fmt"
"strings"

"github.com/crossplane/crossplane-runtime/pkg/fieldpath"
"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"

"github.com/pkg/errors"
"helm.sh/helm/v3/pkg/release"
Expand All @@ -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.

)

// generateObservation generates release observation for the input release object
Expand Down Expand Up @@ -112,3 +122,53 @@ func isUpToDate(ctx context.Context, kube client.Client, in *v1beta1.ReleasePara
func isPending(s release.Status) bool {
return s == release.StatusPendingInstall || s == release.StatusPendingUpgrade || s == release.StatusPendingRollback
}

func connectionDetails(ctx context.Context, kube client.Client, connDetails []v1beta1.ConnectionDetail, relName, relNamespace string) (managed.ConnectionDetails, error) {
mcd := managed.ConnectionDetails{}

for _, cd := range connDetails {
ro := unstructuredFromObjectRef(cd.ObjectReference)
if err := kube.Get(ctx, types.NamespacedName{Name: ro.GetName(), Namespace: ro.GetNamespace()}, &ro); err != nil {
return mcd, errors.Wrap(err, "cannot get object")
}

// TODO(hasan): consider making this check configurable, i.e. possible to skip via a field in spec
if !partOfRelease(ro, relName, relNamespace) {
return mcd, errors.Errorf(errObjectNotPartOfRelease, cd.ObjectReference)
}

paved := fieldpath.Pave(ro.Object)
v, err := paved.GetValue(cd.FieldPath)
if err != nil {
return mcd, errors.Wrapf(err, "failed to get value at fieldPath: %s", cd.FieldPath)
}
s := fmt.Sprintf("%v", v)
fv := []byte(s)
// prevent secret data being encoded twice
if cd.Kind == "Secret" && cd.APIVersion == "v1" && strings.HasPrefix(cd.FieldPath, "data") {
fv, err = base64.StdEncoding.DecodeString(s)
if err != nil {
return mcd, errors.Wrap(err, "failed to decode secret data")
}
}

mcd[cd.ToConnectionSecretKey] = fv
}

return mcd, nil
}

func unstructuredFromObjectRef(r corev1.ObjectReference) unstructured.Unstructured {
u := unstructured.Unstructured{}
u.SetAPIVersion(r.APIVersion)
u.SetKind(r.Kind)
u.SetName(r.Name)
u.SetNamespace(r.Namespace)

return u
}

func partOfRelease(u unstructured.Unstructured, relName, relNamespace string) bool {
a := u.GetAnnotations()
return a[helmReleaseNameAnnotation] == relName && a[helmReleaseNamespaceAnnotation] == relNamespace
}
120 changes: 118 additions & 2 deletions pkg/controller/release/observe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import (
"context"
"testing"

"k8s.io/apimachinery/pkg/runtime"

"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
"github.com/crossplane/crossplane-runtime/pkg/test"
"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/crossplane-contrib/provider-helm/apis/release/v1beta1"
Expand Down Expand Up @@ -372,3 +374,117 @@ func Test_isUpToDate(t *testing.T) {
})
}
}

func Test_connectionDetails(t *testing.T) {
type args struct {
kube client.Client
connDetails []v1beta1.ConnectionDetail
relName string
relNamespace string
}
type want struct {
out managed.ConnectionDetails
err error
}
cases := map[string]struct {
args
want
}{
"Fail_NotPartOfRelease": {
args: args{
kube: &test.MockClient{
MockGet: func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
if o, ok := obj.(*unstructured.Unstructured); o.GetKind() == "Secret" && ok && key.Name == testSecretName && key.Namespace == testNamespace {
*obj.(*unstructured.Unstructured) = unstructured.Unstructured{
Object: map[string]interface{}{
"data": map[string]interface{}{
"db-password": "MTIzNDU=",
},
},
}
}
return nil
},
},
connDetails: []v1beta1.ConnectionDetail{
{
ObjectReference: corev1.ObjectReference{
Kind: "Secret",
Namespace: testNamespace,
Name: testSecretName,
APIVersion: "v1",
FieldPath: "data.db-password",
},
ToConnectionSecretKey: "password",
},
},
relName: testReleaseName,
relNamespace: testNamespace,
},
want: want{
out: managed.ConnectionDetails{},
err: errors.Errorf(errObjectNotPartOfRelease, corev1.ObjectReference{
Kind: "Secret",
Namespace: testNamespace,
Name: testSecretName,
APIVersion: "v1",
FieldPath: "data.db-password",
}),
},
},
"Success_PartOfRelease": {
args: args{
kube: &test.MockClient{
MockGet: func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
if o, ok := obj.(*unstructured.Unstructured); o.GetKind() == "Secret" && ok && key.Name == testSecretName && key.Namespace == testNamespace {
*obj.(*unstructured.Unstructured) = unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"annotations": map[string]interface{}{
helmReleaseNameAnnotation: testReleaseName,
helmReleaseNamespaceAnnotation: testNamespace,
},
},
"data": map[string]interface{}{
"db-password": "MTIzNDU=",
},
},
}
}
return nil
},
},
connDetails: []v1beta1.ConnectionDetail{
{
ObjectReference: corev1.ObjectReference{
Kind: "Secret",
Namespace: testNamespace,
Name: testSecretName,
APIVersion: "v1",
FieldPath: "data.db-password",
},
ToConnectionSecretKey: "password",
},
},
relName: testReleaseName,
relNamespace: testNamespace,
},
want: want{
out: managed.ConnectionDetails{
"password": []byte("12345"),
},
},
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got, gotErr := connectionDetails(context.Background(), tc.args.kube, tc.args.connDetails, tc.args.relName, tc.args.relNamespace)
if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" {
t.Fatalf("connectionDetails(...): -want error, +got error: %s", diff)
}
if diff := cmp.Diff(tc.want.out, got); diff != "" {
t.Errorf("connectionDetails(...): -want result, +got result: %s", diff)
}
})
}
}