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

fix: logging-operator-logging logging.yaml template #539

Merged

Conversation

sermilrod
Copy link
Contributor

@sermilrod sermilrod commented Aug 3, 2020

Q A
Bug fix? yes
New feature? no
API breaks? no
Deprecations? no
Related tickets fixes #540
License Apache 2.0

What's in this PR?

Modifications to logging.yaml template to fix logging object when it renders without TLS enabled

Why?

Running tls.enabled=false produces invalid yaml

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

@CLAassistant
Copy link

CLAassistant commented Aug 3, 2020

CLA assistant check
All committers have signed the CLA.

@ahma ahma requested review from ahma and tarokkk August 3, 2020 12:23
@ahma ahma self-assigned this Aug 3, 2020
@ahma ahma added bug Something isn't working chart labels Aug 3, 2020
@sermilrod sermilrod force-pushed the fix/logging-operator-logging-template branch from de938f7 to 4909636 Compare August 3, 2020 13:07
Signed-off-by: Sergio Millan Rodriguez <sergio.rodriguez@ticketmaster.co.uk>
@sermilrod sermilrod force-pushed the fix/logging-operator-logging-template branch from 4909636 to 484005a Compare August 3, 2020 13:11
@sermilrod sermilrod closed this Aug 3, 2020
@sermilrod sermilrod reopened this Aug 3, 2020
@sermilrod
Copy link
Contributor Author

@ahma @tarokkk could you please review this? Thanks in advance

Copy link
Contributor

@tarokkk tarokkk left a comment

Choose a reason for hiding this comment

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

Is there any reason to add sharedSecret and sharedKey when tls disabled?

enabled: false
secretName: {{ .Values.tls.fluentdSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentd-tls" ) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
secretName: {{ .Values.tls.fluentdSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentd-tls" ) }}

enabled: false
secretName: {{ .Values.tls.fluentdSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentd-tls" ) }}
sharedKey: "{{ .Values.tls.sharedKey }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sharedKey: "{{ .Values.tls.sharedKey }}"

enabled: false
secretName: {{ .Values.tls.fluentbitSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentbit-tls" ) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
secretName: {{ .Values.tls.fluentbitSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentbit-tls" ) }}

enabled: false
secretName: {{ .Values.tls.fluentbitSecretName | default (printf "%s-%s" (include "logging-operator-logging.name" . ) "fluentbit-tls" ) }}
sharedKey: "{{ .Values.tls.sharedKey }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sharedKey: "{{ .Values.tls.sharedKey }}"

@sermilrod
Copy link
Contributor Author

sermilrod commented Aug 3, 2020

Is there any reason to add sharedSecret and sharedKey when tls disabled?

It is required by the Fluentbit/Fluentd spec regardless of the value of the field tls.enabled and the controller asks for it even though I agree it should not be required

https://github.com/banzaicloud/logging-operator/blob/d959ac156c27da7057f64764be3f885dbb720ae4/pkg/sdk/api/v1beta1/fluentd_types.go#L89

https://github.com/banzaicloud/logging-operator/blob/a6971bee791489ab70f9463c0bfb51f705a2aea3/charts/logging-operator/crds/logging.banzaicloud.io_loggings.yaml#L2915

By the look of it only sharedKey could be omitted

@tarokkk
Copy link
Contributor

tarokkk commented Aug 3, 2020

I think we should change that to omitempty as well to be consistent in the API. However, we can merge this and do some follow up commits to clean things up-

Copy link
Contributor

@ahma ahma left a comment

Choose a reason for hiding this comment

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

LGTM Thanks @sermilrod !

@ahma ahma merged commit 8663dae into kube-logging:master Aug 3, 2020
simonkey007 pushed a commit to simonkey007/logging-operator that referenced this pull request Mar 11, 2021
Signed-off-by: Sergio Millan Rodriguez <sergio.rodriguez@ticketmaster.co.uk>
EppO pushed a commit to EppO/logging-operator that referenced this pull request Apr 21, 2021
Signed-off-by: Sergio Millan Rodriguez <sergio.rodriguez@ticketmaster.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging object does not render valid yaml when passing tls.enabled=false
4 participants