Add ingress options #1128

frzifus · 2022-09-27T08:48:46Z

Part of: #902

Example:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
  mode: "deployment"
  ingress:
    type: ingress
    hostname: ""
    annotations:
      something.com: "true"
  config: |
    receivers:
      otlp:
        protocols:
          grpc:
          http:
    processors:
      memory_limiter:
        check_interval: 1s
        limit_percentage: 75
        spike_limit_percentage: 15
      batch:
        send_batch_size: 10000
        timeout: 10s

    exporters:
      logging:

    service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [logging]

Configure kind:

cat <<EOF | kind create cluster --config=-
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
  kubeadmConfigPatches:
  - |
    kind: InitConfiguration
    nodeRegistration:
      kubeletExtraArgs:
        node-labels: "ingress-ready=true"
  extraPortMappings:
  - containerPort: 80
    hostPort: 80
    protocol: TCP
  - containerPort: 443
    hostPort: 443
    protocol: TCP
EOF

pavolloffay

There are missing tests for this PR - controller, e2e.

apis/v1alpha1/opentelemetrycollector_types.go

Signed-off-by: Benedikt Bongartz <[email protected]>

pavolloffay · 2022-10-10T15:33:16Z

linting failed

frzifus · 2022-10-10T15:39:02Z

y, iam working on it. Think i messed up some things while reordering my commits 🤦

pavolloffay

Some comments:

the webhook is missing tests
there is a missing constant for none with a defaulting webhook (if a webhook is needed to set it as default).

apis/v1alpha1/ingress_type.go

apis/v1alpha1/opentelemetrycollector_types.go

apis/v1alpha1/opentelemetrycollector_webhook.go

pavolloffay · 2022-10-10T15:38:28Z

apis/v1alpha1/opentelemetrycollector_webhook.go

@@ -166,5 +167,12 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error {

 	}

+	mode := strings.ToLower(string(r.Spec.Mode))
+	if r.Spec.Ingress.Type == IngressTypeNginx && (mode == "deployment" || mode == "daemonset" || mode == "statefulset") {


for mode it's better to use the defined mode constants

The if statement does not seem correct. It can be easier to compare if the mode is sidecar as it is the only not supported config.

you mean r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode != ModeSidecar ? Thats the check i made in the beginning. But as soon as another mode gets added, the condition is invalid.

I mean r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode == ModeSidecar then throw an error.

If a new mode is added we don't know if ingress will be supported or not

my intention to test for the supported modis was to be a bit more restrictive. If a new mode is added, the documentation will have to be adapted anyway. But have there no strong opinion and changed it. :)

apis/v1alpha1/opentelemetrycollector_webhook.go

pavolloffay · 2022-10-10T15:47:33Z

apis/v1alpha1/opentelemetrycollector_types.go

+
+	// Hostname by which the ingress proxy can be reached.
+	// +optional
+	Hostname string `json:"hostname,omitempty"`


Could you please explain this configuration? Why is it needed?

https://kubernetes.io/docs/concepts/services-networking/ingress/

PS: I don't have much experience with ingress so I might ask something stupid here.

With the ingress entry we want to direct incoming traffic on port 80 or 443 to our service. Without a set hostname, this applies to all domains, which can not be intentional. For example if traffic with the same path but from another domain should be directed to another service.

It could look like this:

NAMESPACE NAME HOST/PORT PATH SERVICES PORT test123 jaeger-skupper example.com /traces jaeger-skupper-collector https-collector test456 jaeger-ingress another-example.com /traces jaeger-ingress-collector grpc-collector

Signed-off-by: Benedikt Bongartz <[email protected]>

apis/v1alpha1/ingress_type.go

apis/v1alpha1/opentelemetrycollector_types.go

pkg/collector/reconcile/ingress.go

Signed-off-by: Benedikt Bongartz <[email protected]>

* add ingress name generation function Signed-off-by: Benedikt Bongartz <[email protected]> * extend otelcol crd with minimalistic ingress options Signed-off-by: Benedikt Bongartz <[email protected]> * support collector ingress reconciling Signed-off-by: Benedikt Bongartz <[email protected]> * register ingress reconciler Signed-off-by: Benedikt Bongartz <[email protected]> * grant permission to create, modify and delete ingress entries Signed-off-by: Benedikt Bongartz <[email protected]> * add ingress integration tests Signed-off-by: Benedikt Bongartz <[email protected]> * verify if collector mode is compatible with ingress settings Signed-off-by: Benedikt Bongartz <[email protected]> * create dedicated ingress type Signed-off-by: Benedikt Bongartz <[email protected]> * follow recommendations Signed-off-by: Benedikt Bongartz <[email protected]> * regenerate Signed-off-by: Benedikt Bongartz <[email protected]> Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the add_ingress_options branch 3 times, most recently from d2d45d4 to b8a6bbc Compare October 6, 2022 12:57

frzifus marked this pull request as ready for review October 6, 2022 13:40

frzifus requested a review from a team October 6, 2022 13:40

pavolloffay reviewed Oct 6, 2022

View reviewed changes

apis/v1alpha1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved

apis/v1alpha1/opentelemetrycollector_types.go Show resolved Hide resolved

frzifus force-pushed the add_ingress_options branch 4 times, most recently from 003e709 to 2bf168b Compare October 10, 2022 12:24

frzifus added 5 commits October 10, 2022 17:03

add ingress name generation function

6e2151b

Signed-off-by: Benedikt Bongartz <[email protected]>

extend otelcol crd with minimalistic ingress options

df7f276

Signed-off-by: Benedikt Bongartz <[email protected]>

support collector ingress reconciling

906d0d0

Signed-off-by: Benedikt Bongartz <[email protected]>

register ingress reconciler

abc520f

Signed-off-by: Benedikt Bongartz <[email protected]>

grant permission to create, modify and delete ingress entries

ddc468c

Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the add_ingress_options branch from 2bf168b to 409d6e4 Compare October 10, 2022 15:03

frzifus force-pushed the add_ingress_options branch from 409d6e4 to 9f50e48 Compare October 10, 2022 15:34

pavolloffay reviewed Oct 10, 2022

View reviewed changes

frzifus force-pushed the add_ingress_options branch 3 times, most recently from dcc99bb to 3854e23 Compare October 10, 2022 18:48

frzifus added 3 commits October 10, 2022 21:21

add ingress integration tests

f2faf42

Signed-off-by: Benedikt Bongartz <[email protected]>

verify if collector mode is compatible with ingress settings

5cd9c95

Signed-off-by: Benedikt Bongartz <[email protected]>

create dedicated ingress type

5689a0d

Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the add_ingress_options branch 3 times, most recently from 54c8a0b to cdd3815 Compare October 10, 2022 19:47

pavolloffay reviewed Oct 11, 2022

View reviewed changes

apis/v1alpha1/ingress_type.go Show resolved Hide resolved

apis/v1alpha1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved

pkg/collector/reconcile/ingress.go Outdated Show resolved Hide resolved

frzifus added 2 commits October 11, 2022 11:19

follow recommendations

9d292d5

Signed-off-by: Benedikt Bongartz <[email protected]>

regenerate

dd031a6

Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the add_ingress_options branch from cdd3815 to dd031a6 Compare October 11, 2022 09:20

frzifus requested a review from pavolloffay October 11, 2022 09:22

pavolloffay approved these changes Oct 11, 2022

View reviewed changes

pavolloffay merged commit 8783fe1 into open-telemetry:main Oct 11, 2022

frzifus deleted the add_ingress_options branch October 11, 2022 10:33

pavolloffay mentioned this pull request Oct 26, 2022

Nominate @frzifus to approver #1207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ingress options #1128

Add ingress options #1128

frzifus commented Sep 27, 2022 •

edited

Loading

pavolloffay left a comment

pavolloffay commented Oct 10, 2022

frzifus commented Oct 10, 2022

pavolloffay left a comment

pavolloffay Oct 10, 2022

pavolloffay Oct 10, 2022

frzifus Oct 10, 2022

pavolloffay Oct 11, 2022

frzifus Oct 11, 2022

pavolloffay Oct 10, 2022

frzifus Oct 10, 2022

Add ingress options #1128

Add ingress options #1128

Conversation

frzifus commented Sep 27, 2022 • edited Loading

pavolloffay left a comment

Choose a reason for hiding this comment

pavolloffay commented Oct 10, 2022

frzifus commented Oct 10, 2022

pavolloffay left a comment

Choose a reason for hiding this comment

pavolloffay Oct 10, 2022

Choose a reason for hiding this comment

pavolloffay Oct 10, 2022

Choose a reason for hiding this comment

frzifus Oct 10, 2022

Choose a reason for hiding this comment

pavolloffay Oct 11, 2022

Choose a reason for hiding this comment

frzifus Oct 11, 2022

Choose a reason for hiding this comment

pavolloffay Oct 10, 2022

Choose a reason for hiding this comment

frzifus Oct 10, 2022

Choose a reason for hiding this comment

frzifus commented Sep 27, 2022 •

edited

Loading