Skip to content

Commit

Permalink
Allow TLS flags to be disabled (#1440)
Browse files Browse the repository at this point in the history 
* Validate presence of tls flags using only the prefix of the flag

Signed-off-by: Ruben Vargas <[email protected]>

* Test explicit disable tls options

Signed-off-by: Ruben Vargas <[email protected]>

* Handle flags update case

Signed-off-by: Ruben Vargas <[email protected]>

Co-authored-by: Juraci Paixão Kröhling <[email protected]>
  • Loading branch information
@rubenvp8510 @jpkrohling
rubenvp8510 and jpkrohling authored Aug 4, 2021
1 parent 9808586 commit 05b0c69
Show file tree
Hide file tree
Showing 8 changed files with 193 additions and 60 deletions.
2 changes: 1 addition & 1 deletion pkg/deployment/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (a *Agent) Get() *appsv1.DaemonSet {

// Enable tls by default for openshift platform
if viper.GetString("platform") == v1.FlagPlatformOpenShift {
if len(util.FindItem("--reporter.grpc.tls.enabled=true", args)) == 0 {
if len(util.FindItem("--reporter.grpc.tls.enabled=", args)) == 0 {
args = append(args, "--reporter.grpc.tls.enabled=true")
args = append(args, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath))
args = append(args, fmt.Sprintf("--reporter.grpc.tls.server-name=%s.%s.svc.cluster.local", service.GetNameForHeadlessCollectorService(a.jaeger), a.jaeger.Namespace))
Expand Down
28 changes: 25 additions & 3 deletions pkg/deployment/agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,10 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) {
defer viper.Reset()

for _, tt := range []struct {
name string
options v1.Options
expectedArgs []string
name string
options v1.Options
expectedArgs []string
nonExpectedArgs []string
}{
{
name: "Openshift CA",
Expand Down Expand Up @@ -232,6 +233,21 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) {
"--reporter.grpc.tls.ca=/my/custom/ca",
},
},
{
name: "Explicit disable TLS",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
"reporter.grpc.tls.enabled": "false",
}),
expectedArgs: []string{
"--a-option=a-value",
"--reporter.grpc.host-port=dns:///my-instance-collector-headless.test:14250",
"--reporter.grpc.tls.enabled=false",
},
nonExpectedArgs: []string{
"--reporter.grpc.tls.enabled=true",
},
},
} {
t.Run(tt.name, func(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{
Expand All @@ -251,6 +267,12 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) {
assert.Greater(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0)
}

if tt.nonExpectedArgs != nil {
for _, arg := range tt.nonExpectedArgs {
assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0)
}
}

assert.Len(t, dep.Spec.Template.Spec.Volumes, 2)
assert.Len(t, dep.Spec.Template.Spec.Containers[0].VolumeMounts, 2)
})
Expand Down
10 changes: 8 additions & 2 deletions pkg/deployment/all_in_one.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,13 @@ func (a *AllInOne) Get() *appsv1.Deployment {

configmap.Update(a.jaeger, commonSpec, &options)
sampling.Update(a.jaeger, commonSpec, &options)
tls.Update(a.jaeger, commonSpec, &options)


// If tls is not explicitly set, update jaeger CR with the tls flags according to the platform
if len(util.FindItem("--collector.grpc.tls.enabled=", options)) == 0 {
tls.Update(a.jaeger, commonSpec, &options)
}

ca.Update(a.jaeger, commonSpec)
ca.AddServiceCA(a.jaeger, commonSpec)
storage.UpdateGRPCPlugin(a.jaeger, commonSpec)
Expand All @@ -74,7 +80,7 @@ func (a *AllInOne) Get() *appsv1.Deployment {
// even though the agent is in the same process as the collector, they communicate via gRPC, and the collector has TLS enabled,
// as it might receive connections from external agents
if viper.GetString("platform") == v1.FlagPlatformOpenShift {
if len(util.FindItem("--reporter.grpc.tls.enabled=true", options)) == 0 {
if len(util.FindItem("--reporter.grpc.tls.enabled=", options)) == 0 {
options = append(options, "--reporter.grpc.tls.enabled=true")
options = append(options, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath))
options = append(options, fmt.Sprintf("--reporter.grpc.tls.server-name=%s.%s.svc.cluster.local", service.GetNameForHeadlessCollectorService(a.jaeger), a.jaeger.Namespace))
Expand Down
78 changes: 60 additions & 18 deletions pkg/deployment/all_in_one_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,27 +338,69 @@ func TestAllInOneArgumentsOpenshiftTLS(t *testing.T) {
viper.Set("platform", v1.FlagPlatformOpenShift)
defer viper.Reset()

jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})
jaeger.Spec.AllInOne.Options = v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
})
for _, tt := range []struct {
name string
options v1.Options
expectedArgs []string
nonExpectedArgs []string
}{
{
name: "Openshift CA",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
}),
expectedArgs: []string{
"--a-option=a-value",
"--collector.grpc.tls.enabled=true",
"--collector.grpc.tls.cert=/etc/tls-config/tls.crt",
"--collector.grpc.tls.key=/etc/tls-config/tls.key",
"--sampling.strategies-file",
"--reporter.grpc.tls.ca",
"--reporter.grpc.tls.enabled",
"--reporter.grpc.tls.server-name",
},
},
{
name: "Explicit disable TLS",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
"reporter.grpc.tls.enabled": "false",
"collector.grpc.tls.enabled": "false",
}),
expectedArgs: []string{
"--a-option=a-value",
"--reporter.grpc.tls.enabled=false",
"--collector.grpc.tls.enabled=false",
"--sampling.strategies-file",
},
nonExpectedArgs: []string{
"--reporter.grpc.tls.enabled=true",
"--collector.grpc.tls.enabled=true",
},
},
} {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})
jaeger.Spec.AllInOne.Options = tt.options

// test
a := NewAllInOne(jaeger)
dep := a.Get()
// test
a := NewAllInOne(jaeger)
dep := a.Get()

// verify
assert.Len(t, dep.Spec.Template.Spec.Containers, 1)
assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 8)
assert.NotEmpty(t, util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.cert=/etc/tls-config/tls.crt", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.key=/etc/tls-config/tls.key", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--sampling.strategies-file", dep.Spec.Template.Spec.Containers[0].Args))
// verify
assert.Len(t, dep.Spec.Template.Spec.Containers, 1)
assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, len(tt.expectedArgs))

for _, arg := range tt.expectedArgs {
assert.NotEmpty(t, util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args))
}

if tt.nonExpectedArgs != nil {
for _, arg := range tt.nonExpectedArgs {
assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0)
}
}
}

assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.ca", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.enabled", dep.Spec.Template.Spec.Containers[0].Args))
assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.server-name", dep.Spec.Template.Spec.Containers[0].Args))
}

func TestAllInOneServiceLinks(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/deployment/collector.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ func (c *Collector) Get() *appsv1.Deployment {
c.jaeger.Spec.Storage.Options.Filter(storageType.OptionsPrefix()))

sampling.Update(c.jaeger, commonSpec, &options)
if len(util.FindItem("--collector.grpc.tls.enabled=true", args)) == 0 {
if len(util.FindItem("--collector.grpc.tls.enabled=", args)) == 0 {
tls.Update(c.jaeger, commonSpec, &options)
ca.Update(c.jaeger, commonSpec)
}
ca.Update(c.jaeger, commonSpec)
storage.UpdateGRPCPlugin(c.jaeger, commonSpec)

// ensure we have a consistent order of the arguments
Expand Down
66 changes: 47 additions & 19 deletions pkg/deployment/collector_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,31 +510,55 @@ func TestCollectorAutoscalersSetMaxReplicas(t *testing.T) {
func TestCollectoArgumentsOpenshiftTLS(t *testing.T) {
viper.Set("platform", v1.FlagPlatformOpenShift)
defer viper.Reset()

for _, tt := range []struct {
name string
options v1.Options
expectedCert string
expectedKey string
name string
options v1.Options
expectedArgs []string
nonExpectedArgs []string
}{
{
name: "Openshift certificates",
name: "Openshift CA",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
}),
expectedCert: "/etc/tls-config/tls.crt",
expectedKey: "/etc/tls-config/tls.key",
expectedArgs: []string{
"--a-option=a-value",
"--collector.grpc.tls.enabled=true",
"--collector.grpc.tls.cert=/etc/tls-config/tls.crt",
"--collector.grpc.tls.key=/etc/tls-config/tls.key",
"--sampling.strategies-file",
},
},
{
name: "Custom certificates",
name: "Custom CA",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
"collector.grpc.tls.enabled": "true",
"collector.grpc.tls.cert": "/my/custom/cert",
"collector.grpc.tls.key": "/my/custom/key",
}),
expectedCert: "/my/custom/cert",
expectedKey: "/my/custom/key",
expectedArgs: []string{
"--a-option=a-value",
"--collector.grpc.tls.enabled=true",
"--collector.grpc.tls.cert=/my/custom/cert",
"--collector.grpc.tls.key=/my/custom/key",
"--sampling.strategies-file",
},
},
{
name: "Explicit disable TLS",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
"collector.grpc.tls.enabled": "false",
}),
expectedArgs: []string{
"--a-option=a-value",
"--collector.grpc.tls.enabled=false",
"--sampling.strategies-file",
},
nonExpectedArgs: []string{
"--collector.grpc.tls.enabled=true",
},
},
} {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -544,15 +568,19 @@ func TestCollectoArgumentsOpenshiftTLS(t *testing.T) {
a := NewCollector(jaeger)
dep := a.Get()

// verify
assert.Len(t, dep.Spec.Template.Spec.Containers, 1)
assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 5)
assert.Greater(t, len(util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[0].Args)), 0)

// the following are added automatically
assert.Greater(t, len(util.FindItem("--collector.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[0].Args)), 0)
assert.Greater(t, len(util.FindItem("--collector.grpc.tls.cert="+tt.expectedCert, dep.Spec.Template.Spec.Containers[0].Args)), 0)
assert.Greater(t, len(util.FindItem("--collector.grpc.tls.key="+tt.expectedKey, dep.Spec.Template.Spec.Containers[0].Args)), 0)
assert.Greater(t, len(util.FindItem("--sampling.strategies-file", dep.Spec.Template.Spec.Containers[0].Args)), 0)
assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, len(tt.expectedArgs))

for _, arg := range tt.expectedArgs {
assert.NotEmpty(t, util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args))
}

if tt.nonExpectedArgs != nil {
for _, arg := range tt.nonExpectedArgs {
assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0)
}
}
})
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/inject/sidecar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func container(jaeger *v1.Jaeger, dep *appsv1.Deployment, agentIdx int) corev1.C

// Enable tls by default for openshift platform
if viper.GetString("platform") == v1.FlagPlatformOpenShift {
if len(util.FindItem("--reporter.grpc.tls.enabled=true", args)) == 0 {
if len(util.FindItem("--reporter.grpc.tls.enabled=", args)) == 0 {
args = append(args, "--reporter.grpc.tls.enabled=true")
args = append(args, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath))
}
Expand Down
63 changes: 49 additions & 14 deletions pkg/inject/sidecar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -768,20 +768,28 @@ func containsOptionWithPrefix(t *testing.T, args []string, prefix string) bool {
}

func TestSidecarArgumentsOpenshiftTLS(t *testing.T) {

viper.Set("platform", v1.FlagPlatformOpenShift)
defer viper.Reset()

for _, tt := range []struct {
name string
options v1.Options
expectedCA string
name string
options v1.Options
expectedArgs []string
nonExpectedArgs []string
}{
{
name: "Openshift CA",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
}),
expectedCA: ca.ServiceCAPath,
expectedArgs: []string{
"--a-option=a-value",
"--reporter.grpc.tls.enabled=true",
"--reporter.grpc.tls.ca=" + ca.ServiceCAPath,
"--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250",
"--agent.tags=",
},
},
{
name: "Custom CA",
Expand All @@ -790,7 +798,29 @@ func TestSidecarArgumentsOpenshiftTLS(t *testing.T) {
"reporter.grpc.tls.enabled": "true",
"reporter.grpc.tls.ca": "/my/custom/ca",
}),
expectedCA: "/my/custom/ca",
expectedArgs: []string{
"--a-option=a-value",
"--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250",
"--reporter.grpc.tls.enabled=true",
"--reporter.grpc.tls.ca=/my/custom/ca",
"--agent.tags=",
},
},
{
name: "Explicit disable TLS",
options: v1.NewOptions(map[string]interface{}{
"a-option": "a-value",
"reporter.grpc.tls.enabled": "false",
}),
expectedArgs: []string{
"--a-option=a-value",
"--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250",
"--reporter.grpc.tls.enabled=false",
"--agent.tags=",
},
nonExpectedArgs: []string{
"--reporter.grpc.tls.enabled=true",
},
},
} {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -803,15 +833,20 @@ func TestSidecarArgumentsOpenshiftTLS(t *testing.T) {
dep = Sidecar(jaeger, dep)

assert.Len(t, dep.Spec.Template.Spec.Containers, 2)
assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, 5)
assert.Greater(t, len(util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--agent.tags", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.ca="+tt.expectedCA, dep.Spec.Template.Spec.Containers[1].Args)), 0)
agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Contains(t, agentTagsMap, "container.name")
assert.Equal(t, agentTagsMap["container.name"], "only_container")
assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, len(tt.expectedArgs))

for _, arg := range tt.expectedArgs {
assert.Greater(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[1].Args)), 0)
}

if tt.nonExpectedArgs != nil {
for _, arg := range tt.nonExpectedArgs {
assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[1].Args)), 0)
}
}

assert.Len(t, dep.Spec.Template.Spec.Volumes, 2)
assert.Len(t, dep.Spec.Template.Spec.Containers[1].VolumeMounts, 2)
})
}
}
Expand Down

0 comments on commit 05b0c69

Please sign in to comment.