feat(controller): don't hardcode experiment ports; always create service

docs/features/experiment.md

@@ -6,6 +6,8 @@ The Experiment CRD allows users to have ephemeral runs of one or more ReplicaSet
 running ephemeral ReplicaSets, the Experiment CRD can launch AnalysisRuns alongside the ReplicaSets.
 Generally, those AnalysisRun is used to confirm that new ReplicaSets are running as expected.
 
+A Service routing traffic to the Experiment ReplicaSet is also generated.
+
 ## Use cases of Experiments
 
 - A user wants to run two versions of an application for a specific duration to enable Kayenta-style
@@ -243,5 +245,9 @@ to `experiment-baseline`, leaving the remaining 90% of traffic to the old stack.
 
 !!! note
     When a weighted experiment step with traffic routing is used, a
-    service is auto-created for each experiment template. The traffic routers use
-    this service to send traffic to the experiment pods.
+    Service is auto-created for each experiment template. The traffic routers use
+    this service to send traffic to the experiment pods.
+
+By default, the generated Service has the name of the ReplicaSet and inherits
+ports and selector from the specRef definition. It can be accessed in using the `{{templates.baseline.replicaset.name}}`
+or `{{templates.canary.replicaset.name}}` variables respectively.

docs/features/specification.md

@@ -328,6 +328,8 @@ spec:
             specRef: stable
           - name: canary
             specRef: canary
+            # optional, set the weight of traffic routed to this version
+            weight: 10
           analyses:
           - name : mann-whitney
             templateName: mann-whitney

experiments/experiment.go

@@ -22,6 +22,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/kubernetes"
 	appslisters "k8s.io/client-go/listers/apps/v1"
 	v1 "k8s.io/client-go/listers/core/v1"
@@ -285,8 +286,19 @@ func (ec *experimentContext) createTemplateService(template *v1alpha1.TemplateSp
 	// Create service with has same name, podTemplateHash, and labels as RS
 	podTemplateHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
 	svc := ec.templateServices[template.Name]
+    var ports []corev1.ServicePort
+    for _, ctr := range rs.Spec.Template.Spec.Containers {
+        for _, port := range ctr.Ports {
+            servicePort := corev1.ServicePort{
+                Protocol:   port.Protocol,
+                Port:       port.ContainerPort,
+                TargetPort: intstr.FromInt(int(port.ContainerPort)),
+            }
+            ports = append(ports, servicePort)
+        }
+    }
 	if svc == nil || svc.Name != rs.Name {
-		newService, err := ec.CreateService(rs.Name, *template, rs.Labels)
+		newService, err := ec.CreateService(rs.Name, *template, rs.Labels, ports)
 		if err != nil {
 			templateStatus.Status = v1alpha1.TemplateStatusError
 			templateStatus.Message = fmt.Sprintf("Failed to create Service for template '%s': %v", template.Name, err)

experiments/experiment_test.go

@@ -496,3 +496,26 @@ func TestDeleteServiceIfServiceFieldNil(t *testing.T) {
 	assert.Equal(t, "", exStatus.TemplateStatuses[0].ServiceName)
 	assert.Nil(t, exCtx.templateServices["bar"])
 }
+
+func TestServiceInheritPortsFromRS(t *testing.T) {
+	templates := generateTemplates("bar")
+	templates[0].Service = &v1alpha1.TemplateService{}
+	templates[0].Template.Spec.Containers[0].Ports = []corev1.ContainerPort{
+		{
+			Name:          "testport",
+			ContainerPort: 80,
+			Protocol:      "TCP",
+		},
+	}
+	ex := newExperiment("foo", templates, "")
+
+	exCtx := newTestContext(ex)
+	rs := templateToRS(ex, templates[0], 0)
+	exCtx.templateRSs["bar"] = rs
+
+	exCtx.reconcile()
+
+	assert.NotNil(t, exCtx.templateServices["bar"])
+	assert.Equal(t, exCtx.templateServices["bar"].Name, "foo-bar")
+	assert.Equal(t, exCtx.templateServices["bar"].Spec.Ports[0].Port, int32(80))
+}

experiments/service.go

@@ -12,7 +12,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
 var experimentKind = v1alpha1.SchemeGroupVersion.WithKind("Experiment")
@@ -59,9 +58,9 @@ func GetServiceForExperiment(experiment *v1alpha1.Experiment, svc *corev1.Servic
 	return nil
 }
 
-func (ec *experimentContext) CreateService(serviceName string, template v1alpha1.TemplateSpec, selector map[string]string) (*corev1.Service, error) {
+func (ec *experimentContext) CreateService(serviceName string, template v1alpha1.TemplateSpec, selector map[string]string, ports []corev1.ServicePort) (*corev1.Service, error) {
 	ctx := context.TODO()
-	serviceAnnotations := newServiceSetAnnotations(ec.ex.Name, template.Name)
+	serviceAnnotations := newServiceAnnotations(ec.ex.Name, template.Name)
 	newService := &corev1.Service{
 		TypeMeta: metav1.TypeMeta{},
 		ObjectMeta: metav1.ObjectMeta{
@@ -73,12 +72,8 @@ func (ec *experimentContext) CreateService(serviceName string, template v1alpha1
 			Annotations: serviceAnnotations,
 		},
 		Spec: corev1.ServiceSpec{
-			Selector: selector,
-			Ports: []corev1.ServicePort{{
-				Protocol:   "TCP",
-				Port:       int32(80),
-				TargetPort: intstr.FromInt(8080),
-			}},
+		    Selector: selector,
+		    Ports: ports,
 		},
 	}
 
@@ -112,7 +107,7 @@ func (ec *experimentContext) deleteService(service corev1.Service) error {
 	return nil
 }
 
-func newServiceSetAnnotations(experimentName, templateName string) map[string]string {
+func newServiceAnnotations(experimentName, templateName string) map[string]string {
 	return map[string]string{
 		v1alpha1.ExperimentNameAnnotationKey:         experimentName,
 		v1alpha1.ExperimentTemplateNameAnnotationKey: templateName,

rollout/experiment.go

@@ -64,9 +64,7 @@ func GetExperimentFromTemplate(r *v1alpha1.Rollout, stableRS, newRS *appsv1.Repl
 			Name:     templateStep.Name,
 			Replicas: templateStep.Replicas,
 		}
-		if templateStep.Weight != nil {
-			template.Service = &v1alpha1.TemplateService{}
-		}
+        template.Service = &v1alpha1.TemplateService{}
 		templateRS := &appsv1.ReplicaSet{}
 		switch templateStep.SpecRef {
 		case v1alpha1.CanarySpecRef:

rollout/experiment_test.go

@@ -791,7 +791,7 @@ func TestRolloutCreateExperimentWithService(t *testing.T) {
 					Replicas: pointer.Int32Ptr(1),
 					Weight:   pointer.Int32Ptr(5),
 				},
-				// Service should NOT be created for "canary-template"
+				// Service should also be created for "canary-template"
 				{
 					Name:     "canary-template",
 					SpecRef:  v1alpha1.CanarySpecRef,
@@ -818,5 +818,46 @@ func TestRolloutCreateExperimentWithService(t *testing.T) {
 	assert.NotNil(t, ex.Spec.Templates[0].Service)
 
 	assert.Equal(t, "canary-template", ex.Spec.Templates[1].Name)
-	assert.Nil(t, ex.Spec.Templates[1].Service)
+	assert.NotNil(t, ex.Spec.Templates[1].Service)
+}
+
+func TestRolloutCreateExperimentWithServicePorts(t *testing.T) {
+	steps := []v1alpha1.CanaryStep{{
+		Experiment: &v1alpha1.RolloutExperimentStep{
+			Templates: []v1alpha1.RolloutExperimentTemplate{
+				// Service should be created for "stable-template"
+				{
+					Name:     "stable-template",
+					SpecRef:  v1alpha1.StableSpecRef,
+					Replicas: pointer.Int32Ptr(1),
+					Weight:   pointer.Int32Ptr(5),
+				},
+				// Service should also be created for "canary-template"
+				{
+					Name:     "canary-template",
+					SpecRef:  v1alpha1.CanarySpecRef,
+					Replicas: pointer.Int32Ptr(1),
+				},
+			},
+		},
+	}}
+
+	r1 := newCanaryRollout("foo", 1, nil, steps, pointer.Int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1))
+	r2 := bumpVersion(r1)
+
+	rs1 := newReplicaSetWithStatus(r1, 1, 1)
+	rs2 := newReplicaSetWithStatus(r2, 1, 1)
+	rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+	r2.Status.CurrentStepIndex = pointer.Int32Ptr(0)
+	r2.Status.StableRS = rs1PodHash
+
+	ex, err := GetExperimentFromTemplate(r2, rs1, rs2)
+	assert.Nil(t, err)
+
+	assert.Equal(t, "stable-template", ex.Spec.Templates[0].Name)
+	assert.NotNil(t, ex.Spec.Templates[0].Service)
+
+	assert.Equal(t, "canary-template", ex.Spec.Templates[1].Name)
+	assert.NotNil(t, ex.Spec.Templates[1].Service)
 }

test/e2e/functional/experiment-dry-run-analysis.yaml

@@ -28,7 +28,6 @@ spec:
   templates:
   - name: baseline
     replicas: 1
-    service: {}
     selector:
       matchLabels:
         app: experiment-with-dry-run

test/e2e/functional/experiment-measurement-retention-analysis.yaml

@@ -31,7 +31,6 @@ spec:
   templates:
   - name: baseline
     replicas: 1
-    service: {}
     selector:
       matchLabels:
         app: experiment-with-mr

test/e2e/functional/experiment-with-service.yaml

@@ -9,7 +9,11 @@ spec:
   templates:
     - name: test
       replicas: 1
-      service: {}
+      service:
+        ports:
+          - protocol: TCP
+            port: 80
+            targetPort: 8080
       selector:
         matchLabels:
           app: experiment-with-service