From d5865240c5bd8b9a05918d3c490426268e9dd38d Mon Sep 17 00:00:00 2001 From: Sean Sullivan Date: Mon, 29 Oct 2018 16:42:21 -0700 Subject: [PATCH] kubectl: fixes expose bug for workload without selectors --- .../mapbasedselectorforobject.go | 66 ++++- .../mapbasedselectorforobject_test.go | 265 +++++++++++++++++- test/cmd/core.sh | 28 ++ .../appsv1beta1deployment-no-selectors.yaml | 18 ++ .../cmd/expose/appsv1beta1deployment.yaml | 21 ++ .../appsv1beta2deployment-no-selectors.yaml | 18 ++ .../cmd/expose/appsv1beta2deployment.yaml | 21 ++ .../expose/appsv1deployment-no-selectors.yaml | 18 ++ .../kubectl/cmd/expose/appsv1deployment.yaml | 21 ++ 9 files changed, 464 insertions(+), 12 deletions(-) create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment-no-selectors.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment-no-selectors.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment-no-selectors.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml diff --git a/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject.go b/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject.go index 422302629e5..3548edf2390 100644 --- a/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject.go +++ b/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) } + } diff --git a/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject_test.go b/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject_test.go index 8734f0c5578..1ca8d1c7a94 100644 --- a/pkg/kubectl/polymorphichelpers/mapbasedselectorforobject_test.go +++ b/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, diff --git a/test/cmd/core.sh b/test/cmd/core.sh index 549703fa0ee..0300e3bb1c0 100755 --- a/test/cmd/core.sh +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment-no-selectors.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment-no-selectors.yaml new file mode 100644 index 00000000000..b721f0b6110 --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta1deployment.yaml new file mode 100644 index 00000000000..dc4f0189290 --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment-no-selectors.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment-no-selectors.yaml new file mode 100644 index 00000000000..321d8414e46 --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1beta2deployment.yaml new file mode 100644 index 00000000000..9ed0aec2261 --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment-no-selectors.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment-no-selectors.yaml new file mode 100644 index 00000000000..5b0c84423bc --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml b/test/fixtures/pkg/kubectl/cmd/expose/appsv1deployment.yaml new file mode 100644 index 00000000000..8852cfe1bb4 --- /dev/null +++ b/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