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

add flag to set log-format #354

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Conversation

seankhliao
Copy link
Contributor

I would like logs to be output in json format.

This uses the same logs api libraries as cert-manager itself,
though it doesn't expose the full logging configuration surface,
just an extra --log-format flag.

Example original output:

I0718 21:24:43.654238   65256 options.go:113] "------------------------------------------------------------------------------------------------------------"
I0718 21:24:43.654292   65256 options.go:114] "WARNING!: --root-ca-file is not defined which means the root CA will be discovered by the configured issuer."
I0718 21:24:43.654298   65256 options.go:115] "WARNING!: It is strongly recommended that a root CA bundle be statically defined."
I0718 21:24:43.654303   65256 options.go:116] "------------------------------------------------------------------------------------------------------------"
I0718 21:24:43.657170   65256 server.go:208] "Starting metrics server" logger="controller-runtime.metrics"
I0718 21:24:43.657222   65256 server.go:247] "Serving metrics server" logger="controller-runtime.metrics" bindAddress="0.0.0.0:9402" secure=false
I0718 21:24:43.657319   65256 server.go:83] "starting server" name="health probe" addr="[::]:6060"
I0718 21:24:43.657582   65256 leaderelection.go:250] attempting to acquire leader lease istio-system/istio-csr...
I0718 21:24:43.933090   65256 internal.go:525] "Stopping and waiting for non leader election runnables" logger="manager"
I0718 21:24:43.933159   65256 internal.go:529] "Stopping and waiting for leader election runnables" logger="manager"
I0718 21:24:43.933176   65256 internal.go:537] "Stopping and waiting for caches" logger="manager"
I0718 21:24:43.933216   65256 internal.go:541] "Stopping and waiting for webhooks" logger="manager"
I0718 21:24:43.933239   65256 internal.go:544] "Stopping and waiting for HTTP servers" logger="manager"
I0718 21:24:43.933283   65256 server.go:68] "shutting down server" name="health probe" addr="[::]:6060"
E0718 21:24:43.933364   65256 internal.go:499] "error received after stop sequence was engaged" err="context canceled" logger="manager"
I0718 21:24:43.933399   65256 server.go:254] "Shutting down metrics server with timeout of 1 minute" logger="controller-runtime.metrics"
I0718 21:24:43.933575   65256 internal.go:548] "Wait completed, proceeding to shutdown the manager" logger="manager"
E0718 21:24:43.933702   65256 leaderelection.go:351] error initially creating leader election record: Post "https://justia.liao.dev:6443/apis/coordination.k8s.io/v1/namespaces/istio-system/leases": context canceled
Error: failed to fetch initial serving certificate: failed to sign serving certificate: failed to create CertificateRequest: namespaces "istio-system" not found
E0718 21:24:43.933794   65256 internal.go:499] "error received after stop sequence was engaged" err="leader election lost" logger="manager"

Example output with --log-format json:

{"ts":1721334309231.8687,"caller":"options/options.go:113","msg":"------------------------------------------------------------------------------------------------------------","v":0}
{"ts":1721334309231.8843,"caller":"options/options.go:114","msg":"WARNING!: --root-ca-file is not defined which means the root CA will be discovered by the configured issuer.","v":0}
{"ts":1721334309231.888,"caller":"options/options.go:115","msg":"WARNING!: It is strongly recommended that a root CA bundle be statically defined.","v":0}
{"ts":1721334309231.891,"caller":"options/options.go:116","msg":"------------------------------------------------------------------------------------------------------------","v":0}
{"ts":1721334309233.4988,"logger":"controller-runtime.metrics","caller":"server/server.go:208","msg":"Starting metrics server","v":0}
{"ts":1721334309233.5322,"logger":"controller-runtime.metrics","caller":"server/server.go:247","msg":"Serving metrics server","v":0,"bindAddress":"0.0.0.0:9402","secure":false}
{"ts":1721334309233.554,"caller":"manager/server.go:83","msg":"starting server","name":"health probe","addr":"[::]:6060","v":0}
{"ts":1721334309233.7185,"caller":"leaderelection/leaderelection.go:250","msg":"attempting to acquire leader lease istio-system/istio-csr...","v":0}
{"ts":1721334309476.7566,"logger":"manager","caller":"manager/internal.go:525","msg":"Stopping and waiting for non leader election runnables","v":0}
{"ts":1721334309476.9492,"logger":"manager","caller":"manager/internal.go:529","msg":"Stopping and waiting for leader election runnables","v":0}
{"ts":1721334309477.0334,"logger":"manager","caller":"manager/internal.go:537","msg":"Stopping and waiting for caches","v":0}
{"ts":1721334309477.0322,"logger":"manager","caller":"manager/internal.go:499","msg":"error received after stop sequence was engaged","err":"context canceled"}
{"ts":1721334309477.069,"logger":"manager","caller":"manager/internal.go:541","msg":"Stopping and waiting for webhooks","v":0}
{"ts":1721334309477.0898,"logger":"manager","caller":"manager/internal.go:544","msg":"Stopping and waiting for HTTP servers","v":0}
{"ts":1721334309477.158,"caller":"manager/server.go:68","msg":"shutting down server","name":"health probe","addr":"[::]:6060","v":0}
{"ts":1721334309477.2427,"logger":"controller-runtime.metrics","caller":"server/server.go:254","msg":"Shutting down metrics server with timeout of 1 minute","v":0}
{"ts":1721334309477.3958,"logger":"manager","caller":"manager/internal.go:548","msg":"Wait completed, proceeding to shutdown the manager","v":0}
{"ts":1721334309477.5764,"caller":"leaderelection/leaderelection.go:351","msg":"error initially creating leader election record: Post \"https://justia.liao.dev:6443/apis/coordination.k8s.io/v1/namespaces/istio-system/leases\": context canceled"}
Error: failed to fetch initial serving certificate: failed to sign serving certificate: failed to create CertificateRequest: namespaces "istio-system" not found
{"ts":1721334309477.7253,"logger":"manager","caller":"manager/internal.go:499","msg":"error received after stop sequence was engaged","err":"leader election lost"}

Signed-off-by: Sean Liao <sean@liao.dev>
@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2024
@cert-manager-prow
Copy link
Contributor

Hi @seankhliao. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cert-manager-prow cert-manager-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 18, 2024
@SgtCoDFish
Copy link
Member

/ok-to-test

@cert-manager-prow cert-manager-prow bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2024
@SgtCoDFish
Copy link
Member

Hey, thanks for this! I love JSON logs and this looks like a great improvement.

I think this feature would massively benefit from adding the log format option to the Helm chart. A similar PR was recently merged in trust-manager for JSON logs which would be a useful reference for implementing it in istio-csr: cert-manager/trust-manager#354

Would you be able to add the option to the Helm chart?

Signed-off-by: Sean Liao <sean+git@liao.dev>
@cert-manager-prow cert-manager-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 22, 2024
@seankhliao
Copy link
Contributor Author

Sure, I've added the helm chart changes

Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks so much for this, really appreciate it! I've tested locally and confirmed it works 😁

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2024
@cert-manager-prow cert-manager-prow bot merged commit d53f5a2 into cert-manager:main Jul 22, 2024
11 of 12 checks passed
@seankhliao seankhliao deleted the json-logs branch July 22, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants