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.

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

Support plugin configurations #4

Closed
traut opened this issue Dec 20, 2023 · 4 comments · Fixed by #14
Closed

Support plugin configurations #4

traut opened this issue Dec 20, 2023 · 4 comments · Fixed by #14
Labels
enhancement New feature or request
Milestone

Comments

@traut
Copy link
Member

traut commented Dec 20, 2023

Background

Some plugins require additional configuration, like credentials / API keys, etc, similar to Terraform's provider configuration.

Design

In the input *.fabric files, there can be configuration blocks that contain additional configuration properties for the plugins.

If the plugin exposes config schema with the required attributes, the config must be present. If it is only optional fields, the config block is not required.

Config blocks examples:

// named data configuration
config data elasticsearch "clusterA" {
    username = "john"
    password = "smith"
}

// named data configuration
config data elasticsearch "clusterB" {
    cloud_id = "x"
    api_key = "y"
}

// "default" configuration for `llm_text` content plugin
config content llm_text {
    parameter_a = "foo"
}

config content llm_text "llama2" {
    parameter_a = "bar"
}

// "default" configuration for `table` content plugin
config content table {
    parameter_b = "foo"
}
  • each config block must have 3 labels:

    • a block type (data or content)
    • a data plugin name (elasticsearch, for example)
    • a plugin config instance name (clusterA or none)
  • each content_config block must have 2 labels:

    • a content plugin name (table for example)
    • a plugin config instance name (llama2 or none)
  • both content or data blocks can have config attribute set or config block specified. The attribute must point to an existing config block (content_config or data_config correspondingly). For example:

    config data elasticsearch "clusterB" {
        username = "jimmy"
        password = "page"
    }
    
    config data llm_text "openai" {
        api_key = "test-key"
    }
    
    document "test-doc" {
    
        data elasticsearch "my-results" {
            config = config.data.elasticsearch.clusterB
            query = "event.type:alert"
        }
    
        content llm_text {
            config = config.content.llm_text.llama2
            prompt = "hey, LLM!"
        }
    
        data elasticsearch "other-results" {
            config {
                username = "john"
                password = "smith"
            }
        }
    }
  • if the config block is specified, it is applied only to the invocation of its parent block

  • the config is parsed according to the "configuration parameters" schema the plugin provides

  • a request to a plugin must support a config properties in addition to execution parameters

    • if config attribute is set in a block, it is resolved and the value is passed as a config object in a plugin request
    • if config attribute in a block is not set, a default config - a config object for data/content type without a name - is used
    • if config attribute in a block is not set and a default config object is not specified, nil is passed in a request
      For example, when this template is executed:
    config data elasticsearch {
        username = "john"
        password = "smith"
    }
    
    document "test-doc" {
        data elasticsearch "my-values" {
            index = "my-index"
            query = "entity.type:alert"
            size = 10
        }
    }

    a call to elasticsearch plugin has 2 inputs: config object (with username and password fields set) and execution parameters (with index, query and size set). The same applies to content blocks.

@traut traut added the enhancement New feature or request label Dec 21, 2023
@traut
Copy link
Member Author

traut commented Dec 22, 2023

I've adjusted the issue according to the discussion in https://gist.github.com/Andrew-Morozko/fec36c1dcbca62fc9fea500a64f198a9

one question there -- does hcl parser support the same name for an attribute and a block? Can it support both config attribute and config block in the data/content block? Maybe instead of allowing inline config block it is always an attribute that accepts either a reference or a dict

@Andrew-Morozko Andrew-Morozko mentioned this issue Dec 26, 2023
3 tasks
@traut traut added this to the v0.2 milestone Dec 30, 2023
This was referenced Jan 2, 2024
@Andrew-Morozko Andrew-Morozko modified the milestones: v0.2, v0.1 Jan 3, 2024
@Andrew-Morozko
Copy link
Contributor

Moved the milestone, I'll include it in parser rewrite

@Andrew-Morozko
Copy link
Contributor

Andrew-Morozko commented Jan 3, 2024

What do we do with shadowing global configs? Right now if two files contain clashing global config for the same plugin it would be considered an error. We can scope config blocks per file, but that would be the only thing with a per-file scope and make the fabric scoping rules more complicated.

However, some way of shadowing default configs may be useful. For example:

a.fabric:

config content plugin_a{
    param = 1
}

feels_bad.fabric:

config content plugin_a "cfg2"{
    param = 2
}

document "test-document" {
    content plugin_a {
        config = config.content.plugin_a.cfg2
        text = 1
    }
    content plugin_a {
        config = config.content.plugin_a.cfg2
        text = 2
    }
    content plugin_a {
        config = config.content.plugin_a.cfg2
        text = 3
    }
}

What if we allow scoped default config blocks?

feels_good.fabric:

document "test-document" {
    config content plugin_a{
        param = 2
    }
    content plugin_a {
        text = 1
    }
    content plugin_a {
        text = 2
    }
    content plugin_a {
        text = 3
    }
}

Pros:

  • Looks pretty, less repetition

Cons:

  • This makes determining at a glance which "default" config is applied to the current plugin call block harder (to be honest this is hard as-is since all files exist in one scope)
  • This complicates parsing, now default config is not a single block, but a parse-time-defined hierarchy
  • Ref block configs are a nightmare of unexpected results (does ref bring in global defaults or use the local? Neither is an obvious option)
  • I don't know how often the feels_good.fabric scenario may play out (multiple calls to the same plugin within the same scope requiring the same config that differs from the global default plugin config). Perhaps the (cost+complexity)/benefit ratio is too bad.

We can decide this later, I was just trying to determine valid block nestings and thought up a use case for the nested config blocks in the document/section.

@traut
Copy link
Member Author

traut commented Jan 3, 2024

I think it is better to separate the configuration of the plugins from the document as much as possible because it reduces the reusability of the template. We have 3 ways to define the plugin config right now:

  • anonymous config for a plugin
    config content plugin_a {
        param = 1
    }
    This configuration is the default configuration for plugin type/name, in the global scope. This block should be unique per plugin type / name
  • named config for a plugin
    config content plugin_a custom_config {
        param = 2
    }
    This approach covers the cases when the same plugin must be used with different configurations. This is intended as an addition to the default configuration but does not require it to be present.
  • inline configuration
    data elasticsearch "other-results" {
        config {
            username = "john"
            password = "smith"
        }
    
        index = "my-index"
        query = "entity.type:alert"
    }
    This approach covers the very ad-hoc cases, when the configuration must be specified for a specific invocation of the plugin. This is not advised, since it is decreases the maintainability and reusability of the code, but allowed.

I think this is already a bit over-engineered 😃 but there might be the need for config shadowing approach you described. It definitely looks cleaner but I would like to postpone adding something like that until it is really needed.

Let me try to walk through the cons you listed:

This makes determining at a glance which "default" config is applied to the current plugin call block harder (to be honest this is hard as-is since all files exist in one scope)

How so? There is one global config per plugin type / name that is used if no explicit config ref is set in config attribute or with config inline block. If there are more than one "default" config defined for content type / name - that's an error

This complicates parsing, now default config is not a single block, but a parse-time-defined hierarchy

I had this flow in my head, when we parse the template files:

  • all root level blocks are parsed, including plugin configs. The in-memory registry of configuration is created
  • during the execution of the data / content blocks, if the plugin requires the configuration, it must be send in the API call
    • if the config attribute set, use a referred config
    • if the inline config block specified, use that inline config
    • if none of the above, use the default config for the plugin

It's a bit of a mix of parsing and execution, I agree. It is easy to understand though, I think -- tell me if the code for it will be messy though

Ref block configs are a nightmare of unexpected results (does ref bring in global defaults or use the local? Neither is an obvious option)

do you mean using the config attribute in ref content / data blocks? Does it not work similarly to other attributes, where we overwrite the fields from the referred block with locally specified? I think the same rule applies -- if config attribute / block specified in the block that refers to some other, the local specified props take precedence.

I don't know how often the feels_good.fabric scenario may play out (multiple calls to the same plugin within the same scope requiring the same config that differs from the global default plugin config). Perhaps the (cost+complexity)/benefit ratio is too bad.

I wonder about that too. I think it might be a rare occurrence and one of the approaches defined above will be enough. Let's see how it will be used. We can always add more complexity later!

@Andrew-Morozko Andrew-Morozko linked a pull request Jan 24, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants