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

Multiple sources and string interpolation for targets in replacements. #5569

Open
2 tasks done
sinetris opened this issue Mar 13, 2024 · 2 comments
Open
2 tasks done
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/under-consideration

Comments

@sinetris
Copy link

sinetris commented Mar 13, 2024

Eschewed features

  • This issue is not requesting templating, unstuctured edits, build-time side-effects from args or env vars, or any other eschewed feature.

What would you like to have added?

We should be able to compose strings from multiple sources using string interpolation and apply the result to multiple targets.

An example of what we might want the interface to look like:

replacements:
- sources:
    # We could use a map or a list (choosing whichever is easier to implement).
    # In this example I chose a map, so that we can keep `source` as is 
    # and not have to choose a name for the field.
    # We will use these keys in the interpolation string in the targets.
    db_service_name:
      # Same format used for `source`
      kind: Service
      name: my-database
    db_service_namespace:
      kind: Service
      name: my-database
      fieldPath: metadata.namespace
    db_service_port:
      kind: Service
      name: my-database
      fieldPath: spec.ports.[name=db-port].port
    app1_db_user:
      kind: ConfigMap
      name: app1
      fieldPath: data.DB_USER
    app2_db_user:
      kind: ConfigMap
      name: app2
      fieldPath: data.DB_USER
    app1_name:
      kind: ConfigMap
      name: app1
    app2_name:
      kind: ConfigMap
      name: app2
  targets:
    - select:
        kind: ConfigMap
        name: app1
      fieldPaths:
        - data.DB_URL
      # In this example I am using '{{ <sources_map_key> }}' in a 'pattern' field for string interpolation,
      # but whatever is chosen is fine with me.
      pattern: "database://{{ app1_db_user }}@{{ db_service_name }}.{{ db_service_namespace }}:{{ db_service_port }}?application_name={{ app1_name }}"
      options:
        create: true
    - select:
        kind: ConfigMap
        name: app2
      fieldPaths:
        - data.DB_CONMNECTION_STRING
      pattern: "application_name={{ app2_name }} user={{ app2_db_user }} port={{ db_service_port }} host={{ db_service_name }}.{{ db_service_namespace }}"
      options:
        create: true

A complete example can be found in the gist https://gist.github.com/sinetris/5b2c044be29969de3268c58e38882461.

Clone the gist and follow the instructions in the README.md to better understand a potential use of this feature.

In short:

git clone https://gist.github.com/5b2c044be29969de3268c58e38882461.git replacements-enhanced
cd replacements-enhanced
./replacements-enhancement-example

Why is this needed?

The need to create strings composed of multiple sources is not currently covered by the replacements specification, and is rather complicated to achieve using pre/post processing.
In the past, we would use vars to achieve such a result, but vars has been deprecated (for very good reasons).
The use of plugins (currently in alpha stage) should mitigate this shortcoming, but it is likely that many people have the same needs and we would end up with a multitude of similar plugins, wasting effort and adding complexity.

Can you accomplish the motivating task without this feature, and if so, how?

As stated in "Why is this needed?", we could use:

  • vars (deprecated)
  • plugins (still in alpha, increased complexity, likelihood of multiple very similar plugins)
  • pre/post processing (would be more of a workaround than a solution)

I considered "abusing" the delimiter field in replacements, but in many cases it would not have covered the requirements.

Another option, the one I currently use, is to use patches with hardcoded values.

Taking the multibases example as a starting point.
Having a database service and the need to create a DATABASE_URL in the format database://<database-service-name>.<database-service-namespace>:<database-service-port>/<database-name>, I would add patch files for each overlay with the value of DATABASE_URL already set.
This "solution" is very repetitive, not-flexible, and error-prone; hence the proposal to extend the current replacements specs.

What other solutions have you considered?

None worth mentioning, but I'm open to suggestions.

Anything else we should know?

I am not advocating the inclusion of a complete templating system (in fact, I would be against it), my requirements are simple string interpolation.

It is still possible that people may abuse this feature, and if it will be accepted, it's best to add examples on when to use other features (for example, it may be best to use delimiter when working on paths).

People will ask for it to be a full templating system, but templating is a complex topic (especially from a security perspective) and it would be better to keep it in plugins (perhaps adding an example of a templating plugin is an idea to consider).

Feature ownership

  • I am interested in contributing this feature myself! 🎉
@sinetris sinetris added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 13, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 13, 2024
@koba1t
Copy link
Member

koba1t commented Mar 13, 2024

Hi @sinetris

I feel this feature is related to templating, which is executing before editing resources.

pattern: "database://{{ app1_db_user }}@{{ db_service_name }}.{{ db_service_namespace }}:{{ db_service_port }}?application_name={{ app1_name }}"

And I think we can't introduce this feature for replacement without breaking the current format.

It is still possible that people may abuse this feature,

I believe some people do if they can. I think it's the only way is to force disabled in the codebase if we want them not to use unwanted behavior.

/triage under-consideration

@k8s-ci-robot k8s-ci-robot added triage/under-consideration and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 13, 2024
@sinetris
Copy link
Author

sinetris commented Mar 14, 2024

Hi @koba1t

I feel this feature is related to templating, which is executing before editing resources.

pattern: "database://{{ app1_db_user }}@{{ db_service_name }}.{{ db_service_namespace }}:{{ db_service_port }}?application_name={{ app1_name }}"

It is related to templating, but it is more string concatenation than templating (no transformations, no loops, etc.), and it's not about changing existing unstructured data in a target, but creating a new string from the collected sources and using the generated string as value in the targets.

Essentially: instead of using the source value in the targets, we build a new string from multiple sources and use that in the targets.

It's the best compromise I found to cover a missing feature that has been requested too many times and by many people.

And I think we can't introduce this feature for replacement without breaking the current format.

This feature should be compatible with existing code, and I wrote some json-schema and related tests for it in https://gist.github.com/sinetris/5b2c044be29969de3268c58e38882461#file-json-schema-replacements-enhanced-json

It is still possible that people may abuse this feature,

I believe some people do if they can. I think it's the only way is to force disabled in the codebase if we want them not to use unwanted behavior.

I am always against adding new features whenever possible, because it is easy to add them but very difficult to remove them (people would complain a lot).
Since it is mostly needed for pod configuration, I was thinking of using init containers instead.
There are still some annotations that might need it, but for most of them the use of delimiter in replacements should be sufficient.
Sure, this feature would make life easier, but if there are other ways to do it using existing features and if we document them, there is no need to add it.

I will keep this issue open so we can discuss possible alternatives or ways to limit its use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/under-consideration
Projects
None yet
Development

No branches or pull requests

3 participants