Merge pull request #85 from FabianKramm/refactor

Helm Update & Fix for potential race condition

Dockerfile

@@ -4,7 +4,7 @@ FROM golang:1.13 as builder
 WORKDIR /workspace
 
 # Install Helm 3
-RUN bash -c "curl -s https://get.helm.sh/helm-v3.2.3-linux-amd64.tar.gz > helm3.tar.gz" && tar -zxvf helm3.tar.gz linux-amd64/helm && chmod +x linux-amd64/helm && mv linux-amd64/helm /workspace/helm && rm helm3.tar.gz && rm -R linux-amd64
+RUN bash -c "curl -s https://get.helm.sh/helm-v3.3.1-linux-amd64.tar.gz > helm3.tar.gz" && tar -zxvf helm3.tar.gz linux-amd64/helm && chmod +x linux-amd64/helm && mv linux-amd64/helm /workspace/helm && rm helm3.tar.gz && rm -R linux-amd64
 
 # Copy the Go Modules manifests
 COPY go.mod go.mod

cmd/kiosk/main.go

@@ -117,8 +117,14 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Inject the client and scheme
-	injectClient(mgr.GetClient(), scheme)
+	// create an uncached client for api routes
+	uncachedClient, err := client2.New(mgr.GetConfig(), client2.Options{
+		Scheme: mgr.GetScheme(),
+		Mapper: mgr.GetRESTMapper(),
+	})
+
+	// Inject the cached, uncached client and scheme
+	injectClient(mgr.GetClient(), uncachedClient, scheme)
 
 	// Add required indices
 	err = controllers.AddManagerIndices(mgr.GetCache())
@@ -221,7 +227,8 @@ func initialize(config *rest.Config) error {
 	return err
 }
 
-func injectClient(client client2.Client, scheme *runtime.Scheme) {
-	tenancy.Client = client
+func injectClient(cachedClient client2.Client, uncachedClient client2.Client, scheme *runtime.Scheme) {
+	tenancy.CachedClient = cachedClient
+	tenancy.UncachedClient = uncachedClient
 	tenancy.Scheme = scheme
 }

gen/cmd/apis/template/unversioned_generator.go

@@ -61,7 +61,7 @@ func (d *unversionedGenerator) Finalize(context *generator.Context, w io.Writer)
 }
 
 var UnversionedAPITemplate = `
-type NewRESTFunc func(client client.Client, scheme *runtime.Scheme) rest.Storage
+type NewRESTFunc func(cachedClient client.Client, uncachedClient client.Client, scheme *runtime.Scheme) rest.Storage
 
 var (
 	{{ range $api := .UnversionedResources -}}
@@ -73,7 +73,7 @@ var (
 			New{{ $api.REST }},
 		)
 		New{{ $api.REST }} = func(getter generic.RESTOptionsGetter) rest.Storage {
-			return New{{ $api.REST }}Func(Client, Scheme)
+			return New{{ $api.REST }}Func(CachedClient, UncachedClient, Scheme)
 		}
 		New{{ $api.REST }}Func NewRESTFunc
 	{{ else -}}
@@ -112,7 +112,7 @@ var (
 		func() runtime.Object { return &{{$subresource.Request}}{} },
 	)
 	New{{$subresource.Kind}}REST = func(getter generic.RESTOptionsGetter) rest.Storage {
-		return New{{$subresource.Kind}}RESTFunc(Client, Scheme)
+		return New{{$subresource.Kind}}RESTFunc(CachedClient, UncachedClient, Scheme)
 	}
 	New{{$subresource.Kind}}RESTFunc NewRESTFunc
 	{{ end -}}

hack/generate.sh

@@ -4,12 +4,11 @@ set -e
 
 echo "Generate apis..."
 
-GO111MODULE=on
+GO111MODULE=off
 go run gen/cmd/apis/main.go --input-dirs github.com/kiosk-sh/kiosk/pkg/apis/... --go-header-file ./hack/boilerplate.go.txt
 
 echo "Generate conversion, deepcopy, defaulter, openapi, client, lister & informers ..."
 
-GO111MODULE=off
 conversion-gen --input-dirs github.com/kiosk-sh/kiosk/pkg/apis/... -o $GOPATH/src --go-header-file ./hack/boilerplate.go.txt -O zz_generated.conversion --extra-peer-dirs k8s.io/apimachinery/pkg/apis/meta/v1,k8s.io/apimachinery/pkg/conversion,k8s.io/apimachinery/pkg/runtime
 deepcopy-gen --input-dirs github.com/kiosk-sh/kiosk/pkg/apis/... -o $GOPATH/src --go-header-file ./hack/boilerplate.go.txt -O zz_generated.deepcopy
 openapi-gen --input-dirs github.com/kiosk-sh/kiosk/pkg/apis/... -o $GOPATH/src --go-header-file ./hack/boilerplate.go.txt -i k8s.io/apimachinery/pkg/apis/meta/v1,k8s.io/apimachinery/pkg/api/resource,k8s.io/apimachinery/pkg/version,k8s.io/apimachinery/pkg/runtime,k8s.io/apimachinery/pkg/util/intstr,k8s.io/api/admission/v1,k8s.io/api/admission/v1beta1,k8s.io/api/admissionregistration/v1,k8s.io/api/admissionregistration/v1beta1,k8s.io/api/apps/v1,k8s.io/api/apps/v1beta1,k8s.io/api/apps/v1beta2,k8s.io/api/auditregistration/v1alpha1,k8s.io/api/authentication/v1,k8s.io/api/authentication/v1beta1,k8s.io/api/authorization/v1,k8s.io/api/authorization/v1beta1,k8s.io/api/autoscaling/v1,k8s.io/api/autoscaling/v2beta1,k8s.io/api/autoscaling/v2beta2,k8s.io/api/batch/v1,k8s.io/api/batch/v1beta1,k8s.io/api/batch/v2alpha1,k8s.io/api/certificates/v1beta1,k8s.io/api/coordination/v1,k8s.io/api/coordination/v1beta1,k8s.io/api/core/v1,k8s.io/api/discovery/v1alpha1,k8s.io/api/events/v1beta1,k8s.io/api/extensions/v1beta1,k8s.io/api/networking/v1,k8s.io/api/networking/v1beta1,k8s.io/api/node/v1alpha1,k8s.io/api/node/v1beta1,k8s.io/api/policy/v1beta1,k8s.io/api/rbac/v1,k8s.io/api/rbac/v1alpha1,k8s.io/api/rbac/v1beta1,k8s.io/api/scheduling/v1,k8s.io/api/scheduling/v1alpha1,k8s.io/api/scheduling/v1beta1,k8s.io/api/settings/v1alpha1,k8s.io/api/storage/v1,k8s.io/api/storage/v1alpha1,k8s.io/api/storage/v1beta1,k8s.io/client-go/pkg/apis/clientauthentication/v1alpha1,k8s.io/client-go/pkg/apis/clientauthentication/v1beta1,k8s.io/api/core/v1 --report-filename violations.report --output-package github.com/kiosk-sh/kiosk/pkg/openapi

pkg/apis/tenancy/inject.go

@@ -8,5 +8,8 @@ import (
 // Scheme will be injected during startup and then passed to the rest storages
 var Scheme *runtime.Scheme
 
-// Client will be injected during startup and then passed to the rest storages
-var Client client.Client
+// CachedClient will be injected during startup and then passed to the rest storages
+var CachedClient client.Client
+
+// UncachedClient will be injected during startup and then passed to the rest storages
+var UncachedClient client.Client

pkg/apiserver/registry/account/rest.go

@@ -40,11 +40,11 @@ type accountREST struct {
 }
 
 // NewAccountREST creates a new account storage that implements the rest interface
-func NewAccountREST(client client.Client, scheme *runtime.Scheme) rest.Storage {
-	ruleClient := authorization.NewRuleClient(client)
+func NewAccountREST(cachedClient client.Client, uncachedClient client.Client, scheme *runtime.Scheme) rest.Storage {
+	ruleClient := authorization.NewRuleClient(cachedClient)
 	return &accountREST{
-		client: client,
-		filter: authorization.NewFilteredLister(client, rbac.New(ruleClient, ruleClient, ruleClient, ruleClient)),
+		client: uncachedClient,
+		filter: authorization.NewFilteredLister(uncachedClient, rbac.New(ruleClient, ruleClient, ruleClient, ruleClient)),
 	}
 }
 

pkg/apiserver/registry/account/rest_test.go

@@ -22,7 +22,7 @@ import (
 
 var clusterBinding = &rbacv1.ClusterRoleBinding{
 	ObjectMeta: metav1.ObjectMeta{
-		Name:            "test",
+		Name: "test",
 	},
 	Subjects: []rbacv1.Subject{
 		{
@@ -61,7 +61,7 @@ func TestGetAccount(t *testing.T) {
 	})
 	ctx := context.TODO()
 	userCtx := request.WithUser(ctx, &user.DefaultInfo{Name: "foo"})
-	accountStorage := NewAccountREST(fakeClient, scheme).(*accountREST)
+	accountStorage := NewAccountREST(fakeClient, fakeClient, scheme).(*accountREST)
 	test, err := accountStorage.Get(userCtx, "test", &metav1.GetOptions{})
 	if err != nil {
 		t.Fatal(err)
@@ -98,8 +98,8 @@ func TestListAccount(t *testing.T) {
 			},
 		}, &rbacv1.ClusterRole{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            "test",
-				UID:             "123",
+				Name: "test",
+				UID:  "123",
 			},
 			Rules: []rbacv1.PolicyRule{
 				{
@@ -114,7 +114,7 @@ func TestListAccount(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := withRequestInfo(request.WithUser(ctx, &user.DefaultInfo{Name: "foo"}), "list", "")
-	accountStorage := NewAccountREST(fakeClient, scheme).(*accountREST)
+	accountStorage := NewAccountREST(fakeClient, fakeClient, scheme).(*accountREST)
 
 	// Get empty list
 	obj, err := accountStorage.List(userCtx, &metainternalversion.ListOptions{})
@@ -173,7 +173,7 @@ func TestCreateAccount(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := request.WithUser(ctx, &user.DefaultInfo{Name: "foo"})
-	accountStorage := NewAccountREST(fakeClient, scheme).(*accountREST)
+	accountStorage := NewAccountREST(fakeClient, fakeClient, scheme).(*accountREST)
 
 	// Allow us to create the account
 	// fakeAuthCache.UserAccounts["foo"] = []string{"*"}
@@ -213,11 +213,11 @@ func TestAccountUpdate(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := request.WithUser(ctx, &user.DefaultInfo{Name: "foo"})
-	accountStorage := NewAccountREST(fakeClient, scheme).(*accountREST)
+	accountStorage := NewAccountREST(fakeClient, fakeClient, scheme).(*accountREST)
 
 	newObj, updated, err := accountStorage.Update(userCtx, "test", &fakeUpdater{out: &tenancy.Account{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:            "test",
+			Name: "test",
 			Labels: map[string]string{
 				"Updated": "true",
 			},
@@ -245,7 +245,7 @@ func TestAccountDelete(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := request.WithUser(ctx, &user.DefaultInfo{Name: "foo"})
-	accountStorage := NewAccountREST(fakeClient, scheme).(*accountREST)
+	accountStorage := NewAccountREST(fakeClient, fakeClient, scheme).(*accountREST)
 
 	_, deleted, err := accountStorage.Delete(userCtx, "test", fakeDeleteValidation, &metav1.DeleteOptions{})
 	if err != nil || deleted == false {

pkg/apiserver/registry/space/rest.go

@@ -52,14 +52,14 @@ type spaceStorage struct {
 }
 
 // NewSpaceREST creates a new space storage that implements the rest interface
-func NewSpaceREST(client client.Client, scheme *runtime.Scheme) rest.Storage {
-	ruleClient := authorization.NewRuleClient(client)
+func NewSpaceREST(cachedClient client.Client, uncachedClient client.Client, scheme *runtime.Scheme) rest.Storage {
+	ruleClient := authorization.NewRuleClient(cachedClient)
 	authorizer := rbac.New(ruleClient, ruleClient, ruleClient, ruleClient)
 	return &spaceStorage{
-		client:     client,
+		client:     uncachedClient,
 		authorizer: authorizer,
 		scheme:     scheme,
-		filter:     authorization.NewFilteredLister(client, authorizer),
+		filter:     authorization.NewFilteredLister(uncachedClient, authorizer),
 	}
 }
 
@@ -240,7 +240,7 @@ func (r *spaceStorage) Create(ctx context.Context, obj runtime.Object, createVal
 	if account != nil {
 		if account.Spec.Space.Limit != nil {
 			namespaceList := &corev1.NamespaceList{}
-			err := r.client.List(ctx, namespaceList, client.MatchingFields{constants.IndexByAccount: account.Name})
+			err := r.client.List(ctx, namespaceList, client.MatchingLabels{constants.SpaceLabelAccount: account.Name})
 			if err != nil {
 				return nil, err
 			}
@@ -295,12 +295,6 @@ func (r *spaceStorage) Create(ctx context.Context, obj runtime.Object, createVal
 		}
 	}
 
-	// Wait till we have the namespace in the cache
-	err := r.waitForAccess(ctx, namespace.Name)
-	if err != nil {
-		return nil, err
-	}
-
 	return ConvertNamespace(namespace), nil
 }
 
@@ -379,26 +373,6 @@ func (r *spaceStorage) initializeSpace(ctx context.Context, namespace *corev1.Na
 	return r.client.Patch(ctx, namespace, client.MergeFrom(originalNamespace))
 }
 
-// waitForAccess blocks until the namespace is created and in our cache
-func (r *spaceStorage) waitForAccess(ctx context.Context, namespace string) error {
-	backoff := retry.DefaultBackoff
-	backoff.Steps = 6 // this effectively waits for 6-ish seconds
-	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
-		err := r.client.Get(ctx, types.NamespacedName{Name: namespace}, &corev1.Namespace{})
-		if err != nil {
-			if kerrors.IsNotFound(err) {
-				return false, nil
-			}
-
-			return false, err
-		}
-
-		return true, nil
-	})
-
-	return err
-}
-
 var _ = rest.Updater(&spaceStorage{})
 var _ = rest.CreaterUpdater(&spaceStorage{})
 

pkg/apiserver/registry/space/rest_test.go

@@ -82,7 +82,7 @@ func TestGetSpace(t *testing.T) {
 	})
 	ctx := context.TODO()
 	userCtx := request.WithUser(ctx, &user.DefaultInfo{Name: "foo"})
-	spaceStorage := NewSpaceREST(fakeClient, scheme).(*spaceStorage)
+	spaceStorage := NewSpaceREST(fakeClient, fakeClient, scheme).(*spaceStorage)
 
 	// We are not allowed to retrieve it so this should return a not found
 	_, err := spaceStorage.Get(withRequestInfo(userCtx, "get", "test"), "test", &metav1.GetOptions{})
@@ -134,7 +134,7 @@ func TestListSpaces(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := withRequestInfo(request.WithUser(ctx, &user.DefaultInfo{Name: "foo"}), "list", "")
-	spaceStorage := NewSpaceREST(fakeClient, scheme).(*spaceStorage)
+	spaceStorage := NewSpaceREST(fakeClient, fakeClient, scheme).(*spaceStorage)
 
 	// Get empty list
 	obj, err := spaceStorage.List(userCtx, &metainternalversion.ListOptions{})
@@ -231,7 +231,7 @@ func TestCreateSpace(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := withRequestInfo(request.WithUser(ctx, &user.DefaultInfo{Name: "foo"}), "create", "")
-	spaceStorage := NewSpaceREST(fakeClient, scheme).(*spaceStorage)
+	spaceStorage := NewSpaceREST(fakeClient, fakeClient, scheme).(*spaceStorage)
 
 	// Try to create if we are not allowed to
 	_, err := spaceStorage.Create(userCtx, &tenancy.Space{
@@ -322,7 +322,7 @@ func TestSpaceUpdate(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := withRequestInfo(request.WithUser(ctx, &user.DefaultInfo{Name: "foo"}), "update", "test")
-	spaceStorage := NewSpaceREST(fakeClient, scheme).(*spaceStorage)
+	spaceStorage := NewSpaceREST(fakeClient, fakeClient, scheme).(*spaceStorage)
 
 	_, updated, err := spaceStorage.Update(userCtx, "test", &fakeUpdater{out: &tenancy.Space{
 		ObjectMeta: metav1.ObjectMeta{
@@ -369,7 +369,7 @@ func TestSpaceDelete(t *testing.T) {
 		})
 	ctx := context.TODO()
 	userCtx := withRequestInfo(request.WithUser(ctx, &user.DefaultInfo{Name: "foo"}), "delete", "test")
-	spaceStorage := NewSpaceREST(fakeClient, scheme).(*spaceStorage)
+	spaceStorage := NewSpaceREST(fakeClient, fakeClient, scheme).(*spaceStorage)
 
 	_, deleted, err := spaceStorage.Delete(userCtx, "test", fakeDeleteValidation, &metav1.DeleteOptions{})
 	if err == nil || kerrors.IsForbidden(err) == false || deleted == true {

pkg/apiserver/registry/util/helper.go

@@ -1,14 +1,12 @@
 package util
 
 import (
-	"context"
 	"fmt"
 	configv1alpha1 "github.com/kiosk-sh/kiosk/pkg/apis/config/v1alpha1"
 	"github.com/kiosk-sh/kiosk/pkg/constants"
 	"github.com/kiosk-sh/kiosk/pkg/util/subject"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
-	client2 "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func IsUserPartOfAccount(userInfo user.Info, account *configv1alpha1.Account) bool {
@@ -27,28 +25,6 @@ func IsUserPartOfAccount(userInfo user.Info, account *configv1alpha1.Account) bo
 	return false
 }
 
-func GetAccountsByUserInfo(ctx context.Context, client client2.Client, userInfo user.Info) ([]*configv1alpha1.Account, error) {
-	subjects := []string{constants.UserPrefix + userInfo.GetName()}
-	for _, group := range userInfo.GetGroups() {
-		subjects = append(subjects, constants.GroupPrefix+group)
-	}
-
-	retList := []*configv1alpha1.Account{}
-	accList := &configv1alpha1.AccountList{}
-	for _, subject := range subjects {
-		err := client.List(ctx, accList, client2.MatchingFields{constants.IndexBySubjects: subject})
-		if err != nil {
-			return nil, err
-		}
-		for _, acc := range accList.Items {
-			c := acc
-			retList = append(retList, &c)
-		}
-	}
-
-	return retList, nil
-}
-
 func ForbiddenMessage(attributes authorizer.Attributes) string {
 	username := ""
 	if user := attributes.GetUser(); user != nil {

pkg/authorization/filter.go

@@ -35,14 +35,15 @@ func (f *filter) List(ctx context.Context, list runtime.Object, groupVersionReso
 		return nil, err
 	}
 
-	err = f.client.List(ctx, list, &client.ListOptions{
+	listOptions := &client.ListOptions{
 		LabelSelector: options.LabelSelector,
-		// TODO: support this, since it is currently not supported by the underlying cache implementation
-		// FieldSelector: options.FieldSelector,
-		Namespace: a.GetNamespace(),
-		Limit:     options.Limit,
-		Continue:  options.Continue,
-	})
+		FieldSelector: options.FieldSelector,
+		Namespace:     a.GetNamespace(),
+		Limit:         options.Limit,
+		Continue:      options.Continue,
+	}
+
+	err = f.client.List(ctx, list, listOptions)
 	if err != nil {
 		return nil, err
 	}