feat(consumer) validate credential secret fields

[rainest]

What this PR does / why we need it:

This is the band-aid solution discussed in Kong/deck#223 (comment)

It's effectively the same set of checks on the go-kong structs, but in the controller instead of deck. This allows us to log richer information than we would if we delegated this check to either deck or go-kong.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): Fix #532

Special notes for your reviewer:
Per comments in deck#223, this is an imperfect stopgap solution to a complicated problem

No new tests, as there's not a whole lot to test here. We could add one with one or more broken credentials, but given the band-aid nature of the solution (checks specific to each credential type, in a codebase that ideally shouldn't have them), it seems like they'd be of limited use. Regressions shouldn't occur unless we remove this code, and we shouldn't be doing that unless we're replacing it with some better system, which would have its own, more effective checks.

[mflendrich]

Looks good but needs to be unit-tested.

[rainest]

@mflendrich per the original PR notes, do we indeed want unit tests for this? We can't unit test efficiently in the controller due to limitations elsewhere, and long-term we want to remove those along with this band-aid solution.

Ideally we want to get to something more generic ("give controller bad credential, controller discards bad credential, bubbles issue up to some layer that informs user via resource status") for any credential type, but we can't yet. Testing each individual known bad credential now does verify this, but seems rather futile long-term; it's test cruft that we'll eventually discard.

[mflendrich]

We can't unit test efficiently in the controller due to limitations elsewhere

Wouldn't adding cases under

kubernetes-ingress-controller/internal/ingress/controller/parser/kongstate/consumer_test.go

Line 11 in 99486c7

func TestConsumer_SetCredential(t *testing.T) {

work?

and long-term we want to remove those along with this band-aid solution.

I think "this is only temporary" isn't a good reason to skip testing, because temporary solutions often tend to exceed their projected lifetime :)

Ideally we want to get to something more generic ("give controller bad credential, controller discards bad credential, bubbles issue up to some layer that informs user via resource status") for any credential type, but we can't yet. Testing each individual known bad credential now does verify this, but seems rather futile long-term; it's test cruft that we'll eventually discard.

We can think of it this way: in this PR we add the statement "if the user provides a credential without identifier (username/client id/etc), KIC will drop it as invalid" to the contract between KIC and the user. We don't need to test "each individual known bad credential" - just credentials with the ID field missing.

[rainest]

It sort of works--the issue is that there's no universal missing ID that we can test. Every type of credential uses a different alternative key field, e.g. key-auth credentials use the key, basic-auth credentials use the username, oauth2 uses the client_id, etc. The structure of each check is identical, but the exact input is different.

For example, test.diff.txt provides a valid test for key-auth, with a minor catch (more on that later--it's not relevant beyond my being bad at Golang), but it doesn't test the rest.

We could get around that by instead breaking the check into its own function that accepts the critical field and returns an error if it's nil, but IMO that strays a bit too far into tail-wagging-dog territory: we only add an additional function and call to make it easier to cover. That'd be easier to justify if the check were complex, but it isn't: an if block with an "equals nil?" condition that formats and returns an error if the condition is true is dead simple, to the point where I'd expect vet to reject it if there's any chance it'd fail.

I understand the concern about the workaround being longer-lived than we'd like, and indeed fear that it will remain here for quite some time myself, but I think there are good mitigating factors:

• The reason why we have the workaround is relatively well-understood.
• The workaround is simple, albeit repetitive.
• The workaround is accompanied by an explanatory comment explaining its purpose to future readers, and it instructs them not to touch it unless we've addressed the root cause.
• If we have addressed the root cause, any existing tests for the workaround should be removed and replaced by something that tests the larger fix. That fix hopefully does allow us to write a single test, because it'd presumably delegate this check to deck or go-kong, and we'd cover it with a single bad input to decodeCredential() or similar.

I don't particularly like omitting tests, but I also don't particularly like clutter in the test cases. In light of that, I propose that we formalize Kong/deck#223 (comment) into a research spike backlog item and then go with one of the following:

• Write a single test similar to the patch. Acknowledge that coverage is incomplete, but that we know all such checks are similar enough that testing each individually doesn't provide much benefit. Imperfect, but minimal clutter.
• Write tests for every check anyway.
• Write tests for none, acknowledging that we understand the risks but think they're mitigated.

Which of those do you prefer?

Anyway, onward with part two, in which I uncovered a new problem that I'm having trouble fixing. The attached test modifies the error printout to omit the consumer username, because that caused a segfault with the copied test parameters. The original code didn't do anything with the consumer username, so using completely empty consumers was fine. I'd wanted to address that with something like invalid.diff.txt, but that makes Go mad:

$ go vet ./...
# github.com/kong/kubernetes-ingress-controller/internal/ingress/controller/parser/kongstate
vet: ./consumer_test.go:57:5: too few values in struct literal

I understand it's unhappy about that because I didn't key the kong.Consumer, but unfortunately doing so is impossible, as that bit has no key. I could add one, but that means I can no longer use use *c.Username elsewhere. I like that shorthand 😞

Is there a way to construct the literal otherwise? Golang issue 9859 suggests you cannot, i.e. there's no way around the "cannot use promoted field Consumer.Username in struct literal of type Consumer" vet error you get if you try and add Username directly to the KIC Consumer literal.

[rainest]

With the Go syntax sorted, test-all.diff.txt is the diff that checks all pending the question above.

As you can see, it's very repetitive, since we don't care about anything other than the critical field, so the credential is just completely blank always--the only thing that changes between the test cases is the credential type. IMO it's basically boilerplate clutter, but I'll add it if you prefer.

[mflendrich]

It sort of works--the issue is that there's no universal missing ID that we can test. Every type of credential uses a different alternative key field, e.g. key-auth credentials use the key, basic-auth credentials use the username, oauth2 uses the client_id, etc. The structure of each check is identical, but the exact input is different.

This is unfortunate indeed, but this structure means that each error case is a separate logical path (see the code coverage printout for this PR - red lines are not covered by any automated test):

Since unit tests are typically expected to cover all meaningful code paths (and a code path rejecting an entity from further processing is meaningful beyond my doubt), we do need to cover them by tests. Here this unfortunately means having a test case for each auth type. But with table-based tests this is should be as simple as an array with 5 (or so) credentials, so repetition will be minimal (if any). It should be possible in a much simpler (less repetitive) way than test-all.diff.txt:

For example, one could have a table-based test with the case array similar to this (it's possible that positive cases are satisfactorily covered by other tests already):

{
  {cred: {a secret decoding to KeyAuth}, wantErr: true},
  {cred: {a secret decoding to KeyAuth}, wantErr: false},

  {cred: {a secret decoding to BasicAuth}, wantErr: true},
  {cred: {a secret decoding to BasicAuth}, wantErr: false},

  // ... and so on, for other credential types
}

The only data that needs to be repeated across cases is what differs between them. It should be possible to reuse a single Consumer fixture for all cases.

I agree with the sentiment that excess repetitiveness is a problem. I think it should be possible to avoid it, though. I'm happy to help if there are further issues with the matter.

I think there are good mitigating factors

Let's have that discussion on zoom, if necessary - that's quicker than talking through GH comments.

I uncovered a new problem

I recall that this is already resolved - right?

[rainest]

It should be possible to reuse a single Consumer fixture for all cases.

Consumer inputs for the existing tests are identical, but outputs aren't. The positive cases test that the Consumers were modified correctly, e.g. the input for one of the JWT cases and its output. I suspect that they instantiate a new Consumer per test to account for that. The new tests in 346b0f8 (formerly the diff) follow that pattern, similar to the one existing negative test.

IMO if we don't want to forsake coverage here then we should go ahead and accept that adding new tests that follow the existing structure means verbosity. The existing code here doesn't have the concept of a generic credential, which is why it needs separate paths to handle each separately.

[mflendrich]

One comment, otherwise LGTM