Merge pull request #66807 from caesarxuchao/remove-connectrequest

pkg/registry/core/node/rest/proxy.go

@@ -45,9 +45,9 @@ var _ = rest.Connecter(&ProxyREST{})
 
 var proxyMethods = []string{"GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OPTIONS"}
 
-// New returns an empty service resource
+// New returns an empty nodeProxyOptions object.
 func (r *ProxyREST) New() runtime.Object {
-	return &api.Node{}
+	return &api.NodeProxyOptions{}
 }
 
 // ConnectMethods returns the list of HTTP methods that can be proxied

pkg/registry/core/pod/rest/subresources.go

@@ -46,9 +46,9 @@ var _ = rest.Connecter(&ProxyREST{})
 
 var proxyMethods = []string{"GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OPTIONS"}
 
-// New returns an empty pod resource
+// New returns an empty podProxyOptions object.
 func (r *ProxyREST) New() runtime.Object {
-	return &api.Pod{}
+	return &api.PodProxyOptions{}
 }
 
 // ConnectMethods returns the list of HTTP methods that can be proxied
@@ -88,9 +88,9 @@ type AttachREST struct {
 // Implement Connecter
 var _ = rest.Connecter(&AttachREST{})
 
-// New creates a new Pod object
+// New creates a new podAttachOptions object.
 func (r *AttachREST) New() runtime.Object {
-	return &api.Pod{}
+	return &api.PodAttachOptions{}
 }
 
 // Connect returns a handler for the pod exec proxy
@@ -125,9 +125,9 @@ type ExecREST struct {
 // Implement Connecter
 var _ = rest.Connecter(&ExecREST{})
 
-// New creates a new Pod object
+// New creates a new podExecOptions object.
 func (r *ExecREST) New() runtime.Object {
-	return &api.Pod{}
+	return &api.PodExecOptions{}
 }
 
 // Connect returns a handler for the pod exec proxy
@@ -162,9 +162,9 @@ type PortForwardREST struct {
 // Implement Connecter
 var _ = rest.Connecter(&PortForwardREST{})
 
-// New returns an empty pod object
+// New returns an empty podPortForwardOptions object
 func (r *PortForwardREST) New() runtime.Object {
-	return &api.Pod{}
+	return &api.PodPortForwardOptions{}
 }
 
 // NewConnectOptions returns the versioned object that represents the

pkg/registry/core/service/proxy.go

@@ -43,7 +43,7 @@ var proxyMethods = []string{"GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OP
 
 // New returns an empty service resource
 func (r *ProxyREST) New() runtime.Object {
-	return &api.Service{}
+	return &api.ServiceProxyOptions{}
 }
 
 // ConnectMethods returns the list of HTTP methods that can be proxied

plugin/pkg/admission/exec/BUILD

@@ -14,10 +14,8 @@ go_library(
         "//pkg/apis/core:go_default_library",
         "//pkg/client/clientset_generated/internalclientset:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",
-        "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library",
     ],
 )
 
@@ -31,7 +29,6 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library",
         "//staging/src/k8s.io/client-go/testing:go_default_library",
     ],
 )

plugin/pkg/admission/exec/admission.go

@@ -20,10 +20,8 @@ import (
 	"fmt"
 	"io"
 
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/admission"
-	"k8s.io/apiserver/pkg/registry/rest"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
@@ -94,15 +92,15 @@ func NewDenyExecOnPrivileged() *DenyExec {
 
 // Validate makes an admission decision based on the request attributes
 func (d *DenyExec) Validate(a admission.Attributes) (err error) {
-	connectRequest, ok := a.GetObject().(*rest.ConnectRequest)
-	if !ok {
-		return errors.NewBadRequest("a connect request was received, but could not convert the request object.")
+	path := a.GetResource().Resource
+	if subresource := a.GetSubresource(); subresource != "" {
+		path = path + "/" + subresource
 	}
 	// Only handle exec or attach requests on pods
-	if connectRequest.ResourcePath != "pods/exec" && connectRequest.ResourcePath != "pods/attach" {
+	if path != "pods/exec" && path != "pods/attach" {
 		return nil
 	}
-	pod, err := d.client.Core().Pods(a.GetNamespace()).Get(connectRequest.Name, metav1.GetOptions{})
+	pod, err := d.client.Core().Pods(a.GetNamespace()).Get(a.GetName(), metav1.GetOptions{})
 	if err != nil {
 		return admission.NewForbidden(a, err)
 	}

plugin/pkg/admission/exec/admission_test.go

@@ -22,7 +22,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/admission"
-	"k8s.io/apiserver/pkg/registry/rest"
 	core "k8s.io/client-go/testing"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
@@ -122,8 +121,7 @@ func testAdmission(t *testing.T, pod *api.Pod, handler *DenyExec, shouldAccept b
 
 	// pods/exec
 	{
-		req := &rest.ConnectRequest{Name: pod.Name, ResourcePath: "pods/exec"}
-		err := handler.Validate(admission.NewAttributesRecord(req, nil, api.Kind("Pod").WithVersion("version"), "test", "name", api.Resource("pods").WithVersion("version"), "exec", admission.Connect, false, nil))
+		err := handler.Validate(admission.NewAttributesRecord(nil, nil, api.Kind("Pod").WithVersion("version"), "test", pod.Name, api.Resource("pods").WithVersion("version"), "exec", admission.Connect, false, nil))
 		if shouldAccept && err != nil {
 			t.Errorf("Unexpected error returned from admission handler: %v", err)
 		}
@@ -134,8 +132,7 @@ func testAdmission(t *testing.T, pod *api.Pod, handler *DenyExec, shouldAccept b
 
 	// pods/attach
 	{
-		req := &rest.ConnectRequest{Name: pod.Name, ResourcePath: "pods/attach"}
-		err := handler.Validate(admission.NewAttributesRecord(req, nil, api.Kind("Pod").WithVersion("version"), "test", "name", api.Resource("pods").WithVersion("version"), "attach", admission.Connect, false, nil))
+		err := handler.Validate(admission.NewAttributesRecord(nil, nil, api.Kind("Pod").WithVersion("version"), "test", pod.Name, api.Resource("pods").WithVersion("version"), "attach", admission.Connect, false, nil))
 		if shouldAccept && err != nil {
 			t.Errorf("Unexpected error returned from admission handler: %v", err)
 		}

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -120,22 +120,17 @@ func ConnectResource(connecter rest.Connecter, scope RequestScope, admit admissi
 			return
 		}
 		if admit != nil && admit.Handles(admission.Connect) {
-			connectRequest := &rest.ConnectRequest{
-				Name:         name,
-				Options:      opts,
-				ResourcePath: restPath,
-			}
 			userInfo, _ := request.UserFrom(ctx)
 			// TODO: remove the mutating admission here as soon as we have ported all plugin that handle CONNECT
 			if mutatingAdmission, ok := admit.(admission.MutationInterface); ok {
-				err = mutatingAdmission.Admit(admission.NewAttributesRecord(connectRequest, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Connect, false, userInfo))
+				err = mutatingAdmission.Admit(admission.NewAttributesRecord(opts, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Connect, false, userInfo))
 				if err != nil {
 					scope.err(err, w, req)
 					return
 				}
 			}
 			if validatingAdmission, ok := admit.(admission.ValidationInterface); ok {
-				err = validatingAdmission.Validate(admission.NewAttributesRecord(connectRequest, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Connect, false, userInfo))
+				err = validatingAdmission.Validate(admission.NewAttributesRecord(opts, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Connect, false, userInfo))
 				if err != nil {
 					scope.err(err, w, req)
 					return

staging/src/k8s.io/apiserver/pkg/registry/rest/BUILD

@@ -29,7 +29,6 @@ go_library(
         "rest.go",
         "table.go",
         "update.go",
-        "zz_generated.deepcopy.go",
     ],
     importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest",
     importpath = "k8s.io/apiserver/pkg/registry/rest",

staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go

@@ -334,19 +334,3 @@ type StorageMetadata interface {
 	// it is not nil. Only the type of the return object matters, the value will be ignored.
 	ProducesObject(verb string) interface{}
 }
-
-// +k8s:deepcopy-gen=true
-// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
-// ConnectRequest is an object passed to admission control for Connect operations
-type ConnectRequest struct {
-	// Name is the name of the object on which the connect request was made
-	Name string
-
-	// Options is the options object passed to the connect request. See the NewConnectOptions method on Connecter
-	Options runtime.Object
-
-	// ResourcePath is the path for the resource in the REST server (ie. "pods/proxy")
-	ResourcePath string
-}
-
-func (obj *ConnectRequest) GetObjectKind() schema.ObjectKind { return schema.EmptyObjectKind }

staging/src/k8s.io/apiserver/pkg/registry/rest/zz_generated.deepcopy.go

-// +build !ignore_autogenerated
-
-/*
-Copyright The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-// Code generated by deepcopy-gen. DO NOT EDIT.
-
-package rest
-
-import (
-	runtime "k8s.io/apimachinery/pkg/runtime"
-)
-
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *ConnectRequest) DeepCopyInto(out *ConnectRequest) {
-	*out = *in
-	if in.Options != nil {
-		out.Options = in.Options.DeepCopyObject()
-	}
-	return
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ConnectRequest.
-func (in *ConnectRequest) DeepCopy() *ConnectRequest {
-	if in == nil {
-		return nil
-	}
-	out := new(ConnectRequest)
-	in.DeepCopyInto(out)
-	return out
-}
-
-// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object.
-func (in *ConnectRequest) DeepCopyObject() runtime.Object {
-	if c := in.DeepCopy(); c != nil {
-		return c
-	}
-	return nil
-}

test/e2e/apimachinery/webhook.go

@@ -52,21 +52,23 @@ const (
 	roleBindingName = "webhook-auth-reader"
 
 	// The webhook configuration names should not be reused between test instances.
-	crWebhookConfigName          = "e2e-test-webhook-config-cr"
-	webhookConfigName            = "e2e-test-webhook-config"
-	mutatingWebhookConfigName    = "e2e-test-mutating-webhook-config"
-	podMutatingWebhookConfigName = "e2e-test-mutating-webhook-pod"
-	crMutatingWebhookConfigName  = "e2e-test-mutating-webhook-config-cr"
-	webhookFailClosedConfigName  = "e2e-test-webhook-fail-closed"
-	webhookForWebhooksConfigName = "e2e-test-webhook-for-webhooks-config"
-	removableValidatingHookName  = "e2e-test-should-be-removable-validating-webhook-config"
-	removableMutatingHookName    = "e2e-test-should-be-removable-mutating-webhook-config"
-	crdWebhookConfigName         = "e2e-test-webhook-config-crd"
+	crWebhookConfigName           = "e2e-test-webhook-config-cr"
+	webhookConfigName             = "e2e-test-webhook-config"
+	attachingPodWebhookConfigName = "e2e-test-webhook-config-attaching-pod"
+	mutatingWebhookConfigName     = "e2e-test-mutating-webhook-config"
+	podMutatingWebhookConfigName  = "e2e-test-mutating-webhook-pod"
+	crMutatingWebhookConfigName   = "e2e-test-mutating-webhook-config-cr"
+	webhookFailClosedConfigName   = "e2e-test-webhook-fail-closed"
+	webhookForWebhooksConfigName  = "e2e-test-webhook-for-webhooks-config"
+	removableValidatingHookName   = "e2e-test-should-be-removable-validating-webhook-config"
+	removableMutatingHookName     = "e2e-test-should-be-removable-mutating-webhook-config"
+	crdWebhookConfigName          = "e2e-test-webhook-config-crd"
 
 	skipNamespaceLabelKey   = "skip-webhook-admission"
 	skipNamespaceLabelValue = "yes"
 	skippedNamespaceName    = "exempted-namesapce"
 	disallowedPodName       = "disallowed-pod"
+	toBeAttachedPodName     = "to-be-attached-pod"
 	hangingPodName          = "hanging-pod"
 	disallowedConfigMapName = "disallowed-configmap"
 	allowedConfigMapName    = "allowed-configmap"
@@ -117,6 +119,12 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		testWebhook(f)
 	})
 
+	It("Should be able to deny attaching pod", func() {
+		webhookCleanup := registerWebhookForAttachingPod(f, context)
+		defer webhookCleanup()
+		testAttachingPodWebhook(f)
+	})
+
 	It("Should be able to deny custom resource creation", func() {
 		testcrd, err := framework.CreateTestCRD(f)
 		if err != nil {
@@ -405,6 +413,53 @@ func registerWebhook(f *framework.Framework, context *certContext) func() {
 	}
 }
 
+func registerWebhookForAttachingPod(f *framework.Framework, context *certContext) func() {
+	client := f.ClientSet
+	By("Registering the webhook via the AdmissionRegistration API")
+
+	namespace := f.Namespace.Name
+	configName := attachingPodWebhookConfigName
+	// A webhook that cannot talk to server, with fail-open policy
+	failOpenHook := failingWebhook(namespace, "fail-open.k8s.io")
+	policyIgnore := v1beta1.Ignore
+	failOpenHook.FailurePolicy = &policyIgnore
+
+	_, err := client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Create(&v1beta1.ValidatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: configName,
+		},
+		Webhooks: []v1beta1.Webhook{
+			{
+				Name: "deny-attaching-pod.k8s.io",
+				Rules: []v1beta1.RuleWithOperations{{
+					Operations: []v1beta1.OperationType{v1beta1.Connect},
+					Rule: v1beta1.Rule{
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"pods/attach"},
+					},
+				}},
+				ClientConfig: v1beta1.WebhookClientConfig{
+					Service: &v1beta1.ServiceReference{
+						Namespace: namespace,
+						Name:      serviceName,
+						Path:      strPtr("/pods/attach"),
+					},
+					CABundle: context.signingCert,
+				},
+			},
+		},
+	})
+	framework.ExpectNoError(err, "registering webhook config %s with namespace %s", configName, namespace)
+
+	// The webhook configuration is honored in 10s.
+	time.Sleep(10 * time.Second)
+
+	return func() {
+		client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Delete(configName, nil)
+	}
+}
+
 func registerMutatingWebhookForConfigMap(f *framework.Framework, context *certContext) func() {
 	client := f.ClientSet
 	By("Registering the mutating configmap webhook via the AdmissionRegistration API")
@@ -642,6 +697,21 @@ func testWebhook(f *framework.Framework) {
 	Expect(err).To(BeNil())
 }
 
+func testAttachingPodWebhook(f *framework.Framework) {
+	By("create a pod")
+	client := f.ClientSet
+	pod := toBeAttachedPod(f)
+	_, err := client.CoreV1().Pods(f.Namespace.Name).Create(pod)
+	Expect(err).To(BeNil())
+
+	By("'kubectl attach' the pod, should be denied by the webhook")
+	_, err = framework.NewKubectlCommand("attach", fmt.Sprintf("--namespace=%v", f.Namespace.Name), pod.Name, "-i", "-c=container1").Exec()
+	Expect(err).NotTo(BeNil())
+	if e, a := "attaching to pod 'to-be-attached-pod' is not allowed", err.Error(); !strings.Contains(a, e) {
+		framework.Failf("unexpected 'kubectl attach' error message. expected to contain %q, got %q", e, a)
+	}
+}
+
 // failingWebhook returns a webhook with rule of create configmaps,
 // but with an invalid client config so that server cannot communicate with it
 func failingWebhook(namespace, name string) v1beta1.Webhook {
@@ -930,6 +1000,22 @@ func hangingPod(f *framework.Framework) *v1.Pod {
 	}
 }
 
+func toBeAttachedPod(f *framework.Framework) *v1.Pod {
+	return &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: toBeAttachedPodName,
+		},
+		Spec: v1.PodSpec{
+			Containers: []v1.Container{
+				{
+					Name:  "container1",
+					Image: imageutils.GetPauseImageName(),
+				},
+			},
+		},
+	}
+}
+
 func nonCompliantConfigMap(f *framework.Framework) *v1.ConfigMap {
 	return &v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{

test/images/webhook/VERSION

-1.12v1
+1.12v2

test/images/webhook/main.go

@@ -60,7 +60,7 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) {
 		return
 	}
 
-	glog.V(2).Info(fmt.Sprintf("handling request: %v", body))
+	glog.V(2).Info(fmt.Sprintf("handling request: %s", body))
 
 	// The AdmissionReview that was sent to the webhook
 	requestedAdmissionReview := v1beta1.AdmissionReview{}
@@ -99,6 +99,10 @@ func servePods(w http.ResponseWriter, r *http.Request) {
 	serve(w, r, admitPods)
 }
 
+func serveAttachingPods(w http.ResponseWriter, r *http.Request) {
+	serve(w, r, denySpecificAttachment)
+}
+
 func serveMutatePods(w http.ResponseWriter, r *http.Request) {
 	serve(w, r, mutatePods)
 }
@@ -130,6 +134,7 @@ func main() {
 
 	http.HandleFunc("/always-deny", serveAlwaysDeny)
 	http.HandleFunc("/pods", servePods)
+	http.HandleFunc("/pods/attach", serveAttachingPods)
 	http.HandleFunc("/mutating-pods", serveMutatePods)
 	http.HandleFunc("/configmaps", serveConfigmaps)
 	http.HandleFunc("/mutating-configmaps", serveMutateConfigmaps)

test/images/webhook/pods.go

@@ -101,3 +101,41 @@ func mutatePods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
 	}
 	return &reviewResponse
 }
+
+// denySpecificAttachment denies `kubectl attach to-be-attached-pod -i -c=container1"
+// or equivalent client requests.
+func denySpecificAttachment(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
+	glog.V(2).Info("handling attaching pods")
+	if ar.Request.Name != "to-be-attached-pod" {
+		return &v1beta1.AdmissionResponse{Allowed: true}
+	}
+	podResource := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}
+	if e, a := podResource, ar.Request.Resource; e != a {
+		err := fmt.Errorf("expect resource to be %s, got %s", e, a)
+		glog.Error(err)
+		return toAdmissionResponse(err)
+	}
+	if e, a := "attach", ar.Request.SubResource; e != a {
+		err := fmt.Errorf("expect subresource to be %s, got %s", e, a)
+		glog.Error(err)
+		return toAdmissionResponse(err)
+	}
+
+	raw := ar.Request.Object.Raw
+	podAttachOptions := corev1.PodAttachOptions{}
+	deserializer := codecs.UniversalDeserializer()
+	if _, _, err := deserializer.Decode(raw, nil, &podAttachOptions); err != nil {
+		glog.Error(err)
+		return toAdmissionResponse(err)
+	}
+	glog.V(2).Info(fmt.Sprintf("podAttachOptions=%#v\n", podAttachOptions))
+	if !podAttachOptions.Stdin || podAttachOptions.Container != "container1" {
+		return &v1beta1.AdmissionResponse{Allowed: true}
+	}
+	return &v1beta1.AdmissionResponse{
+		Allowed: false,
+		Result: &metav1.Status{
+			Message: "attaching to pod 'to-be-attached-pod' is not allowed",
+		},
+	}
+}

test/utils/image/manifest.go

@@ -49,7 +49,7 @@ func (i *ImageConfig) SetVersion(version string) {
 }
 
 var (
-	AdmissionWebhook         = ImageConfig{e2eRegistry, "webhook", "1.12v1", false}
+	AdmissionWebhook         = ImageConfig{e2eRegistry, "webhook", "1.12v2", false}
 	APIServer                = ImageConfig{e2eRegistry, "sample-apiserver", "1.0", false}
 	AppArmorLoader           = ImageConfig{gcRegistry, "apparmor-loader", "0.1", false}
 	BusyBox                  = ImageConfig{dockerHubRegistry, "busybox", "1.29", false}