Reset before request in order to work around VCert's "unmatched key modulus"

[maelvls]

⚠️ THIS PR WAS NOT MERGED UPSTREAM YET ⚠️

We found out that Venafi#273 was causing venafi-enhanced-issuer to show the error:

vcert error: your data contains problems: request doesn't match certificate: unmatched key modulus

We found that it was caused by running request and reset(restart=true) back to back.

To work around this issue, we decided to unconditionally reset any prior failed enrollment with reset(restart=false) so that we don't get stuck with "Fix any errors, and then click Retry." (60% of the time) or "WebSDK CertRequest" (40% of the time), and to revert the changes made in Venafi#269.

For context, we compared two approaches: (1) unconditionally reset, and (2) conditionally reset synchronously.

Unconditionally Reset (this PR)

We will go with this solution as an "intermediate" solution.

Instead of conditionally resetting, we always reset. Since we don’t reset and retrieve back to back, we avoid the race condition that was found in Venafi#273.

reset(restart=false)
request
loop {
    retrieve
    if err, return the error.
    if 200, return the cert.
    if 202, continue.
}

I stress-tested TPP to calculate the impact of reset(restart=false) under load. The VM was on GCP, I used TPP 23.1 with ExpressSQL, a pd-balanced disk, 4 vCPUs, 16 GB memory.

• Scenario 1: I ran request; retrieve_every_second 1000 times in parallel = 1 min 57 sec
• Scenario 2: I ran reset(restart=false); request; retrieve_every_second 1000 times in parallel = 2 min 11 sec

The slow down is 9%.

Conditionally Reset Synchronously

Dmitry Philimonov suggested that we use WorkToDoTimeout to prevent the constant polling. And since we don’t reset and retrieve back to back, we avoid the race condition that was found in Venafi#273.

loop {
    request(work_todo_timeout=60)
    if 500 and "Fix any errors, and then click Retry" {
        reset(restart=false)
        continue
    }
    if other error, return error.
    if 200, return cert.
}

This solution will have to be implemented outside of RetrieveCertificate and RequestCertificate, which means users of the Go SDK will need to change the way they are using the library.

For example:

for {
    cert, err := connector.RequestCertificate(vcert.Request{WorkToDoTimeout: 60 * time.Second})
    if vcert.ErrIsClickToRetry(err) {
        err = connector.ResetCertificate(vcert.Request{WorkToDoTimeout: 0, Restart: false})
        if err != nil {
            return err
        }
        continue
    }
    if err != nil {
        return err
    }
    return cert
}

[inteon]

I left some very minor remarks.
The best way to test this is using integration tests, which are not available on this fork.
So, I would like to merge this PR without adding tests to fix this issue quickly, we did manually test the fix.
Once the remarks are addressed, it think it is ready to be merged.

[maelvls]

I added unit tests by mocking the HTTP calls. The reason I wanted to add these tests is because it is the only way to "review" what the error messages look like. I think it is useful to be able to review the error messages since it is a UI element.

I also slightly changed the reset func that you had written. The "not found" check was a bit weird, so I decided to change it so that it returns an error that we can errors.As.

[inteon]

@maelvls All of my feedback has been resolved, feel free to merge this PR.

[irbekrm]

Note: the issue addressed by this PR has not been fixed in v4.24.0 Venafi#273 (comment) so this fork is still used by cert-manager.