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

Validate Node Name #48

Merged
merged 4 commits into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,14 @@ spec:
spec:
clusterPermissions:
- rules:
- apiGroups:
- ""
resources:
- nodes
verbs:
- get
- list
- watch
- apiGroups:
- ""
resources:
Expand Down
8 changes: 8 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ metadata:
creationTimestamp: null
name: manager-role
rules:
- apiGroups:
- ""
resources:
- nodes
verbs:
- get
- list
- watch
- apiGroups:
- ""
resources:
Expand Down
15 changes: 11 additions & 4 deletions controllers/fenceagentsremediation_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func (r *FenceAgentsRemediationReconciler) SetupWithManager(mgr ctrl.Manager) er

//+kubebuilder:rbac:groups=core,resources=pods/exec,verbs=create
//+kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection
//+kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch
//+kubebuilder:rbac:groups=fence-agents-remediation.medik8s.io,resources=fenceagentsremediations,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=fence-agents-remediation.medik8s.io,resources=fenceagentsremediations/status,verbs=get;update;patch
//+kubebuilder:rbac:groups=fence-agents-remediation.medik8s.io,resources=fenceagentsremediations/finalizers,verbs=update
Expand Down Expand Up @@ -79,7 +80,16 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct
r.Log.Error(err, "failed to get FAR CR")
return emptyResult, err
}
// TODO: Validate FAR CR name to nodeName. Run isNodeNameValid
// Validate FAR CR name to match a nodeName from the cluster
r.Log.Info("Check FAR CR's name")
valid, err := farUtils.IsNodeNameValid(r.Client, req.Name)
razo7 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return emptyResult, err
}
if !valid {
r.Log.Info("consider recreating the CR - invalid CR's name to the cluster node name", "FAR CR's Name", req.Name)
razo7 marked this conversation as resolved.
Show resolved Hide resolved
return emptyResult, nil
}
// Fetch the FAR's pod
r.Log.Info("Fetch FAR's pod")
pod, err := farUtils.GetFenceAgentsRemediationPod(r.Client)
Expand All @@ -99,7 +109,6 @@ func (r *FenceAgentsRemediationReconciler) Reconcile(ctx context.Context, req ct
//TODO: better seperation between errors from wrong shared parameters values and wrong node parameters values
return emptyResult, err
}

return emptyResult, nil
}

Expand Down Expand Up @@ -131,5 +140,3 @@ func appendParamToSlice(fenceAgentParams []string, paramName v1alpha1.ParameterN
}
return fenceAgentParams
}

// TODO: Add isNodeNameValid function which call listNodeNames to validate the FAR's name with the cluster node names
55 changes: 41 additions & 14 deletions controllers/fenceagentsremediation_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ import (
. "github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrl "sigs.k8s.io/controller-runtime"

"github.com/medik8s/fence-agents-remediation/api/v1alpha1"
farUtils "github.com/medik8s/fence-agents-remediation/pkg/utils"
)

const (
Expand All @@ -47,6 +49,7 @@ var (
var _ = Describe("FAR Controller", func() {
var (
underTestFAR *v1alpha1.FenceAgentsRemediation
node *v1.Node
)

testShareParam := map[v1alpha1.ParameterName]string{
Expand All @@ -69,39 +72,53 @@ var _ = Describe("FAR Controller", func() {
underTestFAR = newFenceAgentsRemediation(validNodeName, fenceAgentIPMI, testShareParam, testNodeParam)

Context("Functionality", func() {
Context("buildFenceAgentParams", func() {
When("FAR's name isn't a node name", func() {
Context("buildFenceAgentParams - check CR name", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: using function names in Ginkgo's node description makes it harder to understand what the test is verifying.
For example, in this context, it might be just "Check CR name against node"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, do you have a suggestion for better naming?
I see a small value of testing the 2 functions in UT, but it does catch errors when I do small changes in them (or their dependencies)

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, in this context, it might be just "Check CR name against node"

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, then I tend to let the context just be the function name (buildFenceAgentParams), and just remove the - check CR name

Copy link
Contributor

@clobrano clobrano Jun 8, 2023

Choose a reason for hiding this comment

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

not super important, but this would be less future-proof IMHO
If you change function name, or signature, you shall then change the test too
Instead, testing a behavior, whatever the implementation is, should always be valid also in the future

EDIT: I removed "change signature", which cannot be future proof, ofc XD

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, checking behavior rather than a function (by it's name) seems more right.
Something to think off for next PRs and writing UTs

When("FAR's name doesn't match a node name", func() {
It("should fail", func() {
underTestFAR.ObjectMeta.Name = dummyNodeName
_, err := buildFenceAgentParams(underTestFAR)
Expect(err).To(HaveOccurred())
razo7 marked this conversation as resolved.
Show resolved Hide resolved
})
})
When("FAR's name does match a node name", func() {
It("should succeed", func() {
underTestFAR.ObjectMeta.Name = validNodeName
_, err := buildFenceAgentParams(underTestFAR)
slintes marked this conversation as resolved.
Show resolved Hide resolved
Expect(err).NotTo(HaveOccurred())
})
})
})
When("creating a resource", func() {
It("should fail when FAR pod is missing", func() {
//Test getFenceAgentsPod func

Context("IsNodeNameValid - check node object", func() {
BeforeEach(func() {
node = getNode(validNodeName)
DeferCleanup(k8sClient.Delete, context.Background(), node)
Expect(k8sClient.Create(context.Background(), node)).To(Succeed())
})
When("FAR's name doesn't match to an existing node name", func() {
It("should fail", func() {
Expect(farUtils.IsNodeNameValid(k8sClient, dummyNodeName)).To(BeFalse())
})
})
When("FAR's name does match to an existing node name", func() {
It("should succeed", func() {
razo7 marked this conversation as resolved.
Show resolved Hide resolved
Expect(farUtils.IsNodeNameValid(k8sClient, validNodeName)).To(BeTrue())
})
})
})
})
Context("Reconcile", func() {
//Scenarios

BeforeEach(func() {
fenceAgentsPod = buildFarPod()
// Create fenceAgentsPod and FAR
Expect(k8sClient.Create(context.Background(), fenceAgentsPod)).NotTo(HaveOccurred())
Expect(k8sClient.Create(context.Background(), underTestFAR)).NotTo(HaveOccurred())
})

AfterEach(func() {
Expect(k8sClient.Delete(context.Background(), fenceAgentsPod)).NotTo(HaveOccurred())
Expect(k8sClient.Delete(context.Background(), underTestFAR)).NotTo(HaveOccurred())
node = getNode(validNodeName)
// DeferCleanUp and Create node, fenceAgentsPod and FAR CR
DeferCleanup(k8sClient.Delete, context.Background(), node)
DeferCleanup(k8sClient.Delete, context.Background(), fenceAgentsPod)
DeferCleanup(k8sClient.Delete, context.Background(), underTestFAR)
Expect(k8sClient.Create(context.Background(), node)).To(Succeed())
Expect(k8sClient.Create(context.Background(), fenceAgentsPod)).To(Succeed())
Expect(k8sClient.Create(context.Background(), underTestFAR)).To(Succeed())
})

When("creating FAR CR", func() {
Expand All @@ -127,6 +144,16 @@ func newFenceAgentsRemediation(nodeName string, agent string, sharedparameters m
}
}

// used for making new node object for test and have a unique resourceVersion
// getNode returns a node object with the name nodeName
func getNode(nodeName string) *corev1.Node {
return &corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: nodeName,
},
}
}

// buildFarPod builds a dummy pod with FAR label and namespace
func buildFarPod() *corev1.Pod {
fenceAgentsPod := &corev1.Pod{}
Expand Down
26 changes: 26 additions & 0 deletions pkg/utils/nodes.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package utils

import (
"context"

corev1 "k8s.io/api/core/v1"
apiErrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// IsNodeNameValid returns an error if nodeName doesn't match any node name int the cluster, otherwise a nil
func IsNodeNameValid(r client.Reader, nodeName string) (bool, error) {
razo7 marked this conversation as resolved.
Show resolved Hide resolved
node := &corev1.Node{}
objNodeName := types.NamespacedName{Name: nodeName}
err := r.Get(context.Background(), objNodeName, node)
if err != nil {
if apiErrors.IsNotFound(err) {
// In case of notFound API error we don't return error, since it is valid result
return false, nil
} else {
return false, err
}
}
return true, nil
}
10 changes: 1 addition & 9 deletions pkg/utils/pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,8 @@ package utils
import (
"context"
"fmt"
"net/http"

corev1 "k8s.io/api/core/v1"
apiErrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -29,12 +26,7 @@ func GetFenceAgentsRemediationPod(r client.Reader) (*corev1.Pod, error) {
return nil, fmt.Errorf("failed fetching FAR pod - %w", err)
}
if len(pods.Items) == 0 {
podNotFoundErr := &apiErrors.StatusError{ErrStatus: metav1.Status{
Status: metav1.StatusFailure,
Code: http.StatusNotFound,
Reason: metav1.StatusReasonNotFound,
}}
return nil, fmt.Errorf("no Fence Agent pods were found - %w", podNotFoundErr)
return nil, fmt.Errorf("no Fence Agent pods were found")
}
return &pods.Items[0], nil
}