Merge pull request #10932 from liggitt/namespace-admission-wait-1.3.1

compensate for raft/cache delay in namespace admission

Author: smarterclayton

pkg/project/admission/lifecycle/admission.go

@@ -4,14 +4,11 @@ import (
 	"fmt"
 	"io"
 	"math/rand"
-	"strings"
 	"time"
 
 	"github.com/golang/glog"
 
 	"k8s.io/kubernetes/pkg/admission"
-	kapi "k8s.io/kubernetes/pkg/api"
-	apierrors "k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/meta"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
@@ -68,17 +65,6 @@ func (e *lifecycle) Admit(a admission.Attributes) (err error) {
 		return nil
 	}
 
-	// we want to allow someone to delete something in case it was phantom created somehow
-	if a.GetOperation() == "DELETE" {
-		return nil
-	}
-
-	name := "Unknown"
-	obj := a.GetObject()
-	if obj != nil {
-		name, _ = meta.NewAccessor().Name(obj)
-	}
-
 	if !e.cache.Running() {
 		return admission.NewForbidden(a, err)
 	}
@@ -88,14 +74,6 @@ func (e *lifecycle) Admit(a admission.Attributes) (err error) {
 		return admission.NewForbidden(a, err)
 	}
 
-	if a.GetOperation() != "CREATE" {
-		return nil
-	}
-
-	if namespace.Status.Phase == kapi.NamespaceTerminating && !e.creatableResources.Has(strings.ToLower(a.GetResource().Resource)) {
-		return apierrors.NewForbidden(a.GetResource().GroupResource(), name, fmt.Errorf("Namespace %s is terminating", a.GetNamespace()))
-	}
-
 	// in case of concurrency issues, we will retry this logic
 	numRetries := 10
 	interval := time.Duration(rand.Int63n(90)+int64(10)) * time.Millisecond
@@ -125,7 +103,7 @@ func (e *lifecycle) Admit(a admission.Attributes) (err error) {
 }
 
 func (e *lifecycle) Handles(operation admission.Operation) bool {
-	return true
+	return operation == admission.Create
 }
 
 func (e *lifecycle) SetProjectCache(c *cache.ProjectCache) {

pkg/project/admission/lifecycle/admission_test.go

@@ -2,29 +2,18 @@ package lifecycle
 
 import (
 	"fmt"
-	"strings"
 	"testing"
-	"time"
 
 	"k8s.io/kubernetes/pkg/admission"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/client/cache"
 	clientsetfake "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
 	"k8s.io/kubernetes/pkg/client/unversioned/testclient"
-	genericapiserveroptions "k8s.io/kubernetes/pkg/genericapiserver/options"
-	kubeletclient "k8s.io/kubernetes/pkg/kubelet/client"
 	"k8s.io/kubernetes/pkg/runtime"
-	etcdstorage "k8s.io/kubernetes/pkg/storage/etcd"
-	"k8s.io/kubernetes/pkg/util/sets"
 
 	buildapi "github.com/openshift/origin/pkg/build/api"
-	otestclient "github.com/openshift/origin/pkg/client/testclient"
-	"github.com/openshift/origin/pkg/cmd/server/origin"
-	"github.com/openshift/origin/pkg/controller/shared"
 	projectcache "github.com/openshift/origin/pkg/project/cache"
-	"github.com/openshift/origin/pkg/quota/controller/clusterquotamapping"
-	"github.com/openshift/origin/pkg/util/restoptions"
 
 	// install all APIs
 	_ "github.com/openshift/origin/pkg/api/install"
@@ -90,105 +79,6 @@ func TestAdmissionExists(t *testing.T) {
 	}
 }
 
-// TestAdmissionLifecycle verifies you cannot create Origin content if namespace is terminating
-func TestAdmissionLifecycle(t *testing.T) {
-	namespaceObj := &kapi.Namespace{
-		ObjectMeta: kapi.ObjectMeta{
-			Name:      "test",
-			Namespace: "",
-		},
-		Status: kapi.NamespaceStatus{
-			Phase: kapi.NamespaceActive,
-		},
-	}
-	store := projectcache.NewCacheStore(cache.IndexFuncToKeyFuncAdapter(cache.MetaNamespaceIndexFunc))
-	store.Add(namespaceObj)
-	mockClient := &testclient.Fake{}
-	cache := projectcache.NewFake(mockClient.Namespaces(), store, "")
-
-	mockClientset := clientsetfake.NewSimpleClientset(namespaceObj)
-	handler := &lifecycle{client: mockClientset}
-	handler.SetProjectCache(cache)
-	build := &buildapi.Build{
-		ObjectMeta: kapi.ObjectMeta{Name: "buildid", Namespace: "other"},
-		Spec: buildapi.BuildSpec{
-			CommonSpec: buildapi.CommonSpec{
-				Source: buildapi.BuildSource{
-					Git: &buildapi.GitBuildSource{
-						URI: "http://github.com/my/repository",
-					},
-					ContextDir: "context",
-				},
-				Strategy: buildapi.BuildStrategy{
-					DockerStrategy: &buildapi.DockerBuildStrategy{},
-				},
-				Output: buildapi.BuildOutput{
-					To: &kapi.ObjectReference{
-						Kind: "DockerImage",
-						Name: "repository/data",
-					},
-				},
-			},
-		},
-		Status: buildapi.BuildStatus{
-			Phase: buildapi.BuildPhaseNew,
-		},
-	}
-	err := handler.Admit(admission.NewAttributesRecord(build, nil, kapi.Kind("Build").WithVersion("version"), build.Namespace, "name", kapi.Resource("builds").WithVersion("version"), "", "CREATE", nil))
-	if err != nil {
-		t.Errorf("Unexpected error returned from admission handler: %v", err)
-	}
-
-	// change namespace state to terminating
-	namespaceObj.Status.Phase = kapi.NamespaceTerminating
-	store.Add(namespaceObj)
-
-	// verify create operations in the namespace cause an error
-	err = handler.Admit(admission.NewAttributesRecord(build, nil, kapi.Kind("Build").WithVersion("version"), build.Namespace, "name", kapi.Resource("builds").WithVersion("version"), "", "CREATE", nil))
-	if err == nil {
-		t.Errorf("Expected error rejecting creates in a namespace when it is terminating")
-	}
-
-	// verify update operations in the namespace can proceed
-	err = handler.Admit(admission.NewAttributesRecord(build, build, kapi.Kind("Build").WithVersion("version"), build.Namespace, "name", kapi.Resource("builds").WithVersion("version"), "", "UPDATE", nil))
-	if err != nil {
-		t.Errorf("Unexpected error returned from admission handler: %v", err)
-	}
-
-	// verify delete operations in the namespace can proceed
-	err = handler.Admit(admission.NewAttributesRecord(nil, nil, kapi.Kind("Build").WithVersion("version"), build.Namespace, "name", kapi.Resource("builds").WithVersion("version"), "", "DELETE", nil))
-	if err != nil {
-		t.Errorf("Unexpected error returned from admission handler: %v", err)
-	}
-
-}
-
-// TestCreatesAllowedDuringNamespaceDeletion checks to make sure that the resources in the whitelist are allowed
-func TestCreatesAllowedDuringNamespaceDeletion(t *testing.T) {
-	etcdHelper := etcdstorage.NewEtcdStorage(nil, kapi.Codecs.LegacyCodec(), "", false, genericapiserveroptions.DefaultDeserializationCacheSize)
-
-	informerFactory := shared.NewInformerFactory(testclient.NewSimpleFake(), otestclient.NewSimpleFake(), shared.DefaultListerWatcherOverrides{}, 1*time.Second)
-	config := &origin.MasterConfig{
-		KubeletClientConfig:           &kubeletclient.KubeletClientConfig{},
-		RESTOptionsGetter:             restoptions.NewSimpleGetter(etcdHelper),
-		EtcdHelper:                    etcdHelper,
-		Informers:                     informerFactory,
-		ClusterQuotaMappingController: clusterquotamapping.NewClusterQuotaMappingController(informerFactory.Namespaces(), informerFactory.ClusterResourceQuotas()),
-	}
-	storageMap := config.GetRestStorage()
-	resources := sets.String{}
-
-	for resource := range storageMap {
-		resources.Insert(strings.ToLower(resource))
-	}
-
-	for resource := range recommendedCreatableResources {
-		if !resources.Has(resource) {
-			t.Errorf("recommendedCreatableResources has resource %v, but that resource isn't registered.", resource)
-		}
-	}
-}
-
 func TestSAR(t *testing.T) {
 	store := projectcache.NewCacheStore(cache.IndexFuncToKeyFuncAdapter(cache.MetaNamespaceIndexFunc))
 	mockClient := &testclient.Fake{}

pkg/project/cache/cache.go

@@ -2,13 +2,15 @@ package cache
 
 import (
 	"fmt"
+	"time"
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/cache"
 	client "k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/watch"
 
+	"github.com/golang/glog"
 	projectapi "github.com/openshift/origin/pkg/project/api"
 	"github.com/openshift/origin/pkg/util/labelselector"
 )
@@ -28,18 +30,26 @@ type ProjectCache struct {
 }
 
 func (p *ProjectCache) GetNamespace(name string) (*kapi.Namespace, error) {
+	key := &kapi.Namespace{ObjectMeta: kapi.ObjectMeta{Name: name}}
+
 	// check for namespace in the cache
-	namespaceObj, exists, err := p.Store.Get(&kapi.Namespace{
-		ObjectMeta: kapi.ObjectMeta{
-			Name:      name,
-			Namespace: "",
-		},
-		Status: kapi.NamespaceStatus{},
-	})
+	namespaceObj, exists, err := p.Store.Get(key)
 	if err != nil {
 		return nil, err
 	}
 
+	if !exists {
+		// give the cache time to observe a recent namespace creation
+		time.Sleep(50 * time.Millisecond)
+		namespaceObj, exists, err = p.Store.Get(key)
+		if err != nil {
+			return nil, err
+		}
+		if exists {
+			glog.V(4).Infof("found %s in cache after waiting", name)
+		}
+	}
+
 	var namespace *kapi.Namespace
 	if exists {
 		namespace = namespaceObj.(*kapi.Namespace)
@@ -50,6 +60,7 @@ func (p *ProjectCache) GetNamespace(name string) (*kapi.Namespace, error) {
 		if err != nil {
 			return nil, fmt.Errorf("namespace %s does not exist", name)
 		}
+		glog.V(4).Infof("found %s via storage lookup", name)
 	}
 	return namespace, nil
 }

test/integration/namespace_lifecycle_admission_test.go

@@ -1,8 +1,13 @@
 package integration
 
 import (
+	"strings"
 	"testing"
 
+	kapi "k8s.io/kubernetes/pkg/api"
+
+	"github.com/openshift/origin/pkg/project/api"
+	routeapi "github.com/openshift/origin/pkg/route/api"
 	testutil "github.com/openshift/origin/test/util"
 	testserver "github.com/openshift/origin/test/util/server"
 )
@@ -14,14 +19,81 @@ func TestNamespaceLifecycleAdmission(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	clusterAdminClient, err := testutil.GetClusterAdminKubeClient(clusterAdminKubeConfig)
+	clusterAdminClient, err := testutil.GetClusterAdminClient(clusterAdminKubeConfig)
+	if err != nil {
+		t.Fatal(err)
+	}
+	clusterAdminKubeClient, err := testutil.GetClusterAdminKubeClient(clusterAdminKubeConfig)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	for _, ns := range []string{"default", "openshift", "openshift-infra"} {
-		if err := clusterAdminClient.Namespaces().Delete(ns); err == nil {
+		if err := clusterAdminKubeClient.Namespaces().Delete(ns); err == nil {
 			t.Fatalf("expected error deleting %q namespace, got none", ns)
 		}
 	}
+
+	// Create a namespace directly (not via a project)
+	ns := &kapi.Namespace{ObjectMeta: kapi.ObjectMeta{Name: "test"}}
+	ns, err = clusterAdminKubeClient.Namespaces().Create(ns)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ns.Spec.Finalizers) == 0 {
+		t.Fatal("expected at least one finalizer")
+	}
+	found := false
+	for _, f := range ns.Spec.Finalizers {
+		if f == api.FinalizerOrigin {
+			found = true
+			break
+		}
+	}
+	if found {
+		t.Fatalf("didn't expect origin finalizer to be present, got %#v", ns.Spec.Finalizers)
+	}
+
+	// Create an origin object
+	route := &routeapi.Route{
+		ObjectMeta: kapi.ObjectMeta{Name: "route"},
+		Spec:       routeapi.RouteSpec{To: routeapi.RouteTargetReference{Kind: "Service", Name: "test"}},
+	}
+	route, err = clusterAdminClient.Routes(ns.Name).Create(route)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Ensure the origin finalizer is added
+	ns, err = clusterAdminKubeClient.Namespaces().Get(ns.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+	found = false
+	for _, f := range ns.Spec.Finalizers {
+		if f == api.FinalizerOrigin {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Fatalf("expected origin finalizer, got %#v", ns.Spec.Finalizers)
+	}
+
+	// Delete the namespace
+	// We don't have to worry about racing the namespace deletion controller because we've only started the master
+	err = clusterAdminKubeClient.Namespaces().Delete(ns.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Try to create an origin object in a terminating namespace and ensure it is forbidden
+	route = &routeapi.Route{
+		ObjectMeta: kapi.ObjectMeta{Name: "route2"},
+		Spec:       routeapi.RouteSpec{To: routeapi.RouteTargetReference{Kind: "Service", Name: "test"}},
+	}
+	_, err = clusterAdminClient.Routes(ns.Name).Create(route)
+	if err == nil || !strings.Contains(err.Error(), "it is being terminated") {
+		t.Fatalf("Expected forbidden error because of a terminating namespace, got %v", err)
+	}
 }