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

Convert CLI inject proxy options to annotations #2547

Merged
merged 7 commits into from
Mar 26, 2019

Conversation

ihcsim
Copy link
Contributor

@ihcsim ihcsim commented Mar 23, 2019

The PR introduces code to convert CLI inject proxy options into config annotations. The annotations ensure that these configs are persisted in subsequent resource updates.

Fixes #2447

Signed-off-by: Ivan Sim [email protected]

@ihcsim ihcsim self-assigned this Mar 23, 2019
@ihcsim ihcsim requested review from alpeb and klingerf March 23, 2019 03:43
@ihcsim ihcsim added priority/P0 Release Blocker area/inject labels Mar 23, 2019
}

if options.dockerRegistry != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to check for image name even if the registry option isn't provided.

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Reusing resourceTransformerInject is neat 🎈

I did find a use case that might need to be addressed.
Say we have a POD annotated with config.linkerd.io/proxy-memory-limit: 10Mi
Then we inject it with inject --proxy-memory-limit 20Mi.
The annotation will get updated correctly, but the memory limit in the sidecar will get set to the old 10Mi, because getOverride() only looks at the annotations from parsing the payload, disregarding the map of overriding annotations that cli/cmd/inject.go created.
First thing that comes to mind to address this would be to augment ResourceConfig to include that map (that cli/cmd/inject.go would pass when calling NewResourceConfig), for getOverride to take into account.

transformer := &resourceTransformerInject{
configs: configs,
overrideAnnotations: overrideAnnotations,
}
Copy link
Member

Choose a reason for hiding this comment

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

How about defining this earlier, and have overridedConfigs() receive it as its sole argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep the arguments of overrideConfigs() to config data. A function that reads overrideConfigs(someKindOfTransformer) sounds like we are overriding the transformer's config, which it isn't because the transformer has no configs of its own, and it's more a YAML-parsing receiver, than a config receiver.

Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

Nice, this looks like the right approach to me, and 👍 to @alpeb's feedback about constructing the overrides.

cli/cmd/inject.go Show resolved Hide resolved
@ihcsim
Copy link
Contributor Author

ihcsim commented Mar 25, 2019

I did find a use case that might need to be addressed.
Say we have a POD annotated with config.linkerd.io/proxy-memory-limit: 10Mi
Then we inject it with inject --proxy-memory-limit 20Mi.
The annotation will get updated correctly, but the memory limit in the sidecar will get set to the old 10Mi, because getOverride() only looks at the annotations from parsing the payload, disregarding the map of overriding annotations that cli/cmd/inject.go created.
First thing that comes to mind to address this would be to augment ResourceConfig to include that map (that cli/cmd/inject.go would pass when calling NewResourceConfig), for getOverride to take into account.

@alpeb My thought is that the payload that is parsed by getOverride() will contain the correct annotations. It looks like the flag-to-annotation translation needs to happen sooner, before calling GetPatch().

@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch from 463085a to 804225c Compare March 26, 2019 02:35
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 463085a: success 🎉
Log output: https://gist.github.com/06e6cba9974f7b450e5fd492a2f5f0a6

@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch from 804225c to 5822eea Compare March 26, 2019 02:42
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 804225c: success 🎉
Log output: https://gist.github.com/22da0799d9d55e487b23e228ff51bf79

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 5822eea: success 🎉
Log output: https://gist.github.com/084ae6de0b071e2fed391f7a5f4956c2

@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch from 57b7781 to 49e67c0 Compare March 26, 2019 03:10
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 0b7c992: success 🎉
Log output: https://gist.github.com/632870a2ae636c31d9b79493288403c2

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 57b7781: fail 👎
Log output: https://gist.github.com/bf8ecc68d6e97f3280ca1e30cd19dd6d

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 49e67c0: success 🎉
Log output: https://gist.github.com/20738729e0668a2155b5dc1f502d5662

@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch from 49e67c0 to 4e80636 Compare March 26, 2019 04:28
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 4e80636: success 🎉
Log output: https://gist.github.com/64dcdf9ea77ceb1c1bbb2881a380af27

@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch 3 times, most recently from 8e7a7d2 to ed35d24 Compare March 26, 2019 06:02
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for afeec47: success 🎉
Log output: https://gist.github.com/c8f9a18b35ccb8751cbc9c2c32213e95

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 8e7a7d2: fail 👎
Log output: https://gist.github.com/21f19172926789c41b8d29d9a751a9c8

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for ed35d24: success 🎉
Log output: https://gist.github.com/d7ccd25356b20d22600b8d87ee2a0881

@ihcsim
Copy link
Contributor Author

ihcsim commented Mar 26, 2019

@klingerf @alpeb I've updated the code to support the scenario @alpeb described above. Please take a look. Thanks.

Ivan Sim added 4 commits March 26, 2019 10:01
The override logic depends on this option to assign different profile suffix.

Signed-off-by: Ivan Sim <[email protected]>
…atch

This ensures that any configs provided via the CLI options are persisted
as annotations before the configs override.

Signed-off-by: Ivan Sim <[email protected]>
@ihcsim ihcsim force-pushed the isim/cli-proxy-configs-override branch from ed35d24 to ec01a9e Compare March 26, 2019 17:10
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for ec01a9e: fail 👎
Log output: https://gist.github.com/eff4730cf0e0305f73f24ae409806add

Signed-off-by: Ivan Sim <[email protected]>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 4b09829: success 🎉
Log output: https://gist.github.com/53354b7343e0fd653b5e8b51d7b4d1a9

Ivan Sim added 2 commits March 26, 2019 13:56
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Thanks for making those updates. Looks good to me.

cli/cmd/inject.go Show resolved Hide resolved
@@ -141,6 +143,13 @@ func (conf *ResourceConfig) WithOwnerRetriever(f OwnerRetrieverFunc) *ResourceCo
return conf
}

// AppendPodAnnotations appends the given annotations to the pod spec in conf
func (conf *ResourceConfig) AppendPodAnnotations(annotations map[string]string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIOLI I'd prefer to call this AddPodAnnotations, since I don't typically think of appending to a map.

@ihcsim ihcsim merged commit 9c5bb4e into master Mar 26, 2019
@ihcsim ihcsim deleted the isim/cli-proxy-configs-override branch March 26, 2019 21:21
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 4c375e2: fail 👎
Log output: https://gist.github.com/7de0b2b566a736e3d544ae99726a46d7

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.

5 participants