Validate collisions of logmodules and OneAgents in the validation webhook #3801

gkrenn · 2024-09-17T16:04:31Z

Description

It is not possible to deploy the logmodule on the same node that the OneAgent.
To prevent misconfigurations we should add a validation to our validation webhook.

Jira

How can this be tested?

Deploy a logmodule dynakube
Deploy a CNFS dynakube => should fail

codecov-commenter · 2024-09-17T16:11:09Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.35802% with 7 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@da71084). Learn more about missing BASE report.
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/api/v1beta3/dynakube/validation/logmodule.go	75.00%	3 Missing and 2 partials ⚠️
pkg/api/v1beta1/dynakube/validation/oneagent.go	94.73%	0 Missing and 1 partial ⚠️
pkg/api/v1beta2/dynakube/validation/oneagent.go	94.73%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3801   +/-   ##
=======================================
  Coverage        ?   65.02%           
=======================================
  Files           ?      398           
  Lines           ?    21469           
  Branches        ?        0           
=======================================
  Hits            ?    13961           
  Misses          ?     6170           
  Partials        ?     1338

Flag	Coverage Δ
unittests	`65.02% <91.35%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/api/v1beta1/dynakube/validation/oneagent.go

0sewa0

other than @waodim 's comment, it works and looks "as ok as it can" considering the need to validate for each old version as well
IMO, we should rethink how we validate when multiple versions are used, as this can get out of hand quickly. WDYT?

My testing setup:

apiVersion: dynatrace.com/v1beta3
kind: DynaKube
metadata:
  name: zib50933zib50933zib50933zib50933zib50933
  namespace: dynatrace
spec:
  apiUrl: https://zib50933.dev.dynatracelabs.com/api
  tokens: zib50933
  dynatraceApiRequestThreshold: 15
  logModule:
    enabled: true
  templates:
    logModule:
      nodeSelector:
        dk-log: deploy
      imageRef:
        repository: us-central1-docker.pkg.dev/cloud-platform-207208/chmu/logmodule
        tag: latest

---
apiVersion: dynatrace.com/v1beta2
kind: DynaKube
metadata:
  name: zib
  namespace: dynatrace
spec:
  apiUrl: https://zib50933.dev.dynatracelabs.com/api
  tokens: zib50933
  enableIstio: true
  dynatraceApiRequestThreshold: 15

  oneAgent:
    cloudNativeFullStack:
      namespaceSelector:
        matchLabels:
          monitored/by: v2
      nodeSelector:
          dk-v2: deploy
      tolerations:
       - effect: NoSchedule
         key: node-role.kubernetes.io/master
         operator: Exists
       - effect: NoSchedule
         key: node-role.kubernetes.io/control-plane
         operator: Exists
---
apiVersion: dynatrace.com/v1beta1
kind: DynaKube
metadata:
  name: zib50933-old
  namespace: dynatrace
spec:
  apiUrl: https://zib50933.dev.dynatracelabs.com/api
  tokens: zib50933

  namespaceSelector:
     matchLabels:
       monitored/by: v1

  oneAgent:
   cloudNativeFullStack:
     nodeSelector:
      dk-v1: deploy
     tolerations:
       - effect: NoSchedule
         key: node-role.kubernetes.io/master
         operator: Exists
       - effect: NoSchedule
         key: node-role.kubernetes.io/control-plane
         operator: Exists

tried different order of application, and if no nodeSelector is specified, the DynaKube gets rejected as expected

waodim

code lgtm, and it also worked for me, but I agree with @0sewa0 that IF possible we should think about other ways of validation without having to touch every api version's validation when adding something.

chrismuellner · 2024-09-18T11:37:06Z

pkg/api/v1beta1/dynakube/validation/oneagent.go

-	errorNodeSelectorConflict = `The DynaKube's specification tries to specify a nodeSelector conflicts with an another Dynakube's nodeSelector, which is not supported.
-The conflicting Dynakube: %s
-`
+	errorNodeSelectorConflict = `The DynaKube specification attempts to deploy a %s, which conflicts with another DynaKube's %s. Only one Agent per node is supported.


this does not give the information anymore, that the nodeSelector is the problem.

That was kind of on purpose, because the nodeSelector is not really true. It also pops up if no nodeSelectors are specified and 2 dynakubes are colliding.

So I would rather state that the dynakubes are colliding and which type of agent is the problem. So WDYF, should we still change it back? 🤔

only way to fix it is by specifying a node selector, so that should be in the message somewhere

pkg/api/v1beta1/dynakube/validation/oneagent.go

pkg/api/v1beta3/dynakube/validation/logmodule.go

0sewa0 · 2024-09-20T08:31:28Z

pkg/api/v1beta3/dynakube/validation/logmodule.go

+	for _, item := range validDynakubes.Items {
+		if item.Name == dk.Name {
+			continue
+		}
+
+		if item.NeedsOneAgent() {
+			if hasConflictingMatchLabels(dk.LogModuleNodeSelector(), item.OneAgentNodeSelector()) {
+				log.Info("requested dynakube has conflicting LogModule nodeSelector", "name", dk.Name, "namespace", dk.Namespace)
+
+				return fmt.Sprintf(errorConflictingLogModule, item.Name)
+			}
+		}
+	}


here you could also collect all the conflicting DynaKubes 😅

just make sure to only list the conflicting dynakubes. if we output the full error for each dynakube we could easily hit the warning limit

We don't need that, as there can only be 1 conflicting dynakube.

I could be many, but it's not very realistic.

Example

apiVersion: dynatrace.com/v1beta2 kind: DynaKube metadata: name: cloudnative namespace: dynatrace spec: apiUrl: https://zib50933.dev.dynatracelabs.com/api tokens: zib50933 enableIstio: true dynatraceApiRequestThreshold: 15 oneAgent: cloudNativeFullStack: namespaceSelector: matchLabels: monitored/by: v2 nodeSelector: dk-v2: deploy tolerations: - effect: NoSchedule key: node-role.kubernetes.io/master operator: Exists - effect: NoSchedule key: node-role.kubernetes.io/control-plane operator: Exists --- apiVersion: dynatrace.com/v1beta1 kind: DynaKube metadata: name: cloudnative-old namespace: dynatrace spec: apiUrl: https://zib50933.dev.dynatracelabs.com/api tokens: zib50933 namespaceSelector: matchLabels: monitored/by: v1 oneAgent: cloudNativeFullStack: nodeSelector: dk-v1: deploy tolerations: - effect: NoSchedule key: node-role.kubernetes.io/master operator: Exists - effect: NoSchedule key: node-role.kubernetes.io/control-plane operator: Exists --- apiVersion: dynatrace.com/v1beta3 kind: DynaKube metadata: name: log-module namespace: dynatrace spec: apiUrl: https://zib50933.dev.dynatracelabs.com/api tokens: zib50933 dynatraceApiRequestThreshold: 15 logModule: enabled: true

😮 - That's good, an edge case I didn't think of, will adapt the code and add it as unit test. Thx

gkrenn added 4 commits September 17, 2024 16:07

Validate v1beta3

d9cd0a6

Validate v1beta2

04ecdb1

Validate v1beta1

a93fcb1

Update error message

2b0fb68

gkrenn added the logmodule label Sep 17, 2024

gkrenn requested a review from a team as a code owner September 17, 2024 16:04

gkrenn requested review from 0sewa0, chrismuellner, albertogdd, waodim, StefanHauth, aorcholski and luhi-DT September 17, 2024 16:04

waodim reviewed Sep 18, 2024

View reviewed changes

pkg/api/v1beta1/dynakube/validation/oneagent.go Outdated Show resolved Hide resolved

0sewa0 previously approved these changes Sep 18, 2024

View reviewed changes

Update error message according to review

294f938

gkrenn dismissed 0sewa0’s stale review via 294f938 September 18, 2024 11:09

gkrenn requested a review from waodim September 18, 2024 11:10

waodim previously approved these changes Sep 18, 2024

View reviewed changes

0sewa0 previously approved these changes Sep 18, 2024

View reviewed changes

Merge branch 'main' into feature/validate-logmodule

5e7b818

gkrenn enabled auto-merge (squash) September 18, 2024 11:38

chrismuellner requested changes Sep 18, 2024

View reviewed changes

Update error message according to review

45ec517

gkrenn dismissed stale reviews from waodim and 0sewa0 via 45ec517 September 18, 2024 12:07

Validate conflict for all dynakubes at once

a59873f

gkrenn requested review from chrismuellner and 0sewa0 September 19, 2024 12:55

gkrenn requested a review from waodim September 19, 2024 12:55

waodim reviewed Sep 20, 2024

View reviewed changes

pkg/api/v1beta3/dynakube/validation/logmodule.go Outdated Show resolved Hide resolved

0sewa0 reviewed Sep 20, 2024

View reviewed changes

Update error message according to review

7e2a45b

gkrenn requested review from 0sewa0 and waodim September 20, 2024 11:53

Validate conflict for all dynakubes at once

fef2520

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate collisions of logmodules and OneAgents in the validation webhook #3801

Validate collisions of logmodules and OneAgents in the validation webhook #3801

gkrenn commented Sep 17, 2024 •

edited

Loading

codecov-commenter commented Sep 17, 2024 •

edited

Loading

0sewa0 left a comment

waodim left a comment

chrismuellner Sep 18, 2024

gkrenn Sep 18, 2024

chrismuellner Sep 18, 2024

gkrenn Sep 18, 2024

0sewa0 Sep 20, 2024

chrismuellner Sep 20, 2024

gkrenn Sep 20, 2024

0sewa0 Sep 20, 2024

gkrenn Sep 20, 2024

gkrenn Sep 20, 2024

Validate collisions of logmodules and OneAgents in the validation webhook #3801

Are you sure you want to change the base?

Validate collisions of logmodules and OneAgents in the validation webhook #3801

Conversation

gkrenn commented Sep 17, 2024 • edited Loading

Description

How can this be tested?

codecov-commenter commented Sep 17, 2024 • edited Loading

Codecov Report

0sewa0 left a comment

Choose a reason for hiding this comment

waodim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkrenn commented Sep 17, 2024 •

edited

Loading

codecov-commenter commented Sep 17, 2024 •

edited

Loading