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

Adding sidecar configuration versioning and inheritance #36

Merged
merged 9 commits into from
Nov 5, 2019

Conversation

byxorna
Copy link
Contributor

@byxorna byxorna commented Nov 4, 2019

What and why?

We add 2 features to the sidecar injector to make use at scale more ergonomic. 1) versioned configurations and 2) inheritance.

Versioning

We found ourselves suffixing the names of sidecars with versioning information, so we could make improvements to sidecars without breaking legacy consumers of a given sidecar. For example, we would name configs name: some-sidecar-v3. To codify this behavior without changing the request interface, we add support for versioned sidecar configurations, similar to how Docker images work.

By changing nothing in your configs, each sidecar configuration now has a Version() which is derived by splitting off the last field after :. By default, this is latest. Here are some examples of name: fields, and the derived version.

  • my-sidecar -> latest
  • my-sidecar:latest -> latest
  • my-sidecar:v420 -> v420
  • my-sidecar:extra:fields:6.9 -> 6.9

This change allows you to maintain versioned configs, as well as "symlink" consumers to the latest version of a sidecar, via :latest.

Inheritance

In addition to versioning, we have found that a lot of our sidecar configurations are actually quite similar, and tend to have only a few differences. This brought us to build inheritance functionality into the configs. By introducing an inherits: field to configs loaded from disk (not ConfigMaps!), we support a config using a base, and merging in any fields from the child as necessary.

Note: some (most) fields in the config format are sets, so we must perform merging in a manner that provides set uniqueness by name field.

For example, a config like the following would load another.yaml, and then add 2 env vars to it. If EXISTING_VAR exists in another.yaml, it will be replaced. NEW_VAR, if not appearing in the env list, will be appended. All configs referenced via inherits: are relative to the directory of the parent file, and cannot traverse upwards in the directory.

name: example:v1
inherits: another.yaml
env:
- name: EXISTING_VAR
  value: overridden
- name: NEW_VAR
  value: new

Testing Steps

  • Added unit tests for this feature (make test)

Reviewers

Required reviewers: @byxorna
Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

⚠️ this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

@byxorna byxorna added the RFR label Nov 4, 2019
@byxorna byxorna requested review from komapa and defect November 4, 2019 21:45
Copy link
Contributor

@komapa komapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great unit tests and this makes a lot of sense to me. One concern around the use of more than one : which I left inline.

internal/pkg/config/config.go Show resolved Hide resolved
@@ -381,13 +383,13 @@ func updateAnnotations(target map[string]string, added map[string]string) (patch
target = map[string]string{}
patch = append(patch, patchOperation{
Op: "add",
Path: "/metadata/annotations/" + keyEscaped,
Path: path.Join("/metadata/annotations/", keyEscaped),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

internal/pkg/config/config_test.go Outdated Show resolved Hide resolved
@byxorna byxorna merged commit d905c09 into master Nov 5, 2019
@komapa komapa deleted the gabe/inheritance-and-versioning branch November 6, 2019 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants