Skip to content

Plumb revision annotations through to services and deployments #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattmoor merged 1 commit into from
Apr 11, 2018
Merged

Plumb revision annotations through to services and deployments #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/controller/configuration/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ func getTestConfiguration() *v1alpha1.Configuration {
"test-label": "test",
"example.com/namespaced-label": "test",
},
Annotations: map[string]string{
"test-annotation-1": "foo",
"test-annotation-2": "bar",
},
},
Spec: v1alpha1.RevisionSpec{
// corev1.Container has a lot of setting. We try to pass many
Expand Down Expand Up @@ -220,6 +224,12 @@ func TestCreateConfigurationsCreatesRevision(t *testing.T) {
}
}

for k, v := range config.Spec.RevisionTemplate.ObjectMeta.Annotations {
if rev.Annotations[k] != v {
t.Errorf("revisionTemplate annotation %s=%s not passed to revision", k, v)
}
}

if len(rev.OwnerReferences) != 1 || config.Name != rev.OwnerReferences[0].Name {
t.Errorf("expected owner references to have 1 ref with name %s", config.Name)
}
Expand Down
22 changes: 12 additions & 10 deletions pkg/controller/revision/ela_autoscaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,16 @@ func MakeElaAutoscalerDeployment(u *v1alpha1.Revision) *v1beta1.Deployment {

labels := MakeElaResourceLabels(u)
labels[ela.AutoscalerLabelKey] = controller.GetRevisionAutoscalerName(u)
annotations := MakeElaResourceAnnotations(u)
annotations[sidecarIstioInjectAnnotation] = "false"

replicas := int32(1)
return &v1beta1.Deployment{
ObjectMeta: meta_v1.ObjectMeta{
Name: controller.GetRevisionAutoscalerName(u),
Namespace: AutoscalerNamespace,
Labels: MakeElaResourceLabels(u),
Name: controller.GetRevisionAutoscalerName(u),
Namespace: AutoscalerNamespace,
Labels: MakeElaResourceLabels(u),
Annotations: MakeElaResourceAnnotations(u),
},
Spec: v1beta1.DeploymentSpec{
Replicas: &replicas,
Expand All @@ -66,10 +69,8 @@ func MakeElaAutoscalerDeployment(u *v1alpha1.Revision) *v1beta1.Deployment {
},
Template: corev1.PodTemplateSpec{
ObjectMeta: meta_v1.ObjectMeta{
Labels: labels,
Annotations: map[string]string{
"sidecar.istio.io/inject": "false",
},
Labels: labels,
Annotations: annotations,
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
Expand Down Expand Up @@ -115,9 +116,10 @@ func MakeElaAutoscalerDeployment(u *v1alpha1.Revision) *v1beta1.Deployment {
func MakeElaAutoscalerService(u *v1alpha1.Revision) *corev1.Service {
return &corev1.Service{
ObjectMeta: meta_v1.ObjectMeta{
Name: controller.GetRevisionAutoscalerName(u),
Namespace: AutoscalerNamespace,
Labels: MakeElaResourceLabels(u),
Name: controller.GetRevisionAutoscalerName(u),
Namespace: AutoscalerNamespace,
Labels: MakeElaResourceLabels(u),
Copy link
Copy Markdown
Contributor

@mdemirhan mdemirhan Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we apply the custom annotations and labels to autoscaler pod or should we skip this part? autoscaler is a system component and it might be better to keep it hidden from the user and not copy labels and annotations that the user specified to it. What do you think?

Copy link
Copy Markdown
Contributor Author

@bsnchan bsnchan Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We chose to plumb the labels and annotations through to the autoscaler as well because we thought the intent was that the user can then target all services and pods that are created as part of creating a Configuration.

Annotations: MakeElaResourceAnnotations(u),
Copy link
Copy Markdown
Contributor

@vaikas vaikas Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pulls in the istio sidecar annotation as well, is that something that we want? Are there any side effects on this?

Copy link
Copy Markdown
Member

@dprotaso dprotaso Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pulls in the istio sidecar annotation as well

Only if a RevisionTemplate's metadata has specified it. Is that your concern?

Copy link
Copy Markdown
Contributor

@vaikas vaikas Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was just curious if this is what we want and if the istio sidecar goes in there, will it impact anything?

Copy link
Copy Markdown
Member

@dprotaso dprotaso Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do some digging to confirm

Copy link
Copy Markdown
Contributor Author

@bsnchan bsnchan Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We confirmed that if the RevisionTemplate has an Istio sidecar annotation on the service, it would not impact anything since the annotation only applies to the Pods.

See the istio webhook config for evidence

},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
Expand Down
16 changes: 9 additions & 7 deletions pkg/controller/revision/ela_pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,15 @@ func MakeElaDeployment(u *v1alpha1.Revision, namespace string) *v1beta1.Deployme
MaxSurge: &elaPodMaxSurge,
}

podTemplateAnnotations := MakeElaResourceAnnotations(u)
podTemplateAnnotations[sidecarIstioInjectAnnotation] = "true"

return &v1beta1.Deployment{
ObjectMeta: meta_v1.ObjectMeta{
Name: controller.GetRevisionDeploymentName(u),
Namespace: namespace,
Labels: MakeElaResourceLabels(u),
Name: controller.GetRevisionDeploymentName(u),
Namespace: namespace,
Labels: MakeElaResourceLabels(u),
Annotations: MakeElaResourceAnnotations(u),
},
Spec: v1beta1.DeploymentSpec{
Replicas: &elaPodReplicaCount,
Expand All @@ -125,10 +129,8 @@ func MakeElaDeployment(u *v1alpha1.Revision, namespace string) *v1beta1.Deployme
},
Template: corev1.PodTemplateSpec{
ObjectMeta: meta_v1.ObjectMeta{
Labels: MakeElaResourceLabels(u),
Annotations: map[string]string{
"sidecar.istio.io/inject": "true",
},
Labels: MakeElaResourceLabels(u),
Annotations: podTemplateAnnotations,
},
},
},
Expand Down
8 changes: 8 additions & 0 deletions pkg/controller/revision/ela_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,11 @@ func MakeElaResourceLabels(u *v1alpha1.Revision) map[string]string {
}
return labels
}

func MakeElaResourceAnnotations(u *v1alpha1.Revision) map[string]string {
annotations := make(map[string]string, len(u.ObjectMeta.Annotations)+1)
for k, v := range u.ObjectMeta.Annotations {
annotations[k] = v
}
return annotations
}
7 changes: 4 additions & 3 deletions pkg/controller/revision/ela_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ var servicePort = 80
func MakeRevisionK8sService(u *v1alpha1.Revision, ns string) *corev1.Service {
return &corev1.Service{
ObjectMeta: meta_v1.ObjectMeta{
Name: controller.GetElaK8SServiceNameForRevision(u),
Namespace: ns,
Labels: MakeElaResourceLabels(u),
Name: controller.GetElaK8SServiceNameForRevision(u),
Namespace: ns,
Labels: MakeElaResourceLabels(u),
Annotations: MakeElaResourceAnnotations(u),
},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/revision/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const (
autoscalerPort = 8080

serviceTimeoutDuration = 5 * time.Minute

sidecarIstioInjectAnnotation = "sidecar.istio.io/inject"
)

var (
Expand Down
65 changes: 52 additions & 13 deletions pkg/controller/revision/revision_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ func getTestRevision() *v1alpha1.Revision {
"testLabel2": "bar",
ela.RouteLabelKey: "test-route",
},
Annotations: map[string]string{
"testAnnotation": "test",
},
},
Spec: v1alpha1.RevisionSpec{
// corev1.Container has a lot of setting. We try to pass many
Expand Down Expand Up @@ -146,6 +149,17 @@ func getTestNotReadyEndpoints(revName string) *corev1.Endpoints {
}
}

func sumMaps(a map[string]string, b map[string]string) map[string]string {
summedMap := make(map[string]string, len(a)+len(b)+2)
for k, v := range a {
summedMap[k] = v
}
for k, v := range b {
summedMap[k] = v
}
return summedMap
}

func newTestController(t *testing.T, elaObjects ...runtime.Object) (
kubeClient *fakekubeclientset.Clientset,
elaClient *fakeclientset.Clientset,
Expand Down Expand Up @@ -263,12 +277,16 @@ func TestCreateRevCreatesStuff(t *testing.T) {
t.Error("Missing queue-proxy container")
}

expectedLabels := map[string]string{
"testLabel1": "foo",
"testLabel2": "bar",
ela.RouteLabelKey: "test-route",
ela.RevisionLabelKey: rev.Name,
}
expectedLabels := sumMaps(
rev.Labels,
map[string]string{ela.RevisionLabelKey: rev.Name},
)
expectedAnnotations := rev.Annotations
expectedPodSpecAnnotations := sumMaps(
rev.Annotations,
map[string]string{"sidecar.istio.io/inject": "true"},
)

if labels := deployment.ObjectMeta.Labels; !reflect.DeepEqual(labels, expectedLabels) {
t.Errorf("Labels not set correctly on deployment: expected %v got %v.",
expectedLabels, labels)
Expand All @@ -277,6 +295,14 @@ func TestCreateRevCreatesStuff(t *testing.T) {
t.Errorf("Label not set correctly in pod template: expected %v got %v.",
expectedLabels, labels)
}
if annotations := deployment.ObjectMeta.Annotations; !reflect.DeepEqual(annotations, expectedAnnotations) {
t.Errorf("Annotations not set correctly on deployment: expected %v got %v.",
expectedAnnotations, annotations)
}
if annotations := deployment.Spec.Template.ObjectMeta.Annotations; !reflect.DeepEqual(annotations, expectedPodSpecAnnotations) {
t.Errorf("Annotations not set correctly in pod template: expected %v got %v.",
expectedPodSpecAnnotations, annotations)
}

// Look for the revision service.
expectedServiceName := fmt.Sprintf("%s-service", rev.Name)
Expand All @@ -300,16 +326,21 @@ func TestCreateRevCreatesStuff(t *testing.T) {
t.Errorf("Label not set correctly for revision service: expected %v got %v.",
expectedLabels, labels)
}
if annotations := service.ObjectMeta.Annotations; !reflect.DeepEqual(annotations, expectedAnnotations) {
t.Errorf("Annotations not set correctly for revision service: expected %v got %v.",
expectedAnnotations, annotations)
}

// Look for the autoscaler deployment.
expectedAutoscalerName := fmt.Sprintf("%s-autoscaler", rev.Name)
expectedAutoscalerLabels := map[string]string{
"testLabel1": "foo",
"testLabel2": "bar",
ela.RouteLabelKey: "test-route",
ela.RevisionLabelKey: rev.Name,
ela.AutoscalerLabelKey: expectedAutoscalerName,
}
expectedAutoscalerLabels := sumMaps(
expectedLabels,
map[string]string{ela.AutoscalerLabelKey: expectedAutoscalerName},
)
expectedAutoscalerPodSpecAnnotations := sumMaps(
rev.Annotations,
map[string]string{"sidecar.istio.io/inject": "false"},
)

asDeployment, err := kubeClient.ExtensionsV1beta1().Deployments(AutoscalerNamespace).Get(expectedAutoscalerName, metav1.GetOptions{})
if err != nil {
Expand All @@ -319,6 +350,10 @@ func TestCreateRevCreatesStuff(t *testing.T) {
t.Errorf("Label not set correctly in autoscaler pod template: expected %v got %v.",
expectedAutoscalerLabels, labels)
}
if annotations := asDeployment.Spec.Template.ObjectMeta.Annotations; !reflect.DeepEqual(annotations, expectedAutoscalerPodSpecAnnotations) {
t.Errorf("Annotations not set correctly in autoscaler pod template: expected %v got %v.",
expectedAutoscalerPodSpecAnnotations, annotations)
}
// Check the autoscaler deployment environment variables
foundAutoscaler := false
for _, container := range asDeployment.Spec.Template.Spec.Containers {
Expand Down Expand Up @@ -346,6 +381,10 @@ func TestCreateRevCreatesStuff(t *testing.T) {
t.Errorf("Label not set correctly autoscaler service: expected %v got %v.",
expectedLabels, labels)
}
if annotations := asService.ObjectMeta.Annotations; !reflect.DeepEqual(annotations, expectedAnnotations) {
t.Errorf("Annotations not set correctly autoscaler service: expected %v got %v.",
expectedAnnotations, annotations)
}

rev, err = elaClient.ElafrosV1alpha1().Revisions(testNamespace).Get(rev.Name, metav1.GetOptions{})
if err != nil {
Expand Down