Skip to content

Commit

Permalink
chore!: unify sources configuration handling (open-feature#560)
Browse files Browse the repository at this point in the history 
## This PR

This PR is best reviewed by going through the new documentation [1]

Restructured flagd startup configuration handling with respect to sync
providers. Documentation is updated [1] & handling of prefixes is
cleared (ex:- removal of grpc(s):// prefix for `--source` based sync
provider startup argument)

## Breaking change

Contains a breaking change with respect to how sync provider URIs are
handled

- **what** -source configuration URI parsing
- **why** - align source configuration handling among different sources
& avoid mix uage of prefixes to aling with documented behavior
- **how** - source configuration no longer validate or parse URI
prefixes. This means, existing source configuration strings/files may
need an upgrade if they used prefixes in URIs. The upgrade is to simply
remove any prefixes from source configuration URIs

For example, 

**old**

``` yaml
sources:
- uri: grpcs://my-flag-source:8080 # mixed usage of schema shared with URI prefixes ❌ 
  provider: grpc
  certPath: /certs/ca.cert
```

**new**

``` yaml
sources:
- uri: my-flag-source:8080
  provider: grpc
  tls: true. # new way to convey secure connection requirement ✅ 
  certPath: /certs/ca.cert
```
____

Is a pre-requisite of
open-feature/open-feature-operator#389


[1] -
https://github.com/open-feature/flagd/blob/2a4783ef61eb3756e7a3f99aae596392906cd765/docs/configuration/configuration.md

---------

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
  • Loading branch information
@Kavindu-Dodan @toddbaert
Kavindu-Dodan and toddbaert authored Mar 28, 2023
1 parent 8c00040 commit 7f4888a
Show file tree
Hide file tree
Showing 8 changed files with 247 additions and 262 deletions.
28 changes: 16 additions & 12 deletions core/pkg/runtime/from_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
msync "sync"
"time"

"github.com/open-feature/flagd/core/pkg/sync/grpc/credentials"

"github.com/open-feature/flagd/core/pkg/service"

"go.opentelemetry.io/otel/exporters/prometheus"
Expand All @@ -21,7 +23,6 @@ import (
"github.com/open-feature/flagd/core/pkg/sync"
"github.com/open-feature/flagd/core/pkg/sync/file"
"github.com/open-feature/flagd/core/pkg/sync/grpc"
"github.com/open-feature/flagd/core/pkg/sync/grpc/credentials"
httpSync "github.com/open-feature/flagd/core/pkg/sync/http"
"github.com/open-feature/flagd/core/pkg/sync/kubernetes"
"github.com/robfig/cron"
Expand Down Expand Up @@ -53,6 +54,7 @@ type SourceConfig struct {

BearerToken string `json:"bearerToken,omitempty"`
CertPath string `json:"certPath,omitempty"`
TLS bool `json:"tls,omitempty"`
ProviderID string `json:"providerID,omitempty"`
Selector string `json:"selector,omitempty"`
}
Expand Down Expand Up @@ -162,10 +164,11 @@ func NewGRPC(config SourceConfig, logger *logger.Logger) *grpc.Sync {
zap.String("component", "sync"),
zap.String("sync", "grpc"),
),
CredentialBuilder: &credentials.CredentialBuilder{},
CertPath: config.CertPath,
ProviderID: config.ProviderID,
Secure: config.TLS,
Selector: config.Selector,
CredentialBuilder: &credentials.CredentialBuilder{},
}
}

Expand Down Expand Up @@ -218,24 +221,19 @@ func ParseSources(sourcesFlag string) ([]SourceConfig, error) {
if err := json.Unmarshal([]byte(sourcesFlag), &syncProvidersParsed); err != nil {
return syncProvidersParsed, fmt.Errorf("unable to parse sync providers: %w", err)
}
for i, sp := range syncProvidersParsed {
for _, sp := range syncProvidersParsed {
if sp.URI == "" {
return syncProvidersParsed, errors.New("sync provider argument parse: uri is a required field")
}
if sp.Provider == "" {
return syncProvidersParsed, errors.New("sync provider argument parse: provider is a required field")
}
switch uriB := []byte(sp.URI); {
case regFile.Match(uriB):
syncProvidersParsed[i].URI = regFile.ReplaceAllString(syncProvidersParsed[i].URI, "")
case regCrd.Match(uriB):
syncProvidersParsed[i].URI = regCrd.ReplaceAllString(syncProvidersParsed[i].URI, "")
}
}
return syncProvidersParsed, nil
}

// ParseSyncProviderURIs uri flag based sync sources to SourceConfig array. Replaces uri prefixes where necessary
// ParseSyncProviderURIs uri flag based sync sources to SourceConfig array. Replaces uri prefixes where necessary to
// derive SourceConfig
func ParseSyncProviderURIs(uris []string) ([]SourceConfig, error) {
syncProvidersParsed := []SourceConfig{}

Expand All @@ -256,10 +254,16 @@ func ParseSyncProviderURIs(uris []string) ([]SourceConfig, error) {
URI: uri,
Provider: syncProviderHTTP,
})
case regGRPC.Match(uriB), regGRPCSecure.Match(uriB):
case regGRPC.Match(uriB):
syncProvidersParsed = append(syncProvidersParsed, SourceConfig{
URI: uri,
URI: regGRPC.ReplaceAllString(uri, ""),
Provider: syncProviderGrpc,
})
case regGRPCSecure.Match(uriB):
syncProvidersParsed = append(syncProvidersParsed, SourceConfig{
URI: regGRPCSecure.ReplaceAllString(uri, ""),
Provider: syncProviderGrpc,
TLS: true,
})
default:
return syncProvidersParsed, fmt.Errorf("invalid sync uri argument: %s, must start with 'file:', "+
Expand Down
64 changes: 39 additions & 25 deletions core/pkg/runtime/from_config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestParseSource(t *testing.T) {
out: []SourceConfig{
{
URI: "config/samples/example_flags.json",
Provider: "file",
Provider: syncProviderFile,
},
},
},
Expand All @@ -34,55 +34,62 @@ func TestParseSource(t *testing.T) {
out: []SourceConfig{
{
URI: "config/samples/example_flags.json",
Provider: "file",
Provider: syncProviderFile,
},
{
URI: "http://test.com",
Provider: "http",
Provider: syncProviderHTTP,
BearerToken: ":)",
},
{
URI: "host:port",
Provider: "grpc",
Provider: syncProviderGrpc,
},
{
URI: "default/my-crd",
Provider: "kubernetes",
Provider: syncProviderKubernetes,
},
},
},
"multiple-syncs-with-options": {
in: `[
{"uri":"file:config/samples/example_flags.json","provider":"file"},
{"uri":"https://test.com","provider":"http","bearerToken":":)"},
{"uri":"host:port","provider":"grpc"},
{"uri":"host:port","provider":"grpcs","providerID":"appA","selector":"source=database"},
{"uri":"core.openfeature.dev/namespace/my-crd","provider":"kubernetes"}
]`,
in: `[{"uri":"config/samples/example_flags.json","provider":"file"},
{"uri":"http://my-flag-source.json","provider":"http","bearerToken":"bearer-dji34ld2l"},
{"uri":"https://secure-remote","provider":"http","bearerToken":"bearer-dji34ld2l"},
{"uri":"default/my-flag-config","provider":"kubernetes"},
{"uri":"grpc-source:8080","provider":"grpc"},
{"uri":"my-flag-source:8080","provider":"grpc", "tls":true, "certPath": "/certs/ca.cert", "providerID": "flagd-weatherapp-sidecar", "selector": "source=database,app=weatherapp"}]
`,
expectErr: false,
out: []SourceConfig{
{
URI: "config/samples/example_flags.json",
Provider: "file",
Provider: syncProviderFile,
},
{
URI: "https://test.com",
Provider: "http",
BearerToken: ":)",
URI: "http://my-flag-source.json",
Provider: syncProviderHTTP,
BearerToken: "bearer-dji34ld2l",
},
{
URI: "host:port",
Provider: "grpc",
URI: "https://secure-remote",
Provider: syncProviderHTTP,
BearerToken: "bearer-dji34ld2l",
},
{
URI: "host:port",
Provider: "grpcs",
ProviderID: "appA",
Selector: "source=database",
URI: "default/my-flag-config",
Provider: syncProviderKubernetes,
},
{
URI: "namespace/my-crd",
Provider: "kubernetes",
URI: "grpc-source:8080",
Provider: syncProviderGrpc,
},
{
URI: "my-flag-source:8080",
Provider: syncProviderGrpc,
TLS: true,
CertPath: "/certs/ca.cert",
ProviderID: "flagd-weatherapp-sidecar",
Selector: "source=database,app=weatherapp",
},
},
},
Expand Down Expand Up @@ -138,6 +145,7 @@ func TestParseSyncProviderURIs(t *testing.T) {
"file:my-file.json",
"https://test.com",
"grpc://host:port",
"grpcs://secure-grpc",
"core.openfeature.dev/default/my-crd",
},
expectErr: false,
Expand All @@ -151,8 +159,14 @@ func TestParseSyncProviderURIs(t *testing.T) {
Provider: "http",
},
{
URI: "grpc://host:port",
URI: "host:port",
Provider: "grpc",
TLS: false,
},
{
URI: "secure-grpc",
Provider: "grpc",
TLS: true,
},
{
URI: "default/my-crd",
Expand Down
26 changes: 10 additions & 16 deletions core/pkg/sync/grpc/credentials/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,35 +5,29 @@ import (
"crypto/x509"
"fmt"
"os"
"strings"

"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
)

const (
// Prefix for GRPC URL inputs. GRPC does not define a standard prefix. This prefix helps to differentiate remote
// URLs for REST APIs (i.e - HTTP) from GRPC endpoints.
Prefix = "grpc://"
PrefixSecure = "grpcs://"

tlsVersion = tls.VersionTLS12
)
const tlsVersion = tls.VersionTLS12

type Builder interface {
Build(string, string) (credentials.TransportCredentials, error)
Build(secure bool, certPath string) (credentials.TransportCredentials, error)
}

type CredentialBuilder struct{}

// Build is a helper to build grpc credentials.TransportCredentials based on source and cert path
func (cb *CredentialBuilder) Build(source string, certPath string) (credentials.TransportCredentials, error) {
if strings.Contains(source, Prefix) {
return insecure.NewCredentials(), nil
}
func (cb *CredentialBuilder) Build(secure bool, certPath string) (credentials.TransportCredentials, error) {
if !secure {
// check if certificate is set & make this an error so that we do not establish an unwanted insecure connection
if certPath != "" {
return nil, fmt.Errorf("provided a non empty certificate %s, but requested an insecure connection."+
" Please check configurations of the grpc sync source", certPath)
}

if !strings.Contains(source, PrefixSecure) {
return nil, fmt.Errorf("invalid source. grpc source must contain prefix %s or %s", Prefix, PrefixSecure)
return insecure.NewCredentials(), nil
}

if certPath == "" {
Expand Down
21 changes: 11 additions & 10 deletions core/pkg/sync/grpc/credentials/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,51 +64,52 @@ func TestCredentialBuilder_Build(t *testing.T) {

tests := []struct {
name string
source string
certPath string
secure bool
expectSecProto string
error bool
}{
{
name: "Insecure source results in insecure connection",
source: Prefix + "some.domain",
secure: false,
certPath: "",
expectSecProto: insecure,
},
{
name: "Secure source results in secure connection",
source: PrefixSecure + "some.domain",
certPath: validCertFile,
secure: true,
expectSecProto: tls,
},
{
name: "Secure source with no certificate results in a secure connection",
source: PrefixSecure + "some.domain",
secure: true,
expectSecProto: tls,
},
{
name: "Invalid cert path results in an error",
source: PrefixSecure + "some.domain",
secure: true,
certPath: "invalid/path",
error: true,
},
{
name: "Invalid certificate results in an error",
source: PrefixSecure + "some.domain",
secure: true,
certPath: invalidCertFile,
error: true,
},
{
name: "Invalid prefix results in an error",
source: "http://some.domain",
error: true,
name: "Prevent insecure if certificate path is set - configuration check",
secure: false,
certPath: validCertFile,
error: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
builder := CredentialBuilder{}
tCred, err := builder.Build(test.source, test.certPath)
tCred, err := builder.Build(test.secure, test.certPath)

if test.error {
if err == nil {
Expand Down
10 changes: 5 additions & 5 deletions core/pkg/sync/grpc/credentials/mock/builder.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 7f4888a

Please sign in to comment.