Skip to content
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.

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

Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

84 changes: 84 additions & 0 deletions pkg/upgrade/v1_18_0.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package upgrade

import (
"context"
"fmt"

"sigs.k8s.io/controller-runtime/pkg/client"
Copy link
Member

Choose a reason for hiding this comment

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

this should be joined with the following block.

our import format tool seems to be flaky sometimes.


v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
"github.com/jaegertracing/jaeger-operator/pkg/service"

log "github.com/sirupsen/logrus"
)

func upgrade1_18_0(ctx context.Context, client client.Client, jaeger v1.Jaeger) (v1.Jaeger, error) {
// Transform collector flags
jaeger.Spec.Collector.Options = migrateCollectorOptions(&jaeger)
// Remove agent flags
jaeger.Spec.Agent.Options = migrateAgentOptions(&jaeger)

return jaeger, nil
}

func migrateCollectorOptions(jaeger *v1.Jaeger) v1.Options {
collectorDeprecatedFlags := []deprecationFlagMap{
Copy link
Member

Choose a reason for hiding this comment

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

Are we using one of these in our code base - either flag or env variable? We are definitely using zipkin (collector.zipkin.http-port) in one or another form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we are using admin-http-port and collector.zipkin.http-port I don't see where we are using the other flags.

{
from: "collector.port",
to: "collector.tchan-server.host-port",
},
{
from: "collector.http-port",
to: "collector.http-server.host-port",
},
{
from: "collector.grpc-port",
to: "collector.grpc-server.host-port",
},
{
from: "collector.zipkin.http-port",
to: "collector.zipkin.host-port",
},
{
from: "admin-http-port",
to: "admin.http.host-port",
},
}
opts := migrateDeprecatedOptions(jaeger, jaeger.Spec.Collector.Options, collectorDeprecatedFlags)
return transformCollectorPorts(jaeger.Logger(), opts, collectorDeprecatedFlags)
}

func migrateAgentOptions(jaeger *v1.Jaeger) v1.Options {
deleteAgentFlags := []deprecationFlagMap{
{from: "collector.host-port"},
{from: "reporter.tchannel.discovery.conn-check-timeout"},
{from: "reporter.tchannel.discovery.min-peers"},
{from: "reporter.tchannel.host-port"},
{from: "reporter.tchannel.report-timeout"},
}

ops := migrateDeprecatedOptions(jaeger, jaeger.Spec.Agent.Options, deleteAgentFlags)
opsMap := ops.GenericMap()

// Removed support for tchannel, so we need to make sure grpc is enabled and properly configured.
if _, ok := opsMap["reporter.grpc.host-port"]; !ok {
opsMap["reporter.grpc.host-port"] = fmt.Sprintf("dns:///%s.%s:14250",
service.GetNameForHeadlessCollectorService(jaeger), jaeger.Namespace)
}
return v1.NewOptions(opsMap)
}

func transformCollectorPorts(logger *log.Entry, opts v1.Options, collectorNewFlagsMap []deprecationFlagMap) v1.Options {
// Transform port number to format :XXX
in := opts.GenericMap()
for _, d := range collectorNewFlagsMap {
logger.WithFields(log.Fields{
"from": d.from,
"to": d.to,
}).Debug("flag value migrated")
if val, exists := in[d.to]; exists {
Copy link
Member

Choose a reason for hiding this comment

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

Let's say somebody already created 1.18 CR with new flag e.g. --admin.http.host-port. Then the value would be :host:port.

I am not sure at what time this method is called, so I am not sure if it can happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the CR is already on 1.18 this upgrade method should not be called, this is only called when we have an CR with version < 1.18 (and before call this v1_18_0 function, v1_17_0 should be called.. and before v1_15_0 and so on..)

in[d.to] = fmt.Sprintf(":%s", val)
}
}
return v1.NewOptions(in)
}
99 changes: 99 additions & 0 deletions pkg/upgrade/v1_18_0_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package upgrade

import (
"context"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
)

func TestUpgradeDeprecatedOptionsv1_18_0(t *testing.T) {
latestVersion := "1.18.0"
nsn := types.NamespacedName{Name: "my-instance"}
existing := v1.NewJaeger(nsn)
existing.Status.Version = "1.17.1"

flagsMap := map[string]string{
"collector.tchan-server.host-port": "collector.port",
"collector.http-server.host-port": "collector.http-port",
"collector.grpc-server.host-port": "collector.grpc-port",
"collector.zipkin.host-port": "collector.zipkin.http-port",
"admin.http.host-port": "admin-http-port",
}

oldOptionsMap := map[string]interface{}{
"collector.port": "4445",
"collector.http-port": "8080",
"collector.grpc-port": "14250",
"collector.zipkin.http-port": "9411",
"admin-http-port": "14269",
}

existing.Spec.Collector.Options = v1.NewOptions(oldOptionsMap)
objs := []runtime.Object{existing}

s := scheme.Scheme
s.AddKnownTypes(v1.SchemeGroupVersion, &v1.Jaeger{})
s.AddKnownTypes(v1.SchemeGroupVersion, &v1.JaegerList{})
cl := fake.NewFakeClient(objs...)

// test
assert.NoError(t, ManagedInstances(context.Background(), cl, cl, latestVersion))

// verify
persisted := &v1.Jaeger{}
assert.NoError(t, cl.Get(context.Background(), nsn, persisted))
// assert.Equal(t, latest.v, persisted.Status.Version)

opts := persisted.Spec.Collector.Options.Map()
for _, newFlag := range []string{
"collector.tchan-server.host-port",
"collector.http-server.host-port",
"collector.grpc-server.host-port",
"collector.zipkin.host-port",
"admin.http.host-port"} {
assert.Contains(t, opts, newFlag)
expectedValue := fmt.Sprintf(":%s", oldOptionsMap[flagsMap[newFlag]])
assert.Equal(t, expectedValue, opts[newFlag])
}
}

func TestUpgradeAgentWithTChannelEnablev1_18_0_(t *testing.T) {
latestVersion := "1.18.0"
nsn := types.NamespacedName{Name: "my-instance"}
existing := v1.NewJaeger(nsn)
existing.Status.Version = "1.17.1"

agentTchanelOptions := map[string]interface{}{
"collector.host-port": "4445",
"reporter.tchannel.discovery.conn-check-timeout": "5",
"reporter.tchannel.discovery.min-peers": "2",
"reporter.tchannel.host-port": "8080",
"reporter.tchannel.report-timeout": "20",
}

existing.Spec.Agent.Options = v1.NewOptions(agentTchanelOptions)
objs := []runtime.Object{existing}

s := scheme.Scheme
s.AddKnownTypes(v1.SchemeGroupVersion, &v1.Jaeger{})
s.AddKnownTypes(v1.SchemeGroupVersion, &v1.JaegerList{})
cl := fake.NewFakeClient(objs...)

assert.NoError(t, ManagedInstances(context.Background(), cl, cl, latestVersion))

// verify
persisted := &v1.Jaeger{}
assert.NoError(t, cl.Get(context.Background(), nsn, persisted))

collectorOpts := persisted.Spec.Agent.Options.Map()

assert.Contains(t, collectorOpts, "reporter.grpc.host-port")
}
1 change: 1 addition & 0 deletions pkg/upgrade/versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ var (
upgrades = map[string]upgradeFunction{
"1.15.0": upgrade1_15_0,
"1.17.0": upgrade1_17_0,
"1.18.0": upgrade1_18_0,
}
)