backend: OAuth SASL extensions support #1445
Conversation
|
|
smaxx
force-pushed
the
oauth_custom_properties
branch
from
6e22963 to
f385996
Compare
smaxx
force-pushed
the
oauth_custom_properties
branch
from
f385996 to
d3ed991
Compare
smaxx
changed the title
| @@ -28,6 +28,7 @@ type KafkaSASLOAuthBearer struct { | |||
| ClientSecret string `yaml:"clientSecret"` | |||
| TokenEndpoint string `yaml:"tokenEndpoint"` | |||
| Scope string `yaml:"scope"` | |||
| Extensions string `yaml:"extensions"` | |||
There was a problem hiding this comment.
| Extensions string `yaml:"extensions"` | |
| Extensions map[string]string `yaml:"extensions"` |
Mapstructure has decode hooks that can parse the maps apropriately when reading from YAML. We should use the apropriate type here. Not entirely sure whether we can use flags for that, but we primarily recommend YAML configuration to our users.
There was a problem hiding this comment.
honestly i thought about this option as well(wouldn't require manual parsing as a major pro), however focused on the option which allows using as both yaml and command line flag.
Theoretically we could have 2nd hidden property which would be populated from a flag and then parsed only when we deal with a flag. What do you think? Or not to overcomplicate things just to support the yaml-based approach?
There was a problem hiding this comment.
imo we can just omit the flag parsing here and keep things simple.
There was a problem hiding this comment.
please see the new version. Long story short: i tried introducing string-to-string map as you suggested, however Go parses map with lowercasing the keys(as far as i understand it is a well known issue in Go world). That is why not to depend on any case-related restrictions i fell back to collection of key-value structs. Works perfectly well
There was a problem hiding this comment.
actually the lowercasing happens in backend/pkg/config/config.go(see around line 163), due to the way how we deal with yaml keys vs env variable names. So either the approach has to be reworked to support whatever possible key format or we need to leave the actual SASL extension key as a property value which by default is not modified during Yaml parsing process
There was a problem hiding this comment.
@weeco please review if the current implementation is valid and makes sense to you. Thanks
smaxx
force-pushed
the
oauth_custom_properties
branch
from
918aaae to
4761b3d
Compare
|
Thanks for the contribution @smaxx , LGTM! |
Kafka SASL OAuth supports sending extensions(custom key-value pairs) during OAuth authentication. These extensions are also supported by franz-go(https://pkg.go.dev/github.com/twmb/franz-go/pkg/sasl/oauth).
So this PR adds a support for these custom properties