Skip to content
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.

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

[fluentd] Add Ingress. #281

Merged
merged 2 commits into from
Nov 15, 2023
Merged

[fluentd] Add Ingress. #281

merged 2 commits into from
Nov 15, 2023

Conversation

pecastro
Copy link
Contributor

Add Ingress resource to fluentd for Deployment/StatefulSet options.

@pecastro pecastro force-pushed the fluentd-add-ingress branch from 18115d4 to 5ed3ba3 Compare November 10, 2022 22:11
@dioguerra dioguerra changed the title fluentd - Add Ingress. [fluentd] Add Ingress. May 2, 2023
@dioguerra
Copy link
Collaborator

This should be ok. Do you mind rebasing?

@pecastro pecastro force-pushed the fluentd-add-ingress branch from 5ed3ba3 to b0a3d8a Compare May 12, 2023 10:35
@pecastro
Copy link
Contributor Author

@dioguerra Done.

Copy link
Collaborator

@dioguerra dioguerra left a comment

Choose a reason for hiding this comment

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

Can you please fix the extraHostnames? it's breaking stuff

Also, can you ident to align the arrays with the objects?

spec:
  {{- if .Values.ingress.tls }}
  tls:
  {{- range .Values.ingress.tls }}
  - hosts:
    {{- range .hosts }}
    - {{ . | quote }}
    {{- end }}
    {{- with .secretName }}
    secretName: {{ . }}
    {{- end }}
  {{- end }}
  {{- end }}
  rules:
  {{- range concat .Values.ingress.hosts }}
  - host: {{ .host | quote }}
    http:
      paths:
      - path: /
        pathType: Prefix
        backend:
          service:
            name: {{ $fullName }}
            port:
              {{- if .port }}
              number: {{ .port }}
              {{- end }}
  {{- end }}

charts/fluentd/values.yaml Show resolved Hide resolved
charts/fluentd/templates/ingress.yaml Outdated Show resolved Hide resolved
charts/fluentd/values.yaml Outdated Show resolved Hide resolved
@dioguerra
Copy link
Collaborator

@pecastro ping

@pecastro pecastro force-pushed the fluentd-add-ingress branch 2 times, most recently from 82192c2 to b0d1908 Compare June 12, 2023 13:17
@pecastro
Copy link
Contributor Author

Pushed a correction.

helm lint .
==> Linting .

1 chart(s) linted, 0 chart(s) failed
pecastro@localhost /download/fluent-helm-charts/charts/fluentd[fluentd-add-ingress]$ helm template fluentd ../fluentd/ -n kube-system -s templates/ingress.yaml  --set ingress.enabled=true
---
# Source: fluentd/templates/ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: fluentd
  labels:
    helm.sh/chart: fluentd-0.4.4
    app.kubernetes.io/name: fluentd
    app.kubernetes.io/instance: fluentd
    app.kubernetes.io/version: "v1.15.2"
    app.kubernetes.io/managed-by: Helm
spec:
  rules:
    - host: "[]"
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: fluentd
                port:

@pecastro
Copy link
Contributor Author

@dioguerra pong.

@amontalban
Copy link

Would be great to have this in the official Helm Chart, in our case we are creating it manually.

@dioguerra what do we need to make it happen?

@dioguerra
Copy link
Collaborator

dioguerra commented Nov 14, 2023

Hello. Check the TODO for the merge:

  • Verify all issues above are solved
  • Update the Chart.yaml helm version

Not mandatory:

  • Ingress can be defined without a specific host, make this work?. Also set a default port?

@amontalban @pecastro - thanks for pinging :D

@pecastro pecastro force-pushed the fluentd-add-ingress branch from 166e575 to 587c385 Compare November 14, 2023 16:39
dioguerra
dioguerra previously approved these changes Nov 14, 2023
Copy link
Collaborator

@dioguerra dioguerra left a comment

Choose a reason for hiding this comment

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

LGTM, thnaks

@dioguerra
Copy link
Collaborator

Please fix issues as found by the CI. @pecastro

Fix indentation.
Add default port number.
Make host optional.

Signed-off-by: Paulo E. Castro <[email protected]>
@dioguerra dioguerra merged commit b0f13f5 into fluent:main Nov 15, 2023
1 of 2 checks passed
@dioguerra
Copy link
Collaborator

Hey @pecastro.

Thank you for your contribution! Would you also be interested to fix the tests?
#440

Cheers,

@pecastro
Copy link
Contributor Author

I'll have a looksie ...

@pecastro pecastro deleted the fluentd-add-ingress branch November 25, 2023 13:50
@discostur
Copy link

May you also add the ingressClass option? Would be really nice ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants