Empty map deserialized as string

ezracelli commented 1 year ago

I think this is the same issue as https://github.com/traefik/traefik/issues/8772, which was unfortunately closed without resolution.

The following traefik dynamic configuration object:

http:
  // -- snip --
  middlewares:
    testPluginMiddleware:
      plugin:
        testPlugin:
          variantA: {}

Is deserialized as (copied from traefik's --log.level=DEBUG logs):

DEBU[XXXX-XX-XXTXX:XX:XX-XX:XX] Configuration received: {"http":{"routers":"/* -- snip -- */","services":"/* -- snip -- */","middlewares":{"testPluginMiddleware":{"plugin":{"testPlugin":{"variantA":""}}}}},"tcp":{},"udp":{},"tls":{}}  providerName=file

variantA sub-option is "" (empty string) instead of {} (empty map) in the JSON serialization
This error happens regardless of the type of config file used (JSON, TOML, and YAML are all affected)

`paerser_test.go`

package traefik_plugin_thumbor_test

import (
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/traefik/paerser/file"
    "github.com/traefik/traefik/v2/pkg/config/dynamic"
    "github.com/traefik/traefik/v2/pkg/tls"
)

func newConfiguration() *dynamic.Configuration {
    return &dynamic.Configuration{
        HTTP: &dynamic.HTTPConfiguration{
            Routers:           make(map[string]*dynamic.Router),
            Middlewares:       make(map[string]*dynamic.Middleware),
            Services:          make(map[string]*dynamic.Service),
            ServersTransports: make(map[string]*dynamic.ServersTransport),
        },
        TCP: &dynamic.TCPConfiguration{
            Routers:     make(map[string]*dynamic.TCPRouter),
            Services:    make(map[string]*dynamic.TCPService),
            Middlewares: make(map[string]*dynamic.TCPMiddleware),
        },
        TLS: &dynamic.TLSConfiguration{
            Stores:  make(map[string]tls.Store),
            Options: make(map[string]tls.Options),
        },
        UDP: &dynamic.UDPConfiguration{
            Routers:  make(map[string]*dynamic.UDPRouter),
            Services: make(map[string]*dynamic.UDPService),
        },
    }
}

const yamlContent = `http:
  middlewares:
    testPluginMiddleware:
      plugin:
        testPlugin:
          variantA: {}
`

func TestYaml(t *testing.T) {
    configuration := newConfiguration()

    err := file.DecodeContent(yamlContent, ".yaml", configuration)
    if err != nil {
        t.Fatal(err)
    }

    assertEmptyMap(t, configuration)
}

func assertEmptyMap(t *testing.T, configuration *dynamic.Configuration) {
    t.Helper()

    value := configuration.HTTP.Middlewares["testPluginMiddleware"].Plugin["testPlugin"]["variantA"]
    assert.IsType(t, "", value) // PASS
    assert.IsType(t, make(map[string]interface{}), value) // FAIL
}

Context

I'm writing a traefik plugin which accepts one of a few possible configuration variants, which I was planning on implementing in the "traefik way" by using the pattern of one of many map fields.

For example, traefik dynamic config accepts:

http:
  middlewares:
    MIDDLEWARE_NAME:
      MIDDLEWARE_VARIANT:
        MIDDLEWARE_VARIANT_OPTION: MIDDLEWARE_VARIANT_OPTION_VALUE

...where "variant" might be addPrefix (the type of the middleware); exactly one variant is accepted.

In the same vein, I was structuring my plugin's config to be something like:

http:
  middlewares:
    MIDDLEWARE_NAME:
      plugin:
        PLUGIN_NAME:
          PLUGIN_VARIANT:
            PLUGIN_VARIANT_OPTION: PLUGIN_VARIANT_OPTION_VALUE

...where "variant" is one of the few possibilities mentioned above; exactly one variant is accepted. However, I ran into this issue because one of my "variants" doesn't have any sub-options, but the key is still required to be present. I believe this does not affect traefik because all its "variants" have sub-options.

`plugin.go`

package plugin

import (
    "context"
    "http"
)

type Config struct {
    VariantA *ConfigVariantA `json:"variantA,omitempty" toml:"variantA,omitempty" yaml:"variantA,omitempty"`
    VariantB *ConfigVariantB `json:"variantB,omitempty" toml:"variantB,omitempty" yaml:"variantB,omitempty"`
}

type ConfigVariantA struct {}

type ConfigVariantB struct {
    Option int `json:"option,omitempty" toml:"option,omitempty" yaml:"option,omitempty"`
}

// -- SNIP --

With the following dynamic config (same as above):

http:
  // -- snip --
  middlewares:
    testPluginMiddleware:
      plugin:
        testPlugin:
          variantA: {}

...I get the following error message from traefik:

ERRO[XXXX-XX-XXTXX:XX:XX-XX:XX] plugin: failed to decode configuration: 1 error(s) decoding:

* 'VariantA' expected a map, got 'string'  entryPointName=web routerName=router@file

ldez commented 1 year ago

Hello,

I confirm the bug, as a workaround, you have to define explicitly the default value of at least one of the fields.

Paerser handles different sources of data (mainly labels, but also files and env vars), and this diversity of sources induces complexity of the side effects.

ezracelli commented 1 year ago

No doubt parsing is a complex task! Would you accept a PR to fix this specifically for the file parser?

ezracelli commented 1 year ago

Ahh, after testing a bit further, I think I see why this is an issue specifically for plugins.

Emtpy maps are only allowed via the "emptyMap" label, which is fine for traefik's typed config. For example: https://github.com/traefik/traefik/blob/d97d3a6726ad6e50110dd2ff2b552a8b17eeed55/pkg/config/dynamic/http_config.go#L50

type Router struct {
        // -- snip --
    TLS         *RouterTLSConfig `json:"tls,omitempty" toml:"tls,omitempty" yaml:"tls,omitempty" label:"allowEmpty" file:"allowEmpty" kv:"allowEmpty" export:"true"`
}

But since a plugin's config is deserialized as a PluginConf (an untyped map[string]interface{}) (https://github.com/traefik/traefik/blob/v2.9.6/pkg/config/dynamic/plugins.go#L8), paerser don't have any "hints" as to what type it's supposed to be, and for empty maps falls back to an empty string.

type Configuration struct {
    Key map[string]interface{} `json:"key" toml:"key" yaml:"key"`
}

const yamlContent = `key:
  emptyMap: {}
`

func TestYaml(t *testing.T) {
    configuration := &Configuration{}

    err := file.DecodeContent(yamlContent, ".yaml", configuration)
    if err != nil {
        t.Fatal(err)
    }

    assertEmptyMap(t, configuration)
}

func assertEmptyMap(t *testing.T, configuration *Configuration) {
    t.Helper()

    value := configuration.Key["emptyMap"]
    assert.IsType(t, "", value) // PASS
    assert.IsType(t, make(map[string]interface{}), value) // FAIL
}

So, I think this only affects deserialization of map[string]interface{} types.

traefik / paerser