NATS backend support #59
Conversation
Add NATS JetStream Object Store backend support
updated go.mod and go.sum for NATS
| // Simple encryption helper | ||
| func helperEncrypt(key []byte, plaintext []byte) ([]byte, error) { |
There was a problem hiding this comment.
I cannot say I am charmed by undocumented DIY crypto implementations.
Also please read the points in #52 (comment)
Perhaps any client side encryption should be implemented as an swappable middleware layer between simpleblob and the application that can be used by multiple backends?
There was a problem hiding this comment.
On the subject of "undocumented DIY crypto implementations."
Far from the case. I would point out that it is (a) nothing but a thin-wrapper around Go stdlib crypto (b) the implementation is basically nothing more than a copy/paste of the example from the Go docs (https://pkg.go.dev/golang.org/x/crypto/chacha20poly1305#example-NewX). The only significant difference is I don't panic() but return the error. 😉
Sadly NATS does not have built-in object-level SSE-C otherwise I would have gladly used that.
Thanks for all the feedback though, much appreciated food for thought.
| // Options enumerates various configuration options | ||
| type Options struct { | ||
| // Params related to reconnection attempts | ||
| DisableRetryOnFailedConnect bool `yaml:"disableRetryOnFailedConnect"` |
There was a problem hiding this comment.
All the other simpleblob backends use disable_retry_on_failed_connect style options, which are more common for yaml, at least in Go.
| // Params related to reconnection attempts | ||
| DisableRetryOnFailedConnect bool `yaml:"disableRetryOnFailedConnect"` | ||
| MaxReconnects int `yaml:"maxReconnects"` | ||
| ReconnectWaitSeconds int `yaml:"reconnectWait"` |
There was a problem hiding this comment.
Options that are a duration should directly use time.Duration.
| // DefaultMaxReconnects - Max Reconnect attempts | ||
| DefaultMaxReconnects = 5 | ||
| // DefaultReconnectWaitSeconds - Time to wait before attempting reconnect | ||
| DefaultReconnectWaitSeconds = 1 |
There was a problem hiding this comment.
Use time.Duration instead, like
| DefaultReconnectWaitSeconds = 1 | |
| DefaultReconnectWait = 1 * time.Second |
| username authType = iota | ||
| token | ||
| nkey | ||
| credentials | ||
| nkeyJwt | ||
| jwt | ||
| invalid |
There was a problem hiding this comment.
Global enums should have a common prefix for readability, otherwise it is hard to understand where that token is suddenly coming from, e.g. AuthUsername, AuthToken, etc.
| // Auth type flag set internally by checkCredentialsAvailability() | ||
| internalUseAuthType authType | ||
| // Converted type for wait seconds | ||
| internalReconnectWaitSeconds time.Duration |
There was a problem hiding this comment.
Don't add Seconds to the name if it's a Duration.
As mentioned elsewhere, there is no need to convert. You can just directly use time.Duration.
| // EncryptionKey - key (hex format,256 bit) for encryption at rest | ||
| EncryptionKey string `yaml:"encryptionKey"` |
There was a problem hiding this comment.
See other comment about homegrown encryption.
| GlobalPrefix string `yaml:"global_prefix"` | ||
| // EncryptionKey - key (hex format,256 bit) for encryption at rest | ||
| EncryptionKey string `yaml:"encryptionKey"` | ||
| // S3 Compatability |
There was a problem hiding this comment.
As per comment below, the problem was the need for tester/DoBackendTests conformance.
| if o.NatsTLSRootCA != "" { | ||
| err := fileExistsAndIsReadable(o.NatsTLSRootCA) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
You may want to wrap this in a hint about which option is incorrect. Same elsewhere.
There was a problem hiding this comment.
Just FYI, my intention with this and most of the NATS options was "you know if you need them", i.e. you know if your environment is using an in-house CA (and therefore you need to provide the root pubkey), you know what auth-type your environment is using (and therefore you know if you need to provide an NKey or a username/password) etc.
Hence I did not expend much effort explaining in detail what every option was.
| // prependGlobalPrefix prepends the GlobalPrefix to the name/prefix | ||
| // passed as input | ||
| func (b *Backend) prependGlobalPrefix(name string) string { | ||
| return b.opt.GlobalPrefix + name | ||
| } | ||
|
|
||
| // setGlobalPrefix updates the global prefix in b and the cached marker name, | ||
| // so it can be dynamically changed in tests. | ||
| func (b *Backend) setGlobalPrefix(prefix string) { | ||
| b.opt.GlobalPrefix = prefix | ||
| } |
There was a problem hiding this comment.
Prefixes made sense in the context of S3 buckets, but perhaps not in the context of NATS buckets that can easily be created on the fly.
There was a problem hiding this comment.
Ultimatley the fundamental problem if I was testing the backend against tester/DoBackendTests then I had little choice to implement prefix support, otherwise DoBackendTests would complain.
Code to add NATS SSE-C Support #55
Was originally PR #56 , but I made a mess of that fork. So this is based off a clean fork with the code in its own branch to keep it clean.
N.B. go.mod will need to be bumped to min. 1.20 due to NATS dependency (that is also why the CI is failing because CI is currently running against old go.mod)