diff --git a/pkg/deployment/agent.go b/pkg/deployment/agent.go index 10f52167f..4c91641bf 100644 --- a/pkg/deployment/agent.go +++ b/pkg/deployment/agent.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "sort" "strings" "github.com/spf13/viper" @@ -52,6 +53,10 @@ func (a *Agent) Get() *appsv1.DaemonSet { commonSpec := util.Merge([]v1.JaegerCommonSpec{a.jaeger.Spec.Agent.JaegerCommonSpec, a.jaeger.Spec.JaegerCommonSpec, baseCommonSpec}) + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(args) + return &appsv1.DaemonSet{ TypeMeta: metav1.TypeMeta{ APIVersion: "apps/v1", diff --git a/pkg/deployment/agent_test.go b/pkg/deployment/agent_test.go index 269e99a5b..ec9d730dc 100644 --- a/pkg/deployment/agent_test.go +++ b/pkg/deployment/agent_test.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "strings" "testing" "github.com/spf13/viper" @@ -130,3 +131,26 @@ func TestAgentLabels(t *testing.T) { assert.Equal(t, a.jaeger.Name, dep.Spec.Template.Labels["app.kubernetes.io/instance"]) assert.Equal(t, fmt.Sprintf("%s-agent", a.jaeger.Name), dep.Spec.Template.Labels["app.kubernetes.io/name"]) } + +func TestAgentOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestAgentOrderOfArguments") + jaeger.Spec.Agent.Strategy = "daemonset" + jaeger.Spec.Agent.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + a := NewAgent(jaeger) + dep := a.Get() + + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 5) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[2], "--c-option")) + + // the following are added automatically + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[3], "--reporter.grpc.host-port")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[4], "--reporter.type")) +} diff --git a/pkg/deployment/all-in-one.go b/pkg/deployment/all-in-one.go index 9e3bdb182..ff30a36ac 100644 --- a/pkg/deployment/all-in-one.go +++ b/pkg/deployment/all-in-one.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "sort" "github.com/spf13/viper" appsv1 "k8s.io/api/apps/v1" @@ -54,6 +55,10 @@ func (a *AllInOne) Get() *appsv1.Deployment { configmap.Update(a.jaeger, commonSpec, &options) sampling.Update(a.jaeger, commonSpec, &options) + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(options) + var envFromSource []corev1.EnvFromSource if len(a.jaeger.Spec.Storage.SecretName) > 0 { envFromSource = append(envFromSource, corev1.EnvFromSource{ diff --git a/pkg/deployment/all-in-one_test.go b/pkg/deployment/all-in-one_test.go index 5ecc05e01..9ebdf3c1e 100644 --- a/pkg/deployment/all-in-one_test.go +++ b/pkg/deployment/all-in-one_test.go @@ -1,6 +1,7 @@ package deployment import ( + "strings" "testing" "github.com/spf13/viper" @@ -240,3 +241,24 @@ func TestAllInOneLabels(t *testing.T) { assert.Equal(t, a.jaeger.Name, dep.Spec.Template.Labels["app.kubernetes.io/instance"]) assert.Equal(t, a.jaeger.Name, dep.Spec.Template.Labels["app.kubernetes.io/name"]) } + +func TestAllInOneOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestAllInOneOrderOfArguments") + jaeger.Spec.AllInOne.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + a := NewAllInOne(jaeger) + dep := a.Get() + + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 4) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[2], "--c-option")) + + // the following are added automatically + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[3], "--sampling.strategies-file")) +} diff --git a/pkg/deployment/collector.go b/pkg/deployment/collector.go index 5100ec16f..cd63f96fc 100644 --- a/pkg/deployment/collector.go +++ b/pkg/deployment/collector.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "sort" "strings" "github.com/spf13/viper" @@ -82,6 +83,10 @@ func (c *Collector) Get() *appsv1.Deployment { sampling.Update(c.jaeger, commonSpec, &options) + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(options) + return &appsv1.Deployment{ TypeMeta: metav1.TypeMeta{ APIVersion: "apps/v1", diff --git a/pkg/deployment/collector_test.go b/pkg/deployment/collector_test.go index c3fd181bd..2d55b1a5a 100644 --- a/pkg/deployment/collector_test.go +++ b/pkg/deployment/collector_test.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "strings" "testing" "github.com/spf13/viper" @@ -422,3 +423,24 @@ func TestCollectorWithIngesterNoOptionsStorageType(t *testing.T) { assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 2) assert.Equal(t, "--kafka.brokers=http://brokers", dep.Spec.Template.Spec.Containers[0].Args[0]) } + +func TestCollectorOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestCollectorOrderOfArguments") + jaeger.Spec.Collector.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + a := NewCollector(jaeger) + dep := a.Get() + + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 4) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[2], "--c-option")) + + // the following are added automatically + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[3], "--sampling.strategies-file")) +} diff --git a/pkg/deployment/ingester.go b/pkg/deployment/ingester.go index 4e6980ded..690e15408 100644 --- a/pkg/deployment/ingester.go +++ b/pkg/deployment/ingester.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "sort" "strings" "github.com/spf13/viper" @@ -76,6 +77,10 @@ func (i *Ingester) Get() *appsv1.Deployment { i.jaeger.Spec.Storage.Options.Filter(storage.OptionsPrefix(i.jaeger.Spec.Storage.Type)), i.jaeger.Spec.Storage.Options.Filter("kafka")) + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(options) + return &appsv1.Deployment{ TypeMeta: metav1.TypeMeta{ APIVersion: "apps/v1", diff --git a/pkg/deployment/ingester_test.go b/pkg/deployment/ingester_test.go index 4b762912a..b6b0924aa 100644 --- a/pkg/deployment/ingester_test.go +++ b/pkg/deployment/ingester_test.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "strings" "testing" "github.com/spf13/viper" @@ -334,9 +335,9 @@ func TestIngesterWithStorageType(t *testing.T) { } assert.Equal(t, envvars, dep.Spec.Template.Spec.Containers[0].Env) assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 3) - assert.Equal(t, "--kafka.topic=mytopic", dep.Spec.Template.Spec.Containers[0].Args[0]) - assert.Equal(t, "--es.server-urls=http://somewhere", dep.Spec.Template.Spec.Containers[0].Args[1]) - assert.Equal(t, "--kafka.brokers=http://brokers", dep.Spec.Template.Spec.Containers[0].Args[2]) + assert.Equal(t, "--es.server-urls=http://somewhere", dep.Spec.Template.Spec.Containers[0].Args[0]) + assert.Equal(t, "--kafka.brokers=http://brokers", dep.Spec.Template.Spec.Containers[0].Args[1]) + assert.Equal(t, "--kafka.topic=mytopic", dep.Spec.Template.Spec.Containers[0].Args[2]) } func TestIngesterLabels(t *testing.T) { @@ -348,6 +349,24 @@ func TestIngesterLabels(t *testing.T) { assert.Equal(t, fmt.Sprintf("%s-ingester", ingester.jaeger.Name), dep.Spec.Template.Labels["app.kubernetes.io/name"]) } +func TestIngesterOrderOfArguments(t *testing.T) { + jaeger := newIngesterJaeger("TestIngesterOrderOfArguments") + jaeger.Spec.Ingester.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + a := NewIngester(jaeger) + dep := a.Get() + + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 3) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[2], "--c-option")) +} + func newIngesterJaeger(name string) *v1.Jaeger { return &v1.Jaeger{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/deployment/query.go b/pkg/deployment/query.go index a94733d9c..aef805169 100644 --- a/pkg/deployment/query.go +++ b/pkg/deployment/query.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "sort" "github.com/spf13/viper" appsv1 "k8s.io/api/apps/v1" @@ -79,6 +80,10 @@ func (q *Query) Get() *appsv1.Deployment { }) } + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(options) + return &appsv1.Deployment{ TypeMeta: metav1.TypeMeta{ APIVersion: "apps/v1", diff --git a/pkg/deployment/query_test.go b/pkg/deployment/query_test.go index 0b1d22047..ef9f74bae 100644 --- a/pkg/deployment/query_test.go +++ b/pkg/deployment/query_test.go @@ -2,6 +2,7 @@ package deployment import ( "fmt" + "strings" "testing" "github.com/spf13/viper" @@ -302,3 +303,21 @@ func TestQueryLabels(t *testing.T) { assert.Equal(t, query.jaeger.Name, dep.Spec.Template.Labels["app.kubernetes.io/instance"]) assert.Equal(t, fmt.Sprintf("%s-query", query.jaeger.Name), dep.Spec.Template.Labels["app.kubernetes.io/name"]) } + +func TestQueryOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestQueryOrderOfArguments") + jaeger.Spec.Query.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + a := NewQuery(jaeger) + dep := a.Get() + + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 3) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[0].Args[2], "--c-option")) +} diff --git a/pkg/inject/oauth-proxy.go b/pkg/inject/oauth-proxy.go index 3ffcf420c..223313d6a 100644 --- a/pkg/inject/oauth-proxy.go +++ b/pkg/inject/oauth-proxy.go @@ -32,18 +32,22 @@ func OAuthProxy(jaeger *v1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment { } func getOAuthProxyContainer(jaeger *v1.Jaeger) corev1.Container { + // keep this sorted! + // see https://github.com/jaegertracing/jaeger-operator/pull/337 + args := []string{ + "--cookie-secret=SECRET", + "--https-address=:8443", + fmt.Sprintf("--openshift-service-account=%s", account.OAuthProxyAccountNameFor(jaeger)), + "--provider=openshift", + "--tls-cert=/etc/tls/private/tls.crt", + "--tls-key=/etc/tls/private/tls.key", + "--upstream=http://localhost:16686", + } + return corev1.Container{ Image: viper.GetString("openshift-oauth-proxy-image"), Name: "oauth-proxy", - Args: []string{ - "--https-address=:8443", - "--provider=openshift", - fmt.Sprintf("--openshift-service-account=%s", account.OAuthProxyAccountNameFor(jaeger)), - "--upstream=http://localhost:16686", - "--tls-cert=/etc/tls/private/tls.crt", - "--tls-key=/etc/tls/private/tls.key", - "--cookie-secret=SECRET", - }, + Args: args, VolumeMounts: []corev1.VolumeMount{{ MountPath: "/etc/tls/private", Name: service.GetTLSSecretNameForQueryService(jaeger), diff --git a/pkg/inject/oauth-proxy_test.go b/pkg/inject/oauth-proxy_test.go index a39fc60ca..d9250d29d 100644 --- a/pkg/inject/oauth-proxy_test.go +++ b/pkg/inject/oauth-proxy_test.go @@ -2,6 +2,7 @@ package inject import ( "fmt" + "sort" "testing" "github.com/stretchr/testify/assert" @@ -54,3 +55,17 @@ func TestOAuthProxyConsistentServiceAccountName(t *testing.T) { } assert.True(t, found) } + +func TestOAuthProxyOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestOAuthProxyConsistentServiceAccountName") + jaeger.Spec.Ingress.Security = v1.IngressSecurityOAuthProxy + dep := OAuthProxy(jaeger, deployment.NewQuery(jaeger).Get()) + + sortedArgs := make([]string, len(dep.Spec.Template.Spec.Containers[1].Args)) + copy(sortedArgs, dep.Spec.Template.Spec.Containers[1].Args) + sort.Strings(sortedArgs) + + assert.Len(t, dep.Spec.Template.Spec.Containers, 2) + assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, 7) + assert.Equal(t, sortedArgs, dep.Spec.Template.Spec.Containers[1].Args) +} diff --git a/pkg/inject/sidecar.go b/pkg/inject/sidecar.go index 1bee63ce1..41bd0b64e 100644 --- a/pkg/inject/sidecar.go +++ b/pkg/inject/sidecar.go @@ -2,6 +2,7 @@ package inject import ( "fmt" + "sort" "strings" log "github.com/sirupsen/logrus" @@ -87,6 +88,11 @@ func Select(target *appsv1.Deployment, availableJaegerPods *v1.JaegerList) *v1.J func container(jaeger *v1.Jaeger) corev1.Container { args := append(jaeger.Spec.Agent.Options.ToArgs(), fmt.Sprintf("--collector.host-port=%s.%s:14267", service.GetNameForCollectorService(jaeger), jaeger.Namespace)) + + // ensure we have a consistent order of the arguments + // see https://github.com/jaegertracing/jaeger-operator/issues/334 + sort.Strings(args) + return corev1.Container{ Image: jaeger.Spec.Agent.Image, Name: "jaeger-agent", diff --git a/pkg/inject/sidecar_test.go b/pkg/inject/sidecar_test.go index 7ae3ef2df..0bb4308f7 100644 --- a/pkg/inject/sidecar_test.go +++ b/pkg/inject/sidecar_test.go @@ -1,6 +1,7 @@ package inject import ( + "strings" "testing" "github.com/spf13/viper" @@ -211,6 +212,25 @@ func TestSelectBasedOnName(t *testing.T) { assert.Equal(t, "the-second-jaeger-instance-available", jaeger.Name) } +func TestSidecarOrderOfArguments(t *testing.T) { + jaeger := v1.NewJaeger("TestQueryOrderOfArguments") + jaeger.Spec.Agent.Options = v1.NewOptions(map[string]interface{}{ + "b-option": "b-value", + "a-option": "a-value", + "c-option": "c-value", + }) + + dep := dep(map[string]string{Annotation: jaeger.Name}, map[string]string{}) + dep = Sidecar(jaeger, dep) + + assert.Len(t, dep.Spec.Template.Spec.Containers, 2) + assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, 4) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[1].Args[0], "--a-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[1].Args[1], "--b-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[1].Args[2], "--c-option")) + assert.True(t, strings.HasPrefix(dep.Spec.Template.Spec.Containers[1].Args[3], "--collector.host-port")) +} + func dep(annotations map[string]string, labels map[string]string) *appsv1.Deployment { return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{