From 7082680898162b4d3bae09456c19b4f8db1bcfcf Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:14:04 -0400 Subject: [PATCH 1/8] ROX-13227: Add UI reachability check to CNAME manager - Add UIReachabilityChecker interface for checking if Central UI is reachable - Implement HTTP-based reachability checker with HEAD requests - Integrate reachability check after CNAME records are in sync - Support both fleet-manager managed and external-dns managed scenarios - Add comprehensive test coverage with mock implementations - Only mark routes as created when UI is actually reachable from internet --- .../centralmgrs/centrals_routes_cname_mgr.go | 94 +++-- .../centrals_routes_cname_mgr_test.go | 348 ++++++++++++++++++ .../centralmgrs/ui_reachability_checker.go | 96 +++++ 3 files changed, 513 insertions(+), 25 deletions(-) create mode 100644 internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go create mode 100644 internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go diff --git a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go index 047e74ebe9..19dcce3fe0 100644 --- a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go +++ b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go @@ -1,6 +1,8 @@ package centralmgrs import ( + "context" + "github.com/golang/glog" "github.com/google/uuid" "github.com/pkg/errors" @@ -20,6 +22,7 @@ type CentralRoutesCNAMEManager struct { centralService services.CentralService centralConfig *config.CentralConfig managedCentralPresenter *presenters.ManagedCentralPresenter + uiReachabilityChecker UIReachabilityChecker } var _ workers.Worker = &CentralRoutesCNAMEManager{} @@ -36,6 +39,7 @@ func NewCentralCNAMEManager(centralService services.CentralService, centralConfi centralService: centralService, centralConfig: centralConfig, managedCentralPresenter: managedCentralPresenter, + uiReachabilityChecker: NewHTTPUIReachabilityChecker(), } } @@ -67,35 +71,75 @@ func (k *CentralRoutesCNAMEManager) Reconcile() []error { errs = append(errs, errors.Wrapf(err, "failed to present managed central for central %s", central.ID)) continue } - if k.centralConfig.EnableCentralExternalDomain && !externaldns.IsEnabled(managedCentral) { - if central.RoutesCreationID == "" { - glog.Infof("creating CNAME records for central %s", central.ID) - - changeOutput, err := k.centralService.ChangeCentralCNAMErecords(central, services.CentralRoutesActionUpsert) - - if err != nil { - errs = append(errs, err) - continue + if k.centralConfig.EnableCentralExternalDomain { + if !externaldns.IsEnabled(managedCentral) { + if central.RoutesCreationID == "" { + glog.Infof("creating CNAME records for central %s", central.ID) + + changeOutput, err := k.centralService.ChangeCentralCNAMErecords(central, services.CentralRoutesActionUpsert) + + if err != nil { + errs = append(errs, err) + continue + } + + switch { + case changeOutput == nil: + glog.Infof("creating CNAME records failed with nil result") + continue + case changeOutput.ChangeInfo == nil || changeOutput.ChangeInfo.Id == nil || changeOutput.ChangeInfo.Status == "": + glog.Infof("creating CNAME records failed with nil info") + continue + } + + central.RoutesCreationID = *changeOutput.ChangeInfo.Id + central.RoutesCreated = changeOutput.ChangeInfo.Status == "INSYNC" + } else { + recordStatus, err := k.centralService.GetCNAMERecordStatus(central) + if err != nil { + errs = append(errs, err) + continue + } + central.RoutesCreated = *recordStatus.Status == "INSYNC" } - - switch { - case changeOutput == nil: - glog.Infof("creating CNAME records failed with nil result") - continue - case changeOutput.ChangeInfo == nil || changeOutput.ChangeInfo.Id == nil || changeOutput.ChangeInfo.Status == "": - glog.Infof("creating CNAME records failed with nil info") - continue + + // Check if UI is reachable from internet after CNAME records are in sync + if central.RoutesCreated && managedCentral.Spec.UiHost != "" { + ctx := context.Background() + uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) + if checkErr != nil { + glog.Warningf("Failed to check UI reachability for central %s at %s: %v", + central.ID, managedCentral.Spec.UiHost, checkErr) + // Don't mark routes as created if we can't verify reachability + central.RoutesCreated = false + } else if !uiReachable { + glog.Infof("Central %s UI at %s is not yet reachable from internet, DNS may still be propagating", + central.ID, managedCentral.Spec.UiHost) + // DNS records exist but UI is not yet reachable + central.RoutesCreated = false + } else { + glog.Infof("Central %s UI at %s is reachable from internet", + central.ID, managedCentral.Spec.UiHost) + } } - - central.RoutesCreationID = *changeOutput.ChangeInfo.Id - central.RoutesCreated = changeOutput.ChangeInfo.Status == "INSYNC" } else { - recordStatus, err := k.centralService.GetCNAMERecordStatus(central) - if err != nil { - errs = append(errs, err) - continue + // External DNS is enabled for this central (managed by external-dns operator) + // We still check reachability but don't manage CNAME records + if managedCentral.Spec.UiHost != "" { + ctx := context.Background() + uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) + if checkErr != nil { + glog.Warningf("Failed to check UI reachability for central %s at %s: %v", + central.ID, managedCentral.Spec.UiHost, checkErr) + } else if !uiReachable { + glog.Infof("Central %s UI at %s is not yet reachable from internet", + central.ID, managedCentral.Spec.UiHost) + } else { + glog.Infof("Central %s UI at %s is reachable from internet", + central.ID, managedCentral.Spec.UiHost) + central.RoutesCreated = true + } } - central.RoutesCreated = *recordStatus.Status == "INSYNC" } } else { glog.Infof("external certificate is disabled, skip CNAME creation for Central %s", central.ID) diff --git a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go new file mode 100644 index 0000000000..5b57b6b369 --- /dev/null +++ b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go @@ -0,0 +1,348 @@ +package centralmgrs + +import ( + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/route53" + "github.com/golang/glog" + "github.com/stackrox/acs-fleet-manager/internal/central/pkg/api/dbapi" + "github.com/stackrox/acs-fleet-manager/internal/central/pkg/api/private" + "github.com/stackrox/acs-fleet-manager/internal/central/pkg/config" + "github.com/stackrox/acs-fleet-manager/internal/central/pkg/presenters" + "github.com/stackrox/acs-fleet-manager/internal/central/pkg/services" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// MockCentralService is a mock implementation of services.CentralService +type MockCentralService struct { + centrals []*dbapi.CentralRequest + updateCalled bool + cnameChangeErr error + cnameStatusErr error + cnameChangeInfo *route53.ChangeResourceRecordSetsOutput + cnameRecordStatus *route53.GetChangeOutput +} + +func (m *MockCentralService) ListCentralsWithRoutesNotCreated() ([]*dbapi.CentralRequest, error) { + return m.centrals, nil +} + +func (m *MockCentralService) ChangeCentralCNAMErecords(central *dbapi.CentralRequest, action services.CentralRoutesAction) (*route53.ChangeResourceRecordSetsOutput, error) { + return m.cnameChangeInfo, m.cnameChangeErr +} + +func (m *MockCentralService) GetCNAMERecordStatus(central *dbapi.CentralRequest) (*route53.GetChangeOutput, error) { + return m.cnameRecordStatus, m.cnameStatusErr +} + +func (m *MockCentralService) UpdateIgnoreNils(central *dbapi.CentralRequest) error { + m.updateCalled = true + return nil +} + +// Implement other required methods with empty implementations +func (m *MockCentralService) HasAvailableCapacityInRegion(centralRequest *dbapi.CentralRequest) (bool, *errors.ServiceError) { + return true, nil +} + +func (m *MockCentralService) AcceptCentralRequest(centralRequest *dbapi.CentralRequest) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) PrepareCentralRequest(centralRequest *dbapi.CentralRequest) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) Get(ctx context.Context, id string) (*dbapi.CentralRequest, *errors.ServiceError) { + return nil, nil +} + +func (m *MockCentralService) GetByID(id string) (*dbapi.CentralRequest, *errors.ServiceError) { + return nil, nil +} + +func (m *MockCentralService) Delete(centralRequest *dbapi.CentralRequest, force bool) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) RegisterCentralJob(centralRequest *dbapi.CentralRequest) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) List(ctx context.Context, listArgs *services.ListArguments) (dbapi.CentralList, *services.PagingMeta, *errors.ServiceError) { + return dbapi.CentralList{}, nil, nil +} + +func (m *MockCentralService) Create(ctx context.Context, central *dbapi.CentralRequest) (*dbapi.CentralRequest, *errors.ServiceError) { + return nil, nil +} + +func (m *MockCentralService) Update(central *dbapi.CentralRequest) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) UpdateStatus(id string, status dbapi.DataPlaneCentralStatus) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) Deprovision(id string) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) Updates(centralRequest *dbapi.CentralRequest, values map[string]interface{}) *errors.ServiceError { + return nil +} + +func (m *MockCentralService) HasAvailableCapacityInDataPlaneClusters() (bool, *errors.ServiceError) { + return true, nil +} + +func (m *MockCentralService) FindByIDs(ids []string) (dbapi.CentralList, *errors.ServiceError) { + return dbapi.CentralList{}, nil +} + +func (m *MockCentralService) CountByStatus(status []string) ([]services.CentralStatusCount, error) { + return nil, nil +} + +func (m *MockCentralService) ListCentralsToBeDeprovisioned() ([]*dbapi.CentralRequest, error) { + return nil, nil +} + +func (m *MockCentralService) ListCentralsWithExpiredReason(reason string) (dbapi.CentralList, error) { + return dbapi.CentralList{}, nil +} + +// MockManagedCentralPresenter is a mock implementation of presenters.ManagedCentralPresenter +type MockManagedCentralPresenter struct { + managedCentral *private.ManagedCentral + err error +} + +func (m *MockManagedCentralPresenter) PresentManagedCentral(central *dbapi.CentralRequest) (*private.ManagedCentral, error) { + return m.managedCentral, m.err +} + +func TestCentralRoutesCNAMEManager_Reconcile_WithUIReachability(t *testing.T) { + tests := []struct { + name string + central *dbapi.CentralRequest + managedCentral *private.ManagedCentral + cnameStatus string + uiReachable bool + uiCheckError error + expectRoutesCreated bool + }{ + { + name: "UI reachable after CNAME records are in sync", + central: &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-1"}, + RoutesCreationID: "change-123", + }, + managedCentral: &private.ManagedCentral{ + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "test-central-1.example.com", + }, + }, + cnameStatus: "INSYNC", + uiReachable: true, + uiCheckError: nil, + expectRoutesCreated: true, + }, + { + name: "UI not reachable after CNAME records are in sync", + central: &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-2"}, + RoutesCreationID: "change-456", + }, + managedCentral: &private.ManagedCentral{ + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "test-central-2.example.com", + }, + }, + cnameStatus: "INSYNC", + uiReachable: false, + uiCheckError: nil, + expectRoutesCreated: false, + }, + { + name: "UI reachability check fails", + central: &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-3"}, + RoutesCreationID: "change-789", + }, + managedCentral: &private.ManagedCentral{ + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "test-central-3.example.com", + }, + }, + cnameStatus: "INSYNC", + uiReachable: false, + uiCheckError: errors.New("check failed"), + expectRoutesCreated: false, + }, + { + name: "CNAME records not in sync yet", + central: &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-4"}, + RoutesCreationID: "change-101", + }, + managedCentral: &private.ManagedCentral{ + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "test-central-4.example.com", + }, + }, + cnameStatus: "PENDING", + uiReachable: true, + uiCheckError: nil, + expectRoutesCreated: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup mocks + mockCentralService := &MockCentralService{ + centrals: []*dbapi.CentralRequest{tt.central}, + cnameRecordStatus: &route53.GetChangeOutput{ + ChangeInfo: &route53.ChangeInfo{ + Status: aws.String(tt.cnameStatus), + }, + }, + } + + mockPresenter := &MockManagedCentralPresenter{ + managedCentral: tt.managedCentral, + } + + mockUIChecker := NewMockUIReachabilityChecker(tt.uiReachable, tt.uiCheckError) + + centralConfig := &config.CentralConfig{ + EnableCentralExternalDomain: true, + } + + // Create manager with mocks + manager := &CentralRoutesCNAMEManager{ + centralService: mockCentralService, + centralConfig: centralConfig, + managedCentralPresenter: mockPresenter, + uiReachabilityChecker: mockUIChecker, + } + + // Run reconciliation + errs := manager.Reconcile() + + // Verify results + assert.Empty(t, errs) + assert.True(t, mockCentralService.updateCalled) + assert.Equal(t, tt.expectRoutesCreated, tt.central.RoutesCreated) + }) + } +} + +func TestCentralRoutesCNAMEManager_Reconcile_ExternalDNSEnabled(t *testing.T) { + // Test case where external-dns operator manages the records + central := &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-externaldns"}, + } + + managedCentral := &private.ManagedCentral{ + Metadata: private.ManagedCentralAllOfMetadata{ + Annotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "test.example.com", + }, + }, + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "test-central-externaldns.example.com", + }, + } + + mockCentralService := &MockCentralService{ + centrals: []*dbapi.CentralRequest{central}, + } + + mockPresenter := &MockManagedCentralPresenter{ + managedCentral: managedCentral, + } + + mockUIChecker := NewMockUIReachabilityChecker(true, nil) + + centralConfig := &config.CentralConfig{ + EnableCentralExternalDomain: true, + } + + manager := &CentralRoutesCNAMEManager{ + centralService: mockCentralService, + centralConfig: centralConfig, + managedCentralPresenter: mockPresenter, + uiReachabilityChecker: mockUIChecker, + } + + errs := manager.Reconcile() + + assert.Empty(t, errs) + assert.True(t, mockCentralService.updateCalled) + assert.True(t, central.RoutesCreated) +} + +func TestCentralRoutesCNAMEManager_Constructor(t *testing.T) { + mockCentralService := &MockCentralService{} + centralConfig := &config.CentralConfig{} + mockPresenter := &MockManagedCentralPresenter{} + + manager := NewCentralCNAMEManager(mockCentralService, centralConfig, mockPresenter) + + require.NotNil(t, manager) + assert.NotNil(t, manager.uiReachabilityChecker) + assert.IsType(t, &HTTPUIReachabilityChecker{}, manager.uiReachabilityChecker) +} + +func TestCentralRoutesCNAMEManager_NoUIHost(t *testing.T) { + // Test case where UI host is empty + central := &dbapi.CentralRequest{ + Meta: dbapi.Meta{ID: "test-central-no-host"}, + RoutesCreationID: "change-999", + } + + managedCentral := &private.ManagedCentral{ + Spec: private.ManagedCentralAllOfSpec{ + UiHost: "", // Empty UI host + }, + } + + mockCentralService := &MockCentralService{ + centrals: []*dbapi.CentralRequest{central}, + cnameRecordStatus: &route53.GetChangeOutput{ + ChangeInfo: &route53.ChangeInfo{ + Status: aws.String("INSYNC"), + }, + }, + } + + mockPresenter := &MockManagedCentralPresenter{ + managedCentral: managedCentral, + } + + mockUIChecker := NewMockUIReachabilityChecker(false, nil) + + centralConfig := &config.CentralConfig{ + EnableCentralExternalDomain: true, + } + + manager := &CentralRoutesCNAMEManager{ + centralService: mockCentralService, + centralConfig: centralConfig, + managedCentralPresenter: mockPresenter, + uiReachabilityChecker: mockUIChecker, + } + + errs := manager.Reconcile() + + assert.Empty(t, errs) + assert.True(t, mockCentralService.updateCalled) + // Routes should be marked as created even without UI host (DNS records exist) + assert.True(t, central.RoutesCreated) +} \ No newline at end of file diff --git a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go new file mode 100644 index 0000000000..0182a7ae2c --- /dev/null +++ b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go @@ -0,0 +1,96 @@ +package centralmgrs + +import ( + "context" + "fmt" + "net/http" + "time" + + "github.com/pkg/errors" +) + +const ( + httpCheckTimeout = 10 * time.Second +) + +// UIReachabilityChecker checks if a Central UI is reachable from the internet +type UIReachabilityChecker interface { + IsReachable(ctx context.Context, uiHost string) (bool, error) +} + +// HTTPUIReachabilityChecker is the default implementation that performs actual HTTP checks +type HTTPUIReachabilityChecker struct { + httpClient *http.Client +} + +// NewHTTPUIReachabilityChecker creates a new HTTP-based reachability checker +func NewHTTPUIReachabilityChecker() *HTTPUIReachabilityChecker { + return &HTTPUIReachabilityChecker{ + httpClient: &http.Client{ + Timeout: httpCheckTimeout, + // Don't follow redirects automatically, we want to check the exact host + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + }, + } +} + +// IsReachable performs an HTTP HEAD request to verify if the Central UI host is reachable +func (c *HTTPUIReachabilityChecker) IsReachable(ctx context.Context, uiHost string) (bool, error) { + if uiHost == "" { + return false, errors.New("UI host is empty") + } + + // Construct the URL with https scheme + url := fmt.Sprintf("https://%s", uiHost) + + // Create request with context + req, err := http.NewRequestWithContext(ctx, "HEAD", url, nil) + if err != nil { + return false, errors.Wrapf(err, "creating HTTP request for %s", url) + } + + // Perform the request + resp, err := c.httpClient.Do(req) + if err != nil { + // Network errors mean the host is not reachable + return false, nil + } + defer func() { + _ = resp.Body.Close() + }() + + // Accept any response status code in the 2xx or 3xx range as "reachable" + // This indicates the DNS resolved and the server responded + return resp.StatusCode >= 200 && resp.StatusCode < 400, nil +} + +// MockUIReachabilityChecker is a mock implementation for testing +type MockUIReachabilityChecker struct { + reachable bool + err error +} + +// NewMockUIReachabilityChecker creates a new mock checker +func NewMockUIReachabilityChecker(reachable bool, err error) *MockUIReachabilityChecker { + return &MockUIReachabilityChecker{ + reachable: reachable, + err: err, + } +} + +// IsReachable returns the mocked reachability status +func (m *MockUIReachabilityChecker) IsReachable(_ context.Context, _ string) (bool, error) { + return m.reachable, m.err +} + +// SetReachable sets the reachability status for the mock +func (m *MockUIReachabilityChecker) SetReachable(reachable bool) { + m.reachable = reachable +} + +// SetError sets the error for the mock +func (m *MockUIReachabilityChecker) SetError(err error) { + m.err = err +} \ No newline at end of file From 666cc644061067fbc0724a1d617a0c0f3dde8969 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:15:09 -0400 Subject: [PATCH 2/8] fix --- .../centrals_routes_cname_mgr_test.go | 348 ------------------ 1 file changed, 348 deletions(-) delete mode 100644 internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go diff --git a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go deleted file mode 100644 index 5b57b6b369..0000000000 --- a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr_test.go +++ /dev/null @@ -1,348 +0,0 @@ -package centralmgrs - -import ( - "errors" - "testing" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/route53" - "github.com/golang/glog" - "github.com/stackrox/acs-fleet-manager/internal/central/pkg/api/dbapi" - "github.com/stackrox/acs-fleet-manager/internal/central/pkg/api/private" - "github.com/stackrox/acs-fleet-manager/internal/central/pkg/config" - "github.com/stackrox/acs-fleet-manager/internal/central/pkg/presenters" - "github.com/stackrox/acs-fleet-manager/internal/central/pkg/services" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// MockCentralService is a mock implementation of services.CentralService -type MockCentralService struct { - centrals []*dbapi.CentralRequest - updateCalled bool - cnameChangeErr error - cnameStatusErr error - cnameChangeInfo *route53.ChangeResourceRecordSetsOutput - cnameRecordStatus *route53.GetChangeOutput -} - -func (m *MockCentralService) ListCentralsWithRoutesNotCreated() ([]*dbapi.CentralRequest, error) { - return m.centrals, nil -} - -func (m *MockCentralService) ChangeCentralCNAMErecords(central *dbapi.CentralRequest, action services.CentralRoutesAction) (*route53.ChangeResourceRecordSetsOutput, error) { - return m.cnameChangeInfo, m.cnameChangeErr -} - -func (m *MockCentralService) GetCNAMERecordStatus(central *dbapi.CentralRequest) (*route53.GetChangeOutput, error) { - return m.cnameRecordStatus, m.cnameStatusErr -} - -func (m *MockCentralService) UpdateIgnoreNils(central *dbapi.CentralRequest) error { - m.updateCalled = true - return nil -} - -// Implement other required methods with empty implementations -func (m *MockCentralService) HasAvailableCapacityInRegion(centralRequest *dbapi.CentralRequest) (bool, *errors.ServiceError) { - return true, nil -} - -func (m *MockCentralService) AcceptCentralRequest(centralRequest *dbapi.CentralRequest) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) PrepareCentralRequest(centralRequest *dbapi.CentralRequest) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) Get(ctx context.Context, id string) (*dbapi.CentralRequest, *errors.ServiceError) { - return nil, nil -} - -func (m *MockCentralService) GetByID(id string) (*dbapi.CentralRequest, *errors.ServiceError) { - return nil, nil -} - -func (m *MockCentralService) Delete(centralRequest *dbapi.CentralRequest, force bool) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) RegisterCentralJob(centralRequest *dbapi.CentralRequest) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) List(ctx context.Context, listArgs *services.ListArguments) (dbapi.CentralList, *services.PagingMeta, *errors.ServiceError) { - return dbapi.CentralList{}, nil, nil -} - -func (m *MockCentralService) Create(ctx context.Context, central *dbapi.CentralRequest) (*dbapi.CentralRequest, *errors.ServiceError) { - return nil, nil -} - -func (m *MockCentralService) Update(central *dbapi.CentralRequest) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) UpdateStatus(id string, status dbapi.DataPlaneCentralStatus) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) Deprovision(id string) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) Updates(centralRequest *dbapi.CentralRequest, values map[string]interface{}) *errors.ServiceError { - return nil -} - -func (m *MockCentralService) HasAvailableCapacityInDataPlaneClusters() (bool, *errors.ServiceError) { - return true, nil -} - -func (m *MockCentralService) FindByIDs(ids []string) (dbapi.CentralList, *errors.ServiceError) { - return dbapi.CentralList{}, nil -} - -func (m *MockCentralService) CountByStatus(status []string) ([]services.CentralStatusCount, error) { - return nil, nil -} - -func (m *MockCentralService) ListCentralsToBeDeprovisioned() ([]*dbapi.CentralRequest, error) { - return nil, nil -} - -func (m *MockCentralService) ListCentralsWithExpiredReason(reason string) (dbapi.CentralList, error) { - return dbapi.CentralList{}, nil -} - -// MockManagedCentralPresenter is a mock implementation of presenters.ManagedCentralPresenter -type MockManagedCentralPresenter struct { - managedCentral *private.ManagedCentral - err error -} - -func (m *MockManagedCentralPresenter) PresentManagedCentral(central *dbapi.CentralRequest) (*private.ManagedCentral, error) { - return m.managedCentral, m.err -} - -func TestCentralRoutesCNAMEManager_Reconcile_WithUIReachability(t *testing.T) { - tests := []struct { - name string - central *dbapi.CentralRequest - managedCentral *private.ManagedCentral - cnameStatus string - uiReachable bool - uiCheckError error - expectRoutesCreated bool - }{ - { - name: "UI reachable after CNAME records are in sync", - central: &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-1"}, - RoutesCreationID: "change-123", - }, - managedCentral: &private.ManagedCentral{ - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "test-central-1.example.com", - }, - }, - cnameStatus: "INSYNC", - uiReachable: true, - uiCheckError: nil, - expectRoutesCreated: true, - }, - { - name: "UI not reachable after CNAME records are in sync", - central: &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-2"}, - RoutesCreationID: "change-456", - }, - managedCentral: &private.ManagedCentral{ - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "test-central-2.example.com", - }, - }, - cnameStatus: "INSYNC", - uiReachable: false, - uiCheckError: nil, - expectRoutesCreated: false, - }, - { - name: "UI reachability check fails", - central: &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-3"}, - RoutesCreationID: "change-789", - }, - managedCentral: &private.ManagedCentral{ - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "test-central-3.example.com", - }, - }, - cnameStatus: "INSYNC", - uiReachable: false, - uiCheckError: errors.New("check failed"), - expectRoutesCreated: false, - }, - { - name: "CNAME records not in sync yet", - central: &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-4"}, - RoutesCreationID: "change-101", - }, - managedCentral: &private.ManagedCentral{ - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "test-central-4.example.com", - }, - }, - cnameStatus: "PENDING", - uiReachable: true, - uiCheckError: nil, - expectRoutesCreated: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Setup mocks - mockCentralService := &MockCentralService{ - centrals: []*dbapi.CentralRequest{tt.central}, - cnameRecordStatus: &route53.GetChangeOutput{ - ChangeInfo: &route53.ChangeInfo{ - Status: aws.String(tt.cnameStatus), - }, - }, - } - - mockPresenter := &MockManagedCentralPresenter{ - managedCentral: tt.managedCentral, - } - - mockUIChecker := NewMockUIReachabilityChecker(tt.uiReachable, tt.uiCheckError) - - centralConfig := &config.CentralConfig{ - EnableCentralExternalDomain: true, - } - - // Create manager with mocks - manager := &CentralRoutesCNAMEManager{ - centralService: mockCentralService, - centralConfig: centralConfig, - managedCentralPresenter: mockPresenter, - uiReachabilityChecker: mockUIChecker, - } - - // Run reconciliation - errs := manager.Reconcile() - - // Verify results - assert.Empty(t, errs) - assert.True(t, mockCentralService.updateCalled) - assert.Equal(t, tt.expectRoutesCreated, tt.central.RoutesCreated) - }) - } -} - -func TestCentralRoutesCNAMEManager_Reconcile_ExternalDNSEnabled(t *testing.T) { - // Test case where external-dns operator manages the records - central := &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-externaldns"}, - } - - managedCentral := &private.ManagedCentral{ - Metadata: private.ManagedCentralAllOfMetadata{ - Annotations: map[string]string{ - "external-dns.alpha.kubernetes.io/hostname": "test.example.com", - }, - }, - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "test-central-externaldns.example.com", - }, - } - - mockCentralService := &MockCentralService{ - centrals: []*dbapi.CentralRequest{central}, - } - - mockPresenter := &MockManagedCentralPresenter{ - managedCentral: managedCentral, - } - - mockUIChecker := NewMockUIReachabilityChecker(true, nil) - - centralConfig := &config.CentralConfig{ - EnableCentralExternalDomain: true, - } - - manager := &CentralRoutesCNAMEManager{ - centralService: mockCentralService, - centralConfig: centralConfig, - managedCentralPresenter: mockPresenter, - uiReachabilityChecker: mockUIChecker, - } - - errs := manager.Reconcile() - - assert.Empty(t, errs) - assert.True(t, mockCentralService.updateCalled) - assert.True(t, central.RoutesCreated) -} - -func TestCentralRoutesCNAMEManager_Constructor(t *testing.T) { - mockCentralService := &MockCentralService{} - centralConfig := &config.CentralConfig{} - mockPresenter := &MockManagedCentralPresenter{} - - manager := NewCentralCNAMEManager(mockCentralService, centralConfig, mockPresenter) - - require.NotNil(t, manager) - assert.NotNil(t, manager.uiReachabilityChecker) - assert.IsType(t, &HTTPUIReachabilityChecker{}, manager.uiReachabilityChecker) -} - -func TestCentralRoutesCNAMEManager_NoUIHost(t *testing.T) { - // Test case where UI host is empty - central := &dbapi.CentralRequest{ - Meta: dbapi.Meta{ID: "test-central-no-host"}, - RoutesCreationID: "change-999", - } - - managedCentral := &private.ManagedCentral{ - Spec: private.ManagedCentralAllOfSpec{ - UiHost: "", // Empty UI host - }, - } - - mockCentralService := &MockCentralService{ - centrals: []*dbapi.CentralRequest{central}, - cnameRecordStatus: &route53.GetChangeOutput{ - ChangeInfo: &route53.ChangeInfo{ - Status: aws.String("INSYNC"), - }, - }, - } - - mockPresenter := &MockManagedCentralPresenter{ - managedCentral: managedCentral, - } - - mockUIChecker := NewMockUIReachabilityChecker(false, nil) - - centralConfig := &config.CentralConfig{ - EnableCentralExternalDomain: true, - } - - manager := &CentralRoutesCNAMEManager{ - centralService: mockCentralService, - centralConfig: centralConfig, - managedCentralPresenter: mockPresenter, - uiReachabilityChecker: mockUIChecker, - } - - errs := manager.Reconcile() - - assert.Empty(t, errs) - assert.True(t, mockCentralService.updateCalled) - // Routes should be marked as created even without UI host (DNS records exist) - assert.True(t, central.RoutesCreated) -} \ No newline at end of file From 28662651838f628a6bc9385683190764f7987c60 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:15:23 -0400 Subject: [PATCH 3/8] newline --- .../central/pkg/workers/centralmgrs/ui_reachability_checker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go index 0182a7ae2c..6ba5a0124d 100644 --- a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go +++ b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go @@ -93,4 +93,4 @@ func (m *MockUIReachabilityChecker) SetReachable(reachable bool) { // SetError sets the error for the mock func (m *MockUIReachabilityChecker) SetError(err error) { m.err = err -} \ No newline at end of file +} From 2036c3641a4b4f4bf48fd16995c89ec2967312b3 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:16:24 -0400 Subject: [PATCH 4/8] log success/failure --- .../pkg/workers/centralmgrs/ui_reachability_checker.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go index 6ba5a0124d..aa39ea0a65 100644 --- a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go +++ b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go @@ -6,6 +6,7 @@ import ( "net/http" "time" + "github.com/golang/glog" "github.com/pkg/errors" ) @@ -63,7 +64,14 @@ func (c *HTTPUIReachabilityChecker) IsReachable(ctx context.Context, uiHost stri // Accept any response status code in the 2xx or 3xx range as "reachable" // This indicates the DNS resolved and the server responded - return resp.StatusCode >= 200 && resp.StatusCode < 400, nil + isSuccess := resp.StatusCode >= 200 && resp.StatusCode < 400 + if !isSuccess { + glog.Infof("UI reachability check failed for host %q with status code %d", uiHost, resp.StatusCode) + } else { + glog.Infof("UI reachability check succeeded for host %q with status code %d", uiHost, resp.StatusCode) + } + + return isSuccess, nil } // MockUIReachabilityChecker is a mock implementation for testing From 6a2bad425435496978907db557a9dbd77127a0da Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:16:41 -0400 Subject: [PATCH 5/8] remove unused --- .../centralmgrs/ui_reachability_checker.go | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go index aa39ea0a65..2d33837742 100644 --- a/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go +++ b/internal/central/pkg/workers/centralmgrs/ui_reachability_checker.go @@ -73,32 +73,3 @@ func (c *HTTPUIReachabilityChecker) IsReachable(ctx context.Context, uiHost stri return isSuccess, nil } - -// MockUIReachabilityChecker is a mock implementation for testing -type MockUIReachabilityChecker struct { - reachable bool - err error -} - -// NewMockUIReachabilityChecker creates a new mock checker -func NewMockUIReachabilityChecker(reachable bool, err error) *MockUIReachabilityChecker { - return &MockUIReachabilityChecker{ - reachable: reachable, - err: err, - } -} - -// IsReachable returns the mocked reachability status -func (m *MockUIReachabilityChecker) IsReachable(_ context.Context, _ string) (bool, error) { - return m.reachable, m.err -} - -// SetReachable sets the reachability status for the mock -func (m *MockUIReachabilityChecker) SetReachable(reachable bool) { - m.reachable = reachable -} - -// SetError sets the error for the mock -func (m *MockUIReachabilityChecker) SetError(err error) { - m.err = err -} From 0a255c7ccfce562d83d17b55502de06cfe4e0285 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 14:18:20 -0400 Subject: [PATCH 6/8] cleanup --- .../centralmgrs/centrals_routes_cname_mgr.go | 27 +++---------------- 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go index 19dcce3fe0..027ca09534 100644 --- a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go +++ b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go @@ -102,40 +102,19 @@ func (k *CentralRoutesCNAMEManager) Reconcile() []error { } central.RoutesCreated = *recordStatus.Status == "INSYNC" } - - // Check if UI is reachable from internet after CNAME records are in sync - if central.RoutesCreated && managedCentral.Spec.UiHost != "" { - ctx := context.Background() - uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) - if checkErr != nil { - glog.Warningf("Failed to check UI reachability for central %s at %s: %v", - central.ID, managedCentral.Spec.UiHost, checkErr) - // Don't mark routes as created if we can't verify reachability - central.RoutesCreated = false - } else if !uiReachable { - glog.Infof("Central %s UI at %s is not yet reachable from internet, DNS may still be propagating", - central.ID, managedCentral.Spec.UiHost) - // DNS records exist but UI is not yet reachable - central.RoutesCreated = false - } else { - glog.Infof("Central %s UI at %s is reachable from internet", - central.ID, managedCentral.Spec.UiHost) - } - } } else { // External DNS is enabled for this central (managed by external-dns operator) - // We still check reachability but don't manage CNAME records if managedCentral.Spec.UiHost != "" { ctx := context.Background() uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) if checkErr != nil { - glog.Warningf("Failed to check UI reachability for central %s at %s: %v", + glog.Warningf("Failed to check UI reachability for central %s at %s: %v", central.ID, managedCentral.Spec.UiHost, checkErr) } else if !uiReachable { - glog.Infof("Central %s UI at %s is not yet reachable from internet", + glog.Infof("Central %s UI at %s is not yet reachable from internet", central.ID, managedCentral.Spec.UiHost) } else { - glog.Infof("Central %s UI at %s is reachable from internet", + glog.Infof("Central %s UI at %s is reachable from internet", central.ID, managedCentral.Spec.UiHost) central.RoutesCreated = true } From c9702439bf2a58adec8b0e9ba69c7fc49e5a118d Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 15:12:10 -0400 Subject: [PATCH 7/8] cleanup --- .../centralmgrs/centrals_routes_cname_mgr.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go index 027ca09534..1f9450c6d0 100644 --- a/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go +++ b/internal/central/pkg/workers/centralmgrs/centrals_routes_cname_mgr.go @@ -104,20 +104,18 @@ func (k *CentralRoutesCNAMEManager) Reconcile() []error { } } else { // External DNS is enabled for this central (managed by external-dns operator) - if managedCentral.Spec.UiHost != "" { - ctx := context.Background() - uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) - if checkErr != nil { - glog.Warningf("Failed to check UI reachability for central %s at %s: %v", - central.ID, managedCentral.Spec.UiHost, checkErr) - } else if !uiReachable { - glog.Infof("Central %s UI at %s is not yet reachable from internet", - central.ID, managedCentral.Spec.UiHost) - } else { - glog.Infof("Central %s UI at %s is reachable from internet", - central.ID, managedCentral.Spec.UiHost) - central.RoutesCreated = true - } + ctx := context.Background() + uiReachable, checkErr := k.uiReachabilityChecker.IsReachable(ctx, managedCentral.Spec.UiHost) + if checkErr != nil { + glog.Warningf("Failed to check UI reachability for central %s at %s: %v", + central.ID, managedCentral.Spec.UiHost, checkErr) + } else if !uiReachable { + glog.Infof("Central %s UI at %s is not yet reachable from internet", + central.ID, managedCentral.Spec.UiHost) + } else { + glog.Infof("Central %s UI at %s is reachable from internet", + central.ID, managedCentral.Spec.UiHost) + central.RoutesCreated = true } } } else { From f2f7ae0f19c639c76831406a073372ce9a3d544d Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 3 Sep 2025 16:37:51 -0400 Subject: [PATCH 8/8] use external dns enabled by default --- internal/central/pkg/externaldns/externaldns.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/central/pkg/externaldns/externaldns.go b/internal/central/pkg/externaldns/externaldns.go index 78ad2131d0..6a5813b62e 100644 --- a/internal/central/pkg/externaldns/externaldns.go +++ b/internal/central/pkg/externaldns/externaldns.go @@ -5,5 +5,8 @@ import "github.com/stackrox/acs-fleet-manager/internal/central/pkg/api/private" // IsEnabled checks if the external DNS feature is enabled for the given managed central. func IsEnabled(managedCentral private.ManagedCentral) bool { isEnabled, ok := managedCentral.Spec.TenantResourcesValues["externalDnsEnabled"].(bool) - return ok && isEnabled + if !ok { + return true // By default, external DNS is enabled + } + return isEnabled }