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

Make error when getting certificates more transparent for user #70

Merged
merged 5 commits into from
Aug 12, 2023

Conversation

datosh
Copy link
Contributor

@datosh datosh commented Aug 8, 2023

As discussed in #69 , user should be informed about nature of error during HTTP get. Since multierr was already used in this project I opted for this solution.

I also added 3 unit tests.
Let me know if I missed anything, since the code involved sleeps I made sure to keep it simple.

go test -count 100 ./verify/trust/ runs in 0.2s on my machine.

// FakeHTTPSGetter is a test double for HTTPSGetter. It can hold a slice
// of body and error responses so Get can be called multiple times and return
// different values, as to simulate different scenarios.
type FakeHTTPSGetter struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is technically a "mock" and not a fake, since it doesn't offer the full functionality of the tested interface. It has set input/output behaviors. I think probably it'd be better to adapt the Getter implementation in testing/mocks.go to suit your needs rather than implement something similar where it can't be reused.

Copy link
Contributor Author

@datosh datosh Aug 8, 2023

Choose a reason for hiding this comment

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

Thanks! I didn't notice the mocks in testing folder.
I have moved my mock over to testing. I opted to keep it as two seperate mocks, since combining it with the available testing.Getter to be both configurable in inputs and (mutliple) outputs seems like a lot of complexity for a mock. Let me know if you agree!
I also had to create trust_test package to break an import cycle back to verify/trust from fakekds.go.

Signed-off-by: Fabian Kammel <[email protected]>
testing/mocks.go Outdated

// Get the next configured response body and error.
func (g *VariableResponseGetter) Get(_ string) ([]byte, error) {
body := g.ResponseBody[g.callCount]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This construction is error-prone due to the unchecked expectation that the arrays are the same length, and that Get will not be called more times than there are entries. There is an additional test condition I'm not sure you want to test, but that is that all the responses that you prepared got discharged–maybe you want to have a Done(t testing.TB) function that errors if that expectation isn't met.

I also am not keen on the Get semantics being call order dependent across different URLs.

Maybe we can have Responses map[string][]GetResponse

with type GetResponse struct {
Occurrences uint
Body []byte
Error error
}

Such that Occurrences is decremented each time it's retrieved, and then if decremented to 0, the array gets the 0th item sliced off.
This makes len(g.Responses[url]) == 0 always a testing internal error that you can return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - these are great points. PTAL at the updated implementation.

Two more things:

This makes len(g.Responses[url]) == 0 always a testing internal error that you can return.

When you say return error, do you mean like the errors.New("404") you as with the existing Getter? I can also see making this even more strict and Fail the test in case there are no configured URLs or no more prepared responses.

Do you want me to merge the two Getter implementations at this point? I also see value in keeping the simple Getter as well, so that tests can use the simpler construction and prevent the verbosity of constructing the responses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think returning a 404 error is a helpful alternative, to avoid having to code every 404 response yourself. That would allow the two implementations to merge more seamlessly. You can change current &test.Getter{Responses: m} constructions to test.SimpleGetter(m) to construct the more involved type underneath. Occurrences could be set to uint max (^uint(0)) in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me! I updated the implementation.

@deeglaze
Copy link
Collaborator

The linter found a typo, but otherwise looks good.

Signed-off-by: Fabian Kammel <[email protected]>
@datosh datosh force-pushed the feature/improve-retry-error-handling branch from 4c26f12 to b29dd6d Compare August 12, 2023 11:52
@deeglaze deeglaze merged commit d8660ce into google:main Aug 12, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants