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.

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

[Issue - 10441] Improved k8s cluster discovery #5291

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Chichichkin
Copy link

πŸš€ 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

…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 <[email protected]>
Copy link
Contributor

github-actions bot commented Mar 6, 2025

Thank you for your submission. We really appreciate it. Before we can accept your contribution, we ask that you sign the Mondoo Contributor License Agreement. You can sign the CLA by adding a new comment to this pull request and pasting exactly the following text.


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


1 out of 2 committers have signed the CLA.
βœ… (imilchev)[https://github.com/imilchev]
❌ @Chichichkin
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@imilchev imilchev self-assigned this Mar 7, 2025
@imilchev
Copy link
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

Copy link
Contributor

Test Results

3β€ˆ478 tests  Β±0   3β€ˆ474 βœ… Β±0   1m 41s ⏱️ -10s
β€‡β€ˆ392 suites Β±0β€‚β€ƒβ€ƒβ€‡β€ˆβ€‡β€‡4 πŸ’€ Β±0 
β€‡β€ˆβ€‡30 files   Β±0β€‚β€ƒβ€ƒβ€‡β€ˆβ€‡β€‡0 ❌ Β±0 

Results for commit 865da99. ± Comparison against base commit bb2d4a9.

@imilchev
Copy link
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
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?

@@ -97,7 +97,7 @@ func TestManifestDiscovery(t *testing.T) {
}
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
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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants