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.

Sign up for GitHub

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

Jump to bottom

pod_mutator supports multiple injections; pod_mutator refactoring #386

Merged
merged 38 commits into from
Nov 30, 2021
Merged

pod_mutator supports multiple injections; pod_mutator refactoring #386

merged 38 commits into from
Nov 30, 2021

Conversation

mjgrzybek
Copy link
Contributor

@mjgrzybek mjgrzybek commented Nov 24, 2021 •
edited

DataIngest is another capability (alongside OneAgent) that can be injected to pods by webhook.
pod_mutator must be extended to support multiple injections.
Using this opportunity, some refactoring was done, too.

Changes

  • DataIngest is injected by default if OneAgent is injected
  • DataIngest injection can be explicitly disabled or enabled (so that it can be injected even if OneAgent is not) using annotation: data-ingest.dynatrace.com/inject=true|false
  • Annotation dynakube.dynatrace.com/injected now holds a lexical sorted, comma separated, list of injected capabilities intsead of "true"
  • pod_mutator.Handle() implementation simplified
  • init.sh supports both injections
  • new UT created: TestPodPartialInjection
  • existing UT aligned with changes

aorcholski and others added 30 commits November 3, 2021 09:16
@aorcholski @mjgrzybek
Pods enriched with additional dims for mint
a964e77 
- dt.kubernetes.workload.kind
- dt.kubernetes.workload.name

Co-authored-by: Michal Grzybek <michal.grzybek@outlook.com>
@aorcholski @mjgrzybek
Webhook has rigths to list
ba2b432 
- replicasets
- statefulsets
- daemonsets
- deployments
@mjgrzybek
webhook roles fixed
8351d7d
@aorcholski @mjgrzybek
review
d4d836b
@aorcholski @mjgrzybek
New client added to pod_mutator to Get() an object from a different n…
74651bf 
…amespace
@aorcholski @mjgrzybek
Handle.ctx used instead of TODO()
4f666df
@mjgrzybek
clusterrole adjusted to actual needs
f4fd780
@aorcholski @mjgrzybek
name MINT removed from sources
80c33d1
@aorcholski @mjgrzybek
Pods enriched with additional dims for mint
3471619 
- dt.kubernetes.workload.kind
- dt.kubernetes.workload.name

Co-authored-by: Michal Grzybek <michal.grzybek@outlook.com>
@mjgrzybek
mint injection can be disabled
f199ebb
@mjgrzybek
ARM builds diabled due to travisCI timeouts
97865c4
@mjgrzybek
mint injection can be disabled #2
b8625db
@mjgrzybek
mint injection can be disabled #3
bbc32d1
@mjgrzybek
data-ingest deployment is no longer oneagent dependent
91c1900
@mjgrzybek
injection-related annotations redefined to support different types of…
af99984 
… injection
@mjgrzybek
InjectionInfo added
062e72b
@mjgrzybek
InjectionInfo unit tests
8dd6c84
@mjgrzybek
Merge remote-tracking branch 'origin/master' into feature/pod-mutator…
2b1d63d 
…-allows-multiple-injections2
@mjgrzybek
linting
129a063
@mjgrzybek
Volmes and VolumeMounts added conditionally based on annotations
6a3b73f
@mjgrzybek
debug print
b39a61f
@mjgrzybek
check if no injection should be done
6c051f9
@mjgrzybek
endless loop for non-one-agent-injected pods fixed
07f27e7
@mjgrzybek
properties file format fixed
64c9f61
@mjgrzybek
Merge remote-tracking branch 'origin/master' into feature/pod-mutator…
2ffd420 
…-allows-multiple-injections2
@mjgrzybek
post merge fixes
9fb18a2
@mjgrzybek
FeatureEnableWebhookReinvocationPolicy for DataIngest; 'oneagent-conf…
e9a6132 
…ig' renamed to 'injection-config'
@mjgrzybek
pod_mutator refactoring in progress
53e3dd8
@mjgrzybek
pod_mutator refactoring in progress #2
a8f78da
@mjgrzybek
pod_mutator refactoring in progress #3
de18ee6
@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch from 6cb08b0 to b960800 Compare November 24, 2021 16:31
@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch from b960800 to 22efbd6 Compare November 24, 2021 16:50
@mjgrzybek mjgrzybek changed the title pod_mutator allows multiple injections; pod_mutator refactoring pod_mutator supports multiple injections; pod_mutator refactoring Nov 25, 2021
@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch 2 times, most recently from 78b7c08 to 14d3253 Compare November 25, 2021 09:46
@mjgrzybek mjgrzybek marked this pull request as ready for review November 25, 2021 14:44
@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch from 14d3253 to 19df67c Compare November 25, 2021 19:33
@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch from 19df67c to 37c3dbb Compare November 26, 2021 05:34
@mjgrzybek mjgrzybek requested a review from toszr November 26, 2021 08:56
@0sewa0
Copy link
Contributor

0sewa0 commented Nov 26, 2021

Just a general question:
I see a lot of methods that don't need to be methods because they don't use the struct they are the methods of.
Is there a reason for it ? Because it would make testing these 'methods' easier (not having to create a dummy struct) if they were not methods to begin with. 😄
Or maybe storing more things in the struct that we have to pass into multiple methods would make these 'not-really-methods' make more sense (I kinda prefer the not-having-them-as-methods way but I'm not 100% decided on it 😅)
(I didn't want to litter the PR with pointing out each 'not-really-a-method')

@0sewa0
Copy link
Contributor

0sewa0 commented Nov 29, 2021

to get only data-ingest injected I have to create a dynakube that has some kind of application injection + data-ingest configured AND add the annotation on the pod to disable app injection ?

@mjgrzybek
Copy link
Contributor Author

Just a general question: I see a lot of methods that don't need to be methods because they don't use the struct they are the methods of. Is there a reason for it ? Because it would make testing these 'methods' easier (not having to create a dummy struct) if they were not methods to begin with. smile Or maybe storing more things in the struct that we have to pass into multiple methods would make these 'not-really-methods' make more sense (I kinda prefer the not-having-them-as-methods way but I'm not 100% decided on it sweat_smile) (I didn't want to litter the PR with pointing out each 'not-really-a-method')

Ah, working on it - forgot to fix IDE suggestions.

@mjgrzybek
Copy link
Contributor Author

to get only data-ingest injected I have to create a dynakube that has some kind of application injection + data-ingest configured AND add the annotation on the pod to disable app injection ?

dynakube should have applicationMonitoring or cloudNativeFullStack enabled. No need to mention data-ingest there.
AND
pods should be annotated:

annotations:
        oneagent.dynatrace.com/inject: "false"
        data-ingest.dynatrace.com/inject: "true"

@mjgrzybek mjgrzybek force-pushed the feature/pod-mutator-allows-multiple-injections2 branch from 2124766 to 2785e04 Compare November 29, 2021 12:14
chrismuellner
chrismuellner requested changes Nov 30, 2021
View reviewed changes
initgeneration/init.sh.test-sample Outdated Show resolved Hide resolved
initgeneration/init.sh.test-sample Outdated Show resolved Hide resolved
webhook/mutation/helper.go Outdated Show resolved Hide resolved
webhook/mutation/pod_mutator.go Outdated Show resolved Hide resolved
webhook/mutation/pod_mutator.go Outdated Show resolved Hide resolved
webhook/mutation/pod_mutator.go Show resolved Hide resolved
webhook/mutation/pod_mutator.go Outdated Show resolved Hide resolved
webhook/mutation/pod_mutator.go Outdated Show resolved Hide resolved
@mjgrzybek mjgrzybek merged commit 50533e4 into master Nov 30, 2021
@chrismuellner chrismuellner deleted the feature/pod-mutator-allows-multiple-injections2 branch February 2, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0sewa0 0sewa0 0sewa0 approved these changes

@chrismuellner chrismuellner chrismuellner approved these changes

@aorcholski aorcholski Awaiting requested review from aorcholski

@gkrenn gkrenn Awaiting requested review from gkrenn

@DTMad DTMad Awaiting requested review from DTMad

@toszr toszr Awaiting requested review from toszr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mjgrzybek @0sewa0 @chrismuellner @aorcholski