feat: support global config for agent types#2436
feat: support global config for agent types#2436danielorihuela wants to merge 6 commits intofeat/support-oci-auth-in-agent-typesfrom
Conversation
35c52c3 to
2766a4c
Compare
2766a4c to
8fab117
Compare
8fab117 to
e348494
Compare
acc0ccf to
bd3c3e4
Compare
| default: "${nr-default:oci.registry}" | ||
| variants: | ||
| ac_config_field: "oci_registry_urls" | ||
| values: [ "docker.io" ] |
There was a problem hiding this comment.
As discussed offline, the default should be consistent with the values in variants (if defined). We can sort it out with documentation but It would be nice to provide some tools to avoid easy failures. Eg:
default: "${nr-default:oci.registry}"
variants:
ac_config_field: "oci_registry_urls"
values: [ "${nr-default:oci.registry}" ]
We could also have sorted that out with a completely different tool (same as we already do with the default:
value: "default-hardcoded-in-agent-type" # Value used if the field pointed to 'ac_config_field' is not defined
ac_config_field: "oci_registry" # Gets the value from config in "defaults.oci_registry` if anyWe would avoid the problem of having two groups of namespaces and it may be less confusing for Agent Type authors. |
Not sure the default: "oci_registry"This is what I had in the first commit. But I didn't like have a "magic replaceable string" there. I like having something like default: "${nr-default:whatever}/more-stuff"The current implementation supports that, and I'm not sure we could with your approach. I'm not against it. Just mentioning this for whenever (if) we discuss the implications with the team. |
This PR adds support for global defaults in Agent Control, which allows setting "base" values for all agent types that will be instantiated in the host machine.
Notice that this feature works for k8s too, even though it's not needed at the moment.
Also, this PR is on top of #2414.
Context
In the refinement, it was decided that the user should be able to configure some default values "globally" (for all agent types) and that these should be overridable in an agent-per-agent basis.
Something along these lines (this is not the final shape of the config).
Essentially, we have two requirements:
We can promote using these as much as possible, but it's the responsibility of the agent type author to use them.
Basically, if a user sets the value of a variable, this takes precedence over the global default.
From a technical point of view, we have two additional requirement.
Review
I think the easier way to review this PR is to check all changes at once. If you want to go commit by commit, here you have the hight level summary. Read it to understand what each commit adds.
defaultfield of thevariablessectionTechnical decisions
Added
nr-defaultnamespaceI added a new namespace called
nr-defaultto handle the rendering of default values in thevariablessection. This was not strictly needed, and someone could argue that's even a bad decision. However, I think it helps with consistency. Let me explain this contradiction.We have several supported namespaces for agent types template rendering. We have
nr-env,nr-ac,nr-subandnr-var. All of them are rendered in thedeploymentsection of the agent type definition. This is the "runtime" part. Adding annr-defaultseems a good idea, until you realize that we are going to use this one in thevariablessection, which is supposed to be static. I don't think that's a big deal. Nowdefaultvalues will be dynamic.My main concern is that we now have two groups of namespaces. One of them work inside
variablesand one of them works insidedeployments.I decided to go down that path to reuse abstractions and keep some degree of consistency with our terminology. I acknowledge that it could be confusing for agent type authors. They might assume they can use
nr-defaultin thedeploymentsection. That's not the case, and I assume that we should solve that through documentation. We could use a different abstraction and have something different fromnr-default. Maybeglobal-default:whatever. However, I'm not sure this will be clearer and it requires extra effort to duplicate and generalize all the code or part of the code that we have for templating.We could also pass
nr-defaultvalues along with the other namespaces to template thedeploymentsection, but I didn't think it was useful. The idea is to have a global default that can be overridden. With the current implementation that separation is a feature, not a bug. It won't work because it's not supposed to work.Regarding other namespaces. I don't they they make sense to support in the
variablessection. I only added support fornr-env. In case an agent type author wants to retrieve the value from an env var by default, and give the user the possibility to change it. I'm not sure this is that useful. We can remove it.Open to feedback!!!
Add
DefaultValueNow that we can template default values in the
variablesection, we need a way to postpone type checking. Hence, I introduced this new structDefaultValuethat allows storing in the default field either a value or a template string.The type checking is postponed from agent type loading to default values templating. Without this, we can't have global default values with types other than string (e.g. bool). This is strictly not necessary for the current feature, but we might need it in the future. It's best to introduce that now.
Two-step rendering
The renderer is getting more complex. Originally, we had a "two-step rendering" process.
deploymentsection (second render step)The master piece in charge of templating is
template_string, which assumes that the received variables contain the final value. Thus, this second step rendering was needed. If thetemplate_stringwas able to detect that a template resulted into another template, we could try replacing them in a loop until we get a final value. This is a potential improvement refactor suggested by @DavSanchez.Now, we need to templatise the default values in the
variablessection. This requires a tweak.deploymentsection (third render step)Why is the extra step (4) needed?
Variables have a specific type (bool, yaml, etc), and the only way we can make sure that the template in the default field can be templatised into the expected type, is by doing it at the beginning. That way, when we fill the agent type variables, we can check that the type is as expected. Otherwise, we would always receive a template string, which is not always the expected type.
Backwards compatibility
The new global defaults are optional. We are not using rust
Optionaltype, but we if the fields are missing when deserializing the data, we will use default values. With that, old agent control configs without default values will still work.