From 012a913d4a12d42269f7aa464b23a78581715d8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Mon, 12 Feb 2024 12:26:39 +0100 Subject: [PATCH 1/4] Fix handling of type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name is not correctly split before it is checked. Split it first. Signed-off-by: Oldřich Jedlička --- internal/manifests/collector/parser/receiver/receiver.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/manifests/collector/parser/receiver/receiver.go b/internal/manifests/collector/parser/receiver/receiver.go index 97147658f4..554e306b5d 100644 --- a/internal/manifests/collector/parser/receiver/receiver.go +++ b/internal/manifests/collector/parser/receiver/receiver.go @@ -104,12 +104,13 @@ func isScraperReceiver(name string) bool { func singlePortFromConfigEndpoint(logger logr.Logger, name string, config map[interface{}]interface{}) *v1.ServicePort { var endpoint interface{} + var receiverType = receiverType(name) switch { // syslog receiver contains the endpoint // that needs to be exposed one level down inside config // i.e. either in tcp or udp section with field key // as `listen_address` - case name == "syslog": + case receiverType == "syslog": var c map[interface{}]interface{} if udp, isUDP := config["udp"]; isUDP && udp != nil { c = udp.(map[interface{}]interface{}) @@ -121,13 +122,13 @@ func singlePortFromConfigEndpoint(logger logr.Logger, name string, config map[in // tcplog and udplog receivers hold the endpoint // value in `listen_address` field - case name == "tcplog" || name == "udplog": + case receiverType == "tcplog" || receiverType == "udplog": endpoint = getAddressFromConfig(logger, name, listenAddressKey, config) // ignore the receiver as it holds the field key endpoint, and it // is a scraper, we only expose endpoint through k8s service objects for // receivers that aren't scrapers. - case isScraperReceiver(name): + case isScraperReceiver(receiverType): return nil default: From 69ab94528201cf0cecedca3e912e83d4ff0b307b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Mon, 12 Feb 2024 13:33:52 +0100 Subject: [PATCH 2/4] Split parsing of Syslog, TCP Log and UDP Log parsers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generic implementation does not allow to specify different endpoints and endpoint-specific protocols, so split the implementation instead of trying to make the generic implementation a complex catch-all-party. Signed-off-by: Oldřich Jedlička --- .../collector/parser/receiver/receiver.go | 19 ---- .../parser/receiver/receiver_syslog.go | 98 +++++++++++++++++++ .../parser/receiver/receiver_syslog_test.go | 64 ++++++++++++ .../parser/receiver/receiver_tcplog.go | 79 +++++++++++++++ .../parser/receiver/receiver_tcplog_test.go | 61 ++++++++++++ .../parser/receiver/receiver_udplog.go | 79 +++++++++++++++ .../parser/receiver/receiver_udplog_test.go | 61 ++++++++++++ 7 files changed, 442 insertions(+), 19 deletions(-) create mode 100644 internal/manifests/collector/parser/receiver/receiver_syslog.go create mode 100644 internal/manifests/collector/parser/receiver/receiver_syslog_test.go create mode 100644 internal/manifests/collector/parser/receiver/receiver_tcplog.go create mode 100644 internal/manifests/collector/parser/receiver/receiver_tcplog_test.go create mode 100644 internal/manifests/collector/parser/receiver/receiver_udplog.go create mode 100644 internal/manifests/collector/parser/receiver/receiver_udplog_test.go diff --git a/internal/manifests/collector/parser/receiver/receiver.go b/internal/manifests/collector/parser/receiver/receiver.go index 554e306b5d..127891747b 100644 --- a/internal/manifests/collector/parser/receiver/receiver.go +++ b/internal/manifests/collector/parser/receiver/receiver.go @@ -106,25 +106,6 @@ func singlePortFromConfigEndpoint(logger logr.Logger, name string, config map[in var endpoint interface{} var receiverType = receiverType(name) switch { - // syslog receiver contains the endpoint - // that needs to be exposed one level down inside config - // i.e. either in tcp or udp section with field key - // as `listen_address` - case receiverType == "syslog": - var c map[interface{}]interface{} - if udp, isUDP := config["udp"]; isUDP && udp != nil { - c = udp.(map[interface{}]interface{}) - endpoint = getAddressFromConfig(logger, name, listenAddressKey, c) - } else if tcp, isTCP := config["tcp"]; isTCP && tcp != nil { - c = tcp.(map[interface{}]interface{}) - endpoint = getAddressFromConfig(logger, name, listenAddressKey, c) - } - - // tcplog and udplog receivers hold the endpoint - // value in `listen_address` field - case receiverType == "tcplog" || receiverType == "udplog": - endpoint = getAddressFromConfig(logger, name, listenAddressKey, config) - // ignore the receiver as it holds the field key endpoint, and it // is a scraper, we only expose endpoint through k8s service objects for // receivers that aren't scrapers. diff --git a/internal/manifests/collector/parser/receiver/receiver_syslog.go b/internal/manifests/collector/parser/receiver/receiver_syslog.go new file mode 100644 index 0000000000..582cb5c2be --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_syslog.go @@ -0,0 +1,98 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "fmt" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser" + "github.com/open-telemetry/opentelemetry-operator/internal/naming" +) + +var _ parser.ComponentPortParser = &SyslogReceiverParser{} + +const parserNameSyslog = "__syslog" + +// SyslogReceiverParser parses the configuration for TCP log receivers. +type SyslogReceiverParser struct { + config map[interface{}]interface{} + logger logr.Logger + name string +} + +// NewSyslogReceiverParser builds a new parser for TCP log receivers. +func NewSyslogReceiverParser(logger logr.Logger, name string, config map[interface{}]interface{}) parser.ComponentPortParser { + return &SyslogReceiverParser{ + logger: logger, + name: name, + config: config, + } +} + +func (o *SyslogReceiverParser) Ports() ([]corev1.ServicePort, error) { + var endpoint interface{} + var endpointName string + var protocol corev1.Protocol + var c map[interface{}]interface{} + + // syslog receiver contains the endpoint + // that needs to be exposed one level down inside config + // i.e. either in tcp or udp section with field key + // as `listen_address` + if tcp, isTCP := o.config["tcp"]; isTCP && tcp != nil { + c = tcp.(map[interface{}]interface{}) + endpointName = "tcp" + endpoint = getAddressFromConfig(o.logger, o.name, listenAddressKey, c) + protocol = corev1.ProtocolTCP + } else if udp, isUDP := o.config["udp"]; isUDP && udp != nil { + c = udp.(map[interface{}]interface{}) + endpointName = "udp" + endpoint = getAddressFromConfig(o.logger, o.name, listenAddressKey, c) + protocol = corev1.ProtocolUDP + } + + switch e := endpoint.(type) { + case nil: + break + case string: + port, err := portFromEndpoint(e) + if err != nil { + o.logger.WithValues(listenAddressKey, e).Error(err, fmt.Sprintf("couldn't parse the %s endpoint's port", endpointName)) + return nil, nil + } + + return []corev1.ServicePort{{ + Port: port, + Name: naming.PortName(o.name, port), + Protocol: protocol, + }}, nil + default: + o.logger.WithValues(listenAddressKey, endpoint).Error(fmt.Errorf("unrecognized type %T of %s endpoint", endpoint, endpointName), + "receiver's endpoint isn't a string") + } + + return []corev1.ServicePort{}, nil +} + +func (o *SyslogReceiverParser) ParserName() string { + return parserNameSyslog +} + +func init() { + Register("syslog", NewSyslogReceiverParser) +} diff --git a/internal/manifests/collector/parser/receiver/receiver_syslog_test.go b/internal/manifests/collector/parser/receiver/receiver_syslog_test.go new file mode 100644 index 0000000000..fd7ea9daf5 --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_syslog_test.go @@ -0,0 +1,64 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestSyslogSelfRegisters(t *testing.T) { + // verify + assert.True(t, IsRegistered("syslog")) +} + +func TestSyslogIsFoundByName(t *testing.T) { + // test + p, err := For(logger, "syslog", map[interface{}]interface{}{}) + assert.NoError(t, err) + + // verify + assert.Equal(t, "__syslog", p.ParserName()) +} + +func TestSyslogConfiguration(t *testing.T) { + for _, tt := range []struct { + desc string + config map[interface{}]interface{} + expected []corev1.ServicePort + }{ + {"Empty configuration", map[interface{}]interface{}{}, []corev1.ServicePort{}}, + {"UDP port configuration", + map[interface{}]interface{}{"udp": map[interface{}]interface{}{"listen_address": "0.0.0.0:1234"}}, + []corev1.ServicePort{{Name: "syslog", Port: 1234, Protocol: corev1.ProtocolUDP}}}, + {"TCP port configuration", + map[interface{}]interface{}{"tcp": map[interface{}]interface{}{"listen_address": "0.0.0.0:1234"}}, + []corev1.ServicePort{{Name: "syslog", Port: 1234, Protocol: corev1.ProtocolTCP}}}, + } { + t.Run(tt.desc, func(t *testing.T) { + // prepare + builder := NewSyslogReceiverParser(logger, "syslog", tt.config) + + // test + ports, err := builder.Ports() + + // verify + assert.NoError(t, err) + assert.Equal(t, ports, tt.expected) + }) + } +} diff --git a/internal/manifests/collector/parser/receiver/receiver_tcplog.go b/internal/manifests/collector/parser/receiver/receiver_tcplog.go new file mode 100644 index 0000000000..c0232415a9 --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_tcplog.go @@ -0,0 +1,79 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "fmt" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser" + "github.com/open-telemetry/opentelemetry-operator/internal/naming" +) + +var _ parser.ComponentPortParser = &TcpLogReceiverParser{} + +const parserNameTcpLog = "__tcplog" + +// TcpLogReceiverParser parses the configuration for TCP log receivers. +type TcpLogReceiverParser struct { + config map[interface{}]interface{} + logger logr.Logger + name string +} + +// NewTcpLogReceiverParser builds a new parser for TCP log receivers. +func NewTcpLogReceiverParser(logger logr.Logger, name string, config map[interface{}]interface{}) parser.ComponentPortParser { + return &TcpLogReceiverParser{ + logger: logger, + name: name, + config: config, + } +} + +func (o *TcpLogReceiverParser) Ports() ([]corev1.ServicePort, error) { + // tcplog receiver hold the endpoint value in `listen_address` field + var endpoint = getAddressFromConfig(o.logger, o.name, listenAddressKey, o.config) + + switch e := endpoint.(type) { + case nil: + break + case string: + port, err := portFromEndpoint(e) + if err != nil { + o.logger.WithValues(listenAddressKey, e).Error(err, "couldn't parse the endpoint's port") + return nil, nil + } + + return []corev1.ServicePort{{ + Port: port, + Name: naming.PortName(o.name, port), + Protocol: corev1.ProtocolTCP, + }}, nil + default: + o.logger.WithValues(listenAddressKey, endpoint).Error(fmt.Errorf("unrecognized type %T", endpoint), "receiver's endpoint isn't a string") + } + + return []corev1.ServicePort{}, nil +} + +func (o *TcpLogReceiverParser) ParserName() string { + return parserNameTcpLog +} + +func init() { + Register("tcplog", NewTcpLogReceiverParser) +} diff --git a/internal/manifests/collector/parser/receiver/receiver_tcplog_test.go b/internal/manifests/collector/parser/receiver/receiver_tcplog_test.go new file mode 100644 index 0000000000..04b4eb03be --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_tcplog_test.go @@ -0,0 +1,61 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestTcpLogSelfRegisters(t *testing.T) { + // verify + assert.True(t, IsRegistered("tcplog")) +} + +func TestTcpLogIsFoundByName(t *testing.T) { + // test + p, err := For(logger, "tcplog", map[interface{}]interface{}{}) + assert.NoError(t, err) + + // verify + assert.Equal(t, "__tcplog", p.ParserName()) +} + +func TestTcpLogConfiguration(t *testing.T) { + for _, tt := range []struct { + desc string + config map[interface{}]interface{} + expected []corev1.ServicePort + }{ + {"Empty configuration", map[interface{}]interface{}{}, []corev1.ServicePort{}}, + {"TCP port configuration", + map[interface{}]interface{}{"listen_address": "0.0.0.0:1234"}, + []corev1.ServicePort{{Name: "tcplog", Port: 1234, Protocol: corev1.ProtocolTCP}}}, + } { + t.Run(tt.desc, func(t *testing.T) { + // prepare + builder := NewTcpLogReceiverParser(logger, "tcplog", tt.config) + + // test + ports, err := builder.Ports() + + // verify + assert.NoError(t, err) + assert.Equal(t, ports, tt.expected) + }) + } +} diff --git a/internal/manifests/collector/parser/receiver/receiver_udplog.go b/internal/manifests/collector/parser/receiver/receiver_udplog.go new file mode 100644 index 0000000000..4f55e5b492 --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_udplog.go @@ -0,0 +1,79 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "fmt" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/parser" + "github.com/open-telemetry/opentelemetry-operator/internal/naming" +) + +var _ parser.ComponentPortParser = &UdpLogReceiverParser{} + +const parserNameUdpLog = "__udplog" + +// UdpLogReceiverParser parses the configuration for UDP log receivers. +type UdpLogReceiverParser struct { + config map[interface{}]interface{} + logger logr.Logger + name string +} + +// NewUdpLogReceiverParser builds a new parser for UDP log receivers. +func NewUdpLogReceiverParser(logger logr.Logger, name string, config map[interface{}]interface{}) parser.ComponentPortParser { + return &UdpLogReceiverParser{ + logger: logger, + name: name, + config: config, + } +} + +func (o *UdpLogReceiverParser) Ports() ([]corev1.ServicePort, error) { + // udplog receiver hold the endpoint value in `listen_address` field + var endpoint = getAddressFromConfig(o.logger, o.name, listenAddressKey, o.config) + + switch e := endpoint.(type) { + case nil: + break + case string: + port, err := portFromEndpoint(e) + if err != nil { + o.logger.WithValues(listenAddressKey, e).Error(err, "couldn't parse the endpoint's port") + return nil, nil + } + + return []corev1.ServicePort{{ + Port: port, + Name: naming.PortName(o.name, port), + Protocol: corev1.ProtocolUDP, + }}, nil + default: + o.logger.WithValues(listenAddressKey, endpoint).Error(fmt.Errorf("unrecognized type %T", endpoint), "receiver's endpoint isn't a string") + } + + return []corev1.ServicePort{}, nil +} + +func (o *UdpLogReceiverParser) ParserName() string { + return parserNameUdpLog +} + +func init() { + Register("udplog", NewUdpLogReceiverParser) +} diff --git a/internal/manifests/collector/parser/receiver/receiver_udplog_test.go b/internal/manifests/collector/parser/receiver/receiver_udplog_test.go new file mode 100644 index 0000000000..1b12191ed5 --- /dev/null +++ b/internal/manifests/collector/parser/receiver/receiver_udplog_test.go @@ -0,0 +1,61 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package receiver + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestUdpLogSelfRegisters(t *testing.T) { + // verify + assert.True(t, IsRegistered("udplog")) +} + +func TestUdpLogIsFoundByName(t *testing.T) { + // test + p, err := For(logger, "udplog", map[interface{}]interface{}{}) + assert.NoError(t, err) + + // verify + assert.Equal(t, "__udplog", p.ParserName()) +} + +func TestUdpLogConfiguration(t *testing.T) { + for _, tt := range []struct { + desc string + config map[interface{}]interface{} + expected []corev1.ServicePort + }{ + {"Empty configuration", map[interface{}]interface{}{}, []corev1.ServicePort{}}, + {"UDP port configuration", + map[interface{}]interface{}{"listen_address": "0.0.0.0:1234"}, + []corev1.ServicePort{{Name: "udplog", Port: 1234, Protocol: corev1.ProtocolUDP}}}, + } { + t.Run(tt.desc, func(t *testing.T) { + // prepare + builder := NewUdpLogReceiverParser(logger, "udplog", tt.config) + + // test + ports, err := builder.Ports() + + // verify + assert.NoError(t, err) + assert.Equal(t, ports, tt.expected) + }) + } +} From dd041128fe441802447eebdb376a0664b9407a49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Mon, 12 Feb 2024 14:52:51 +0100 Subject: [PATCH 3/4] Handle port numbers together with protocols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is legal to have the same protocol number and different protocol, so handle it accordingly. Also interpret empty protocol as TCP, like the K8s does. Signed-off-by: Oldřich Jedlička --- internal/manifests/collector/service.go | 27 ++++- internal/manifests/collector/service_test.go | 101 +++++++++++++++---- 2 files changed, 101 insertions(+), 27 deletions(-) diff --git a/internal/manifests/collector/service.go b/internal/manifests/collector/service.go index b2b4035ac1..bda5fce76b 100644 --- a/internal/manifests/collector/service.go +++ b/internal/manifests/collector/service.go @@ -186,8 +186,25 @@ func Service(params manifests.Params) (*corev1.Service, error) { }, nil } -func filterPort(logger logr.Logger, candidate corev1.ServicePort, portNumbers map[int32]bool, portNames map[string]bool) *corev1.ServicePort { - if portNumbers[candidate.Port] { +type PortNumberKey struct { + Port int32 + Protocol corev1.Protocol +} + +func newPortNumberKeyByPort(port int32) PortNumberKey { + return PortNumberKey{Port: port, Protocol: corev1.ProtocolTCP} +} + +func newPortNumberKey(port int32, protocol corev1.Protocol) PortNumberKey { + if protocol == "" { + // K8s defaults to TCP if protocol is empty, so evaluate the port the same + protocol = corev1.ProtocolTCP + } + return PortNumberKey{Port: port, Protocol: protocol} +} + +func filterPort(logger logr.Logger, candidate corev1.ServicePort, portNumbers map[PortNumberKey]bool, portNames map[string]bool) *corev1.ServicePort { + if portNumbers[newPortNumberKey(candidate.Port, candidate.Protocol)] { return nil } @@ -212,12 +229,12 @@ func filterPort(logger logr.Logger, candidate corev1.ServicePort, portNumbers ma return &candidate } -func extractPortNumbersAndNames(ports []corev1.ServicePort) (map[int32]bool, map[string]bool) { - numbers := map[int32]bool{} +func extractPortNumbersAndNames(ports []corev1.ServicePort) (map[PortNumberKey]bool, map[string]bool) { + numbers := map[PortNumberKey]bool{} names := map[string]bool{} for _, port := range ports { - numbers[port.Port] = true + numbers[newPortNumberKey(port.Port, port.Protocol)] = true names[port.Name] = true } diff --git a/internal/manifests/collector/service_test.go b/internal/manifests/collector/service_test.go index fcc6e5b8d7..8c263dc694 100644 --- a/internal/manifests/collector/service_test.go +++ b/internal/manifests/collector/service_test.go @@ -30,9 +30,19 @@ import ( func TestExtractPortNumbersAndNames(t *testing.T) { t.Run("should return extracted port names and numbers", func(t *testing.T) { - ports := []v1.ServicePort{{Name: "web", Port: 8080}, {Name: "tcp", Port: 9200}} - expectedPortNames := map[string]bool{"web": true, "tcp": true} - expectedPortNumbers := map[int32]bool{8080: true, 9200: true} + ports := []v1.ServicePort{ + {Name: "web", Port: 8080}, + {Name: "tcp", Port: 9200}, + {Name: "web-explicit", Port: 80, Protocol: v1.ProtocolTCP}, + {Name: "syslog-udp", Port: 514, Protocol: v1.ProtocolUDP}, + } + expectedPortNames := map[string]bool{"web": true, "tcp": true, "web-explicit": true, "syslog-udp": true} + expectedPortNumbers := map[PortNumberKey]bool{ + newPortNumberKey(8080, v1.ProtocolTCP): true, + newPortNumberKey(9200, v1.ProtocolTCP): true, + newPortNumberKey(80, v1.ProtocolTCP): true, + newPortNumberKey(514, v1.ProtocolUDP): true, + } actualPortNumbers, actualPortNames := extractPortNumbersAndNames(ports) assert.Equal(t, expectedPortNames, actualPortNames) @@ -46,38 +56,85 @@ func TestFilterPort(t *testing.T) { tests := []struct { name string candidate v1.ServicePort - portNumbers map[int32]bool + portNumbers map[PortNumberKey]bool portNames map[string]bool expected v1.ServicePort }{ { - name: "should filter out duplicate port", - candidate: v1.ServicePort{Name: "web", Port: 8080}, - portNumbers: map[int32]bool{8080: true, 9200: true}, - portNames: map[string]bool{"test": true, "metrics": true}, + name: "should filter out duplicate port", + candidate: v1.ServicePort{Name: "web", Port: 8080}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + }, + + { + name: "should filter out duplicate port, protocol specified (TCP)", + candidate: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolTCP}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + }, + + { + name: "should filter out duplicate port, protocol specified (UDP)", + candidate: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolUDP}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKey(8080, v1.ProtocolUDP): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + }, + + { + name: "should not filter unique port", + candidate: v1.ServicePort{Name: "web", Port: 8090}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + expected: v1.ServicePort{Name: "web", Port: 8090}, + }, + + { + name: "should not filter same port with different protocols", + candidate: v1.ServicePort{Name: "web", Port: 8080}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKey(8080, v1.ProtocolUDP): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + expected: v1.ServicePort{Name: "web", Port: 8080}, + }, + + { + name: "should not filter same port with different protocols, candidate has specified port (TCP vs UDP)", + candidate: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolTCP}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKey(8080, v1.ProtocolUDP): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + expected: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolTCP}, }, { - name: "should not filter unique port", - candidate: v1.ServicePort{Name: "web", Port: 8090}, - portNumbers: map[int32]bool{8080: true, 9200: true}, - portNames: map[string]bool{"test": true, "metrics": true}, - expected: v1.ServicePort{Name: "web", Port: 8090}, + name: "should not filter same port with different protocols, candidate has specified port (UDP vs TCP)", + candidate: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolUDP}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"test": true, "metrics": true}, + expected: v1.ServicePort{Name: "web", Port: 8080, Protocol: v1.ProtocolUDP}, }, { - name: "should change the duplicate portName", - candidate: v1.ServicePort{Name: "web", Port: 8090}, - portNumbers: map[int32]bool{8080: true, 9200: true}, - portNames: map[string]bool{"web": true, "metrics": true}, - expected: v1.ServicePort{Name: "port-8090", Port: 8090}, + name: "should change the duplicate portName", + candidate: v1.ServicePort{Name: "web", Port: 8090}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"web": true, "metrics": true}, + expected: v1.ServicePort{Name: "port-8090", Port: 8090}, }, { - name: "should return nil if fallback name clashes with existing portName", - candidate: v1.ServicePort{Name: "web", Port: 8090}, - portNumbers: map[int32]bool{8080: true, 9200: true}, - portNames: map[string]bool{"web": true, "port-8090": true}, + name: "should return nil if fallback name clashes with existing portName", + candidate: v1.ServicePort{Name: "web", Port: 8090}, + portNumbers: map[PortNumberKey]bool{ + newPortNumberKeyByPort(8080): true, newPortNumberKeyByPort(9200): true}, + portNames: map[string]bool{"web": true, "port-8090": true}, }, } for _, test := range tests { From 72d8af364fd2ee6ee8cb53b77ac2f516a4759119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Old=C5=99ich=20Jedli=C4=8Dka?= Date: Tue, 13 Feb 2024 12:22:32 +0100 Subject: [PATCH 4/4] Changelog updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Oldřich Jedlička --- .../fix-protocol-handling-in-serviceports.yaml | 18 ++++++++++++++++++ .chloggen/fix-syslog-tcplog-udplog.yaml | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 .chloggen/fix-protocol-handling-in-serviceports.yaml create mode 100644 .chloggen/fix-syslog-tcplog-udplog.yaml diff --git a/.chloggen/fix-protocol-handling-in-serviceports.yaml b/.chloggen/fix-protocol-handling-in-serviceports.yaml new file mode 100644 index 0000000000..29985f54a9 --- /dev/null +++ b/.chloggen/fix-protocol-handling-in-serviceports.yaml @@ -0,0 +1,18 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Fixed handling of protocol in exposed ports." + +# One or more tracking issues related to the change +issues: [2619] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + Make distinction not only on the port number, but also on protocol. This fix allows to have multiple exposed + ServicePorts with the same port number, but different protocols. diff --git a/.chloggen/fix-syslog-tcplog-udplog.yaml b/.chloggen/fix-syslog-tcplog-udplog.yaml new file mode 100644 index 0000000000..cedf0cb242 --- /dev/null +++ b/.chloggen/fix-syslog-tcplog-udplog.yaml @@ -0,0 +1,18 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Fixed handling of exposed port protocol in syslog, tcplog and udplog receivers." + +# One or more tracking issues related to the change +issues: [767,2619] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + Please note that the operator currently exposes just one port (tcp or udp) of syslog receiver due to the current + receiver implementation (patches are welcome).