Unexported struct fields in the Config can lead to crash on startup

[ctrox]

In order to reuse an API client, I put it as an unexported field into the Config, like so:

package main

import (
	"github.com/Kong/go-pdk/server"
	management "github.com/rancher/rancher/pkg/client/generated/management/v3"
)

type Config struct {
	rancherClient *management.Client
}

func New() interface{} {
	return &Config{}
}

func main() {
	server.StartServer(New, "v0.0.1", 0)
}

go run . -dump

Note that the above might crash your computer when run! Be ready to ctrl-c to end the process. For some reason, this particular struct causes the whole memory to fill up pretty quickly. The struct contains lots of fields and nesting levels, but I was unable to reproduce the issue with a more simple struct.

Anyway, a simple fix would be to just ignore unexported fields. My understanding is that unexported fields won't work in the config anyway, since it needs to be Unmarshaled with json.Unmarshal later. I will post a PR with that fix but let me know if you would take a different approach here.

[javierguerragiraldez]

Hi,
it's hard to tell what is happening here, but yes; unexported fields should be ignored by the (un)marshalling.

that said, the config structure is being created at different points in time by calling the New() function. there you can ensure those non-exported fields are correctly dealt with.

note that it's normal to create config objects that are never actually used, and also they're dropped (and disposed by the GC) without notice.

EDIT: after checking the PR I see what you mean. yes, unexported fields shouldn't be added to the schema. I'll do a review there.

[ctrox]

EDIT: after checking the PR I see what you mean. yes, unexported fields shouldn't be added to the schema. I'll do a review there.

Thanks!

But just to be clear holding state in the config objects should not be done? At least so far I have not noticed any issues with that. If so, how would you recommend holding some state in a plugin, with global vars?

In a perfect world a plugin would be completely stateless, but for example being able to cache some things is almost required for some use cases.

[javierguerragiraldez]

But just to be clear holding state in the config objects should not be done? At least so far I have not noticed any issues with that. If so, how would you recommend holding some state in a plugin, with global vars?

It's OK to use private fields for whatever purpose. The caveat is that config objects can be dropped by the framework without warning, so you can't depend on them being persistent. Also, there might be any number of them at the same time, so it's not a good place to put semaphores to arbitrate data stored somewhere else.

Specifically:

• your New() function is called a number of times. There you can set any field before returning a new config instance.
• the exported fields are filled with the configuration data from the Kong config (either database, declarative or from a Control Plane)
• the config objects are used to call phase handlers. They're free to modify them too, but it would probably be unwise to change exported fields.
• if any config object has been unused for a while (default 60 secs), it can be discarded.
• if the configuration changes (either from the admin API or from a Control Plane), new config objects are created (by calling New() and filling exported fields with the new configuration data). the old objects will be unused and eventually discarded.
• If the Kong configuration contains different instances of the same plugin, they're associated with different config objects.
• For each configuration instance, there will be more than one config object instance at the same time, even if they're identical (this simplifies having multiple requests "on flight" at the same time).
• There's no guarantee that different phases of the same request would get the same config object instance.

In all, there's very few uses that would be best served by private fields in the config object. In most cases I guess global variables (protected by mutexes), internal service threads and/or database connections (if you want persistence) are more generally useful.

[ctrox]

Thanks for the explanation, that is really helpful! I will avoid storing things in the config struct then.