-
Notifications
You must be signed in to change notification settings - Fork 25
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
Creating mutating webhook for daemonsets for auto-annotation #65
Conversation
@@ -15,6 +15,22 @@ const ( | |||
defaultCollectorConfigMapEntry = "cwagentconfig.json" | |||
) | |||
|
|||
// AnnotationConfig details the resources that have enabled | |||
// auto-annotation for each instrumentation type. | |||
type AnnotationConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just to create the necessary functions to test the webhook, this is subject to change
} | ||
|
||
// getAutoAnnotatedLang returns the list of languages to be auto-annotated for the given kubernetes workload (deployment, daemon-set, stateful-set) | ||
// TODO can this function be made generic for supporting all workloads instead of just daemon-set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to suggestions if there is a better way to handle this functionality
) | ||
|
||
// +kubebuilder:webhook:path=/mutate-v1-daemonset,mutating=true,failurePolicy=ignore,groups="apps",resources=daemonsets,verbs=create;update,versions=v1,name=mdaemonset.kb.io,sideEffects=none,admissionReviewVersions=v1 | ||
// +kubebuilder:rbac:groups="",resources=namespaces,verbs=list;watch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need to list/watch namespaces do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah well I think this is used to watch namespaces incase the resource is not created yet, I think the pod mutator does something similar - https://github.com/aws/amazon-cloudwatch-agent-operator/blob/main/internal/webhook/podmutation/webhookhandler.go#L66
} | ||
|
||
// we use the req.Namespace here because the pod might have not been created yet | ||
ns := corev1.Namespace{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont think we need ns
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ties in to the previous comment, upstream has this change incase the resource is not created yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Added some minor comments.
Description of changes:
Creating a new mutating webhook for daemonsets that will:
Testing
Testing while watching the default namespace
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.