Add ACL and User management clients

[andrewstucki]

This adds some high-level clients for managing Users and ACLs in a Redpanda cluster. The general API around usage for the clients are:

ACL synchronization

var syncer *acls.Syncer
syncer.Sync(ctx, user) // or when we eventually want to do group-based ACL management, syncer.Sync(ctx, group)
syncer.DeleteAll(ctx, user) // same as above

User management

var client *users.Client
client.Has(ctx, user)
client.Create(ctx, user)
client.Delete(ctx, user)

The difference in the API abstractions, (i.e. Sync v. Create is because creation is really a one time operation, we just check to make sure a user with the given SASL mechanism is created in the cluster, ACLs however, can be changed all the time for a given user, so we'll pretty much be periodically "synchronizing" state until we clean up all ACLs when the custom resource is deleted.

[chrisseto]

No major blockers!

[chrisseto]

I generally consider checks on receivers being nil a code smell as it discourages performing nil checks. In what situation would this be used, especially considering that there's a Unknown type?

[andrewstucki]

TBH, this is pretty much just a bit of defensive programming and test ergonomics. It allows me to elide doing an explicit nil check elswhere with what should be otherwise be an unreachable nil. PatternType defaults to literal if unspecified in the CR creation payload, so this just does the same -- if the pointer itself is nil just return literal. The only time I really imagine this coming up is in tests where I'm passing a directly initialized ACLRule without a PatternType specified rather than going through the API server which will default the field.

[chrisseto]

I'm not asking for any changes here or arguing that you should change your style. I'm curious to see where the conversation takes us.

I consider this to be "anxious code". It's uncertain about how it's going to be used and tries to account for cases that are unexpected, which can lead to difficult to debug behavior. It may be an unpopular opinion but I'd prefer to generate panics in such cases or find ways to avoid them entirely. This is assuming that somewhere higher in the chain there exists a recover call as I wouldn't want to entirely crash the operator.

What's interesting here is that we now have a non-zero default (literal), an unknown value, and a zero value for the pointer variant of this struct.

((*PatternType)(nil)).ToKafka() // kmsg.ACLResourcePatternTypeLiteral
(PatternType("")).ToKafka() // kmsg.ACLResourcePatternTypeUnkown

If we instead change this to a value receiver and made the zero value of the struct the same as the default value, you'd have a type that's more consistent and close to impossible to miss-use:

	patternTypeToKafka = map[PatternType]kmsg.ACLResourcePatternType{
                 "":  kmsg.ACLResourcePatternTypeLiteral,
		PatternTypeLiteral:  kmsg.ACLResourcePatternTypeLiteral,
		PatternTypePrefixed: kmsg.ACLResourcePatternTypePrefixed,
	}

func (p PatternType) ToKafka() kmsg.ACLResourcePatternType {
    if patternType, ok := patternTypeToKafka[p]; ok {
        return patternType
    }
    return kmsg.ResourcePatternTypeUnknown
}


((*PatternType)(nil)).ToKafka() // Compile error
((PatternType)("")).ToKafka() // kmsg.Literal
((PatternType)("literal")).ToKafka() // kmsg.Literal
((PatternType)("other")).ToKafka() // kmsg.Unknown

Really go's enums are lacking a lot of power. I like to try to account for the edges up front as they're difficult to change later on. Though it matters about... 25% of the time?

[andrewstucki]

Yeah, so this seems to be a funny thing with zero-values. I guess what I'm trying to do with this code is make the parsing behavior roughly equivalent to the json payload behavior so we can just do a simple call chain rather than nil check everywhere. With the use of a pointer + a default for the api-server the exact behavior of passing in values is:

1. Pass nil --> literal because of defaults
2. Pass "literal" --> literal
3. Pass "prefixed" --> prefixed
4. Pass "other" --> rejected because unknown
5. Pass "" --> rejected because unknown

The cases in question are 1 and 5. I might be able to adjust the CRD definition such that we're specifying a string rather than a pointer, adding an omitempty and still defaulting properly, in which case we'd remove the nil condition.

I just dislike the fact that doing so would mean that the struct itself is just relying on the particularities of Go's JSON marshaler to drop zero-values with an omitempty tag, such that you'd have to manually construct a json payload to ever pass an empty string directly.

If you still feel strongly about this though, I have no problem changing it in a follow-up.

[chrisseto]

The cases in question are 1 and 5. I might be able to adjust the CRD definition such that we're specifying a string rather than a pointer, adding an omitempty and still defaulting properly, in which case we'd remove the nil condition.

Hmmm, I seem to recall there being some gotcha's with defaulting and specified non-nil values. I misremembered it on the other aspects of defaulting so I could very well be wrong here too.

I just dislike the fact that doing so would mean that the struct itself is just relying on the particularities of Go's JSON marshaler to drop zero-values with an omitempty tag, such that you'd have to manually construct a json payload to ever pass an empty string directly.

This is actually why I've historically leaned away from defaulting. My primarily clients where typed usages in Go, so respecting Go's peculiarities was important. The majority of our users won't be typed usages now.

If you still feel strongly about this though, I have no problem changing it in a follow-up.

You make excellent points! Let's leave it as is for now. It may be good to make some rules of thumbs for dealing with CRDs as they follow a different set of conventions that standard go structs. Namely, do we ignore cases that aren't technically possible due to the API servers validation and defaulting or try to find ways to bridge the gaps.

[chrisseto]

It seems like the passwordGenerator itself should hold the Kubernetes client here. That would let you test this client with a simple in memory password store and remove any kubernetes considerations all together.

[andrewstucki]

I'm going to opt, at least for now, to keep the Kubernetes client within the user client -- especially since there's already some code testing the Kubernetes part of the password generation. For doing more direct testing of the creation routines, I'm pretty much doing a no-no and calling the unexported create implementation directly with a password, so that way I can separate out tests for the API calls v. the secret creation.

Ideally I think you're right and I could refactor out some sort of password store and pass in an interface, but I think this is readable/tested well enough for now that this could be a later pass.

[chrisseto]

Nothing to change in this PR but the more restrictive timeouts here are nice. We all seemed to have pretty strong feelings about wanting go test to run quite fast. I generally like to have test contexts informed by the -timeout flag as a whole, there's a helper in the charts repo for doing exactly that.

Would it be worth standardizing on -short running with -timeout ~2m and enforcing that within CI?

[andrewstucki]

I'd be fine with that -- I've never fully run all of them locally, but I'm assuming that go test ./... is currently just unit (or lightweight integration) tests?

That does bring up another question though, since we've been talking about making all e2e-style tests go-testable... I'm going to be toying soon with how we should go about doing some full e2e tests sans kuttl. What are your thoughts on build flagging the e2e tests or just putting them in some top-level folder that we can skip on the typical unit-based go test run?

[chrisseto]

The helm charts repo is currently using -short to opt out of integration/e2e style tests. I'm not the biggest fan of that method, it was just the easiest to add at the time.

Build flags work pretty well but I would suggest that we use build flags to toggle the implementation of helper along the lines of: testutil.SkipIfNotIntegration(t). That way e2e/integration tests will still get built, linted, and skipped by default. Flagging out an entire package gets a bit frustrating as go test ./... won't catch breaking changes 😅

[chrisseto]

Sorry, I was not very clear at all here. I wasn't asking for a error check. I don't think strings.Builder can return errors in these cases IIRC, kinda like how hashes implement io.Writer but never return errors. I was musing about golangci-lint either missing something or having an oddly helpful special case in place.

[chrisseto]

Nice!