From 9396345e0ff05375a989a54eb16c95b6826e8571 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Wed, 1 Nov 2023 11:35:25 +0530 Subject: [PATCH 1/9] node health controller tenancy --- .../controllers/nodehealth/controller_test.go | 250 ++++++++++-------- 1 file changed, 138 insertions(+), 112 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index b21c52e521f8..8aab9a8973d6 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -6,6 +6,7 @@ package nodehealth import ( "context" "fmt" + "github.com/hashicorp/consul/agent/structs" "testing" "github.com/oklog/ulid/v2" @@ -45,15 +46,11 @@ var ( } ) -func resourceID(rtype *pbresource.Type, name string) *pbresource.ID { +func resourceID(rtype *pbresource.Type, name string, tenancy *pbresource.Tenancy) *pbresource.ID { return &pbresource.ID{ - Type: rtype, - Tenancy: &pbresource.Tenancy{ - Partition: "default", - Namespace: "default", - PeerName: "local", - }, - Name: name, + Type: rtype, + Tenancy: tenancy, + Name: name, } } @@ -70,102 +67,56 @@ type nodeHealthControllerTestSuite struct { nodeWarning *pbresource.ID nodeCritical *pbresource.ID nodeMaintenance *pbresource.ID + isEnterprise bool + tenancies []*pbresource.Tenancy } -func (suite *nodeHealthControllerTestSuite) SetupTest() { - suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register, types.RegisterDNSPolicy) - suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())} - - // The rest of the setup will be to prime the resource service with some data - suite.nodeNoHealth = resourcetest.Resource(pbcatalog.NodeType, "test-node-no-health"). - WithData(suite.T(), nodeData). - Write(suite.T(), suite.resourceClient).Id - - suite.nodePassing = resourcetest.Resource(pbcatalog.NodeType, "test-node-passing"). +func (suite *nodeHealthControllerTestSuite) writeNode(name string, tenancy *pbresource.Tenancy) *pbresource.ID { + return resourcetest.Resource(pbcatalog.NodeType, name). WithData(suite.T(), nodeData). + WithTenancy(tenancy). Write(suite.T(), suite.resourceClient).Id +} - suite.nodeWarning = resourcetest.Resource(pbcatalog.NodeType, "test-node-warning"). - WithData(suite.T(), nodeData). - Write(suite.T(), suite.resourceClient).Id - - suite.nodeCritical = resourcetest.Resource(pbcatalog.NodeType, "test-node-critical"). - WithData(suite.T(), nodeData). - Write(suite.T(), suite.resourceClient).Id - - suite.nodeMaintenance = resourcetest.Resource(pbcatalog.NodeType, "test-node-maintenance"). - WithData(suite.T(), nodeData). - Write(suite.T(), suite.resourceClient).Id - - nodeHealthDesiredStatus := map[string]pbcatalog.Health{ - suite.nodePassing.Name: pbcatalog.Health_HEALTH_PASSING, - suite.nodeWarning.Name: pbcatalog.Health_HEALTH_WARNING, - suite.nodeCritical.Name: pbcatalog.Health_HEALTH_CRITICAL, - suite.nodeMaintenance.Name: pbcatalog.Health_HEALTH_MAINTENANCE, - } - - // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing - // we are strategically naming health statuses so that they will be returned in an order with - // the most precedent status being in the middle of the list. This will ensure that statuses - // seen later can overide a previous status and that statuses seen later do not override if - // they would lower the overall status such as going from critical -> warning. - precedenceHealth := []pbcatalog.Health{ - pbcatalog.Health_HEALTH_PASSING, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_MAINTENANCE, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_PASSING, - } - - for _, node := range []*pbresource.ID{suite.nodePassing, suite.nodeWarning, suite.nodeCritical, suite.nodeMaintenance} { - for idx, health := range precedenceHealth { - if nodeHealthDesiredStatus[node.Name] >= health { - resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d", node.Name, idx)). - WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). - WithOwner(node). - Write(suite.T(), suite.resourceClient) - } - } - } - - // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs - // to be something that doesn't care about its owner. All we want to prove is that we are - // filtering out non-HealthStatus types appropriately. - resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). - WithData(suite.T(), dnsPolicyData). - WithOwner(suite.nodeNoHealth). - Write(suite.T(), suite.resourceClient) +func (suite *nodeHealthControllerTestSuite) SetupTest() { + suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register, types.RegisterDNSPolicy) + suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())} + suite.isEnterprise = structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" + suite.tenancies = resourcetest.TestTenancies() } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { - // This resource id references a resource type that will not be - // registered with the resource service. The ListByOwner call - // should produce an InvalidArgument error. This test is meant - // to validate how that error is handled (its propagated back - // to the caller) - ref := resourceID( - &pbresource.Type{Group: "not", GroupVersion: "v1", Kind: "found"}, - "irrelevant", - ) - health, err := getNodeHealth(context.Background(), suite.runtime, ref) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) - require.Error(suite.T(), err) - require.Equal(suite.T(), codes.InvalidArgument, status.Code(err)) + suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy) { + // This resource id references a resource type that will not be + // registered with the resource service. The ListByOwner call + // should produce an InvalidArgument error. This test is meant + // to validate how that error is handled (its propagated back + // to the caller) + ref := resourceID( + &pbresource.Type{Group: "not", GroupVersion: "v1", Kind: "found"}, + "irrelevant", + tenancy, + ) + health, err := getNodeHealth(context.Background(), suite.runtime, ref) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) + require.Error(suite.T(), err) + require.Equal(suite.T(), codes.InvalidArgument, status.Code(err)) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { - // This test is meant to ensure that when the node doesn't exist - // no error is returned but also no data is. The default passing - // status should then be returned in the same manner as the node - // existing but with no associated HealthStatus resources. - ref := resourceID(pbcatalog.NodeType, "foo") - ref.Uid = ulid.Make().String() - health, err := getNodeHealth(context.Background(), suite.runtime, ref) - - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy) { + // This test is meant to ensure that when the node doesn't exist + // no error is returned but also no data is. The default passing + // status should then be returned in the same manner as the node + // existing but with no associated HealthStatus resources. + ref := resourceID(pbcatalog.NodeType, "foo", tenancy) + ref.Uid = ulid.Make().String() + health, err := getNodeHealth(context.Background(), suite.runtime, ref) + + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { @@ -199,30 +150,35 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() } func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { - // This test ensures that removed nodes are ignored. In particular we don't - // want to propagate the error and indefinitely keep re-reconciling in this case. - err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ - ID: resourceID(pbcatalog.NodeType, "not-found"), + suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy) { + // This test ensures that removed nodes are ignored. In particular we don't + // want to propagate the error and indefinitely keep re-reconciling in this case. + err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ + ID: resourceID(pbcatalog.NodeType, "not-found", tenancy), + }) + require.NoError(suite.T(), err) }) - require.NoError(suite.T(), err) } func (suite *nodeHealthControllerTestSuite) TestReconcilePropagateReadError() { - // This test aims to ensure that errors other than NotFound errors coming - // from the initial resource read get propagated. This case is very unrealistic - // as the controller should not have given us a request ID for a resource type - // that doesn't exist but this was the easiest way I could think of to synthesize - // a Read error. - ref := resourceID( - &pbresource.Type{Group: "not", GroupVersion: "v1", Kind: "found"}, - "irrelevant", - ) - - err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ - ID: ref, + suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy) { + // This test aims to ensure that errors other than NotFound errors coming + // from the initial resource read get propagated. This case is very unrealistic + // as the controller should not have given us a request ID for a resource type + // that doesn't exist but this was the easiest way I could think of to synthesize + // a Read error. + ref := resourceID( + &pbresource.Type{Group: "not", GroupVersion: "v1", Kind: "found"}, + "irrelevant", + tenancy, + ) + + err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ + ID: ref, + }) + require.Error(suite.T(), err) + require.Equal(suite.T(), codes.InvalidArgument, status.Code(err)) }) - require.Error(suite.T(), err) - require.Equal(suite.T(), codes.InvalidArgument, status.Code(err)) } func (suite *nodeHealthControllerTestSuite) testReconcileStatus(id *pbresource.ID, expectedStatus *pbresource.Condition) *pbresource.Resource { @@ -364,3 +320,73 @@ func (suite *nodeHealthControllerTestSuite) TestController() { func TestNodeHealthController(t *testing.T) { suite.Run(t, new(nodeHealthControllerTestSuite)) } + +func (suite *nodeHealthControllerTestSuite) constructTestCaseName(name string, tenancy *pbresource.Tenancy) string { + if !suite.isEnterprise { + return name + } + return fmt.Sprintf("%s_%s_Namespace_%s_Partition", name, tenancy.Namespace, tenancy.Partition) +} + +func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancy *pbresource.Tenancy) { + + // The rest of the setup will be to prime the resource service with some data + + suite.nodePassing = suite.writeNode("test-node-passing", tenancy) + + suite.nodeWarning = suite.writeNode("test-node-warning", tenancy) + + suite.nodeCritical = suite.writeNode("test-node-critical", tenancy) + + suite.nodeMaintenance = suite.writeNode("test-node-maintenance", tenancy) + + nodeHealthDesiredStatus := map[string]pbcatalog.Health{ + suite.nodePassing.Name: pbcatalog.Health_HEALTH_PASSING, + suite.nodeWarning.Name: pbcatalog.Health_HEALTH_WARNING, + suite.nodeCritical.Name: pbcatalog.Health_HEALTH_CRITICAL, + suite.nodeMaintenance.Name: pbcatalog.Health_HEALTH_MAINTENANCE, + } + + // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing + // we are strategically naming health statuses so that they will be returned in an order with + // the most precedent status being in the middle of the list. This will ensure that statuses + // seen later can overide a previous status and that statuses seen later do not override if + // they would lower the overall status such as going from critical -> warning. + precedenceHealth := []pbcatalog.Health{ + pbcatalog.Health_HEALTH_PASSING, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_MAINTENANCE, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_PASSING, + } + + for _, node := range []*pbresource.ID{suite.nodePassing, suite.nodeWarning, suite.nodeCritical, suite.nodeMaintenance} { + for idx, health := range precedenceHealth { + if nodeHealthDesiredStatus[node.Name] >= health { + resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d", node.Name, idx)). + WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). + WithOwner(node). + Write(suite.T(), suite.resourceClient) + } + } + } + + // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs + // to be something that doesn't care about its owner. All we want to prove is that we are + // filtering out non-HealthStatus types appropriately. + resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). + WithData(suite.T(), dnsPolicyData). + WithOwner(suite.nodeNoHealth). + Write(suite.T(), suite.resourceClient) +} + +func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy)) { + for _, tenancy := range suite.tenancies { + suite.setupNodesWithTenancy(tenancy) + suite.Run(suite.constructTestCaseName(name, tenancy), func() { + t(tenancy) + }) + } +} From 7816e74b85d6f509725eb41ae2e9e572e69476e5 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Thu, 2 Nov 2023 00:16:35 +0530 Subject: [PATCH 2/9] some prog --- .../controllers/nodehealth/controller_test.go | 318 ++++++++++-------- internal/resource/resourcetest/tenancy.go | 13 + 2 files changed, 190 insertions(+), 141 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index 8aab9a8973d6..f8efbc595e8a 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "github.com/hashicorp/consul/agent/structs" + "github.com/stretchr/testify/mock" "testing" "github.com/oklog/ulid/v2" @@ -15,6 +16,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + mockres "github.com/hashicorp/consul/agent/grpc-external/services/resource" svctest "github.com/hashicorp/consul/agent/grpc-external/services/resource/testing" "github.com/hashicorp/consul/internal/catalog/internal/types" "github.com/hashicorp/consul/internal/controller" @@ -63,10 +65,10 @@ type nodeHealthControllerTestSuite struct { ctl nodeHealthReconciler nodeNoHealth *pbresource.ID - nodePassing *pbresource.ID - nodeWarning *pbresource.ID - nodeCritical *pbresource.ID - nodeMaintenance *pbresource.ID + nodePassing []*pbresource.ID + nodeWarning []*pbresource.ID + nodeCritical []*pbresource.ID + nodeMaintenance []*pbresource.ID isEnterprise bool tenancies []*pbresource.Tenancy } @@ -79,14 +81,22 @@ func (suite *nodeHealthControllerTestSuite) writeNode(name string, tenancy *pbre } func (suite *nodeHealthControllerTestSuite) SetupTest() { - suite.resourceClient = svctest.RunResourceService(suite.T(), types.Register, types.RegisterDNSPolicy) + mockTenancyBridge := &mockres.MockTenancyBridge{} + mockTenancyBridge.On("PartitionExists", mock.Anything).Return(true, nil) + mockTenancyBridge.On("NamespaceExists", mock.Anything, mock.Anything).Return(true, nil) + mockTenancyBridge.On("IsPartitionMarkedForDeletion", mock.Anything).Return(false, nil) + mockTenancyBridge.On("IsNamespaceMarkedForDeletion", mock.Anything, mock.Anything).Return(false, nil) + cfg := mockres.Config{ + TenancyBridge: mockTenancyBridge, + } + suite.resourceClient = svctest.RunResourceServiceWithConfig(suite.T(), cfg, types.Register, types.RegisterDNSPolicy) suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())} suite.isEnterprise = structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" suite.tenancies = resourcetest.TestTenancies() } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { - suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy, indx int) { // This resource id references a resource type that will not be // registered with the resource service. The ListByOwner call // should produce an InvalidArgument error. This test is meant @@ -105,7 +115,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { - suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy, indx int) { // This test is meant to ensure that when the node doesn't exist // no error is returned but also no data is. The default passing // status should then be returned in the same manner as the node @@ -120,37 +130,47 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth) - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthNoStatus", func(tenancy *pbresource.Tenancy, indx int) { + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth) + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing) - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthPassingStatus", func(tenancy *pbresource.Tenancy, indx int) { + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing[indx]) + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical) - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthCriticalStatus", func(tenancy *pbresource.Tenancy, indx int) { + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical[indx]) + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning) - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_WARNING, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthWarningStatus", func(tenancy *pbresource.Tenancy, indx int) { + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning[indx]) + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_WARNING, health) + }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance) - require.NoError(suite.T(), err) - require.Equal(suite.T(), pbcatalog.Health_HEALTH_MAINTENANCE, health) + suite.runTestCaseWithTenancies("TestGetNodeHealthMaintenanceStatus", func(tenancy *pbresource.Tenancy, indx int) { + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance[indx]) + require.NoError(suite.T(), err) + require.Equal(suite.T(), pbcatalog.Health_HEALTH_MAINTENANCE, health) + }) } func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { - suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy, indx int) { // This test ensures that removed nodes are ignored. In particular we don't // want to propagate the error and indefinitely keep re-reconciling in this case. err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ @@ -161,7 +181,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { } func (suite *nodeHealthControllerTestSuite) TestReconcilePropagateReadError() { - suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy, indx int) { // This test aims to ensure that errors other than NotFound errors coming // from the initial resource read get propagated. This case is very unrealistic // as the controller should not have given us a request ID for a resource type @@ -206,60 +226,70 @@ func (suite *nodeHealthControllerTestSuite) testReconcileStatus(id *pbresource.I } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { - suite.testReconcileStatus(suite.nodePassing, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_TRUE, - Reason: "HEALTH_PASSING", - Message: NodeHealthyMessage, + suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy, indx int) { + suite.testReconcileStatus(suite.nodePassing[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_TRUE, + Reason: "HEALTH_PASSING", + Message: NodeHealthyMessage, + }) }) } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { - suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_FALSE, - Reason: "HEALTH_WARNING", - Message: NodeUnhealthyMessage, + suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy, indx int) { + suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_FALSE, + Reason: "HEALTH_WARNING", + Message: NodeUnhealthyMessage, + }) }) } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { - suite.testReconcileStatus(suite.nodeCritical, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_FALSE, - Reason: "HEALTH_CRITICAL", - Message: NodeUnhealthyMessage, + suite.runTestCaseWithTenancies("TestReconcile_StatusCritical", func(tenancy *pbresource.Tenancy, indx int) { + suite.testReconcileStatus(suite.nodeCritical[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_FALSE, + Reason: "HEALTH_CRITICAL", + Message: NodeUnhealthyMessage, + }) }) } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { - suite.testReconcileStatus(suite.nodeMaintenance, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_FALSE, - Reason: "HEALTH_MAINTENANCE", - Message: NodeUnhealthyMessage, + suite.runTestCaseWithTenancies("TestReconcile_StatusMaintenance", func(tenancy *pbresource.Tenancy, indx int) { + suite.testReconcileStatus(suite.nodeMaintenance[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_FALSE, + Reason: "HEALTH_MAINTENANCE", + Message: NodeUnhealthyMessage, + }) }) } func (suite *nodeHealthControllerTestSuite) TestReconcile_AvoidRereconciliationWrite() { - res1 := suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_FALSE, - Reason: "HEALTH_WARNING", - Message: NodeUnhealthyMessage, - }) + suite.runTestCaseWithTenancies("TestReconcile_AvoidRereconciliationWrite", func(tenancy *pbresource.Tenancy, indx int) { + res1 := suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_FALSE, + Reason: "HEALTH_WARNING", + Message: NodeUnhealthyMessage, + }) - res2 := suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ - Type: StatusConditionHealthy, - State: pbresource.Condition_STATE_FALSE, - Reason: "HEALTH_WARNING", - Message: NodeUnhealthyMessage, - }) + res2 := suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + Type: StatusConditionHealthy, + State: pbresource.Condition_STATE_FALSE, + Reason: "HEALTH_WARNING", + Message: NodeUnhealthyMessage, + }) - // If another status write was performed then the versions would differ. This - // therefore proves that after a second reconciliation without any change in status - // that we are not making subsequent status writes. - require.Equal(suite.T(), res1.Version, res2.Version) + // If another status write was performed then the versions would differ. This + // therefore proves that after a second reconciliation without any change in status + // that we are not making subsequent status writes. + require.Equal(suite.T(), res1.Version, res2.Version) + }) } func (suite *nodeHealthControllerTestSuite) waitForReconciliation(id *pbresource.ID, reason string) { @@ -279,42 +309,44 @@ func (suite *nodeHealthControllerTestSuite) waitForReconciliation(id *pbresource }) } func (suite *nodeHealthControllerTestSuite) TestController() { - // create the controller manager - mgr := controller.NewManager(suite.resourceClient, testutil.Logger(suite.T())) - - // register our controller - mgr.Register(NodeHealthController()) - mgr.SetRaftLeader(true) - ctx, cancel := context.WithCancel(context.Background()) - suite.T().Cleanup(cancel) - - // run the manager - go mgr.Run(ctx) - - // ensure that the node health eventually gets set. - suite.waitForReconciliation(suite.nodePassing, "HEALTH_PASSING") - - // rewrite the resource - this will cause the nodes health - // to be rereconciled but wont result in any health change - resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing.Name). - WithData(suite.T(), &pbcatalog.Node{ - Addresses: []*pbcatalog.NodeAddress{ - { - Host: "198.18.0.1", + suite.runTestCaseWithTenancies("TestController", func(tenancy *pbresource.Tenancy, indx int) { + // create the controller manager + mgr := controller.NewManager(suite.resourceClient, testutil.Logger(suite.T())) + + // register our controller + mgr.Register(NodeHealthController()) + mgr.SetRaftLeader(true) + ctx, cancel := context.WithCancel(context.Background()) + suite.T().Cleanup(cancel) + + // run the manager + go mgr.Run(ctx) + + // ensure that the node health eventually gets set. + suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_PASSING") + + // rewrite the resource - this will cause the nodes health + // to be rereconciled but wont result in any health change + resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing[indx].Name). + WithData(suite.T(), &pbcatalog.Node{ + Addresses: []*pbcatalog.NodeAddress{ + { + Host: "198.18.0.1", + }, }, - }, - }). - Write(suite.T(), suite.resourceClient) + }). + Write(suite.T(), suite.resourceClient) - // wait for rereconciliation to happen - suite.waitForReconciliation(suite.nodePassing, "HEALTH_PASSING") + // wait for rereconciliation to happen + suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_PASSING") - resourcetest.Resource(pbcatalog.HealthStatusType, "failure"). - WithData(suite.T(), &pbcatalog.HealthStatus{Type: "fake", Status: pbcatalog.Health_HEALTH_CRITICAL}). - WithOwner(suite.nodePassing). - Write(suite.T(), suite.resourceClient) + resourcetest.Resource(pbcatalog.HealthStatusType, "failure"). + WithData(suite.T(), &pbcatalog.HealthStatus{Type: "fake", Status: pbcatalog.Health_HEALTH_CRITICAL}). + WithOwner(suite.nodePassing[indx]). + Write(suite.T(), suite.resourceClient) - suite.waitForReconciliation(suite.nodePassing, "HEALTH_CRITICAL") + suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_CRITICAL") + }) } func TestNodeHealthController(t *testing.T) { @@ -328,65 +360,69 @@ func (suite *nodeHealthControllerTestSuite) constructTestCaseName(name string, t return fmt.Sprintf("%s_%s_Namespace_%s_Partition", name, tenancy.Namespace, tenancy.Partition) } -func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancy *pbresource.Tenancy) { +func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*pbresource.Tenancy) { // The rest of the setup will be to prime the resource service with some data + suite.nodePassing = make([]*pbresource.ID, len(tenancies)) + suite.nodeWarning = make([]*pbresource.ID, len(tenancies)) + suite.nodeCritical = make([]*pbresource.ID, len(tenancies)) + suite.nodeMaintenance = make([]*pbresource.ID, len(tenancies)) + + for indx, tenancy := range tenancies { + + suite.nodePassing[indx] = suite.writeNode("test-node-passing", tenancy) + suite.nodeWarning[indx] = suite.writeNode("test-node-warning", tenancy) + suite.nodeCritical[indx] = suite.writeNode("test-node-critical", tenancy) + suite.nodeMaintenance[indx] = suite.writeNode("test-node-maintenance", tenancy) + + nodeHealthDesiredStatus := map[string]pbcatalog.Health{ + suite.nodePassing[indx].Name: pbcatalog.Health_HEALTH_PASSING, + suite.nodeWarning[indx].Name: pbcatalog.Health_HEALTH_WARNING, + suite.nodeCritical[indx].Name: pbcatalog.Health_HEALTH_CRITICAL, + suite.nodeMaintenance[indx].Name: pbcatalog.Health_HEALTH_MAINTENANCE, + } - suite.nodePassing = suite.writeNode("test-node-passing", tenancy) - - suite.nodeWarning = suite.writeNode("test-node-warning", tenancy) - - suite.nodeCritical = suite.writeNode("test-node-critical", tenancy) - - suite.nodeMaintenance = suite.writeNode("test-node-maintenance", tenancy) - - nodeHealthDesiredStatus := map[string]pbcatalog.Health{ - suite.nodePassing.Name: pbcatalog.Health_HEALTH_PASSING, - suite.nodeWarning.Name: pbcatalog.Health_HEALTH_WARNING, - suite.nodeCritical.Name: pbcatalog.Health_HEALTH_CRITICAL, - suite.nodeMaintenance.Name: pbcatalog.Health_HEALTH_MAINTENANCE, - } - - // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing - // we are strategically naming health statuses so that they will be returned in an order with - // the most precedent status being in the middle of the list. This will ensure that statuses - // seen later can overide a previous status and that statuses seen later do not override if - // they would lower the overall status such as going from critical -> warning. - precedenceHealth := []pbcatalog.Health{ - pbcatalog.Health_HEALTH_PASSING, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_MAINTENANCE, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_PASSING, - } + // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing + // we are strategically naming health statuses so that they will be returned in an order with + // the most precedent status being in the middle of the list. This will ensure that statuses + // seen later can overide a previous status and that statuses seen later do not override if + // they would lower the overall status such as going from critical -> warning. + precedenceHealth := []pbcatalog.Health{ + pbcatalog.Health_HEALTH_PASSING, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_MAINTENANCE, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_PASSING, + } - for _, node := range []*pbresource.ID{suite.nodePassing, suite.nodeWarning, suite.nodeCritical, suite.nodeMaintenance} { - for idx, health := range precedenceHealth { - if nodeHealthDesiredStatus[node.Name] >= health { - resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d", node.Name, idx)). - WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). - WithOwner(node). - Write(suite.T(), suite.resourceClient) + for _, node := range []*pbresource.ID{suite.nodePassing[indx], suite.nodeWarning[indx], suite.nodeCritical[indx], suite.nodeMaintenance[indx]} { + for idx, health := range precedenceHealth { + if nodeHealthDesiredStatus[node.Name] >= health { + resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d-%s-%s", node.Name, idx, tenancy.Partition, tenancy.Namespace)). + WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). + WithOwner(node). + Write(suite.T(), suite.resourceClient) + } } } - } - // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs - // to be something that doesn't care about its owner. All we want to prove is that we are - // filtering out non-HealthStatus types appropriately. - resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). - WithData(suite.T(), dnsPolicyData). - WithOwner(suite.nodeNoHealth). - Write(suite.T(), suite.resourceClient) + // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs + // to be something that doesn't care about its owner. All we want to prove is that we are + // filtering out non-HealthStatus types appropriately. + resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). + WithData(suite.T(), dnsPolicyData). + WithOwner(suite.nodeNoHealth). + Write(suite.T(), suite.resourceClient) + } } -func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy)) { - for _, tenancy := range suite.tenancies { - suite.setupNodesWithTenancy(tenancy) +func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy, int)) { + suite.setupNodesWithTenancy(suite.tenancies) + for indx, tenancy := range suite.tenancies { suite.Run(suite.constructTestCaseName(name, tenancy), func() { - t(tenancy) + t(tenancy, indx) }) } } diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index 5f5c0525b6f4..454af2124bc6 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -50,3 +50,16 @@ func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource. return nil } } + +// TestTenancies returns a list of tenancies which represent +// the namespace and partition combinations that can be used in unit tests +func TestTenancies() []*pbresource.Tenancy { + //isEnterprise := structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" + + tenancies := []*pbresource.Tenancy{Tenancy("default.default")} + //if isEnterprise { + tenancies = append(tenancies, Tenancy("default.bar"), Tenancy("foo.default"), Tenancy("foo.bar")) + //} + + return tenancies +} From db673ee9ae0d2ba076d9b872c8844fc5511b01f3 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Thu, 2 Nov 2023 00:20:25 +0530 Subject: [PATCH 3/9] some fixes --- .../internal/controllers/nodehealth/controller_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index f8efbc595e8a..896207d3c72f 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -64,7 +64,7 @@ type nodeHealthControllerTestSuite struct { ctl nodeHealthReconciler - nodeNoHealth *pbresource.ID + nodeNoHealth []*pbresource.ID nodePassing []*pbresource.ID nodeWarning []*pbresource.ID nodeCritical []*pbresource.ID @@ -131,7 +131,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { suite.runTestCaseWithTenancies("TestGetNodeHealthNoStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth[indx]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) }) @@ -363,6 +363,7 @@ func (suite *nodeHealthControllerTestSuite) constructTestCaseName(name string, t func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*pbresource.Tenancy) { // The rest of the setup will be to prime the resource service with some data + suite.nodeNoHealth = make([]*pbresource.ID, len(tenancies)) suite.nodePassing = make([]*pbresource.ID, len(tenancies)) suite.nodeWarning = make([]*pbresource.ID, len(tenancies)) suite.nodeCritical = make([]*pbresource.ID, len(tenancies)) @@ -370,6 +371,7 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p for indx, tenancy := range tenancies { + suite.nodeNoHealth[indx] = suite.writeNode("test-node-no-health", tenancy) suite.nodePassing[indx] = suite.writeNode("test-node-passing", tenancy) suite.nodeWarning[indx] = suite.writeNode("test-node-warning", tenancy) suite.nodeCritical[indx] = suite.writeNode("test-node-critical", tenancy) @@ -413,7 +415,7 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p // filtering out non-HealthStatus types appropriately. resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). WithData(suite.T(), dnsPolicyData). - WithOwner(suite.nodeNoHealth). + WithOwner(suite.nodeNoHealth[indx]). Write(suite.T(), suite.resourceClient) } } From d21b86927556ed59e7512d3aeed7f7ce70266e61 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Thu, 2 Nov 2023 00:24:19 +0530 Subject: [PATCH 4/9] revert --- .../internal/controllers/nodehealth/controller_test.go | 2 +- internal/resource/resourcetest/tenancy.go | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index 896207d3c72f..f5bdea91654f 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -413,7 +413,7 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs // to be something that doesn't care about its owner. All we want to prove is that we are // filtering out non-HealthStatus types appropriately. - resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy"). + resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy-"+tenancy.Partition+"-"+tenancy.Namespace). WithData(suite.T(), dnsPolicyData). WithOwner(suite.nodeNoHealth[indx]). Write(suite.T(), suite.resourceClient) diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index 454af2124bc6..3dfbe5499e1f 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -4,6 +4,7 @@ package resourcetest import ( + "github.com/hashicorp/consul/agent/structs" "strings" "testing" @@ -54,12 +55,12 @@ func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource. // TestTenancies returns a list of tenancies which represent // the namespace and partition combinations that can be used in unit tests func TestTenancies() []*pbresource.Tenancy { - //isEnterprise := structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" + isEnterprise := structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" tenancies := []*pbresource.Tenancy{Tenancy("default.default")} - //if isEnterprise { - tenancies = append(tenancies, Tenancy("default.bar"), Tenancy("foo.default"), Tenancy("foo.bar")) - //} + if isEnterprise { + tenancies = append(tenancies, Tenancy("default.bar"), Tenancy("foo.default"), Tenancy("foo.bar")) + } return tenancies } From 21e900d6a7585b04fa8907d013c4b91b30dc06b0 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Mon, 6 Nov 2023 14:03:49 +0530 Subject: [PATCH 5/9] pr comment resolved --- .../controllers/nodehealth/controller_test.go | 141 ++++++++++-------- internal/resource/resourcetest/tenancy.go | 12 ++ 2 files changed, 91 insertions(+), 62 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index f5bdea91654f..b92af75db736 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "github.com/hashicorp/consul/agent/structs" - "github.com/stretchr/testify/mock" "testing" "github.com/oklog/ulid/v2" @@ -64,11 +63,11 @@ type nodeHealthControllerTestSuite struct { ctl nodeHealthReconciler - nodeNoHealth []*pbresource.ID - nodePassing []*pbresource.ID - nodeWarning []*pbresource.ID - nodeCritical []*pbresource.ID - nodeMaintenance []*pbresource.ID + nodeNoHealth map[string]*pbresource.ID + nodePassing map[string]*pbresource.ID + nodeWarning map[string]*pbresource.ID + nodeCritical map[string]*pbresource.ID + nodeMaintenance map[string]*pbresource.ID isEnterprise bool tenancies []*pbresource.Tenancy } @@ -82,21 +81,23 @@ func (suite *nodeHealthControllerTestSuite) writeNode(name string, tenancy *pbre func (suite *nodeHealthControllerTestSuite) SetupTest() { mockTenancyBridge := &mockres.MockTenancyBridge{} - mockTenancyBridge.On("PartitionExists", mock.Anything).Return(true, nil) - mockTenancyBridge.On("NamespaceExists", mock.Anything, mock.Anything).Return(true, nil) - mockTenancyBridge.On("IsPartitionMarkedForDeletion", mock.Anything).Return(false, nil) - mockTenancyBridge.On("IsNamespaceMarkedForDeletion", mock.Anything, mock.Anything).Return(false, nil) + suite.tenancies = resourcetest.TestTenancies() + for _, tenancy := range suite.tenancies { + mockTenancyBridge.On("PartitionExists", tenancy.Partition).Return(true, nil) + mockTenancyBridge.On("NamespaceExists", tenancy.Partition, tenancy.Namespace).Return(true, nil) + mockTenancyBridge.On("IsPartitionMarkedForDeletion", tenancy.Partition).Return(false, nil) + mockTenancyBridge.On("IsNamespaceMarkedForDeletion", tenancy.Partition, tenancy.Namespace).Return(false, nil) + } cfg := mockres.Config{ TenancyBridge: mockTenancyBridge, } suite.resourceClient = svctest.RunResourceServiceWithConfig(suite.T(), cfg, types.Register, types.RegisterDNSPolicy) suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())} suite.isEnterprise = structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" - suite.tenancies = resourcetest.TestTenancies() } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { - suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy, indx int) { + suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy) { // This resource id references a resource type that will not be // registered with the resource service. The ListByOwner call // should produce an InvalidArgument error. This test is meant @@ -115,7 +116,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { - suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy, indx int) { + suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy) { // This test is meant to ensure that when the node doesn't exist // no error is returned but also no data is. The default passing // status should then be returned in the same manner as the node @@ -130,47 +131,52 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthNoStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth[indx]) + suite.runTestCaseWithTenancies("TestGetNodeHealthNoStatus", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth[tenancyString]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthPassingStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing[indx]) + suite.runTestCaseWithTenancies("TestGetNodeHealthPassingStatus", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing[tenancyString]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthCriticalStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical[indx]) + suite.runTestCaseWithTenancies("TestGetNodeHealthCriticalStatus", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical[tenancyString]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthWarningStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning[indx]) + suite.runTestCaseWithTenancies("TestGetNodeHealthWarningStatus", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning[tenancyString]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_WARNING, health) }) } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthMaintenanceStatus", func(tenancy *pbresource.Tenancy, indx int) { - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance[indx]) + suite.runTestCaseWithTenancies("TestGetNodeHealthMaintenanceStatus", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance[tenancyString]) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_MAINTENANCE, health) }) } func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { - suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy, indx int) { + suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy) { // This test ensures that removed nodes are ignored. In particular we don't // want to propagate the error and indefinitely keep re-reconciling in this case. err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ @@ -181,7 +187,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { } func (suite *nodeHealthControllerTestSuite) TestReconcilePropagateReadError() { - suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy, indx int) { + suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy) { // This test aims to ensure that errors other than NotFound errors coming // from the initial resource read get propagated. This case is very unrealistic // as the controller should not have given us a request ID for a resource type @@ -226,8 +232,9 @@ func (suite *nodeHealthControllerTestSuite) testReconcileStatus(id *pbresource.I } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { - suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy, indx int) { - suite.testReconcileStatus(suite.nodePassing[indx], &pbresource.Condition{ + suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + suite.testReconcileStatus(suite.nodePassing[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_TRUE, Reason: "HEALTH_PASSING", @@ -237,8 +244,9 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { - suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy, indx int) { - suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", @@ -248,8 +256,9 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { - suite.runTestCaseWithTenancies("TestReconcile_StatusCritical", func(tenancy *pbresource.Tenancy, indx int) { - suite.testReconcileStatus(suite.nodeCritical[indx], &pbresource.Condition{ + suite.runTestCaseWithTenancies("TestReconcile_StatusCritical", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + suite.testReconcileStatus(suite.nodeCritical[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_CRITICAL", @@ -259,8 +268,9 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { - suite.runTestCaseWithTenancies("TestReconcile_StatusMaintenance", func(tenancy *pbresource.Tenancy, indx int) { - suite.testReconcileStatus(suite.nodeMaintenance[indx], &pbresource.Condition{ + suite.runTestCaseWithTenancies("TestReconcile_StatusMaintenance", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + suite.testReconcileStatus(suite.nodeMaintenance[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_MAINTENANCE", @@ -270,15 +280,16 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_AvoidRereconciliationWrite() { - suite.runTestCaseWithTenancies("TestReconcile_AvoidRereconciliationWrite", func(tenancy *pbresource.Tenancy, indx int) { - res1 := suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + suite.runTestCaseWithTenancies("TestReconcile_AvoidRereconciliationWrite", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) + res1 := suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", Message: NodeUnhealthyMessage, }) - res2 := suite.testReconcileStatus(suite.nodeWarning[indx], &pbresource.Condition{ + res2 := suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", @@ -309,7 +320,8 @@ func (suite *nodeHealthControllerTestSuite) waitForReconciliation(id *pbresource }) } func (suite *nodeHealthControllerTestSuite) TestController() { - suite.runTestCaseWithTenancies("TestController", func(tenancy *pbresource.Tenancy, indx int) { + suite.runTestCaseWithTenancies("TestController", func(tenancy *pbresource.Tenancy) { + tenancyString := resourcetest.ToTenancyString(tenancy) // create the controller manager mgr := controller.NewManager(suite.resourceClient, testutil.Logger(suite.T())) @@ -323,11 +335,11 @@ func (suite *nodeHealthControllerTestSuite) TestController() { go mgr.Run(ctx) // ensure that the node health eventually gets set. - suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_PASSING") + suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_PASSING") // rewrite the resource - this will cause the nodes health // to be rereconciled but wont result in any health change - resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing[indx].Name). + resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing[tenancyString].Name). WithData(suite.T(), &pbcatalog.Node{ Addresses: []*pbcatalog.NodeAddress{ { @@ -335,17 +347,19 @@ func (suite *nodeHealthControllerTestSuite) TestController() { }, }, }). + WithTenancy(tenancy). Write(suite.T(), suite.resourceClient) // wait for rereconciliation to happen - suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_PASSING") + suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_PASSING") resourcetest.Resource(pbcatalog.HealthStatusType, "failure"). WithData(suite.T(), &pbcatalog.HealthStatus{Type: "fake", Status: pbcatalog.Health_HEALTH_CRITICAL}). - WithOwner(suite.nodePassing[indx]). + WithOwner(suite.nodePassing[tenancyString]). + WithTenancy(tenancy). Write(suite.T(), suite.resourceClient) - suite.waitForReconciliation(suite.nodePassing[indx], "HEALTH_CRITICAL") + suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_CRITICAL") }) } @@ -363,25 +377,27 @@ func (suite *nodeHealthControllerTestSuite) constructTestCaseName(name string, t func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*pbresource.Tenancy) { // The rest of the setup will be to prime the resource service with some data - suite.nodeNoHealth = make([]*pbresource.ID, len(tenancies)) - suite.nodePassing = make([]*pbresource.ID, len(tenancies)) - suite.nodeWarning = make([]*pbresource.ID, len(tenancies)) - suite.nodeCritical = make([]*pbresource.ID, len(tenancies)) - suite.nodeMaintenance = make([]*pbresource.ID, len(tenancies)) + suite.nodeNoHealth = make(map[string]*pbresource.ID) + suite.nodePassing = make(map[string]*pbresource.ID) + suite.nodeWarning = make(map[string]*pbresource.ID) + suite.nodeCritical = make(map[string]*pbresource.ID) + suite.nodeMaintenance = make(map[string]*pbresource.ID) + + for _, tenancy := range tenancies { - for indx, tenancy := range tenancies { + tenancyString := resourcetest.ToTenancyString(tenancy) - suite.nodeNoHealth[indx] = suite.writeNode("test-node-no-health", tenancy) - suite.nodePassing[indx] = suite.writeNode("test-node-passing", tenancy) - suite.nodeWarning[indx] = suite.writeNode("test-node-warning", tenancy) - suite.nodeCritical[indx] = suite.writeNode("test-node-critical", tenancy) - suite.nodeMaintenance[indx] = suite.writeNode("test-node-maintenance", tenancy) + suite.nodeNoHealth[tenancyString] = suite.writeNode("test-node-no-health", tenancy) + suite.nodePassing[tenancyString] = suite.writeNode("test-node-passing", tenancy) + suite.nodeWarning[tenancyString] = suite.writeNode("test-node-warning", tenancy) + suite.nodeCritical[tenancyString] = suite.writeNode("test-node-critical", tenancy) + suite.nodeMaintenance[tenancyString] = suite.writeNode("test-node-maintenance", tenancy) nodeHealthDesiredStatus := map[string]pbcatalog.Health{ - suite.nodePassing[indx].Name: pbcatalog.Health_HEALTH_PASSING, - suite.nodeWarning[indx].Name: pbcatalog.Health_HEALTH_WARNING, - suite.nodeCritical[indx].Name: pbcatalog.Health_HEALTH_CRITICAL, - suite.nodeMaintenance[indx].Name: pbcatalog.Health_HEALTH_MAINTENANCE, + suite.nodePassing[tenancyString].Name: pbcatalog.Health_HEALTH_PASSING, + suite.nodeWarning[tenancyString].Name: pbcatalog.Health_HEALTH_WARNING, + suite.nodeCritical[tenancyString].Name: pbcatalog.Health_HEALTH_CRITICAL, + suite.nodeMaintenance[tenancyString].Name: pbcatalog.Health_HEALTH_MAINTENANCE, } // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing @@ -399,7 +415,7 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p pbcatalog.Health_HEALTH_PASSING, } - for _, node := range []*pbresource.ID{suite.nodePassing[indx], suite.nodeWarning[indx], suite.nodeCritical[indx], suite.nodeMaintenance[indx]} { + for _, node := range []*pbresource.ID{suite.nodePassing[tenancyString], suite.nodeWarning[tenancyString], suite.nodeCritical[tenancyString], suite.nodeMaintenance[tenancyString]} { for idx, health := range precedenceHealth { if nodeHealthDesiredStatus[node.Name] >= health { resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d-%s-%s", node.Name, idx, tenancy.Partition, tenancy.Namespace)). @@ -415,16 +431,17 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p // filtering out non-HealthStatus types appropriately. resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy-"+tenancy.Partition+"-"+tenancy.Namespace). WithData(suite.T(), dnsPolicyData). - WithOwner(suite.nodeNoHealth[indx]). + WithOwner(suite.nodeNoHealth[tenancyString]). + WithTenancy(tenancy). Write(suite.T(), suite.resourceClient) } } -func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy, int)) { +func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy)) { suite.setupNodesWithTenancy(suite.tenancies) - for indx, tenancy := range suite.tenancies { + for _, tenancy := range suite.tenancies { suite.Run(suite.constructTestCaseName(name, tenancy), func() { - t(tenancy, indx) + t(tenancy) }) } } diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index 3dfbe5499e1f..74c987507bbb 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -38,6 +38,18 @@ func Tenancy(s string) *pbresource.Tenancy { } } +func ToTenancyString(tenancy *pbresource.Tenancy) string { + tenancyString := "" + if tenancy.Partition != "" { + tenancyString += tenancy.Partition + } + if tenancy.Namespace != "" { + tenancyString += "." + tenancyString += tenancy.Namespace + } + return tenancyString +} + func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource.Tenancy { switch reg.Scope { case resource.ScopeNamespace: From 25978580e178797705a4aff17a87fc0ea1fbf41d Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Mon, 6 Nov 2023 15:59:18 +0530 Subject: [PATCH 6/9] removed name --- .../controllers/nodehealth/controller_test.go | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index b92af75db736..aaa0476fc226 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -97,7 +97,7 @@ func (suite *nodeHealthControllerTestSuite) SetupTest() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { - suite.runTestCaseWithTenancies("TestGetNodeHealthListError", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // This resource id references a resource type that will not be // registered with the resource service. The ListByOwner call // should produce an InvalidArgument error. This test is meant @@ -116,7 +116,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthListError() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { - suite.runTestCaseWithTenancies("TestGetNodeHealthNoNode", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // This test is meant to ensure that when the node doesn't exist // no error is returned but also no data is. The default passing // status should then be returned in the same manner as the node @@ -131,7 +131,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthNoStatus", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth[tenancyString]) require.NoError(suite.T(), err) @@ -140,7 +140,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthPassingStatus", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing[tenancyString]) require.NoError(suite.T(), err) @@ -149,7 +149,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthCriticalStatus", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical[tenancyString]) require.NoError(suite.T(), err) @@ -158,7 +158,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthWarningStatus", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning[tenancyString]) require.NoError(suite.T(), err) @@ -167,7 +167,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { } func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() { - suite.runTestCaseWithTenancies("TestGetNodeHealthMaintenanceStatus", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance[tenancyString]) require.NoError(suite.T(), err) @@ -176,7 +176,7 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() } func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { - suite.runTestCaseWithTenancies("TestReconcileNodeNotFound", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // This test ensures that removed nodes are ignored. In particular we don't // want to propagate the error and indefinitely keep re-reconciling in this case. err := suite.ctl.Reconcile(context.Background(), suite.runtime, controller.Request{ @@ -187,7 +187,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcileNodeNotFound() { } func (suite *nodeHealthControllerTestSuite) TestReconcilePropagateReadError() { - suite.runTestCaseWithTenancies("TestReconcilePropagateReadError", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { // This test aims to ensure that errors other than NotFound errors coming // from the initial resource read get propagated. This case is very unrealistic // as the controller should not have given us a request ID for a resource type @@ -232,7 +232,7 @@ func (suite *nodeHealthControllerTestSuite) testReconcileStatus(id *pbresource.I } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { - suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) suite.testReconcileStatus(suite.nodePassing[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, @@ -244,7 +244,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { - suite.runTestCaseWithTenancies("TestReconcile_StatusPassing", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, @@ -256,7 +256,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { - suite.runTestCaseWithTenancies("TestReconcile_StatusCritical", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) suite.testReconcileStatus(suite.nodeCritical[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, @@ -268,7 +268,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { - suite.runTestCaseWithTenancies("TestReconcile_StatusMaintenance", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) suite.testReconcileStatus(suite.nodeMaintenance[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, @@ -280,7 +280,7 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { } func (suite *nodeHealthControllerTestSuite) TestReconcile_AvoidRereconciliationWrite() { - suite.runTestCaseWithTenancies("TestReconcile_AvoidRereconciliationWrite", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) res1 := suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ Type: StatusConditionHealthy, @@ -320,7 +320,7 @@ func (suite *nodeHealthControllerTestSuite) waitForReconciliation(id *pbresource }) } func (suite *nodeHealthControllerTestSuite) TestController() { - suite.runTestCaseWithTenancies("TestController", func(tenancy *pbresource.Tenancy) { + suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { tenancyString := resourcetest.ToTenancyString(tenancy) // create the controller manager mgr := controller.NewManager(suite.resourceClient, testutil.Logger(suite.T())) @@ -367,11 +367,8 @@ func TestNodeHealthController(t *testing.T) { suite.Run(t, new(nodeHealthControllerTestSuite)) } -func (suite *nodeHealthControllerTestSuite) constructTestCaseName(name string, tenancy *pbresource.Tenancy) string { - if !suite.isEnterprise { - return name - } - return fmt.Sprintf("%s_%s_Namespace_%s_Partition", name, tenancy.Namespace, tenancy.Partition) +func (suite *nodeHealthControllerTestSuite) appendTenancyInfo(tenancy *pbresource.Tenancy) string { + return fmt.Sprintf("%s_Namespace_%s_Partition", tenancy.Namespace, tenancy.Partition) } func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*pbresource.Tenancy) { @@ -437,10 +434,10 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*p } } -func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(name string, t func(*pbresource.Tenancy)) { +func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(t func(*pbresource.Tenancy)) { suite.setupNodesWithTenancy(suite.tenancies) for _, tenancy := range suite.tenancies { - suite.Run(suite.constructTestCaseName(name, tenancy), func() { + suite.Run(suite.appendTenancyInfo(tenancy), func() { t(tenancy) }) } From b55e6f211583a0997c078b7713203a7f7d0cd840 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Tue, 7 Nov 2023 09:07:30 +0530 Subject: [PATCH 7/9] cleanup nodes --- .../controllers/nodehealth/controller_test.go | 194 ++++++++++-------- internal/resource/resourcetest/tenancy.go | 12 -- 2 files changed, 108 insertions(+), 98 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index aaa0476fc226..4b512f6222ac 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -63,11 +63,11 @@ type nodeHealthControllerTestSuite struct { ctl nodeHealthReconciler - nodeNoHealth map[string]*pbresource.ID - nodePassing map[string]*pbresource.ID - nodeWarning map[string]*pbresource.ID - nodeCritical map[string]*pbresource.ID - nodeMaintenance map[string]*pbresource.ID + nodeNoHealth *pbresource.ID + nodePassing *pbresource.ID + nodeWarning *pbresource.ID + nodeCritical *pbresource.ID + nodeMaintenance *pbresource.ID isEnterprise bool tenancies []*pbresource.Tenancy } @@ -132,8 +132,8 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoNode() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth[tenancyString]) + + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeNoHealth) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) }) @@ -141,8 +141,8 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthNoStatus() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing[tenancyString]) + + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodePassing) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_PASSING, health) }) @@ -150,8 +150,8 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthPassingStatus() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical[tenancyString]) + + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeCritical) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_CRITICAL, health) }) @@ -159,8 +159,8 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthCriticalStatus() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning[tenancyString]) + + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeWarning) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_WARNING, health) }) @@ -168,8 +168,8 @@ func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthWarningStatus() { func (suite *nodeHealthControllerTestSuite) TestGetNodeHealthMaintenanceStatus() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance[tenancyString]) + + health, err := getNodeHealth(context.Background(), suite.runtime, suite.nodeMaintenance) require.NoError(suite.T(), err) require.Equal(suite.T(), pbcatalog.Health_HEALTH_MAINTENANCE, health) }) @@ -233,8 +233,8 @@ func (suite *nodeHealthControllerTestSuite) testReconcileStatus(id *pbresource.I func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - suite.testReconcileStatus(suite.nodePassing[tenancyString], &pbresource.Condition{ + + suite.testReconcileStatus(suite.nodePassing, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_TRUE, Reason: "HEALTH_PASSING", @@ -245,8 +245,8 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusPassing() { func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ + + suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", @@ -257,8 +257,8 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusWarning() { func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - suite.testReconcileStatus(suite.nodeCritical[tenancyString], &pbresource.Condition{ + + suite.testReconcileStatus(suite.nodeCritical, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_CRITICAL", @@ -269,8 +269,8 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusCritical() { func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - suite.testReconcileStatus(suite.nodeMaintenance[tenancyString], &pbresource.Condition{ + + suite.testReconcileStatus(suite.nodeMaintenance, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_MAINTENANCE", @@ -281,15 +281,15 @@ func (suite *nodeHealthControllerTestSuite) TestReconcile_StatusMaintenance() { func (suite *nodeHealthControllerTestSuite) TestReconcile_AvoidRereconciliationWrite() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) - res1 := suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ + + res1 := suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", Message: NodeUnhealthyMessage, }) - res2 := suite.testReconcileStatus(suite.nodeWarning[tenancyString], &pbresource.Condition{ + res2 := suite.testReconcileStatus(suite.nodeWarning, &pbresource.Condition{ Type: StatusConditionHealthy, State: pbresource.Condition_STATE_FALSE, Reason: "HEALTH_WARNING", @@ -321,7 +321,7 @@ func (suite *nodeHealthControllerTestSuite) waitForReconciliation(id *pbresource } func (suite *nodeHealthControllerTestSuite) TestController() { suite.runTestCaseWithTenancies(func(tenancy *pbresource.Tenancy) { - tenancyString := resourcetest.ToTenancyString(tenancy) + // create the controller manager mgr := controller.NewManager(suite.resourceClient, testutil.Logger(suite.T())) @@ -335,11 +335,11 @@ func (suite *nodeHealthControllerTestSuite) TestController() { go mgr.Run(ctx) // ensure that the node health eventually gets set. - suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_PASSING") + suite.waitForReconciliation(suite.nodePassing, "HEALTH_PASSING") // rewrite the resource - this will cause the nodes health // to be rereconciled but wont result in any health change - resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing[tenancyString].Name). + resourcetest.Resource(pbcatalog.NodeType, suite.nodePassing.Name). WithData(suite.T(), &pbcatalog.Node{ Addresses: []*pbcatalog.NodeAddress{ { @@ -351,15 +351,15 @@ func (suite *nodeHealthControllerTestSuite) TestController() { Write(suite.T(), suite.resourceClient) // wait for rereconciliation to happen - suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_PASSING") + suite.waitForReconciliation(suite.nodePassing, "HEALTH_PASSING") resourcetest.Resource(pbcatalog.HealthStatusType, "failure"). WithData(suite.T(), &pbcatalog.HealthStatus{Type: "fake", Status: pbcatalog.Health_HEALTH_CRITICAL}). - WithOwner(suite.nodePassing[tenancyString]). + WithOwner(suite.nodePassing). WithTenancy(tenancy). Write(suite.T(), suite.resourceClient) - suite.waitForReconciliation(suite.nodePassing[tenancyString], "HEALTH_CRITICAL") + suite.waitForReconciliation(suite.nodePassing, "HEALTH_CRITICAL") }) } @@ -371,74 +371,96 @@ func (suite *nodeHealthControllerTestSuite) appendTenancyInfo(tenancy *pbresourc return fmt.Sprintf("%s_Namespace_%s_Partition", tenancy.Namespace, tenancy.Partition) } -func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancies []*pbresource.Tenancy) { +func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancy *pbresource.Tenancy) { // The rest of the setup will be to prime the resource service with some data - suite.nodeNoHealth = make(map[string]*pbresource.ID) - suite.nodePassing = make(map[string]*pbresource.ID) - suite.nodeWarning = make(map[string]*pbresource.ID) - suite.nodeCritical = make(map[string]*pbresource.ID) - suite.nodeMaintenance = make(map[string]*pbresource.ID) - - for _, tenancy := range tenancies { - - tenancyString := resourcetest.ToTenancyString(tenancy) - - suite.nodeNoHealth[tenancyString] = suite.writeNode("test-node-no-health", tenancy) - suite.nodePassing[tenancyString] = suite.writeNode("test-node-passing", tenancy) - suite.nodeWarning[tenancyString] = suite.writeNode("test-node-warning", tenancy) - suite.nodeCritical[tenancyString] = suite.writeNode("test-node-critical", tenancy) - suite.nodeMaintenance[tenancyString] = suite.writeNode("test-node-maintenance", tenancy) - - nodeHealthDesiredStatus := map[string]pbcatalog.Health{ - suite.nodePassing[tenancyString].Name: pbcatalog.Health_HEALTH_PASSING, - suite.nodeWarning[tenancyString].Name: pbcatalog.Health_HEALTH_WARNING, - suite.nodeCritical[tenancyString].Name: pbcatalog.Health_HEALTH_CRITICAL, - suite.nodeMaintenance[tenancyString].Name: pbcatalog.Health_HEALTH_MAINTENANCE, - } + suite.nodeNoHealth = suite.writeNode("test-node-no-health", tenancy) + suite.nodePassing = suite.writeNode("test-node-passing", tenancy) + suite.nodeWarning = suite.writeNode("test-node-warning", tenancy) + suite.nodeCritical = suite.writeNode("test-node-critical", tenancy) + suite.nodeMaintenance = suite.writeNode("test-node-maintenance", tenancy) + + nodeHealthDesiredStatus := map[string]pbcatalog.Health{ + suite.nodePassing.Name: pbcatalog.Health_HEALTH_PASSING, + suite.nodeWarning.Name: pbcatalog.Health_HEALTH_WARNING, + suite.nodeCritical.Name: pbcatalog.Health_HEALTH_CRITICAL, + suite.nodeMaintenance.Name: pbcatalog.Health_HEALTH_MAINTENANCE, + } - // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing - // we are strategically naming health statuses so that they will be returned in an order with - // the most precedent status being in the middle of the list. This will ensure that statuses - // seen later can overide a previous status and that statuses seen later do not override if - // they would lower the overall status such as going from critical -> warning. - precedenceHealth := []pbcatalog.Health{ - pbcatalog.Health_HEALTH_PASSING, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_MAINTENANCE, - pbcatalog.Health_HEALTH_CRITICAL, - pbcatalog.Health_HEALTH_WARNING, - pbcatalog.Health_HEALTH_PASSING, - } + // In order to exercise the behavior to ensure that its not a last-status-wins sort of thing + // we are strategically naming health statuses so that they will be returned in an order with + // the most precedent status being in the middle of the list. This will ensure that statuses + // seen later can overide a previous status and that statuses seen later do not override if + // they would lower the overall status such as going from critical -> warning. + precedenceHealth := []pbcatalog.Health{ + pbcatalog.Health_HEALTH_PASSING, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_MAINTENANCE, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_PASSING, + } - for _, node := range []*pbresource.ID{suite.nodePassing[tenancyString], suite.nodeWarning[tenancyString], suite.nodeCritical[tenancyString], suite.nodeMaintenance[tenancyString]} { - for idx, health := range precedenceHealth { - if nodeHealthDesiredStatus[node.Name] >= health { - resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d-%s-%s", node.Name, idx, tenancy.Partition, tenancy.Namespace)). - WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). - WithOwner(node). - Write(suite.T(), suite.resourceClient) - } + for _, node := range []*pbresource.ID{suite.nodePassing, suite.nodeWarning, suite.nodeCritical, suite.nodeMaintenance} { + for idx, health := range precedenceHealth { + if nodeHealthDesiredStatus[node.Name] >= health { + resourcetest.Resource(pbcatalog.HealthStatusType, fmt.Sprintf("test-check-%s-%d-%s-%s", node.Name, idx, tenancy.Partition, tenancy.Namespace)). + WithData(suite.T(), &pbcatalog.HealthStatus{Type: "tcp", Status: health}). + WithOwner(node). + Write(suite.T(), suite.resourceClient) } } + } - // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs - // to be something that doesn't care about its owner. All we want to prove is that we are - // filtering out non-HealthStatus types appropriately. - resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy-"+tenancy.Partition+"-"+tenancy.Namespace). - WithData(suite.T(), dnsPolicyData). - WithOwner(suite.nodeNoHealth[tenancyString]). - WithTenancy(tenancy). - Write(suite.T(), suite.resourceClient) + // create a DNSPolicy to be owned by the node. The type doesn't really matter it just needs + // to be something that doesn't care about its owner. All we want to prove is that we are + // filtering out non-HealthStatus types appropriately. + resourcetest.Resource(pbcatalog.DNSPolicyType, "test-policy-"+tenancy.Partition+"-"+tenancy.Namespace). + WithData(suite.T(), dnsPolicyData). + WithOwner(suite.nodeNoHealth). + WithTenancy(tenancy). + Write(suite.T(), suite.resourceClient) +} + +func (suite *nodeHealthControllerTestSuite) cleanUpNodes() { + req := &pbresource.DeleteRequest{ + Id: suite.nodeNoHealth, } + _, err := suite.resourceClient.Delete(context.Background(), req) + require.NoError(suite.T(), err) + + req = &pbresource.DeleteRequest{ + Id: suite.nodeCritical, + } + _, err = suite.resourceClient.Delete(context.Background(), req) + require.NoError(suite.T(), err) + + req = &pbresource.DeleteRequest{ + Id: suite.nodeWarning, + } + _, err = suite.resourceClient.Delete(context.Background(), req) + require.NoError(suite.T(), err) + + req = &pbresource.DeleteRequest{ + Id: suite.nodePassing, + } + _, err = suite.resourceClient.Delete(context.Background(), req) + require.NoError(suite.T(), err) + + req = &pbresource.DeleteRequest{ + Id: suite.nodeMaintenance, + } + _, err = suite.resourceClient.Delete(context.Background(), req) + require.NoError(suite.T(), err) } func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(t func(*pbresource.Tenancy)) { - suite.setupNodesWithTenancy(suite.tenancies) for _, tenancy := range suite.tenancies { + suite.setupNodesWithTenancy(tenancy) suite.Run(suite.appendTenancyInfo(tenancy), func() { t(tenancy) }) + suite.cleanUpNodes() } } diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index 74c987507bbb..3dfbe5499e1f 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -38,18 +38,6 @@ func Tenancy(s string) *pbresource.Tenancy { } } -func ToTenancyString(tenancy *pbresource.Tenancy) string { - tenancyString := "" - if tenancy.Partition != "" { - tenancyString += tenancy.Partition - } - if tenancy.Namespace != "" { - tenancyString += "." - tenancyString += tenancy.Namespace - } - return tenancyString -} - func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource.Tenancy { switch reg.Scope { case resource.ScopeNamespace: From efde78271b438d91995afd3e93679b9a58aa7cc6 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Wed, 8 Nov 2023 12:15:42 +0530 Subject: [PATCH 8/9] some fixes --- .../controllers/nodehealth/controller_test.go | 45 +++++-------------- 1 file changed, 12 insertions(+), 33 deletions(-) diff --git a/internal/catalog/internal/controllers/nodehealth/controller_test.go b/internal/catalog/internal/controllers/nodehealth/controller_test.go index 4b512f6222ac..f032225a2728 100644 --- a/internal/catalog/internal/controllers/nodehealth/controller_test.go +++ b/internal/catalog/internal/controllers/nodehealth/controller_test.go @@ -58,7 +58,7 @@ func resourceID(rtype *pbresource.Type, name string, tenancy *pbresource.Tenancy type nodeHealthControllerTestSuite struct { suite.Suite - resourceClient pbresource.ResourceServiceClient + resourceClient *resourcetest.Client runtime controller.Runtime ctl nodeHealthReconciler @@ -91,7 +91,8 @@ func (suite *nodeHealthControllerTestSuite) SetupTest() { cfg := mockres.Config{ TenancyBridge: mockTenancyBridge, } - suite.resourceClient = svctest.RunResourceServiceWithConfig(suite.T(), cfg, types.Register, types.RegisterDNSPolicy) + client := svctest.RunResourceServiceWithConfig(suite.T(), cfg, types.Register, types.RegisterDNSPolicy) + suite.resourceClient = resourcetest.NewClient(client) suite.runtime = controller.Runtime{Client: suite.resourceClient, Logger: testutil.Logger(suite.T())} suite.isEnterprise = structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" } @@ -424,43 +425,21 @@ func (suite *nodeHealthControllerTestSuite) setupNodesWithTenancy(tenancy *pbres } func (suite *nodeHealthControllerTestSuite) cleanUpNodes() { - req := &pbresource.DeleteRequest{ - Id: suite.nodeNoHealth, - } - _, err := suite.resourceClient.Delete(context.Background(), req) - require.NoError(suite.T(), err) - - req = &pbresource.DeleteRequest{ - Id: suite.nodeCritical, - } - _, err = suite.resourceClient.Delete(context.Background(), req) - require.NoError(suite.T(), err) - - req = &pbresource.DeleteRequest{ - Id: suite.nodeWarning, - } - _, err = suite.resourceClient.Delete(context.Background(), req) - require.NoError(suite.T(), err) - - req = &pbresource.DeleteRequest{ - Id: suite.nodePassing, - } - _, err = suite.resourceClient.Delete(context.Background(), req) - require.NoError(suite.T(), err) - - req = &pbresource.DeleteRequest{ - Id: suite.nodeMaintenance, - } - _, err = suite.resourceClient.Delete(context.Background(), req) - require.NoError(suite.T(), err) + suite.resourceClient.MustDelete(suite.T(), suite.nodeNoHealth) + suite.resourceClient.MustDelete(suite.T(), suite.nodeCritical) + suite.resourceClient.MustDelete(suite.T(), suite.nodeWarning) + suite.resourceClient.MustDelete(suite.T(), suite.nodePassing) + suite.resourceClient.MustDelete(suite.T(), suite.nodeMaintenance) } func (suite *nodeHealthControllerTestSuite) runTestCaseWithTenancies(t func(*pbresource.Tenancy)) { for _, tenancy := range suite.tenancies { - suite.setupNodesWithTenancy(tenancy) suite.Run(suite.appendTenancyInfo(tenancy), func() { + suite.setupNodesWithTenancy(tenancy) + suite.T().Cleanup(func() { + suite.cleanUpNodes() + }) t(tenancy) }) - suite.cleanUpNodes() } } From 9cdf3131dfd6eac76029f7e6b6f1c623a16397d2 Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Wed, 8 Nov 2023 12:26:43 +0530 Subject: [PATCH 9/9] merge main --- internal/resource/resourcetest/tenancy.go | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index ca5b3487963e..826b20d17d6f 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -8,7 +8,6 @@ import ( "strings" "testing" - "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/proto-public/pbresource" ) @@ -16,7 +15,7 @@ import ( // TestTenancies returns a list of tenancies which represent // the namespace and partition combinations that can be used in unit tests func TestTenancies() []*pbresource.Tenancy { - isEnterprise := (structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default") + isEnterprise := structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" tenancies := []*pbresource.Tenancy{Tenancy("default.default")} if isEnterprise { @@ -65,16 +64,3 @@ func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource. return nil } } - -// TestTenancies returns a list of tenancies which represent -// the namespace and partition combinations that can be used in unit tests -func TestTenancies() []*pbresource.Tenancy { - isEnterprise := structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty() == "default" - - tenancies := []*pbresource.Tenancy{Tenancy("default.default")} - if isEnterprise { - tenancies = append(tenancies, Tenancy("default.bar"), Tenancy("foo.default"), Tenancy("foo.bar")) - } - - return tenancies -}