Skip to content

[Issue - 10441] Improved k8s cluster discovery#5291

Merged
imilchev merged 3 commits intomainfrom
chichichkin/I10441
May 16, 2025
Merged

[Issue - 10441] Improved k8s cluster discovery#5291
imilchev merged 3 commits intomainfrom
chichichkin/I10441

Conversation

@Chichichkin
Copy link
Copy Markdown
Contributor

🚀 Overview

Addressing the issue 👉 #10441

🎯 User-facing change?

✅ The new way of building the platform IDs relies solely on the namespaces now.

⚠️ Risk Assessment: what can go wrong?

🟢 n/a

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 6, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@imilchev imilchev self-assigned this Mar 7, 2025
@imilchev
Copy link
Copy Markdown
Member

imilchev commented Mar 7, 2025

we need to hold off merging this PR. I believe this change conflicts with how we build platformids when scanning manifests. I need to take a look how to set this up such that it works for both manifests and clusters

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 10, 2025

Test Results

3 693 tests  ±0   3 689 ✅ ±0   2m 11s ⏱️ ±0s
  399 suites ±0       4 💤 ±0 
   30 files   ±0       0 ❌ ±0 

Results for commit d367408. ± Comparison against base commit e9f797a.

♻️ This comment has been updated with latest results.

@imilchev
Copy link
Copy Markdown
Member

imilchev commented Mar 10, 2025

This PR makes sure we have more stable platform IDs for k8s assets. With the previous implementation different platform IDs were generating for all workloads depending whether the scan would discover the cluster asset or not.

With this change the platform ids should always be the same:

When scanning a cluster: <-- this is different from the current implementation

  • cluster asset has it's own platform ID, which is based on the kube-system namespace UID //platformid.api.mondoo.app/runtime/k8s/uid/1bda2ae0-27fe-49ed-90fd-39d3bb8da955
  • workload IDs have platform IDs based on the parent namespace's UID //platformid.api.mondoo.app/runtime/k8s/uid/ee1df06e-55c1-4ce3-b0a7-820efed0417a/namespace/default/pods/name/nginx

When scanning manifests: <-- this has not changed

  • manifest asset has it's own platform ID based on the path hash for the manifest/directory //platformid.api.mondoo.app/runtime/k8s/uid/ed37af0ce97030c84a07fb9d3311841f5f180a77573c664da6fbe3d8dc481896
  • workload IDs have platform IDs based on the manifest/directory path //platformid.api.mondoo.app/runtime/k8s/uid/ed37af0ce97030c84a07fb9d3311841f5f180a77573c664da6fbe3d8dc481896/namespace/mondoo-operator/deployments/name/mondoo-operator-controller-manager

Copy link
Copy Markdown
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

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

I can currently only think of two one cases where this might not work:

  • The user has no privileges to access kube-system. Are we still able to get the uuid from the namespace list?

Did I read this correctly, that we do not discover the cluster when a namespace filter is set?

inv, err := resources.Discover(pluginRuntime, cnquery.Features{})
require.NoError(t, err)
require.Len(t, inv.Spec.Assets, 2)
require.Len(t, inv.Spec.Assets, 3)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the comments in this PR, the manifest part didn't change. Why did this increase?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we discover the namespaces now. Before we skipped over them which was actually a bug. We turned on namespace discovery for api scans, but not for manifests

@Chichichkin
Copy link
Copy Markdown
Contributor Author

I have read the Mondoo CLA Document and I hereby sign the CLA

@imilchev
Copy link
Copy Markdown
Member

@czunker we cannot do anything about the case when a user has no access to list cluster namespaces. The only solution is to explicitly state which namespaces you want to scan, e.g. cnspec scan k8s --namespaces default. I made sure we log a meaningful error message in those cases

Copy link
Copy Markdown
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @Chichichkin @imilchev

Aleksandr Chagochkin and others added 3 commits April 4, 2025 12:12
…Namespaces discovery instead. NewNamespacePlatformId also refactored in order to generate correct PlatformID. Some small changes to address redundancy in provider.go

Signed-off-by: Aleksandr Chagochkin <chagochkin@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
@imilchev imilchev force-pushed the chichichkin/I10441 branch from bc4b7ee to d367408 Compare April 4, 2025 09:13
@imilchev imilchev merged commit 1516123 into main May 16, 2025
18 checks passed
@imilchev imilchev deleted the chichichkin/I10441 branch May 16, 2025 09:01
@github-actions github-actions Bot locked and limited conversation to collaborators May 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants