-
Notifications
You must be signed in to change notification settings - Fork 61
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
Update pkg/venafi to allow use of a custom logger #130
base: master
Are you sure you want to change the base?
Conversation
Need to make sure this doesn't cause issues for consumers like HashiCorp Vault. I know that our choice of logger was the root cause of a memory leak issue for our Vault integration. I don't recall if it was logging from VCert, from the Vault plugin, or both. |
okay and where the loggers are declared can be changed as well we are encountering issues with the global logger since we are unable to replace the |
Bumping this to see if there has been any conversation with customers and how this will play into the new v4 versioning since those choosing to use the next version will have to change their go imports? |
@BrandonRoehl thank you for the PR and apologies for the delay. Our team is currently focused on meeting our Q3 objectives but your PR is something that I've currently got targeted for the next VCert release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code changes looks go,
ran all test suite an ran well
thanks @BrandonRoehl, for the contribution.
This comment has the details of the Vault issue I referred to earlier: Venafi/vault-pki-monitor-venafi#27 (comment). And this PR that resolved it: #71. Given that, I'm not sure we can merge this PR until we confirm with HashiCorp that Vault no longer has issues with the custom logger. |
The part of #71 that looks to have resolved it is actually the move from |
Wanted to ping this again wondering what the status of the investigation with HashiCorp has been? |
@BrandonRoehl apologies for the delay. Yes, but unfortunately the news is not good 😞 HashiCorp Engineering reviewed your proposed change and said they agreed with my fears that it would cause a regression of the earlier issue if it is making a new logger from os.Stderr. They provided us with some guidance on how to confirm and we found the following:
So, regrettably, because this change would break one of our most popular integrations we won't be able to accept the PR in its current form. |
Going to attempt pre allocating the logger to see if it is an issue with excessive construction and allocation across go routines. Is there an easy way for me to validate this change? That way I don't have to bug you guys if I don't suspect it to work. |
Changes were found to break integration
@BrandonRoehl this PR has gotten stale, checking to see if you were able to find any alternative that wouldn't break our integration with HashiCorp Vault (vault-pki-monitor-venafi)? Unfortunately there's no easy way to validate. It requires building a new version of the vault-pki-monitor-venafi with an updated dependency on the modified version of vcert and then testing the resulting plugin with Vault. |
With it being a https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/log/log.go;l=328 The issue with the old code was not the custom logger rather the rapid re-allocation of a default logger that causes file markers to be consumed thus eventually not logging at all. This was solved by not allocating every time. That it's self was just because of not calling Can say this will not break the integration by simply not setting logger thus leaving the default. This has been a long standing issue now at Jamf and have to keep our own fork. You can verify doing a custom SDK build but trace the new calls with files since that is what was causing it to not log |
Untie the global logger from the Vcert SDK.
This way we are able to pass in a custom
log.Logger
so we can log to a file or the syslog or whatever we want in the integration.