Update pkg/venafi to allow use of a custom logger

[BrandonRoehl]

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.

[tr1ck3r]

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.

[BrandonRoehl]

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 io.Writer on it and it collides with our other log.Loggers we are using

[BrandonRoehl]

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?

[tr1ck3r]

@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.

[tr1ck3r]

This comment has the details of the Vault issue I referred to earlier: https://github.com/Venafi/vault-pki-monitor-venafi/issues/27#issuecomment-582177227. And this PR that resolved it: https://github.com/Venafi/vcert/pull/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.

[BrandonRoehl]

The part of #71 that looks to have resolved it is actually the move from Printf to Println this is because their integration was using gRPC and Printf requires manual log flushing where as Println will force a flush on write end. Looked at the comments on https://github.com/hashicorp/go-plugin/issues/93 because where as log.New(...).Println() would have been equivalent to log.Println() but log.New(...).Printf() is not equivalent even though one could have ended in \n since both were writing to os.Stderr anyways this can be verified by checking the source here https://golang.org/src/log/log.go?s=3396:3435#L76

[BrandonRoehl]

Wanted to ping this again wondering what the status of the investigation with HashiCorp has been?

[tr1ck3r]

@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:

1. The vault-pki-monitor-venafi’s import functionality stopped working after a few hundred certificates when we updated its backend structure to use a custom logger.
2. When the current vault-pki-monitor-venafi master branch was built using this PR of VCert, the import functionality also stopped working after a few hundred certificates.
3. Using a custom logger with the VCert CLI worked as expected, text was output to the console the same as it does with the original logger.

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.

[BrandonRoehl]

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.

[tr1ck3r]

@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.

[BrandonRoehl]

With it being a nil logger there will not be a custom logger here. Thus there is no possibility for it to be different where c.log.Println and log.Println are exactly the same code wise too.

https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/log/log.go;l=328 https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/log/log.go;l=200

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 New

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