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

feat(plugin): support fluentd loki output #346

Merged
merged 5 commits into from
Jul 14, 2022

Conversation

fatpa
Copy link
Contributor

@fatpa fatpa commented Jul 12, 2022

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #310

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:

apiVersion: fluentd.fluent.io/v1alpha1
kind: ClusterOutput
metadata:
  name: fluentd-output-loki
  labels:
    output.fluentd.fluent.io/enabled: "true"
spec: 
  outputs: 
  - loki:
      host: loki-logging-data.kubesphere-logging-system.svc
      port: 3100

@benjaminhuo
Copy link
Member

@fatpa Thanks for the contribution!
Would you sign your commit as below?

git commit -s --amend
git push -f

Thanks!

Signed-off-by: Fatpa <fatpa.cai@gmail.com>
Signed-off-by: Fatpa <fatpa.cai@gmail.com>
Signed-off-by: Fatpa <fatpa.cai@gmail.com>
Signed-off-by: Fatpa <fatpa.cai@gmail.com>
@fatpa fatpa force-pushed the support-fluentd-loki-output-type branch from 458c810 to 3f2c4e5 Compare July 12, 2022 11:22
@fatpa
Copy link
Contributor Author

fatpa commented Jul 12, 2022

@fatpa Thanks for the contribution! Would you sign your commit as below?

git commit -s --amend
git push -f

Thanks!

Of course. Done.

@fatpa fatpa changed the title feat(plugin): Support fluentd loki output feat(plugin): support fluentd loki output Jul 12, 2022
@wenchajun
Copy link
Member

As far as I know, the fluentd image doesn't have the loki plugin built in, so we should modify this dockerfile file.
https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-watcher/fluentd/Dockerfile.amd64#L38

@wanjunlei
Copy link
Collaborator

Why add a new TLS plugin in apis/fluentd/v1alpha1/plugins, we should use the TLS plugin in apis/fluentbit/v1alpha2/plugins.

in addition, we should move the TLS, ConfigMapLoader, and SecretLoader to a new package to avoid duplicate definitions.

@fatpa
Copy link
Contributor Author

fatpa commented Jul 13, 2022

Why add a new TLS plugin in apis/fluentd/v1alpha1/plugins, we should use the TLS plugin in apis/fluentbit/v1alpha2/plugins.

in addition, we should move the TLS, ConfigMapLoader, and SecretLoader to a new package to avoid duplicate definitions.

Just follow the current design for the apis/fluentd and avoid the differences between fluentd and fluentbit.
I also think the common package is the best to avoid duplicate definitions, and we should create one more PR for it, instead of this one.

@fatpa
Copy link
Contributor Author

fatpa commented Jul 13, 2022

As far as I know, the fluentd image doesn't have the loki plugin built in, so we should modify this dockerfile file. https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-watcher/fluentd/Dockerfile.amd64#L38

I will add it.

Signed-off-by: Fatpa <fatpa.cai@gmail.com>
@fatpa fatpa force-pushed the support-fluentd-loki-output-type branch from f5a5ad4 to acd27bf Compare July 13, 2022 09:28
@benjaminhuo
Copy link
Member

Why add a new TLS plugin in apis/fluentd/v1alpha1/plugins, we should use the TLS plugin in apis/fluentbit/v1alpha2/plugins.

in addition, we should move the TLS, ConfigMapLoader, and SecretLoader to a new package to avoid duplicate definitions.

@wenchajun You can create another PR to move tls to a common place

@benjaminhuo
Copy link
Member

@fatpa Thanks very much for the contribution!

@benjaminhuo benjaminhuo merged commit b39727b into fluent:master Jul 14, 2022
@benjaminhuo
Copy link
Member

@fatpa Would you please create another PR to add docs to the Fluentd Loki plugin as https://github.com/fluent/fluent-operator/blob/master/docs/plugins/fluentd/index.md ?

Thanks
Benjamin

@fatpa
Copy link
Contributor Author

fatpa commented Jul 14, 2022

Sure. I will make it in a few days.

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.

Add Loki output support for Fluentd
4 participants