Skip to content

feat(signoz): adds new http ingress for otel-collector #667

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.

Sign up for GitHub

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

Open
suyash-811 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(signoz): adds new http ingress for otel-collector #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0833636
feat(signoz): adds new http ingress for otel-collector
May 15, 2025
c1ea477
fix(signoz): fixes incorrect gRPC service port
May 15, 2025
bd4189f
fix(signoz): reverts changes to default grpc ingress. creates new fie…
May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions charts/signoz/templates/otel-collector/ingress-http.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
{{- if .Values.otelCollector.httpIngress.enabled -}}
{{- $fullName := include "otelCollector.fullname" . -}}
{{- $ingressApiIsStable := eq (include "ingress.isStable" .) "true" -}}
{{- $ingressSupportsPathType := eq (include "ingress.supportsPathType" .) "true" -}}
Comment on lines +1 to +4

@coderabbitai coderabbitai Bot May 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Proper conditional rendering with feature flag
Rendering is correctly gated by .Values.otelCollector.httpIngress.enabled and helper variables are initialized for later use.

Ensure that otelCollector.httpIngress.enabled and related settings are documented in values.yaml and README.md:

🏁 Script executed:

#!/bin/bash
# Verify httpIngress settings are documented
rg -n 'httpIngress' charts/signoz/values.yaml README.md

Length of output: 101

Document HTTP Ingress in README.md
charts/signoz/values.yaml already defines the otelCollector.httpIngress block (line 1237), but the feature is not mentioned in README.md. Please add documentation for the new HTTP Ingress flags, for example under the “Configuration” or “Values” section:

  • otelCollector.httpIngress.enabled
  • otelCollector.httpIngress.hosts (and any other relevant fields)

This will keep your feature flags discoverable and maintain consistency between values.yaml and the chart documentation.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

🤖 Prompt for AI Agents 
In charts/signoz/templates/otel-collector/ingress-http.yaml lines 1 to 4, the
HTTP Ingress feature flag otelCollector.httpIngress.enabled is properly used,
but the related configuration options such as otelCollector.httpIngress.enabled
and otelCollector.httpIngress.hosts are not documented in README.md. To fix
this, add clear documentation for these HTTP Ingress flags in the README.md file
under the Configuration or Values section, describing their purpose and usage to
ensure users can discover and understand these settings.

apiVersion: {{ include "ingress.apiVersion" . }}
kind: Ingress
metadata:
name: {{ $fullName }}-http
labels:
{{- include "otelCollector.labels" . | nindent 4 }}
{{- with .Values.otelCollector.httpIngress.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
spec:
{{- if and .Values.otelCollector.httpIngress.className (semverCompare ">=1.18-0" .Capabilities.KubeVersion.GitVersion) }}
ingressClassName: {{ .Values.otelCollector.httpIngress.className }}
{{- end }}
{{- if .Values.otelCollector.httpIngress.tls }}
tls:
{{- range .Values.otelCollector.httpIngress.tls }}
- hosts:
{{- range .hosts }}
- {{ . | quote }}
{{- end }}
{{- with .secretName }}
secretName: {{ . }}
{{- end }}
{{- end }}
{{- end }}
rules:
{{- range .Values.otelCollector.httpIngress.hosts }}
- host: {{ .host | quote }}
http:
paths:
{{- range .paths }}
- path: {{ .path }}
{{- if $ingressSupportsPathType }}
pathType: {{ .pathType }}
{{- end }}
Comment on lines +38 to +40

@coderabbitai coderabbitai Bot May 17, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Guard pathType on value existence
Currently, pathType is only conditioned on ingress version support. If .pathType is unset, this may render an empty pathType:.

Wrap both checks together:

- {{- if $ingressSupportsPathType }}
+ {{- if and $ingressSupportsPathType .pathType }}
    pathType: {{ .pathType }}
  {{- end }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{{- if $ingressSupportsPathType }}
pathType: {{ .pathType }}
{{- end }}
{{- if and $ingressSupportsPathType .pathType }}
pathType: {{ .pathType }}
{{- end }}
🤖 Prompt for AI Agents 
In charts/signoz/templates/otel-collector/ingress-http.yaml around lines 38 to
40, the pathType field is only conditionally rendered based on ingress version
support, but it does not check if the .pathType value is set, which can result
in an empty pathType: line. Update the condition to check both that ingress
supports pathType and that .pathType is set before rendering the pathType field
to avoid rendering an empty value.

@suyash-811 suyash-811 May 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@grandwizard28 for the change mentioned above:

  1. Should that be applied to the file? If yes, should i also add it to the default (grpc) ingress?
  2. Instead of checking the pathType, how do you feel about enforcing a default pathType of ImplementationSpecific ?

@coderabbitai coderabbitai Bot May 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

backend:
{{- if $ingressApiIsStable }}
service:
name: {{ $fullName }}
port:
number: {{ .port }}
{{- else }}
serviceName: {{ $fullName }}
servicePort: {{ .port }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}
27 changes: 26 additions & 1 deletion charts/signoz/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,7 @@ otelCollector:
# -- Extra volumes for OtelCollector pod
extraVolumes: []
ingress:
# -- Enable ingress for OtelCollector
# -- Enable gRPC ingress for OtelCollector
enabled: false
# -- Ingress Class Name to be used to identify ingress controllers
className: ""
Expand All @@ -1222,6 +1222,31 @@ otelCollector:
# kubernetes.io/ingress.class: nginx
# kubernetes.io/tls-acme: "true"
# -- OtelCollector Ingress Host names with their path details
hosts:
- host: otelcollector.domain.com
paths:
- path: /
pathType: ImplementationSpecific
port: 4317
# -- OtelCollector Ingress TLS
tls: []
# - secretName: chart-example-tls
# hosts:
# - otelcollector.domain.com

httpIngress:
# -- Enable HTTP ingress for OtelCollector. Creates separate ingress for HTTP.
enabled: false
# -- Ingress Class Name to be used to identify ingress controllers
className: ""
# -- Annotations to OtelCollector Ingress
annotations: {}
# cert-manager.io/cluster-issuer: letsencrypt-prod
# nginx.ingress.kubernetes.io/ssl-redirect: "true"
# nginx.ingress.kubernetes.io/backend-protocol: "HTTP"
# kubernetes.io/ingress.class: nginx
# kubernetes.io/tls-acme: "true"
# -- OtelCollector Ingress Host names with their path details
hosts:
- host: otelcollector.domain.com
paths:
Expand Down