Skip to content

Commit

Permalink
Fix: Bind only enabled ports on agent pods (#211)
Browse files Browse the repository at this point in the history 
* refactor: Remove obsolete agent-socket port 42666

* fix: Only attach enabled ports on agent pod

If opentelemetry.grpc.enabled = false or opentelemetry.http.enabled =
false, ports 55680, 4317 and 4318 should not be attached to the
Daemonset.

* ci: Stabilize e2e test, retry to cat backend files

* fixup: Exit retry loop with break

* ci: Fix e2e to check for pod name again if exec failed

If the old agent pod is still terminating, it might be picked up
incorrectly in the later test. Therefore, every failed exec should
result in fetching the pod name again.

* test: Improve ports unit test

* test: Extend test coverage of ports builder tests

* Apply suggestions from code review

Co-authored-by: Milica-Cvrkota-IBM <[email protected]>

* test: Removed double check in e2e test

* fix: Tolerate legacy settings OLTP.Enabled.Enabled

If no legacy setting is defined, OLTP should be enabled by default.
If HTTP or GRPC is explicitly defined, use this value.
But, if OLTP is explicitly disabled by the legacy config, keep that
behavior and do not impliticly enable ports

* test: Refactor/simplify inline_types and unit tests

---------

Co-authored-by: Milica-Cvrkota-IBM <[email protected]>
  • Loading branch information
@konrad-ohms @Milica-Cvrkota-IBM
konrad-ohms and Milica-Cvrkota-IBM authored Aug 13, 2024
1 parent 7d800cb commit ac6c932
Show file tree
Hide file tree
Showing 8 changed files with 381 additions and 198 deletions.
29 changes: 18 additions & 11 deletions api/v1/inline_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"k8s.io/apimachinery/pkg/api/resource"

"github.com/instana/instana-agent-operator/pkg/map_defaulter"
"github.com/instana/instana-agent-operator/pkg/pointer"
)

type AgentMode string
Expand Down Expand Up @@ -368,21 +367,29 @@ type OpenTelemetry struct {
}

func (otlp OpenTelemetry) GrpcIsEnabled() bool {
switch otlp.GRPC {
case nil:
return true
default:
return pointer.DerefOrDefault(otlp.GRPC.Enabled, true)
// explicit opt-out or opt-in via new setting
if otlp.GRPC != nil && otlp.GRPC.Enabled != nil {
return *otlp.GRPC.Enabled
}
// explicit opt-out via legacy setting
if otlp.Enabled.Enabled != nil && !*otlp.Enabled.Enabled {
return false
}
// enabled by default
return true
}

func (otlp OpenTelemetry) HttpIsEnabled() bool {
switch otlp.HTTP {
case nil:
return true
default:
return pointer.DerefOrDefault(otlp.HTTP.Enabled, true)
// explicit opt-out or opt-in via new setting
if otlp.HTTP != nil && otlp.HTTP.Enabled != nil {
return *otlp.HTTP.Enabled
}
// explicit opt-out via legacy setting
if otlp.Enabled.Enabled != nil && !*otlp.Enabled.Enabled {
return false
}
// enabled by default
return true
}

func (otlp OpenTelemetry) IsEnabled() bool {
Expand Down
302 changes: 206 additions & 96 deletions api/v1/inline_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ limitations under the License.
package v1

import (
"encoding/json"
"fmt"
"testing"

Expand All @@ -28,8 +27,6 @@ import (

"github.com/instana/instana-agent-operator/pkg/map_defaulter"
"github.com/instana/instana-agent-operator/pkg/optional"
"github.com/instana/instana-agent-operator/pkg/or_die"
"github.com/instana/instana-agent-operator/pkg/pointer"
)

func TestImageSpec_Image(t *testing.T) {
Expand Down Expand Up @@ -83,116 +80,229 @@ func TestImageSpec_Image(t *testing.T) {
}
}

var (
allPossibleEnabled = []Enabled{
{},
func TestOpenTelemetry_GrpcIsEnabled(t *testing.T) {
enabled := true
disabled := false

for _, test := range []struct {
name string
openTelemetrySettings OpenTelemetry
expected bool
}{
{
Enabled: pointer.To(true),
name: "Enable by default, if nothing is defined",
openTelemetrySettings: OpenTelemetry{},
expected: true,
},
{
Enabled: pointer.To(false),
name: "Legacy setting enabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}},
expected: true,
},
}
allPossibleEnabledPtr = []*Enabled{
nil,
{},
{
Enabled: pointer.To(true),
name: "Legacy setting disabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}},
expected: false,
},
{
Enabled: pointer.To(false),
name: "New GRPC setting enabled, no legacy setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}},
expected: true,
},
}
)

func jsonStringOrDie(obj interface{}) string {
return string(
or_die.New[[]byte]().ResultOrDie(
func() ([]byte, error) {
return json.Marshal(obj)
},
),
)
}

func testForOtlp(t *testing.T, otlp *OpenTelemetry, getExpected func(otlp *OpenTelemetry) bool, getActual func() bool) {
t.Run(
jsonStringOrDie(otlp), func(t *testing.T) {
assertions := require.New(t)

expected := getExpected(otlp)
actual := getActual()

assertions.Equal(expected, actual)
{
name: "New GRPC setting disabled, no legacy setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &disabled}},
expected: false,
},
)
}

func grpcIsEnabled_expected(otlp *OpenTelemetry) bool {
switch grpc := otlp.GRPC; grpc {
case nil:
return true
default:
switch enabled := grpc.Enabled; enabled {
case nil:
return true
default:
return *enabled
}
}
}

func TestOpenTelemetry_GrpcIsEnabled(t *testing.T) {
for _, enabled := range allPossibleEnabled {
for _, grpc := range allPossibleEnabledPtr {
otlp := &OpenTelemetry{
Enabled: enabled,
GRPC: grpc,
}
testForOtlp(t, otlp, grpcIsEnabled_expected, otlp.GrpcIsEnabled)
}
}
}

func httpIsEnabled_expected(otlp *OpenTelemetry) bool {
switch http := otlp.HTTP; http {
case nil:
return true
default:
switch enabled := http.Enabled; enabled {
case nil:
return true
default:
return *enabled
}
{
name: "Explicit opt-in with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "Explicitly opt-out with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}, GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New setting opt-out, legacy setting opt-in -> new should win",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, GRPC: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New setting opt-in, legacy setting opt-out -> new should win",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}, GRPC: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New setting opt-in GRPC=true is not interfering with HTTP=true setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New setting opt-in GRPC=false is not interfering with HTTP=true setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: false,
},
{
name: "New setting opt-in GRPC=true is not interfering with HTTP=false setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: true,
},
} {
t.Run(test.name, func(t *testing.T) {
assertions := require.New(t)
assertions.Equal(test.expected, test.openTelemetrySettings.GrpcIsEnabled(), test.name)
})
}
}

func TestOpenTelemetry_HttpIsEnabled(t *testing.T) {
for _, http := range allPossibleEnabledPtr {
otlp := &OpenTelemetry{
HTTP: http,
}
testForOtlp(t, otlp, httpIsEnabled_expected, otlp.HttpIsEnabled)
}
}
enabled := true
disabled := false

func isEnabled_expected(otlp *OpenTelemetry) bool {
return grpcIsEnabled_expected(otlp) || httpIsEnabled_expected(otlp)
for _, test := range []struct {
name string
openTelemetrySettings OpenTelemetry
expected bool
}{
{
name: "Enable by default, if nothing is defined",
openTelemetrySettings: OpenTelemetry{},
expected: true,
},
{
name: "Legacy setting enabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "Legacy setting disabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New HTTP setting enabled, no legacy setting",
openTelemetrySettings: OpenTelemetry{HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New HTTP setting disabled, no legacy setting",
openTelemetrySettings: OpenTelemetry{HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "Explicit opt-in with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "Explicitly opt-out with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}, GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New setting opt-out, legacy setting opt-in -> new should win",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New setting opt-in, legacy setting opt-out -> new should win",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New setting opt-in HTTP=true is not interfering with GRPC=true setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New setting opt-in HTTP=false is not interfering with GRPC=true setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New setting opt-in HTTP=true is not interfering with GRPC=false setting",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
} {
t.Run(test.name, func(t *testing.T) {
assertions := require.New(t)
assertions.Equal(test.expected, test.openTelemetrySettings.HttpIsEnabled(), test.name)
})
}
}

func TestOpenTelemetry_IsEnabled(t *testing.T) {
for _, enabled := range allPossibleEnabled {
for _, grpc := range allPossibleEnabledPtr {
for _, http := range allPossibleEnabledPtr {
otlp := &OpenTelemetry{
Enabled: enabled,
GRPC: grpc,
HTTP: http,
}
testForOtlp(t, otlp, isEnabled_expected, otlp.IsEnabled)
}
}
enabled := true
disabled := false

for _, test := range []struct {
name string
openTelemetrySettings OpenTelemetry
expected bool
}{
{
name: "Enable by default, if nothing is defined",
openTelemetrySettings: OpenTelemetry{},
expected: true,
},
{
name: "Legacy setting enabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "Legacy setting disabled",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New HTTP setting enabled, no legacy setting",
openTelemetrySettings: OpenTelemetry{HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New HTTP setting disabled, no legacy setting, fallback to GPRC enabled -> overall enabled",
openTelemetrySettings: OpenTelemetry{HTTP: &Enabled{Enabled: &disabled}},
expected: true,
},
{
name: "New GRPC setting disabled, no legacy setting, fallback to HTTP enabled -> overall enabled",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &disabled}},
expected: true,
},
{
name: "Explicit opt-in with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "Explicitly opt-out with all settings",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &disabled}, GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New settings opt-out, legacy setting opt-in -> new should win",
openTelemetrySettings: OpenTelemetry{Enabled: Enabled{Enabled: &enabled}, GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: false,
},
{
name: "New settings explicit opt-in for HTTP and opt-out for GRPC -> overall true",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &disabled}, HTTP: &Enabled{Enabled: &enabled}},
expected: true,
},
{
name: "New settings explicit opt-out for HTTP and opt-in for GRPC -> overall true",
openTelemetrySettings: OpenTelemetry{GRPC: &Enabled{Enabled: &enabled}, HTTP: &Enabled{Enabled: &disabled}},
expected: true,
},
} {
t.Run(test.name, func(t *testing.T) {
assertions := require.New(t)
assertions.Equal(test.expected, test.openTelemetrySettings.IsEnabled(), test.name)
})
}
}

Expand Down
Loading

0 comments on commit ac6c932

Please sign in to comment.