diff --git a/pkg/project/admission/lifecycle/admission.go b/pkg/project/admission/lifecycle/admission.go index 32e742ec2f40..528b84bbee5e 100644 --- a/pkg/project/admission/lifecycle/admission.go +++ b/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) { diff --git a/pkg/project/admission/lifecycle/admission_test.go b/pkg/project/admission/lifecycle/admission_test.go index f29ebf8c7610..23ff1b581443 100644 --- a/pkg/project/admission/lifecycle/admission_test.go +++ b/pkg/project/admission/lifecycle/admission_test.go @@ -2,9 +2,7 @@ package lifecycle import ( "fmt" - "strings" "testing" - "time" "k8s.io/kubernetes/pkg/admission" kapi "k8s.io/kubernetes/pkg/api" @@ -12,19 +10,10 @@ import ( "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{} diff --git a/pkg/project/cache/cache.go b/pkg/project/cache/cache.go index e1bb12510d90..9592768d85f3 100644 --- a/pkg/project/cache/cache.go +++ b/pkg/project/cache/cache.go @@ -2,6 +2,7 @@ package cache import ( "fmt" + "time" kapi "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/client/cache" @@ -9,6 +10,7 @@ import ( "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 } diff --git a/test/integration/namespace_lifecycle_admission_test.go b/test/integration/namespace_lifecycle_admission_test.go index 092f7cc888d2..868b99e7462c 100644 --- a/test/integration/namespace_lifecycle_admission_test.go +++ b/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) + } } diff --git a/vendor/k8s.io/kubernetes/plugin/pkg/admission/namespace/lifecycle/admission.go b/vendor/k8s.io/kubernetes/plugin/pkg/admission/namespace/lifecycle/admission.go index 614f1a2db281..a2f22dd87014 100644 --- a/vendor/k8s.io/kubernetes/plugin/pkg/admission/namespace/lifecycle/admission.go +++ b/vendor/k8s.io/kubernetes/plugin/pkg/admission/namespace/lifecycle/admission.go @@ -21,6 +21,8 @@ import ( "io" "time" + "github.com/golang/glog" + clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" "k8s.io/kubernetes/pkg/admission" @@ -35,6 +37,13 @@ import ( const PluginName = "NamespaceLifecycle" +// how long to wait for a missing namespace before re-checking the cache (and then doing a live lookup) +// this accomplishes two things: +// 1. It allows a watch-fed cache time to observe a namespace creation event +// 2. It allows time for a namespace creation to distribute to members of a storage cluster, +// so the live lookup has a better chance of succeeding even if it isn't performed against the leader. +const missingNamespaceWait = 50 * time.Millisecond + func init() { admission.RegisterPlugin(PluginName, func(client clientset.Interface, config io.Reader) (admission.Interface, error) { return NewLifecycle(client, sets.NewString(api.NamespaceDefault, api.NamespaceSystem)), nil @@ -80,19 +89,28 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { return nil } - namespaceObj, exists, err := l.store.Get(&api.Namespace{ - ObjectMeta: api.ObjectMeta{ - Name: a.GetNamespace(), - Namespace: "", - }, - }) + namespaceObj, exists, err := l.store.Get(&api.Namespace{ObjectMeta: api.ObjectMeta{Name: a.GetNamespace()}}) if err != nil { return errors.NewInternalError(err) } + if !exists && a.GetOperation() == admission.Create { + // give the cache time to observe the namespace before rejecting a create. + // this helps when creating a namespace and immediately creating objects within it. + time.Sleep(missingNamespaceWait) + namespaceObj, exists, err = l.store.Get(&api.Namespace{ObjectMeta: api.ObjectMeta{Name: a.GetNamespace()}}) + if err != nil { + return errors.NewInternalError(err) + } + if exists { + glog.V(4).Infof("found %s in cache after waiting", a.GetNamespace()) + } + } + // refuse to operate on non-existent namespaces if !exists { - // in case of latency in our caches, make a call direct to storage to verify that it truly exists or not + // as a last resort, make a call directly to storage + // this also benefits from the Sleep() above allowing for propagation in HA storage cases namespaceObj, err = l.client.Core().Namespaces().Get(a.GetNamespace()) if err != nil { if errors.IsNotFound(err) { @@ -100,6 +118,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { } return errors.NewInternalError(err) } + glog.V(4).Infof("found %s via storage lookup", a.GetNamespace()) } // ensure that we're not trying to create objects in terminating namespaces