Fixes #522 CRD and operator conform to fluentd-loki-output-plugin documentation

[antrema]

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #522 Made CRD and operator conform to fluentd-loki-output-plugin documentation

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:

[wenchajun]

This is a public document, is it necessary to remove these sections?

[antrema]

It is not necessary, but it is not interpreted in the controller

[wenchajun]

The data structures defined here are intended to be used for future extensions and it is recommended that the corresponding sections are not deleted.
https://github.com/fluent/fluent-operator/blob/master/docs/plugins/fluentd/tls.md

[antrema]

Ok, I understand your point of view, but if I undelete them, they will be automatically added to the CRD, you don't think it can be confusing ? what do you think about unlinking the loki struct from the *plugins.TLS struct ?

[benjaminhuo]

The tls docs for fluentd https://github.com/fluent/fluent-operator/blob/master/docs/plugins/fluentd/tls.md is copied from fluentbit tls setting which is incorrect.

As @wenchajun mentioned fluentd has its own tls setting which is different with fluentbit's and should be optional to the fluentd input and output plugins: The transport section must be under <match>, <source>, and <filter> sections. It specifies the transport protocol, version, and certificates.

So fluentd's tls setting should conformed to fluentd's doc, not fluentbit's: https://docs.fluentd.org/configuration/transport-section

Fluentd's tls setting should include params from 5 sections:

• https://docs.fluentd.org/configuration/transport-section#tls-setting
• https://docs.fluentd.org/configuration/transport-section#signed-public-ca-parameters
• https://docs.fluentd.org/configuration/transport-section#generated-and-signed-by-private-ca-parameters
• https://docs.fluentd.org/configuration/transport-section#generated-and-signed-by-private-ca-certs-or-self-signed-parameters
• https://docs.fluentd.org/configuration/transport-section#cert-digest-algorithm-parameter

Fluentd tls samples:
https://stackoverflow.com/questions/50190993/how-to-get-fluentd-td-agent-tls-ssl-encryption-for-in-forward-to-work
fluent/fluentd#3802

[benjaminhuo]

why removing fields in TLS?

[antrema]

there are not used in controller

[benjaminhuo]

It's true that the Fluentd Loki plugin is copy from fluentbit Loki plugin which is incorrect.
The TLS is from fluentbit https://docs.fluentbit.io/manual/pipeline/outputs/tcp-and-tls https://docs.fluentbit.io/manual/pipeline/outputs/opensearch

@wenchajun @wanjunlei Please double check if the TLS should be removed from the fluentd code

[wanjunlei]

It's true that the Fluentd Loki plugin is copy from fluentbit Loki plugin which is incorrect. The TLS is from fluentbit https://docs.fluentbit.io/manual/pipeline/outputs/tcp-and-tls https://docs.fluentbit.io/manual/pipeline/outputs/opensearch

@wenchajun @wanjunlei Please double check if the TLS should be removed from the fluentd code

The fluentd plugin http, forward have their own tls settings and does not use the TLS, so I think loki can set its own tls
settings like them.

And the fluentd plugins do not have a common tls setting, so I agree to delete the TLS, and the plugins should define the tls setting themselves

[benjaminhuo]

The tls docs for fluentd https://github.com/fluent/fluent-operator/blob/master/docs/plugins/fluentd/tls.md is copied from fluentbit tls setting which is incorrect.

As @wenchajun mentioned fluentd has its own tls setting which is different with fluentbit's and should be optional to the fluentd input and output plugins: The transport section must be under <match>, <source>, and <filter> sections. It specifies the transport protocol, version, and certificates.

So fluentd's tls setting should conformed to fluentd's doc, not fluentbit's: https://docs.fluentd.org/configuration/transport-section

Fluentd's tls setting should include params from 5 sections:

• https://docs.fluentd.org/configuration/transport-section#tls-setting
• https://docs.fluentd.org/configuration/transport-section#signed-public-ca-parameters
• https://docs.fluentd.org/configuration/transport-section#generated-and-signed-by-private-ca-parameters
• https://docs.fluentd.org/configuration/transport-section#generated-and-signed-by-private-ca-certs-or-self-signed-parameters
• https://docs.fluentd.org/configuration/transport-section#cert-digest-algorithm-parameter

Fluentd tls samples:
https://stackoverflow.com/questions/50190993/how-to-get-fluentd-td-agent-tls-ssl-encryption-for-in-forward-to-work
fluent/fluentd#3802

[benjaminhuo]

It's true that the Fluentd Loki plugin is copy from fluentbit Loki plugin which is incorrect. The TLS is from fluentbit https://docs.fluentbit.io/manual/pipeline/outputs/tcp-and-tls https://docs.fluentbit.io/manual/pipeline/outputs/opensearch
@wenchajun @wanjunlei Please double check if the TLS should be removed from the fluentd code

The fluentd plugin http, forward have their own tls settings and does not use the TLS, so I think loki can set its own tls settings like them.

And the fluentd plugins do not have a common tls setting, so I agree to delete the TLS, and the plugins should define the tls setting themselves

See my comments in #523 (comment)

[wanjunlei]

There has been a Transport struct.

[benjaminhuo]

There has been a Transport struct.

Nice, then we should use Transport instead of the TLS in Loki @antrema

[antrema]

There has been a Transport struct.

Nice, then we should use Transport instead of the TLS in Loki @antrema

You completely lost me
The plugin fluentd-loki-output-plugin is maintained by Grafana and its definition is fully documented here
Why do you want to absolutely match an existing struct (Transport or TLS), the resulting CRDs will completely differ from the plugin fluentd-loki-output-plugin documentation and users will be confused
I think we just have to match the Loki struct to the plugin structure
As it is done on the other output plugins:

• like Forwardoutput plugin: https://github.com/fluent/fluent-operator/blob/master/apis/fluentd/v1alpha1/plugins/output/forward.go
• like Http output plugin: https://github.com/fluent/fluent-operator/blob/master/apis/fluentd/v1alpha1/plugins/output/http.go
  In both struct the TLS parameters are completely redefined

To make me clear, i will rework my PR to match my explanations

[benjaminhuo]

The plugin fluentd-loki-output-plugin is maintained by Grafana and its definition is fully documented here
Why do you want to absolutely match an existing struct (Transport or TLS), the resulting CRDs will completely differ from the plugin fluentd-loki-output-plugin documentation and users will be confused
I think we just have to match the Loki struct to the plugin structure

@antrema You're right. It should be consistent with the plugin def.
The HTTP input plugin has the transport section in which the tls config should be put.
But the HTTP output plugin simply put tls settings as plain fields.

Thanks for finding and fixing this bug!