Skip to content
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.

Sign up for GitHub

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

Jump to bottom

context: AppIfConfigured returns error; consider not-yet-provisioned modules #6292

Merged
merged 3 commits into from
May 20, 2024
Merged

context: AppIfConfigured returns error; consider not-yet-provisioned modules #6292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
478ee55
context: Add new `AppStrict()` method to avoid instantiating empty apps
francislavoie May 2, 2024
1e51488
Rename AppStrict -> AppIfConfigured
mholt May 20, 2024
610c343
Merge branch 'master' into ctx-app-strict
mholt May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 25 additions & 3 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,32 @@ func (ctx Context) App(name string) (any, error) {
return modVal, nil
}

// AppStrict is like App, but it returns an error if the app
// has not been configured. This is useful when the app is
// required and its absence is a configuration error.
func (ctx Context) AppStrict(name string) (any, error) {
if app, ok := ctx.cfg.apps[name]; ok {
return app, nil
}
appRaw := ctx.cfg.AppsRaw[name]
if appRaw == nil {
return nil, fmt.Errorf("app module %s is not configured", name)
}
return ctx.App(name)
}

// AppIfConfigured returns an app by its name if it has been
// configured. Can be called instead of App() to avoid
// instantiating an empty app when that's not desirable. If
// the app has not been loaded, nil is returned.
// configured and provisioned. Can be called instead of App()
// to avoid instantiating an empty app when that's not desirable.
// If the app has not been loaded, nil is returned.
//
// Since this method does not return the app if it has not been
// provisioned, this could be misleading if the provisioning order
// caused the app to not be provisioned yet. This could return
// nil even if the app is configured, but not yet provisioned.
// In that case, the caller should probably use AppStrict() instead
// to ensure the app is provisioned, and no app will be instantiated
// if it is not configured.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the more I read this, the more I think we need to just fix this: if the app has been configured, load and provision it if needed. Otherwise, don't. This method is called AppIfConfigured not AppIfProvisionedAlready...

What do you think if we try to do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you had a particular need when you changed it in 0e2c7e1, are those semantics not needed anymore?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think either change is quite right though...

The earlier version would provision an app that maybe shouldn't be provisioned (I forgot about the nondeterminstic order). The change makes it never provision the app, but also disregards the loading order.

AppIfConfigured should provision the app if, but ONLY if, it will be provisioned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I follow. So what do you think should be done? Just rename AppStrict to AppIfConfigured (and drop the current one & functionality)?

Why did the difference matter for the cert cache commit again? Can you give an example of when it would have been wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a dummy, AppStrict() already does exactly what I'm describing, lol.

So yeah, let's replace AppIfConfigured with AppStrict. This does add an error return to the signature but we had it originally and I don't think there's anyone using this yet? (maybe 1 or 2? I apologize if that breaks anyone, but it's a very very easy patch, and I think this is an important fix).

//
// We return any type instead of the App type because it is not
// intended for the caller of this method to be the one to start
Expand Down
7 changes: 5 additions & 2 deletions modules/caddypki/adminapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@ func (a *adminAPI) Provision(ctx caddy.Context) error {
a.ctx = ctx
a.log = ctx.Logger(a) // TODO: passing in 'a' is a hack until the admin API is officially extensible (see #5032)

// Avoid initializing PKI if it wasn't configured
if pkiApp := a.ctx.AppIfConfigured("pki"); pkiApp != nil {
// Avoid initializing PKI if it wasn't configured.
// We intentionally ignore the error since it's not
// fatal if the PKI app is not explicitly configured.
pkiApp, err := ctx.AppStrict("pki")
if err == nil {
a.pkiApp = pkiApp.(*PKI)
}

Expand Down
12 changes: 6 additions & 6 deletions modules/caddytls/capools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,9 @@ func (PKIRootCAPool) CaddyModule() caddy.ModuleInfo {

// Loads the PKI app and load the root certificates into the certificate pool
func (p *PKIRootCAPool) Provision(ctx caddy.Context) error {
pkiApp := ctx.AppIfConfigured("pki")
if pkiApp == nil {
return fmt.Errorf("PKI app not configured")
pkiApp, err := ctx.AppStrict("pki")
if err != nil {
return fmt.Errorf("pki_root CA pool requires that a PKI app is configured: %v", err)
}
pki := pkiApp.(*caddypki.PKI)
for _, caID := range p.Authority {
Expand Down Expand Up @@ -260,9 +260,9 @@ func (PKIIntermediateCAPool) CaddyModule() caddy.ModuleInfo {

// Loads the PKI app and load the intermediate certificates into the certificate pool
func (p *PKIIntermediateCAPool) Provision(ctx caddy.Context) error {
pkiApp := ctx.AppIfConfigured("pki")
if pkiApp == nil {
return fmt.Errorf("PKI app not configured")
pkiApp, err := ctx.AppStrict("pki")
if err != nil {
return fmt.Errorf("pki_intermediate CA pool requires that a PKI app is configured: %v", err)
}
pki := pkiApp.(*caddypki.PKI)
for _, caID := range p.Authority {
Expand Down