feat(agent): add pod mutation webhook to inject agent #991
Conversation
ebaron
changed the title
|
I added a config sample to do |
|
/build_test |
| prefixWithKind := strings.ToLower(gvk.Kind) | ||
| if short { | ||
| // Prefix is 5 characters: 4 from kind, and hyphen | ||
| prefixWithKind = prefixWithKind[:4] // TODO can we use a weaker and shorter hash and keep the full kind? |
There was a problem hiding this comment.
@andrewazores what do you think about using a non-cryptographic hash function for this instead?
With https://pkg.go.dev/hash/fnv, we could use:
cryostat-<128-bit hash> instead of cryo-<224-bit hash>
There was a problem hiding this comment.
Do you mean just swapping out the hash functions, or would this also include changing the prefix before the hash or changing the hash inputs?
|
|
| @@ -138,6 +141,70 @@ func (r *Reconciler) reconcileAgentService(ctx context.Context, cr *model.Cryost | |||
| }) | |||
| } | |||
|
|
|||
| func (r *Reconciler) newAgentHeadlessService(cr *model.CryostatInstance, namespace string) *corev1.Service { | |||
| return &corev1.Service{ | |||
There was a problem hiding this comment.
If I understand correctly, this Service is made headless by having a ClusterIP: nil in its spec. I guess here that is done implicitly, but would it be worth setting it explicitly for readability?
| existing.Value += " " + agentArg | ||
| } else { | ||
| envs = append(envs, corev1.EnvVar{ | ||
| Name: "JAVA_TOOL_OPTIONS", |
There was a problem hiding this comment.
Maybe this could be another label-based extension point? Just in case the application is already setting this to some other value - ex in its Deployment, for the purpose of launching another Agent. The user might have another environment variable they can use for a similar purpose.
There was a problem hiding this comment.
As I continue thinking on this I can't really come up with a scenario where this won't work. Maybe if the application has some strange entrypoint script and bootup logic that would cause this environment variable setting to get overridden on that side?
| return nil, nil | ||
| } | ||
|
|
||
| func isTargetNamespace(targetNamespaces []string, podNamespace string) bool { |
There was a problem hiding this comment.
Maybe this can be replaced with https://pkg.go.dev/slices#Contains ?
|
Other than the minor comments above this seems to be working quite well. Exciting to see! |
Welcome to Cryostat! 👋
Before contributing, make sure you have:
mainbranch[chore, ci, docs, feat, fix, test]git commit -S -m "YOUR_COMMIT_MESSAGE"Fixes: #970
Description of the change:
Motivation for the change:
How to manually test:
make deploymake create_cryostat_crmake sample_appJAVA_OPTS_APPENDto not inject the built-in Cryostat agent.I've filed issues for the TODOs in this PR. There are a few of them, but they're all pretty small compared to this PR:
#992, #993, #994, #995, #996