Skip to content

Commit

Permalink
Unproject workloads no longer targeted by a ServiceBinding (#348)
Browse files Browse the repository at this point in the history 
When a previously bound workload is no longer targeted by a
ServiceBinding it is now unprojected. Previously, the projection was
orphaned as the controller only managed workload matching the reference
on the ServiceBinding resource. This could happen for a few different
reasons:

- the name of the referenced workload was updated on the ServiceBinding
- the label selector matching the workload was updated on the
  ServiceBinding
- the labels on the workload were updated to no longer match the
  selector on the ServiceBinding.

In order to find previously bound workloads, we now list all resources
matching the workload refs GVK in the namespace.  That list is filtered
to resources that are currently projected, or match the new criteria.
All matching workloads are unprojected, but only resources matching the
current ref are re-projected.

To avoid a much larger search area, the workloadRef apiVersion and kind
fields are now immutable. Users who need to update either of these
values will need to delete the ServiceBinding and create a new resource
with the desired values.

Signed-off-by: Scott Andrews <[email protected]>
  • Loading branch information
@scothis
scothis authored Oct 14, 2023
1 parent d12e4ec commit a964673
Show file tree
Hide file tree
Showing 15 changed files with 759 additions and 163 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ jobs:
uses: actions/checkout@v4
with:
repository: servicebinding/conformance.git
ref: v0.3.1
ref: v0.3.2
fetch-depth: 1
path: conformance-tests

Expand Down
165 changes: 165 additions & 0 deletions apis/v1beta1/servicebinding_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
)

Expand Down Expand Up @@ -273,3 +275,166 @@ func TestServiceBindingValidate(t *testing.T) {
})
}
}

func TestServiceBindingValidate_Immutable(t *testing.T) {
tests := []struct {
name string
seed *ServiceBinding
old runtime.Object
expected field.ErrorList
}{
{
name: "allow update workload name",
seed: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "Deloyment",
Name: "new-workload",
},
},
},
old: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "Deloyment",
Name: "old-workload",
},
},
},
expected: field.ErrorList{},
},
{
name: "reject update workload apiVersion",
seed: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "Deloyment",
Name: "my-workload",
},
},
},
old: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "extensions/v1beta1",
Kind: "Deloyment",
Name: "my-workload",
},
},
},
expected: field.ErrorList{
{
Type: field.ErrorTypeForbidden,
Field: "spec.workload.apiVersion",
Detail: "Workload apiVersion is immutable. Delete and recreate the ServiceBinding to update.",
BadValue: "",
},
},
},
{
name: "reject update workload kind",
seed: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "Deloyment",
Name: "my-workload",
},
},
},
old: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "StatefulSet",
Name: "my-workload",
},
},
},
expected: field.ErrorList{
{
Type: field.ErrorTypeForbidden,
Field: "spec.workload.kind",
Detail: "Workload kind is immutable. Delete and recreate the ServiceBinding to update.",
BadValue: "",
},
},
},
{
name: "unkonwn old object",
seed: &ServiceBinding{
Spec: ServiceBindingSpec{
Name: "my-binding",
Service: ServiceBindingServiceReference{
APIVersion: "v1",
Kind: "Secret",
Name: "my-service",
},
Workload: ServiceBindingWorkloadReference{
APIVersion: "apps/v1",
Kind: "Deloyment",
Name: "new-workload",
},
},
},
old: &corev1.Pod{},
expected: field.ErrorList{
{
Type: field.ErrorTypeInternal,
Field: "<nil>",
Detail: "old object must be of type v1beta1.ServiceBinding",
},
},
},
}

for _, c := range tests {
t.Run(c.name, func(t *testing.T) {
expectedErr := c.expected.ToAggregate()

_, actualUpdateErr := c.seed.ValidateUpdate(c.old)
if diff := cmp.Diff(expectedErr, actualUpdateErr); diff != "" {
t.Errorf("ValidateCreate (-expected, +actual): %s", diff)
}
})
}
}
39 changes: 37 additions & 2 deletions apis/v1beta1/servicebinding_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ limitations under the License.
package v1beta1

import (
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/conversion"
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)
Expand Down Expand Up @@ -52,9 +55,41 @@ func (r *ServiceBinding) ValidateCreate() (admission.Warnings, error) {

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *ServiceBinding) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
// TODO(user): check for immutable fields, if any
r.Default()
return nil, r.validate().ToAggregate()

errs := field.ErrorList{}

// check immutable fields
var ro *ServiceBinding
if o, ok := old.(*ServiceBinding); ok {
ro = o
} else if o, ok := old.(conversion.Convertible); ok {
ro = &ServiceBinding{}
if err := o.ConvertTo(ro); err != nil {
return nil, err
}
} else {
errs = append(errs,
field.InternalError(nil, fmt.Errorf("old object must be of type v1beta1.ServiceBinding")),
)
}
if len(errs) == 0 {
if r.Spec.Workload.APIVersion != ro.Spec.Workload.APIVersion {
errs = append(errs,
field.Forbidden(field.NewPath("spec", "workload", "apiVersion"), "Workload apiVersion is immutable. Delete and recreate the ServiceBinding to update."),
)
}
if r.Spec.Workload.Kind != ro.Spec.Workload.Kind {
errs = append(errs,
field.Forbidden(field.NewPath("spec", "workload", "kind"), "Workload kind is immutable. Delete and recreate the ServiceBinding to update."),
)
}
}

// validate new object
errs = append(errs, r.validate()...)

return nil, errs.ToAggregate()
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
Expand Down
58 changes: 36 additions & 22 deletions controllers/servicebinding_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ import (

"github.com/vmware-labs/reconciler-runtime/apis"
"github.com/vmware-labs/reconciler-runtime/reconcilers"
corev1 "k8s.io/api/core/v1"
"github.com/vmware-labs/reconciler-runtime/tracker"
apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
ctlr "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand Down Expand Up @@ -63,13 +65,7 @@ func ResolveBindingSecret(hooks lifecycle.ServiceBindingHooks) reconcilers.SubRe
Sync: func(ctx context.Context, resource *servicebindingv1beta1.ServiceBinding) error {
c := reconcilers.RetrieveConfigOrDie(ctx)

ref := corev1.ObjectReference{
APIVersion: resource.Spec.Service.APIVersion,
Kind: resource.Spec.Service.Kind,
Namespace: resource.Namespace,
Name: resource.Spec.Service.Name,
}
secretName, err := hooks.GetResolver(TrackingClient(c)).LookupBindingSecret(ctx, ref)
secretName, err := hooks.GetResolver(TrackingClient(c)).LookupBindingSecret(ctx, resource)
if err != nil {
if apierrs.IsNotFound(err) {
// leave Unknown, the provisioned service may be created shortly
Expand Down Expand Up @@ -122,20 +118,26 @@ func ResolveWorkloads(hooks lifecycle.ServiceBindingHooks) reconcilers.SubReconc
SyncWithResult: func(ctx context.Context, resource *servicebindingv1beta1.ServiceBinding) (reconcile.Result, error) {
c := reconcilers.RetrieveConfigOrDie(ctx)

ref := corev1.ObjectReference{
APIVersion: resource.Spec.Workload.APIVersion,
Kind: resource.Spec.Workload.Kind,
Namespace: resource.Namespace,
Name: resource.Spec.Workload.Name,
trackingRef := tracker.Reference{
APIGroup: schema.FromAPIVersionAndKind(resource.Spec.Workload.APIVersion, "").Group,
Kind: resource.Spec.Workload.Kind,
Namespace: resource.Namespace,
}
workloads, err := hooks.GetResolver(TrackingClient(c)).LookupWorkloads(ctx, ref, resource.Spec.Workload.Selector)
if err != nil {
if apierrs.IsNotFound(err) {
// leave Unknown, the workload may be created shortly
resource.GetConditionManager().MarkUnknown(servicebindingv1beta1.ServiceBindingConditionWorkloadProjected, "WorkloadNotFound", "the workload was not found")
// TODO use track rather than requeue
return reconcile.Result{Requeue: true}, nil
if resource.Spec.Workload.Name != "" {
trackingRef.Name = resource.Spec.Workload.Name
}
if resource.Spec.Workload.Selector != nil {
selector, err := metav1.LabelSelectorAsSelector(resource.Spec.Workload.Selector)
if err != nil {
// should never get here
return reconcile.Result{}, err
}
trackingRef.Selector = selector
}
c.Tracker.TrackReference(trackingRef, resource)

workloads, err := hooks.GetResolver(c).LookupWorkloads(ctx, resource)
if err != nil {
if apierrs.IsForbidden(err) {
// set False, the operator needs to give access to the resource
// see https://servicebinding.io/spec/core/1.0.0/#considerations-for-role-based-access-control-rbac-1
Expand All @@ -144,12 +146,24 @@ func ResolveWorkloads(hooks lifecycle.ServiceBindingHooks) reconcilers.SubReconc
} else {
resource.GetConditionManager().MarkFalse(servicebindingv1beta1.ServiceBindingConditionWorkloadProjected, "WorkloadForbidden", "the controller does not have permission to get the workload")
}
// TODO use track rather than requeue
return reconcile.Result{Requeue: true}, nil
return reconcile.Result{}, nil
}
// TODO handle other err cases
return reconcile.Result{}, err
}
if resource.Spec.Workload.Name != "" {
found := false
for _, workload := range workloads {
if workload.(metav1.Object).GetName() == resource.Spec.Workload.Name {
found = true
break
}
}
if !found {
// leave Unknown, the workload may be created shortly
resource.GetConditionManager().MarkUnknown(servicebindingv1beta1.ServiceBindingConditionWorkloadProjected, "WorkloadNotFound", "the workload was not found")
}
}

StashWorkloads(ctx, workloads)

Expand Down
Loading

0 comments on commit a964673

Please sign in to comment.