Merge pull request #70395 from seans3/expose-bug-fix

kubectl: fixes expose bug for workload without selectors

pkg/kubectl/polymorphichelpers/mapbasedselectorforobject.go

@@ -50,27 +50,52 @@ func mapBasedSelectorForObject(object runtime.Object) (string, error) {
 		return generate.MakeLabels(t.Spec.Selector), nil
 
 	case *extensionsv1beta1.Deployment:
-		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
-		// operator, DoubleEquals operator and In operator with only one element in the set.
-		if len(t.Spec.Selector.MatchExpressions) > 0 {
-			return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
+		// "extensions" deployments use pod template labels if selector is not set.
+		var labels map[string]string
+		if t.Spec.Selector != nil {
+			// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
+			// operator, DoubleEquals operator and In operator with only one element in the set.
+			if len(t.Spec.Selector.MatchExpressions) > 0 {
+				return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
+			}
+			labels = t.Spec.Selector.MatchLabels
+		} else {
+			labels = t.Spec.Template.Labels
 		}
-		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
+		if len(labels) == 0 {
+			return "", fmt.Errorf("the deployment has no labels or selectors and cannot be exposed")
+		}
+		return generate.MakeLabels(labels), nil
+
 	case *appsv1.Deployment:
+		// "apps" deployments must have the selector set.
+		if t.Spec.Selector == nil || len(t.Spec.Selector.MatchLabels) == 0 {
+			return "", fmt.Errorf("invalid deployment: no selectors, therefore cannot be exposed")
+		}
 		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
 		// operator, DoubleEquals operator and In operator with only one element in the set.
 		if len(t.Spec.Selector.MatchExpressions) > 0 {
 			return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
 		}
 		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
+
 	case *appsv1beta2.Deployment:
+		// "apps" deployments must have the selector set.
+		if t.Spec.Selector == nil || len(t.Spec.Selector.MatchLabels) == 0 {
+			return "", fmt.Errorf("invalid deployment: no selectors, therefore cannot be exposed")
+		}
 		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
 		// operator, DoubleEquals operator and In operator with only one element in the set.
 		if len(t.Spec.Selector.MatchExpressions) > 0 {
 			return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
 		}
 		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
+
 	case *appsv1beta1.Deployment:
+		// "apps" deployments must have the selector set.
+		if t.Spec.Selector == nil || len(t.Spec.Selector.MatchLabels) == 0 {
+			return "", fmt.Errorf("invalid deployment: no selectors, therefore cannot be exposed")
+		}
 		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
 		// operator, DoubleEquals operator and In operator with only one element in the set.
 		if len(t.Spec.Selector.MatchExpressions) > 0 {
@@ -79,20 +104,40 @@ func mapBasedSelectorForObject(object runtime.Object) (string, error) {
 		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
 
 	case *extensionsv1beta1.ReplicaSet:
-		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
-		// operator, DoubleEquals operator and In operator with only one element in the set.
-		if len(t.Spec.Selector.MatchExpressions) > 0 {
-			return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
+		// "extensions" replicasets use pod template labels if selector is not set.
+		var labels map[string]string
+		if t.Spec.Selector != nil {
+			// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
+			// operator, DoubleEquals operator and In operator with only one element in the set.
+			if len(t.Spec.Selector.MatchExpressions) > 0 {
+				return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
+			}
+			labels = t.Spec.Selector.MatchLabels
+		} else {
+			labels = t.Spec.Template.Labels
 		}
-		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
+		if len(labels) == 0 {
+			return "", fmt.Errorf("the replica set has no labels or selectors and cannot be exposed")
+		}
+		return generate.MakeLabels(labels), nil
+
 	case *appsv1.ReplicaSet:
+		// "apps" replicasets must have the selector set.
+		if t.Spec.Selector == nil || len(t.Spec.Selector.MatchLabels) == 0 {
+			return "", fmt.Errorf("invalid replicaset: no selectors, therefore cannot be exposed")
+		}
 		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
 		// operator, DoubleEquals operator and In operator with only one element in the set.
 		if len(t.Spec.Selector.MatchExpressions) > 0 {
 			return "", fmt.Errorf("couldn't convert expressions - \"%+v\" to map-based selector format", t.Spec.Selector.MatchExpressions)
 		}
 		return generate.MakeLabels(t.Spec.Selector.MatchLabels), nil
+
 	case *appsv1beta2.ReplicaSet:
+		// "apps" replicasets must have the selector set.
+		if t.Spec.Selector == nil || len(t.Spec.Selector.MatchLabels) == 0 {
+			return "", fmt.Errorf("invalid replicaset: no selectors, therefore cannot be exposed")
+		}
 		// TODO(madhusudancs): Make this smarter by admitting MatchExpressions with Equals
 		// operator, DoubleEquals operator and In operator with only one element in the set.
 		if len(t.Spec.Selector.MatchExpressions) > 0 {
@@ -103,4 +148,5 @@ func mapBasedSelectorForObject(object runtime.Object) (string, error) {
 	default:
 		return "", fmt.Errorf("cannot extract pod selector from %T", object)
 	}
+
 }

pkg/kubectl/polymorphichelpers/mapbasedselectorforobject_test.go

@@ -19,6 +19,9 @@ package polymorphichelpers
 import (
 	"testing"
 
+	appsv1 "k8s.io/api/apps/v1"
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	appsv1beta2 "k8s.io/api/apps/v1beta2"
 	corev1 "k8s.io/api/core/v1"
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -69,6 +72,7 @@ func TestMapBasedSelectorForObject(t *testing.T) {
 			object:    &corev1.Service{},
 			expectErr: true,
 		},
+		// extensions/v1beta1 Deployment with labels and selectors
 		{
 			object: &extensionsv1beta1.Deployment{
 				Spec: extensionsv1beta1.DeploymentSpec{
@@ -77,10 +81,33 @@ func TestMapBasedSelectorForObject(t *testing.T) {
 							"foo": "bar",
 						},
 					},
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
 				},
 			},
 			expectSelector: "foo=bar",
 		},
+		// extensions/v1beta1 Deployment with only labels (no selectors) -- use labels
+		{
+			object: &extensionsv1beta1.Deployment{
+				Spec: extensionsv1beta1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// extensions/v1beta1 Deployment with bad selector
 		{
 			object: &extensionsv1beta1.Deployment{
 				Spec: extensionsv1beta1.DeploymentSpec{
@@ -95,9 +122,17 @@ func TestMapBasedSelectorForObject(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		// apps/v1 Deployment with labels and selectors
 		{
-			object: &extensionsv1beta1.ReplicaSet{
-				Spec: extensionsv1beta1.ReplicaSetSpec{
+			object: &appsv1.Deployment{
+				Spec: appsv1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
 					Selector: &metav1.LabelSelector{
 						MatchLabels: map[string]string{
 							"foo": "bar",
@@ -107,6 +142,161 @@ func TestMapBasedSelectorForObject(t *testing.T) {
 			},
 			expectSelector: "foo=bar",
 		},
+		// apps/v1 Deployment with only labels (no selectors) -- error
+		{
+			object: &appsv1.Deployment{
+				Spec: appsv1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// apps/v1 Deployment with no labels or selectors -- error
+		{
+			object: &appsv1.Deployment{
+				Spec: appsv1.DeploymentSpec{},
+			},
+			expectErr: true,
+		},
+		// apps/v1 Deployment with empty labels -- error
+		{
+			object: &appsv1.Deployment{
+				Spec: appsv1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{}, // Empty labels map
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// apps/v1beta2 Deployment with labels and selectors
+		{
+			object: &appsv1beta2.Deployment{
+				Spec: appsv1beta2.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"foo": "bar",
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// apps/v1beta2 Deployment with only labels (no selectors) -- error
+		{
+			object: &appsv1beta2.Deployment{
+				Spec: appsv1beta2.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// apps/v1beta2 Deployment with no labels or selectors -- error
+		{
+			object: &appsv1beta2.Deployment{
+				Spec: appsv1beta2.DeploymentSpec{},
+			},
+			expectErr: true,
+		},
+		// apps/v1beta1 Deployment with labels and selectors
+		{
+			object: &appsv1beta1.Deployment{
+				Spec: appsv1beta1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"foo": "bar",
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// apps/v1beta1 Deployment with only labels (no selectors) -- error
+		{
+			object: &appsv1beta1.Deployment{
+				Spec: appsv1beta1.DeploymentSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// apps/v1beta1 Deployment with no labels or selectors -- error
+		{
+			object: &appsv1beta1.Deployment{
+				Spec: appsv1beta1.DeploymentSpec{},
+			},
+			expectErr: true,
+		},
+		// extensions/v1beta1 ReplicaSet with labels and selectors
+		{
+			object: &extensionsv1beta1.ReplicaSet{
+				Spec: extensionsv1beta1.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"foo": "bar",
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// extensions/v1beta1 ReplicaSet with only labels -- no selectors; use labels
+		{
+			object: &extensionsv1beta1.ReplicaSet{
+				Spec: extensionsv1beta1.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// extensions/v1beta1 ReplicaSet with bad label selector -- error
 		{
 			object: &extensionsv1beta1.ReplicaSet{
 				Spec: extensionsv1beta1.ReplicaSetSpec{
@@ -121,6 +311,77 @@ func TestMapBasedSelectorForObject(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		// apps/v1 ReplicaSet with labels and selectors
+		{
+			object: &appsv1.ReplicaSet{
+				Spec: appsv1.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"foo": "bar",
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// apps/v1 ReplicaSet with only labels (no selectors) -- error
+		{
+			object: &appsv1.ReplicaSet{
+				Spec: appsv1.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// apps/v1beta2 ReplicaSet with labels and selectors
+		{
+			object: &appsv1beta2.ReplicaSet{
+				Spec: appsv1beta2.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"foo": "bar",
+						},
+					},
+				},
+			},
+			expectSelector: "foo=bar",
+		},
+		// apps/v1beta2 ReplicaSet with only labels (no selectors) -- error
+		{
+			object: &appsv1beta2.ReplicaSet{
+				Spec: appsv1beta2.ReplicaSetSpec{
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{
+								"foo": "bar",
+							},
+						},
+					},
+				},
+			},
+			expectErr: true,
+		},
+		// Node can not be exposed -- error
 		{
 			object:    &corev1.Node{},
 			expectErr: true,

test/cmd/core.sh

@@ -1094,6 +1094,34 @@ run_rc_tests() {
   # Clean-up
   kubectl delete deployment/nginx-deployment "${kube_flags[@]}"
 
+  ### Expose deployments by creating a service
+  # Uses deployment selectors for created service
+  output_message=$(kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml --port 80 2>&1 "${kube_flags[@]}")
+  # Post-condition: service created for deployment.
+  kube::test::if_has_string "${output_message}" 'service/expose-test-deployment exposed'
+  # Clean-up
+  kubectl delete service/expose-test-deployment "${kube_flags[@]}"
+  # Uses deployment selectors for created service
+  output_message=$(kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment.yaml --port 80 2>&1 "${kube_flags[@]}")
+  # Post-condition: service created for deployment.
+  kube::test::if_has_string "${output_message}" 'service/expose-test-deployment exposed'
+  # Clean-up
+  kubectl delete service/expose-test-deployment "${kube_flags[@]}"
+  # Uses deployment selectors for created service
+  output_message=$(kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment.yaml --port 80 2>&1 "${kube_flags[@]}")
+  # Post-condition: service created for deployment.
+  kube::test::if_has_string "${output_message}" 'service/expose-test-deployment exposed'
+  # Clean-up
+  kubectl delete service/expose-test-deployment "${kube_flags[@]}"
+  # Contains no selectors, should fail.
+  output_message=$(! kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment-no-selectors.yaml --port 80 2>&1 "${kube_flags[@]}")
+  # Post-condition: service created for deployment.
+  kube::test::if_has_string "${output_message}" 'invalid deployment: no selectors'
+  # Contains no selectors, should fail.
+  output_message=$(! kubectl expose -f test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment-no-selectors.yaml --port 80 2>&1 "${kube_flags[@]}")
+  # Post-condition: service created for deployment.
+  kube::test::if_has_string "${output_message}" 'invalid deployment: no selectors'
+
   ### Expose a deployment as a service
   kubectl create -f test/fixtures/doc-yaml/user-guide/deployment.yaml "${kube_flags[@]}"
   # Pre-condition: 3 replicas

test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment-no-selectors.yaml

@@ -0,0 +1,18 @@
+apiVersion: apps/v1beta1
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80

test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment.yaml

@@ -0,0 +1,21 @@
+apiVersion: apps/v1beta1
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      name: nginx
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80

test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment-no-selectors.yaml

@@ -0,0 +1,18 @@
+apiVersion: apps/v1beta2
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80

test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment.yaml

@@ -0,0 +1,21 @@
+apiVersion: apps/v1beta2
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      name: nginx
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80

test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment-no-selectors.yaml

@@ -0,0 +1,18 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80

test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml

@@ -0,0 +1,21 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: expose-test-deployment
+  labels:
+    name: expose-test-deployment
+spec:
+  replicas: 3
+  selector:
+    matchLabels:
+      name: nginx
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+        ports:
+        - containerPort: 80