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

feat: custom sidecar config #4280

Merged
merged 19 commits into from
May 20, 2022
Merged

Conversation

parkanzky
Copy link
Contributor

Summary

Add CRD for applying patches to sidecar and init containers.

Two small deviations from the proposal -

  1. Only one global config rather than one under each of sidecar and init. This is because the CRD already covers both.
  2. Just validates that the CRD can be use to generate a jsonpatch rather than validating that a patched pod is valid. This is because users can chain patches together, and all that really matters is that the final result is valid, not any intermediate step. If they end up with an invalid config, they will get a useful error message from k8s.

Making this a draft until I can get the injector tests working locally.

Full changelog

  • Add CRD to patch injected sidecar and init containers

Issues resolved

Fix #4273

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Update UPGRADE.md with any steps users will need to take when upgrading.
  • Add backport-to-stable label if the code follows our backporting policy

Paul Parkanzky added 9 commits May 11, 2022 09:58
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
}
patchStr += `}]`

return jsonpatch.DecodePatch([]byte(patchStr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Patch is a []Operation and Operation is a map[string]*json.RawMessage, RawMessage is a []byte.

So to me you can rebuild this all more easily with a

func ToJsonPatch(in []patchBlock) (jsonpatch.Patch) {
res := []jsonPath.Operation{}
for _, o := range in {
   res = append(res, map[string]*json.RawMessage{
       "op": (&o.op).(*json.RawMessage),
       ....
   })
}
return res
}

Seems like you'd solve potential injection problems and also a json deserialization cost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, also building raw json by appending strings is pretty ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed, not exactly like in the comment. Please take a look

pkg/plugins/runtime/k8s/webhooks/injector/injector.go Outdated Show resolved Hide resolved
pkg/plugins/runtime/k8s/webhooks/injector/injector.go Outdated Show resolved Hide resolved
Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
@lahabana
Copy link
Contributor

BTW I don't see the default in the CP no problem for adding it in a follow up PR but can you open an issue and add it to the project so we don't forget about it :)

Signed-off-by: Paul Parkanzky <paul.parkanzky@konghq.com>
@jakubdyszkiewicz
Copy link
Contributor

BTW I don't see the default in the CP no problem for adding it in a follow up PR but can you open an issue and add it to the project so we don't forget about it :)

Why do we need a default?

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
…-config

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #4280 (adfae5c) into master (bb4e42b) will decrease coverage by 0.09%.
The diff coverage is 37.16%.

@@            Coverage Diff             @@
##           master    #4280      +/-   ##
==========================================
- Coverage   55.75%   55.65%   -0.10%     
==========================================
  Files         935      937       +2     
  Lines       56434    56569     +135     
==========================================
+ Hits        31462    31483      +21     
- Misses      22498    22599     +101     
- Partials     2474     2487      +13     
Impacted Files Coverage Δ
...s/k8s/native/api/v1alpha1/zz_generated.deepcopy.go 35.65% <0.00%> (-1.63%) ⬇️
pkg/plugins/runtime/k8s/metadata/annotations.go 100.00% <ø> (ø)
pkg/plugins/runtime/k8s/plugin.go 1.04% <0.00%> (-0.03%) ⬇️
...s/runtime/k8s/webhooks/containerpatch_validator.go 0.00% <0.00%> (ø)
pkg/plugins/runtime/k8s/webhooks/pod_mutator.go 0.00% <0.00%> (ø)
...est/framework/deployments/testserver/deployment.go 0.00% <0.00%> (ø)
...est/framework/deployments/testserver/kubernetes.go 0.00% <0.00%> (ø)
.../plugins/runtime/k8s/webhooks/injector/injector.go 68.77% <70.37%> (-0.28%) ⬇️
pkg/config/plugins/runtime/k8s/config.go 86.54% <100.00%> (+0.07%) ⬆️
...sources/k8s/native/api/v1alpha1/container_patch.go 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb4e42b...adfae5c. Read the comment docs.

lukidzi added 2 commits May 19, 2022 19:23
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lukidzi lukidzi marked this pull request as ready for review May 20, 2022 01:56
@lukidzi lukidzi requested a review from a team as a code owner May 20, 2022 01:56
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

That's getting in a good shape! One question re:selecting a patch that doesn't exist and should we add a webhook to validate that patches are only installed in the namespace of the CP?

@jakubdyszkiewicz
Copy link
Contributor

That's getting in a good shape! One question re:selecting a patch that doesn't exist and should we add a webhook to validate that patches are only installed in the namespace of the CP?

yes we should, let me do it

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@bartsmykla bartsmykla changed the title Feat/custom sidecar config feat: custom sidecar config May 20, 2022
jakubdyszkiewicz and others added 2 commits May 20, 2022 12:40
@jakubdyszkiewicz jakubdyszkiewicz merged commit cbf7881 into master May 20, 2022
@jakubdyszkiewicz jakubdyszkiewicz deleted the feat/custom-sidecar-config branch May 20, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement custom sidecar and init container patch CRD.
5 participants