Skip to content

Commit

Permalink
Using discovery client to properly check for cluster-scoped resources (
Browse files Browse the repository at this point in the history
…#1319)

Summary:
- [x] proper applying and re-applying of cluster-scoped resources in the apply task
- [x] ownership is **NOT** set for cluster-scoped resources to the `Instance`

**Note**: the second change has implications, because we would previously set `ownerReference` of **all** resources (even cluster-scoped ones like CRDs) to the `Instance` and though k8s [GC docs](https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/) clearly state that _"Cross-namespace owner references are disallowed by design"_ it would still garbage collect them when the Instance was uninstalled. This is not the case anymore. Operators must utilize the `cleanup` plan to remove cluster-scoped resources.

Fixes #1288 #1265

Co-authored-by: Andreas Neumann <aneumann@mesosphere.com>
  • Loading branch information
Aleksey Dukhovniy and ANeumann82 committed Feb 11, 2020
1 parent 2fb38e4 commit ff85649
Show file tree
Hide file tree
Showing 17 changed files with 260 additions and 61 deletions.
7 changes: 6 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,15 @@ export GO111MODULE=on
.PHONY: all
all: test manager

# Run unit tests
.PHONY: test
# Run tests
test:
ifdef _INTELLIJ_FORCE_SET_GOFLAGS
# Run tests from a Goland terminal. Goland already set '-mod=readonly'
go test ./pkg/... ./cmd/... -v -coverprofile cover.out
else
go test ./pkg/... ./cmd/... -v -mod=readonly -coverprofile cover.out
endif

# Run e2e tests
.PHONY: e2e-test
Expand Down
16 changes: 12 additions & 4 deletions cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/kudobuilder/kudo/pkg/controller/instance"
"github.com/kudobuilder/kudo/pkg/controller/operator"
"github.com/kudobuilder/kudo/pkg/controller/operatorversion"
"github.com/kudobuilder/kudo/pkg/test/utils"
"github.com/kudobuilder/kudo/pkg/version"
kudohook "github.com/kudobuilder/kudo/pkg/webhook"
)
Expand Down Expand Up @@ -113,11 +114,18 @@ func main() {
}

log.Print("Setting up instance controller")
discoveryClient, err := utils.GetDiscoveryClient(mgr)
if err != nil {
log.Println(err)
os.Exit(1)
}

err = (&instance.Reconciler{
Client: mgr.GetClient(),
Config: mgr.GetConfig(),
Recorder: mgr.GetEventRecorderFor("instance-controller"),
Scheme: mgr.GetScheme(),
Client: mgr.GetClient(),
Config: mgr.GetConfig(),
Discovery: discoveryClient,
Recorder: mgr.GetEventRecorderFor("instance-controller"),
Scheme: mgr.GetScheme(),
}).SetupWithManager(mgr)
if err != nil {
log.Printf("unable to register instance controller to the manager: %v", err)
Expand Down
10 changes: 6 additions & 4 deletions pkg/controller/instance/instance_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/client-go/discovery"
"k8s.io/client-go/rest"

"github.com/thoas/go-funk"
Expand Down Expand Up @@ -60,9 +61,10 @@ const (
// Reconciler reconciles an Instance object.
type Reconciler struct {
client.Client
Config *rest.Config
Recorder record.EventRecorder
Scheme *runtime.Scheme
Discovery discovery.DiscoveryInterface
Config *rest.Config
Recorder record.EventRecorder
Scheme *runtime.Scheme
}

// SetupWithManager registers this reconciler with the controller manager
Expand Down Expand Up @@ -236,7 +238,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
return reconcile.Result{}, err
}
log.Printf("InstanceController: Going to proceed in execution of active plan %s on instance %s/%s", activePlan.Name, instance.Namespace, instance.Name)
newStatus, err := workflow.Execute(activePlan, metadata, r.Client, r.Config, &renderer.DefaultEnhancer{Scheme: r.Scheme}, time.Now())
newStatus, err := workflow.Execute(activePlan, metadata, r.Client, r.Discovery, r.Config, &renderer.DefaultEnhancer{Scheme: r.Scheme, Discovery: r.Discovery}, time.Now())

// ---------- 5. Update status of instance after the execution proceeded ----------
if newStatus != nil {
Expand Down
14 changes: 10 additions & 4 deletions pkg/controller/instance/instance_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/kudobuilder/kudo/pkg/apis"
"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/engine"
"github.com/kudobuilder/kudo/pkg/test/utils"
"github.com/kudobuilder/kudo/pkg/util/kudo"
)

Expand Down Expand Up @@ -125,11 +126,16 @@ func instancePlanFinished(key client.ObjectKey, planName string, c client.Client
func startTestManager(t *testing.T) (chan struct{}, *sync.WaitGroup, client.Client) {
mgr, err := manager.New(cfg, manager.Options{})
assert.Nil(t, err, "Error when creating manager")

discoveryClient, err := utils.GetDiscoveryClient(mgr)
assert.NoError(t, err, "Error when creating discovery client")

err = (&Reconciler{
Client: mgr.GetClient(),
Config: mgr.GetConfig(),
Recorder: mgr.GetEventRecorderFor("instance-controller"),
Scheme: mgr.GetScheme(),
Client: mgr.GetClient(),
Discovery: discoveryClient,
Config: mgr.GetConfig(),
Recorder: mgr.GetEventRecorderFor("instance-controller"),
Scheme: mgr.GetScheme(),
}).SetupWithManager(mgr)

stop := make(chan struct{})
Expand Down
26 changes: 20 additions & 6 deletions pkg/engine/renderer/enhancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import (
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/discovery"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

"github.com/kudobuilder/kudo/pkg/engine/resource"
"github.com/kudobuilder/kudo/pkg/util/kudo"
)

Expand All @@ -22,12 +24,13 @@ type Enhancer interface {

// DefaultEnhancer is implementation of Enhancer that applies the defined conventions by directly editing runtime.Objects (Unstructured).
type DefaultEnhancer struct {
Scheme *runtime.Scheme
Scheme *runtime.Scheme
Discovery discovery.DiscoveryInterface
}

// Apply accepts templates to be rendered in kubernetes and enhances them with our own KUDO conventions
// These include the way we name our objects and what labels we apply to them
func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata) (objsToAdd []runtime.Object, err error) {
func (de *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata) (objsToAdd []runtime.Object, err error) {
objs := make([]runtime.Object, 0, len(templates))

for _, v := range templates {
Expand All @@ -45,14 +48,25 @@ func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
return nil, fmt.Errorf("adding labels on parsed object: %v", err)
}
if err = addAnnotations(unstructMap, metadata); err != nil {
return nil, fmt.Errorf("adding annotations on parsed object: %v", err)
return nil, fmt.Errorf("adding annotations on parsed object %s: %v", obj.GetObjectKind(), err)
}

objUnstructured := &unstructured.Unstructured{Object: unstructMap}

objUnstructured.SetNamespace(metadata.InstanceNamespace)
if err = setControllerReference(metadata.ResourcesOwner, objUnstructured, k.Scheme); err != nil {
return nil, fmt.Errorf("setting controller reference on parsed object: %v", err)
isNamespaced, err := resource.IsNamespacedObject(obj, de.Discovery)
if err != nil {
return nil, fmt.Errorf("failed to determine if object %s is namespaced: %v", obj.GetObjectKind(), err)
}

// Note: Cross-namespace owner references are disallowed by design. This means:
// 1) Namespace-scoped dependents can only specify owners in the same namespace, and owners that are cluster-scoped.
// 2) Cluster-scoped dependents can only specify cluster-scoped owners, but not namespace-scoped owners.
// More: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/
if isNamespaced {
objUnstructured.SetNamespace(metadata.InstanceNamespace)
if err = setControllerReference(metadata.ResourcesOwner, objUnstructured, de.Scheme); err != nil {
return nil, fmt.Errorf("setting controller reference on parsed object %s: %v", obj.GetObjectKind(), err)
}
}

// This is pretty important, if we don't convert it back to the original type everything will be Unstructured.
Expand Down
9 changes: 6 additions & 3 deletions pkg/engine/renderer/enhancer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ func TestEnhancerApply_embeddedMetadataStatefulSet(t *testing.T) {
meta := metadata()

e := &DefaultEnhancer{
Scheme: utils.Scheme(),
Scheme: utils.Scheme(),
Discovery: utils.FakeDiscoveryClient(),
}

objs, err := e.Apply(tpls, meta)
Expand Down Expand Up @@ -74,7 +75,8 @@ func TestEnhancerApply_embeddedMetadataCronjob(t *testing.T) {
meta := metadata()

e := &DefaultEnhancer{
Scheme: utils.Scheme(),
Scheme: utils.Scheme(),
Discovery: utils.FakeDiscoveryClient(),
}

objs, err := e.Apply(tpls, meta)
Expand Down Expand Up @@ -115,7 +117,8 @@ func TestEnhancerApply_noAdditionalMetadata(t *testing.T) {
meta := metadata()

e := &DefaultEnhancer{
Scheme: utils.Scheme(),
Scheme: utils.Scheme(),
Discovery: utils.FakeDiscoveryClient(),
}

objs, err := e.Apply(tpls, meta)
Expand Down
62 changes: 62 additions & 0 deletions pkg/engine/resource/object_key.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package resource

import (
"fmt"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/discovery"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// ObjectKeyFromObject method wraps client.ObjectKeyFromObject method by additionally checking if passed object is
// a cluster-scoped resource (e.g. CustomResourceDefinition, ClusterRole etc.) and removing the namespace from the
// key since cluster-scoped resources are not namespaced.
func ObjectKeyFromObject(r runtime.Object, di discovery.DiscoveryInterface) (client.ObjectKey, error) {
key, err := client.ObjectKeyFromObject(r)
if err != nil {
return client.ObjectKey{}, fmt.Errorf("failed to get an object key from object %v: %v", r.GetObjectKind(), err)
}

// if the resource is cluster-scoped we need to clear then namespace from the key
isNamespaced, err := IsNamespacedObject(r, di)
if err != nil {
return client.ObjectKey{}, fmt.Errorf("failed to determine if the resource %v is cluster-scoped: %v", r.GetObjectKind(), err)
}

if !isNamespaced {
key.Namespace = ""
}
return key, nil
}

func IsNamespacedObject(r runtime.Object, di discovery.DiscoveryInterface) (bool, error) {
gvk := r.GetObjectKind().GroupVersionKind()
return isNamespaced(gvk, di)
}

// isNamespaced method return true if given runtime.Object is a namespaced (not cluster-scoped) resource. It uses the
// discovery client to fetch all API resources (with Groups and Versions), searches for a resource with the passed GVK
// and returns true if it's namespaced. Method returns an error if passed GVK wasn't found in the discovered resource list.
func isNamespaced(gvk schema.GroupVersionKind, di discovery.DiscoveryInterface) (bool, error) {
// Fetch namespaced API resources
_, apiResources, err := di.ServerGroupsAndResources()
if err != nil {
return false, fmt.Errorf("failed to fetch server groups and resources: %v", err)
}

for _, rr := range apiResources {
gv, err := schema.ParseGroupVersion(rr.GroupVersion)
if err != nil {
continue
}
for _, r := range rr.APIResources {
if gvk == gv.WithKind(r.Kind) {
return r.Namespaced, nil
}
//log.Printf("[%s], Name: %s: %v", gvk, r.Name, r.Namespaced)
}
}

return false, fmt.Errorf("a resource with GVK %v seems to be missing in API resource list", gvk)
}
61 changes: 61 additions & 0 deletions pkg/engine/resource/object_key_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package resource

import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/discovery"

"github.com/kudobuilder/kudo/pkg/test/utils"
)

func Test_isNamespaced(t *testing.T) {
fdc := utils.FakeDiscoveryClient()

tests := []struct {
name string
gvk schema.GroupVersionKind
di discovery.DiscoveryInterface
want bool
wantErr bool
}{
{
name: "pod",
gvk: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"},
di: fdc,
want: true,
wantErr: false,
},
{
name: "namespace",
gvk: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"},
di: fdc,
want: false,
wantErr: false,
},
{
name: "customresourcedefinition",
gvk: schema.GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1beta1", Kind: "CustomResourceDefinition"},
di: fdc,
want: false,
wantErr: false,
},
{
name: "fake",
gvk: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Fake"},
di: fdc,
want: false,
wantErr: true,
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
got, err := isNamespaced(tt.gvk, tt.di)

assert.True(t, (err != nil) == tt.wantErr)
assert.Equal(t, tt.want, got)
})
}
}
2 changes: 2 additions & 0 deletions pkg/engine/task/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"regexp"

"k8s.io/client-go/discovery"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand All @@ -16,6 +17,7 @@ import (
// Context is a engine.task execution context containing k8s client, templates parameters etc.
type Context struct {
Client client.Client
Discovery discovery.DiscoveryInterface
Config *rest.Config
Enhancer renderer.Enhancer
Meta renderer.Metadata
Expand Down
Loading

0 comments on commit ff85649

Please sign in to comment.