deggja / netfetch

Kubernetes tool for scanning clusters for network policies and identifying unprotected workloads.
MIT License
401 stars 25 forks source link

feat: added tests #128

Closed roopeshsn closed 7 months ago

roopeshsn commented 9 months ago

Added tests for the below functions in

visualizer.go:

scanner.go:

cilium-scanner.go:

Fixes #46

roopeshsn commented 9 months ago

The problem in the test function is that, the type of fake clientset (fake.NewSimpleClientset()) and actual clientset (returned by GetClientset()) are not same.

# github.com/deggja/netfetch/backend/pkg/k8s [github.com/deggja/netfetch/backend/pkg/k8s.test]
pkg/k8s/visualizer_test.go:17:14: cannot use fake.NewSimpleClientset() (value of type *"k8s.io/client-go/kubernetes/fake".Clientset) as kubernetes.Clientset value in assignment
?       github.com/deggja/netfetch/backend      [no test files]
?       github.com/deggja/netfetch/backend/cmd  [no test files]
FAIL    github.com/deggja/netfetch/backend/pkg/k8s [build failed]
?       github.com/deggja/netfetch/backend/statik       [no test files]
deggja commented 8 months ago

@roopeshsn I added some proposed changes to your PR. That should solve the issue regarding the fake clientset. Also took the liberty to look at some other small things.

roopeshsn commented 8 months ago

@roopeshsn I added some proposed changes to your PR. That should solve the issue regarding the fake clientset. Also took the liberty to look at some other small things.

Cool! I'll proceed with writing a test for getNetworkPolicyYAML.

roopeshsn commented 8 months ago

Need your review for the test, TestGetNetworkPolicyYAML @deggja

deggja commented 8 months ago

Need your review for the test, TestGetNetworkPolicyYAML @deggja

Will review this during the weekend.

deggja commented 8 months ago

I added a comment to your PR @roopeshsn

I'm just trying to understand why you want to use that approach as I dont know much about writing tests in Go.

Will a simplified test comparing string to string not work sufficiently?

e.g.

func TestGetNetworkPolicyYAML(t *testing.T) {
    // Setup fake clientset with a test policy
    var clientset kubernetes.Interface = fake.NewSimpleClientset()
    _, err := clientset.NetworkingV1().NetworkPolicies("test-namespace").Create(context.TODO(), &netv1.NetworkPolicy{
        ObjectMeta: metav1.ObjectMeta{
            Name:      "test-policy",
            Namespace: "test-namespace",
        },
        Spec: netv1.NetworkPolicySpec{
            PodSelector: metav1.LabelSelector{
                MatchLabels: map[string]string{"app": "test"},
            },
        },
    }, metav1.CreateOptions{})
    if err != nil {
        t.Fatalf("Failed to create test network policy: %v", err)
    }

    // Call the function under test
    YAML, err := getNetworkPolicyYAML(clientset, "test-namespace", "test-policy")
    if err != nil {
        t.Fatalf("Failed to get network policy YAML: %v", err)
    }

    // Sample expected YAML string
    expectedYAML := `apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: test-policy
  namespace: test-namespace
spec:
  podSelector:
    matchLabels:
      app: test
  policyTypes: []
`

    // Compare the actual YAML with the expected YAML
    if YAML != expectedYAML {
        t.Errorf("Expected YAML to be:\n%s\nGot:\n%s", expectedYAML, YAML)
    }
}
roopeshsn commented 8 months ago

I added a comment to your PR @roopeshsn

I'm just trying to understand why you want to use that approach as I dont know much about writing tests in Go.

Will a simplified test comparing string to string not work sufficiently?

e.g.

func TestGetNetworkPolicyYAML(t *testing.T) {
    // Setup fake clientset with a test policy
    var clientset kubernetes.Interface = fake.NewSimpleClientset()
    _, err := clientset.NetworkingV1().NetworkPolicies("test-namespace").Create(context.TODO(), &netv1.NetworkPolicy{
        ObjectMeta: metav1.ObjectMeta{
            Name:      "test-policy",
            Namespace: "test-namespace",
        },
        Spec: netv1.NetworkPolicySpec{
            PodSelector: metav1.LabelSelector{
                MatchLabels: map[string]string{"app": "test"},
            },
        },
    }, metav1.CreateOptions{})
    if err != nil {
        t.Fatalf("Failed to create test network policy: %v", err)
    }

    // Call the function under test
    YAML, err := getNetworkPolicyYAML(clientset, "test-namespace", "test-policy")
    if err != nil {
        t.Fatalf("Failed to get network policy YAML: %v", err)
    }

    // Sample expected YAML string
    expectedYAML := `apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: test-policy
  namespace: test-namespace
spec:
  podSelector:
    matchLabels:
      app: test
  policyTypes: []
`

    // Compare the actual YAML with the expected YAML
    if YAML != expectedYAML {
        t.Errorf("Expected YAML to be:\n%s\nGot:\n%s", expectedYAML, YAML)
    }
}

I tried this approach first. But the problem is that the string returned by the getNetworkPolicyYAML function has whitespaces in it, and it's not easy to compare with the expected string. 

roopeshsn commented 7 months ago

Hi, @deggja! After taking a look at the ScanNetworkPolicies function, my thought is to not write a test case for it as it uses the utility functions, where there are test cases in place for it. So if those test cases passed we can say the ScanNetworkPolicies function is also good. Once it's been refactored we can think to write a test case if needed. For now, I think this PR is fine to get merged after your review. What do you think?

deggja commented 7 months ago

Hi, @deggja! After taking a look at the ScanNetworkPolicies function, my thought is to not write a test case for it as it uses the utility functions, where there are test cases in place for it. So if those test cases passed we can say the ScanNetworkPolicies function is also good. Once it's been refactored we can think to write a test case if needed. For now, I think this PR is fine to get merged after your review. What do you think?

I agree. Thanks for the contribution @roopeshsn.