From 25e92914d65412789a7c20959c10da6c3ccaa9fd Mon Sep 17 00:00:00 2001 From: Yuchen Cheng Date: Fri, 23 Jul 2021 02:49:31 +0800 Subject: [PATCH] Fix overwritten default labels in label selectors of `Service` (#1490) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix overwritten default labels in label selectors of Service Signed-off-by: Yuchen Cheng * Remove reserved labels in the normailize operation Signed-off-by: Yuchen Cheng * Add span events when removing reserved labels Signed-off-by: Yuchen Cheng Co-authored-by: Juraci Paixão Kröhling --- pkg/deployment/all_in_one.go | 5 ++++- pkg/deployment/all_in_one_test.go | 36 +++++++++++++++++++++++++++++++ pkg/strategy/controller.go | 16 ++++++++++++++ pkg/strategy/controller_test.go | 13 +++++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) diff --git a/pkg/deployment/all_in_one.go b/pkg/deployment/all_in_one.go index 4d683348db..e0b5444c0c 100644 --- a/pkg/deployment/all_in_one.go +++ b/pkg/deployment/all_in_one.go @@ -231,7 +231,10 @@ func (a *AllInOne) Get() *appsv1.Deployment { // Services returns a list of services to be deployed along with the all-in-one deployment func (a *AllInOne) Services() []*corev1.Service { - labels := a.labels() + // merge defined labels with default labels + spec := util.Merge([]v1.JaegerCommonSpec{a.jaeger.Spec.AllInOne.JaegerCommonSpec, a.jaeger.Spec.JaegerCommonSpec, v1.JaegerCommonSpec{Labels: a.labels()}}) + labels := spec.Labels + return append(service.NewCollectorServices(a.jaeger, labels), service.NewQueryService(a.jaeger, labels), service.NewAgentService(a.jaeger, labels), diff --git a/pkg/deployment/all_in_one_test.go b/pkg/deployment/all_in_one_test.go index be2b1a05e6..28af394dc9 100644 --- a/pkg/deployment/all_in_one_test.go +++ b/pkg/deployment/all_in_one_test.go @@ -93,6 +93,42 @@ func TestAllInOneLabels(t *testing.T) { assert.Equal(t, "false", dep.Spec.Selector.MatchLabels["another"]) } +func TestAllInOneOverwrittenDefaultLabels(t *testing.T) { + jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestAllInOneOverwrittenDefaultLabels"}) + jaeger.Spec.Labels = map[string]string{ + "name": "operator", + "hello": "jaeger", + "app.kubernetes.io/name": "my-jaeger", // Override default labels + } + jaeger.Spec.AllInOne.Labels = map[string]string{ + "hello": "world", // Override top level annotation + "another": "false", + } + + allinone := NewAllInOne(jaeger) + dep := allinone.Get() + + assert.Equal(t, "operator", dep.Spec.Template.Labels["name"]) + assert.Equal(t, "world", dep.Spec.Template.Labels["hello"]) + assert.Equal(t, "false", dep.Spec.Template.Labels["another"]) + assert.Equal(t, "my-jaeger", dep.Spec.Template.Labels["app.kubernetes.io/name"]) + + // Deployment selectors should be the same as the template labels. + assert.Equal(t, "operator", dep.Spec.Selector.MatchLabels["name"]) + assert.Equal(t, "world", dep.Spec.Selector.MatchLabels["hello"]) + assert.Equal(t, "false", dep.Spec.Selector.MatchLabels["another"]) + assert.Equal(t, "my-jaeger", dep.Spec.Selector.MatchLabels["app.kubernetes.io/name"]) + + // Service selectors should be the same as the template labels. + services := allinone.Services() + for _, svc := range services { + assert.Equal(t, "operator", svc.Spec.Selector["name"]) + assert.Equal(t, "world", svc.Spec.Selector["hello"]) + assert.Equal(t, "false", svc.Spec.Selector["another"]) + assert.Equal(t, "my-jaeger", svc.Spec.Selector["app.kubernetes.io/name"]) + } +} + func TestAllInOneHasOwner(t *testing.T) { name := "TestAllInOneHasOwner" a := NewAllInOne(v1.NewJaeger(types.NamespacedName{Name: name})) diff --git a/pkg/strategy/controller.go b/pkg/strategy/controller.go index d52590e52f..5e03b16853 100644 --- a/pkg/strategy/controller.go +++ b/pkg/strategy/controller.go @@ -78,6 +78,22 @@ func normalize(ctx context.Context, jaeger *v1.Jaeger) { jaeger.Spec.Storage.Type = v1.JaegerMemoryStorage } + // remove reserved labels + for _, labels := range []map[string]string{ + jaeger.Spec.JaegerCommonSpec.Labels, + jaeger.Spec.AllInOne.JaegerCommonSpec.Labels, + jaeger.Spec.Query.JaegerCommonSpec.Labels, + } { + if _, ok := labels["app.kubernetes.io/instance"]; ok { + span.AddEvent(fmt.Sprintf("the reserved label 'app.kubernetes.io/instance' is overwritten, falling back to %s", jaeger.Name)) + delete(labels, "app.kubernetes.io/instance") + } + if _, ok := labels["app.kubernetes.io/managed-by"]; ok { + span.AddEvent("the reserved label 'app.kubernetes.io/managed-by' is overwritten, falling back to jaeger-operator") + delete(labels, "app.kubernetes.io/managed-by") + } + } + // normalize the deployment strategy if jaeger.Spec.Strategy != v1.DeploymentStrategyProduction && jaeger.Spec.Strategy != v1.DeploymentStrategyStreaming { jaeger.Spec.Strategy = v1.DeploymentStrategyAllInOne diff --git a/pkg/strategy/controller_test.go b/pkg/strategy/controller_test.go index 54e5bab126..9b0a437490 100644 --- a/pkg/strategy/controller_test.go +++ b/pkg/strategy/controller_test.go @@ -178,6 +178,19 @@ func TestAcceptExplicitValueFromSecurityWhenOnOpenShift(t *testing.T) { assert.Equal(t, v1.IngressSecurityNoneExplicit, jaeger.Spec.Ingress.Security) } +func TestRemoveReservedLabels(t *testing.T) { + jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestRemoveReservedLabels"}) + jaeger.Spec.Labels = map[string]string{ + "app.kubernetes.io/instance": "custom-instance", + "app.kubernetes.io/managed-by": "custom-managed-by", + } + + normalize(context.Background(), jaeger) + + assert.NotContains(t, jaeger.Spec.Labels, "app.kubernetes.io/instance") + assert.NotContains(t, jaeger.Spec.Labels, "app.kubernetes.io/managed-by") +} + func TestNormalizeIndexCleaner(t *testing.T) { trueVar := true falseVar := false