-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: validate that configs/secrets used match those provided #1423
Changes from all commits
4d6ec5f
3b76d00
97c1a39
af7077f
2a982c4
317d58a
1f5637f
c0200e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ func (b BuildStartedListenerFunc) OnBuildStarted(project Project) { b(project) } | |
// Engine for building a set of modules. | ||
type Engine struct { | ||
client ftlv1connect.ControllerServiceClient | ||
projectConfig *projectconfig.Config | ||
projectMetas *xsync.MapOf[ProjectKey, projectMeta] | ||
moduleDirs []string | ||
externalDirs []string | ||
|
@@ -82,10 +83,11 @@ func WithListener(listener Listener) Option { | |
// pull in missing schemas. | ||
// | ||
// "dirs" are directories to scan for local modules. | ||
func New(ctx context.Context, client ftlv1connect.ControllerServiceClient, moduleDirs []string, externalDirs []string, options ...Option) (*Engine, error) { | ||
func New(ctx context.Context, client ftlv1connect.ControllerServiceClient, projConfig *projectconfig.Config, moduleDirs []string, externalDirs []string, options ...Option) (*Engine, error) { | ||
ctx = rpc.ContextWithClient(ctx, client) | ||
e := &Engine{ | ||
client: client, | ||
projectConfig: projConfig, | ||
moduleDirs: moduleDirs, | ||
externalDirs: externalDirs, | ||
projectMetas: xsync.NewMapOf[ProjectKey, projectMeta](), | ||
|
@@ -496,13 +498,115 @@ func (e *Engine) buildWithCallback(ctx context.Context, callback buildCallback, | |
allErrors = append(allErrors, err) | ||
} | ||
|
||
allErrors = append(allErrors, e.validateConfigsAndSecretsMatch(ctx, builtModules)...) | ||
|
||
if len(allErrors) > 0 { | ||
return errors.Join(allErrors...) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (e *Engine) validateConfigsAndSecretsMatch(ctx context.Context, builtModules map[string]*schema.Module) []error { | ||
errs := []error{} | ||
logger := log.FromContext(ctx) | ||
|
||
configsProvidedGlobally := make(map[string]bool) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's quite a lot of duplication here. I think this could be refactored to use a function that takes a map of merged global+module entries and compares them to a map of used module entries, and returns errors with the differences. Pass in the "type" of entries (config or secret) as a string for more useful errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keys in the merged map could be in the form |
||
secretsProvidedGlobally := make(map[string]bool) | ||
if e.projectConfig != nil { | ||
for configName := range e.projectConfig.Global.Config { | ||
configsProvidedGlobally[configName] = true | ||
} | ||
for secretName := range e.projectConfig.Global.Secrets { | ||
secretsProvidedGlobally[secretName] = true | ||
} | ||
} | ||
|
||
configsUsed := make(map[string]bool) | ||
secretsUsed := make(map[string]bool) | ||
for moduleName, module := range builtModules { | ||
configsUsedInModule, secretsUsedInModule, moduleErrs := e.validateConfigsAndSecretsMatchForModule(ctx, moduleName, module, configsProvidedGlobally, secretsProvidedGlobally) | ||
errs = append(errs, moduleErrs...) | ||
for configName := range configsUsedInModule { | ||
configsUsed[configName] = true | ||
} | ||
for secretName := range secretsUsedInModule { | ||
secretsUsed[secretName] = true | ||
} | ||
} | ||
|
||
if e.projectConfig != nil { | ||
for configName := range e.projectConfig.Global.Config { | ||
if _, isUsed := configsUsed[configName]; !isUsed { | ||
logger.Warnf("config %q is provided globally in ftl-project.toml, but is not required by any modules", configName) | ||
} | ||
} | ||
for secretName := range e.projectConfig.Global.Secrets { | ||
if _, isUsed := secretsUsed[secretName]; !isUsed { | ||
logger.Warnf("secret %q is provided globally in ftl-project.toml, but is not required by any modules", secretName) | ||
} | ||
} | ||
} | ||
|
||
return errs | ||
} | ||
|
||
// validateConfigsAndSecretsMatchForModule is a helper function for validateConfigsAndSecretsMatch. | ||
// `globalConfig` and `globalSecrets` store the names of all the configs/secrets defined globally in | ||
// ftl-project.toml, with O(1) `contains` checks. This function logs warnings for any module-level | ||
// configs/secrets that are provided but not used, then returns maps whose keys are all the | ||
// configs/secrets used by this module. | ||
func (e *Engine) validateConfigsAndSecretsMatchForModule(ctx context.Context, moduleName string, module *schema.Module, globalConfig map[string]bool, globalSecrets map[string]bool) (map[string]bool, map[string]bool, []error) { | ||
errs := []error{} | ||
logger := log.FromContext(ctx) | ||
|
||
configsUsed := make(map[string]bool) | ||
secretsUsed := make(map[string]bool) | ||
for _, d := range module.Decls { | ||
switch d := d.(type) { | ||
case *schema.Config: | ||
configsUsed[d.Name] = true | ||
case *schema.Secret: | ||
secretsUsed[d.Name] = true | ||
default: | ||
} | ||
} | ||
|
||
// Index all provided configs into configsProvided and warn for unused configs | ||
configsProvided := maps.Clone(globalConfig) | ||
secretsProvided := maps.Clone(globalSecrets) | ||
if e.projectConfig != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use pointers to convey optionality. |
||
moduleConfigAndSecrets, moduleConfigAndSecretsExists := e.projectConfig.Modules[moduleName] | ||
if moduleConfigAndSecretsExists { | ||
for configName := range moduleConfigAndSecrets.Config { | ||
configsProvided[configName] = true | ||
if _, isUsed := configsUsed[configName]; !isUsed { | ||
logger.Warnf("config %q is provided for module %q in ftl-project.toml, but is not required", configName, moduleName) | ||
} | ||
} | ||
for secretName := range moduleConfigAndSecrets.Secrets { | ||
secretsProvided[secretName] = true | ||
if _, isUsed := secretsUsed[secretName]; !isUsed { | ||
logger.Warnf("secret %q is provided for module %q in ftl-project.toml, but is not required", secretName, moduleName) | ||
} | ||
} | ||
} | ||
} | ||
|
||
for configName := range configsUsed { | ||
if _, isProvided := configsProvided[configName]; !isProvided { | ||
errs = append(errs, fmt.Errorf("config %q is not provided in ftl-project.toml, but is required by module %q", configName, moduleName)) | ||
} | ||
} | ||
for secretName := range secretsUsed { | ||
if _, isProvided := secretsProvided[secretName]; !isProvided { | ||
errs = append(errs, fmt.Errorf("secret %q is not provided in ftl-project.toml, but is required by module %q", secretName, moduleName)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors should include the positional information from the schema. |
||
} | ||
} | ||
|
||
return configsUsed, secretsUsed, errs | ||
} | ||
|
||
func (e *Engine) tryBuild(ctx context.Context, mustBuild map[ProjectKey]bool, key ProjectKey, builtModules map[string]*schema.Module, schemas chan *schema.Module, callback buildCallback) error { | ||
logger := log.FromContext(ctx) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[global.configuration] | ||
ftlEndpoint = "http://ftlEndpoint" | ||
ftlEndpointAlternate = "http://ftlEndpointAlternate" | ||
|
||
[modules.configsecret.configuration] | ||
githubAccessToken = "keychain://githubAccessToken" | ||
someServiceAccessToken = "keychain://someServiceAccessToken" | ||
|
||
[modules.configsecret.secrets] | ||
encryptionKey = "inline://notASensitiveSecret" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package configsecret | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/TBD54566975/ftl/go-runtime/ftl" // Import the FTL SDK. | ||
) | ||
|
||
var configGlobal = ftl.Config[string]("ftlEndpoint") | ||
var configGithub = ftl.Config[string]("githubAccessToken") | ||
var configMissing = ftl.Config[string]("missingConfig") | ||
|
||
var secretEncrypt = ftl.Secret[string]("encryptionKey") | ||
var secretMissing = ftl.Secret[string]("missingSecret") | ||
|
||
type EchoRequest struct { | ||
Name ftl.Option[string] `json:"name"` | ||
} | ||
|
||
type EchoResponse struct { | ||
Message string `json:"message"` | ||
} | ||
|
||
//ftl:verb | ||
func Echo(ctx context.Context, req EchoRequest) (EchoResponse, error) { | ||
return EchoResponse{Message: fmt.Sprintf("Hello, %s!", req.Name.Default("anonymous"))}, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
module = "configsecret" | ||
language = "go" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
module ftl/configsecret | ||
|
||
go 1.22.2 | ||
|
||
require github.com/TBD54566975/ftl v0.202.5 | ||
|
||
require ( | ||
connectrpc.com/connect v1.16.1 // indirect | ||
connectrpc.com/grpcreflect v1.2.0 // indirect | ||
connectrpc.com/otelconnect v0.7.0 // indirect | ||
github.com/alecthomas/concurrency v0.0.2 // indirect | ||
github.com/alecthomas/participle/v2 v2.1.1 // indirect | ||
github.com/alecthomas/types v0.14.0 // indirect | ||
github.com/alessio/shellescape v1.4.2 // indirect | ||
github.com/danieljoos/wincred v1.2.0 // indirect | ||
github.com/go-logr/logr v1.4.1 // indirect | ||
github.com/go-logr/stdr v1.2.2 // indirect | ||
github.com/godbus/dbus/v5 v5.1.0 // indirect | ||
github.com/jackc/pgpassfile v1.0.0 // indirect | ||
github.com/jackc/pgservicefile v0.0.0-20221227161230-091c0ba34f0a // indirect | ||
github.com/jackc/pgx/v5 v5.5.5 // indirect | ||
github.com/jackc/puddle/v2 v2.2.1 // indirect | ||
github.com/jpillora/backoff v1.0.0 // indirect | ||
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect | ||
github.com/mattn/go-isatty v0.0.20 // indirect | ||
github.com/multiformats/go-base36 v0.2.0 // indirect | ||
github.com/swaggest/jsonschema-go v0.3.70 // indirect | ||
github.com/swaggest/refl v1.3.0 // indirect | ||
github.com/zalando/go-keyring v0.2.4 // indirect | ||
go.opentelemetry.io/otel v1.26.0 // indirect | ||
go.opentelemetry.io/otel/metric v1.26.0 // indirect | ||
go.opentelemetry.io/otel/trace v1.26.0 // indirect | ||
golang.org/x/crypto v0.22.0 // indirect | ||
golang.org/x/exp v0.0.0-20240416160154-fe59bbe5cc7f // indirect | ||
golang.org/x/mod v0.17.0 // indirect | ||
golang.org/x/net v0.24.0 // indirect | ||
golang.org/x/sync v0.7.0 // indirect | ||
golang.org/x/sys v0.20.0 // indirect | ||
golang.org/x/text v0.14.0 // indirect | ||
google.golang.org/protobuf v1.34.0 // indirect | ||
) | ||
|
||
replace github.com/TBD54566975/ftl => ../../../.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projectconfig.Config
is not the appropriate type for this use. It is the raw configuration struct used to unmarshal TOML files, so it shouldn't be used outside that scope.ModuleContext
OTOH is the appropriate type to use here, but IIRC there's no mechanism to get one prior to build. This needs some more thought, but my gut instinct is telling me we need an abstraction to retrieveModuleConfig
s from aprojectconfig.Config
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if implementing the above is too much work we should abandon this ticket for now as it's not super high priority. Perhaps timebox it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhhh got it, that explains a lot. I'll table this for now and pick up one of the
next
issues. We've talked a few times lately about possibly refactoring the interfaces for the TOML files, so it probably makes the most sense to come up with a holistic design for that that includes this use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just happened to see #919 in
next
:) Just to make sure I'm reading correctly, is that issue basically the blocker to this one?