diff --git a/controllers/cmd/placement/cmd.go b/controllers/cmd/placement/cmd.go index 79042da8..9403c80b 100644 --- a/controllers/cmd/placement/cmd.go +++ b/controllers/cmd/placement/cmd.go @@ -3,6 +3,7 @@ package placement import ( "fmt" "os" + "strings" secretmanager "cloud.google.com/go/secretmanager/apiv1" "github.com/spf13/cobra" @@ -21,10 +22,21 @@ func envOr(key, fallback string) string { return fallback } +// parseDomains splits a comma-separated domain list, trimming whitespace and +// dropping empty entries. +func parseDomains(csv string) []string { + var domains []string + for _, d := range strings.Split(csv, ",") { + if d = strings.TrimSpace(d); d != "" { + domains = append(domains, d) + } + } + return domains +} + // NewCommand returns the placement subcommand. func NewCommand(rf *setup.RootFlags) *cobra.Command { - var candidateNames, baseDomains []string - var smProject, maestroHTTPAddr string + var smProject, hcDNSDomains string cmd := &cobra.Command{ Use: "placement", @@ -33,8 +45,8 @@ func NewCommand(rf *setup.RootFlags) *cobra.Command { if v := envOr("SECRETMANAGER_PROJECT", ""); v != "" && !cmd.Flags().Changed("secretmanager-project") { smProject = v } - if v := envOr("MAESTRO_HTTP_ADDR", ""); v != "" && !cmd.Flags().Changed("maestro-http-addr") { - maestroHTTPAddr = v + if smProject == "" { + return fmt.Errorf("--secretmanager-project is required (or $SECRETMANAGER_PROJECT)") } ctx := cmd.Context() @@ -44,26 +56,15 @@ func NewCommand(rf *setup.RootFlags) *cobra.Command { return fmt.Errorf("create logger: %w", err) } - var selector placement.Selector - var candidates []placement.Candidate - - if smProject != "" { - smClient, err := secretmanager.NewClient(ctx) - if err != nil { - return fmt.Errorf("create secret manager client: %w", err) - } - defer smClient.Close() //nolint:errcheck - selector = placement.NewDynamicSelector(smClient, smProject, maestroHTTPAddr) - } else { - candidates = make([]placement.Candidate, 0, len(candidateNames)) - for i, name := range candidateNames { - c := placement.Candidate{Name: name} - if i < len(baseDomains) { - c.BaseDomains = []string{baseDomains[i]} - } - candidates = append(candidates, c) - } - selector = placement.NewRoundRobinSelector() + smClient, err := secretmanager.NewClient(ctx) + if err != nil { + return fmt.Errorf("create secret manager client: %w", err) + } + defer smClient.Close() //nolint:errcheck + + selector, err := placement.NewDynamicSelector(smClient, smProject, parseDomains(hcDNSDomains)) + if err != nil { + return fmt.Errorf("create selector: %w", err) } scheme := setup.NewScheme() @@ -72,7 +73,7 @@ func NewCommand(rf *setup.RootFlags) *cobra.Command { return fmt.Errorf("create manager: %w", err) } - rec := placement.NewReconciler(selector, candidates, log, mgr.GetClient()) + rec := placement.NewReconciler(selector, log, mgr.GetClient()) if err := ctrl.NewControllerManagedBy(mgr). For(&privatev1.Cluster{}). @@ -85,10 +86,8 @@ func NewCommand(rf *setup.RootFlags) *cobra.Command { }, } - cmd.Flags().StringSliceVar(&candidateNames, "candidates", nil, "MC names (comma-separated); ignored when --secretmanager-project is set") - cmd.Flags().StringSliceVar(&baseDomains, "base-domains", nil, "Base domains per MC, paired with --candidates") - cmd.Flags().StringVar(&smProject, "secretmanager-project", "", "GCP project for Secret Manager MC/DNS discovery [$SECRETMANAGER_PROJECT]; enables dynamic selector") - cmd.Flags().StringVar(&maestroHTTPAddr, "maestro-http-addr", "http://maestro.hyperfleet.svc.cluster.local:8000", "Maestro HTTP API URL for consumer discovery [$MAESTRO_HTTP_ADDR]") + cmd.Flags().StringVar(&smProject, "secretmanager-project", "", "GCP project for Secret Manager mc-registration discovery [$SECRETMANAGER_PROJECT] (required)") + cmd.Flags().StringVar(&hcDNSDomains, "hc-dns-domains", "", "Comma-separated list of HC DNS zone domains to round-robin across") return cmd } diff --git a/controllers/cmd/placement/cmd_test.go b/controllers/cmd/placement/cmd_test.go new file mode 100644 index 00000000..7627a664 --- /dev/null +++ b/controllers/cmd/placement/cmd_test.go @@ -0,0 +1,28 @@ +package placement + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseDomains(t *testing.T) { + tests := []struct { + name string + input string + expect []string + }{ + {"single domain", "example.com", []string{"example.com"}}, + {"multiple domains", "a.example.com,b.example.com", []string{"a.example.com", "b.example.com"}}, + {"trims whitespace", " a.example.com , b.example.com ", []string{"a.example.com", "b.example.com"}}, + {"drops empty entries from trailing comma", "a.example.com,", []string{"a.example.com"}}, + {"drops whitespace-only entries", "a.example.com, , ,b.example.com", []string{"a.example.com", "b.example.com"}}, + {"empty string → nil", "", nil}, + {"only commas → nil", ",,,", nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expect, parseDomains(tc.input)) + }) + } +} diff --git a/controllers/placement/dynamic_selector.go b/controllers/placement/dynamic_selector.go index fc5dd5c6..e4810867 100644 --- a/controllers/placement/dynamic_selector.go +++ b/controllers/placement/dynamic_selector.go @@ -4,9 +4,8 @@ import ( "context" "encoding/json" "fmt" - "io" - "net/http" - "strings" + "log/slog" + "sync" "sync/atomic" "time" @@ -15,6 +14,17 @@ import ( "google.golang.org/api/iterator" ) +// mcRegistrationLabelFilter is the Secret Manager list filter used to discover +// mc-registration-* secrets created by gcp-hcp-infra +// (terraform/modules/management-cluster/region-registration.tf). +const mcRegistrationLabelFilter = "labels.mc-registration=true" + +// Selector abstracts MC + DNS domain selection so the Reconciler can be tested +// without Secret Manager wiring. +type Selector interface { + Select(ctx context.Context) (mcName, baseDomain string, err error) +} + // secretLookup abstracts Secret Manager operations so they can be replaced in // tests without a live GCP connection. type secretLookup interface { @@ -56,193 +66,137 @@ func (r *realSMClient) accessSecretVersion(ctx context.Context, name string) ([] return result.Payload.Data, nil } -// DynamicSelector discovers eligible management clusters and DNS zones at -// selection time by cross-checking Secret Manager secrets against Maestro -// consumers, mirroring the logic of the YAML placement controller's Job script. +// mcRegistrationPayload is the JSON content of an mc-registration-* secret +// version, written by terraform/modules/management-cluster/region-registration.tf. +type mcRegistrationPayload struct { + ProjectID string `json:"projectId"` + Mode string `json:"mode"` // "active" or "maintenance" +} + +// defaultCacheTTL is the default duration eligible MC results are cached before +// re-querying Secret Manager. MC registration changes are infrequent, so 30s +// staleness is acceptable and avoids quota pressure under burst reconciliations. +const defaultCacheTTL = 30 * time.Second + +// DynamicSelector discovers eligible management clusters at selection time by +// reading mc-registration-* Secret Manager secrets, and round-robins across a +// statically configured list of HC DNS zone domains. // // MC discovery: -// 1. List SM secrets with label maestro-consumer-name:* → candidate MC names -// 2. List Maestro consumers → registered MC names -// 3. Eligible = intersection of the two sets +// 1. List SM secrets labeled mc-registration=true +// 2. Access each secret's latest version and parse the JSON payload +// ({"projectId": "...", "mode": "active|maintenance"}) +// 3. Eligible = secrets where mode == "active" // -// DNS zone discovery: -// 1. Find SM secret matching name:argocd-cluster with label infra-type:region -// 2. Access latest version → parse meta_hc_dns_domains (comma-separated) +// DNS domains are injected at construction time (Helm value → CLI flag), +// since they are a region-level resource shared across all MCs, not +// discoverable per-MC. // -// Selection is round-robin across eligible MCs and domains. +// Selection is round-robin across eligible MCs and configured domains. +// Eligible MC results are cached for cacheTTL to reduce Secret Manager RPCs. type DynamicSelector struct { - smLookup secretLookup - project string - maestroURL string - httpClient *http.Client + smLookup secretLookup + project string + domains []string mcCounter atomic.Uint64 domCounter atomic.Uint64 + + cacheMu sync.Mutex + cachedMCs []string + cachedAt time.Time + cacheTTL time.Duration } +// Compile-time check: DynamicSelector implements Selector. +var _ Selector = (*DynamicSelector)(nil) + // NewDynamicSelector creates a DynamicSelector. // project is the GCP project ID for Secret Manager lookups. -// maestroURL is the Maestro HTTP base URL (e.g. "http://maestro.hyperfleet.svc.cluster.local:8000"). -func NewDynamicSelector(smClient *secretmanager.Client, project, maestroURL string) *DynamicSelector { - return &DynamicSelector{ - smLookup: &realSMClient{c: smClient}, - project: project, - maestroURL: maestroURL, - httpClient: &http.Client{Timeout: 30 * time.Second}, +// domains is the list of HC DNS zone domains to round-robin across; it must +// be non-empty. +func NewDynamicSelector(smClient *secretmanager.Client, project string, domains []string) (*DynamicSelector, error) { + if len(domains) == 0 { + return nil, fmt.Errorf("hc dns domains: at least one domain is required") } + return &DynamicSelector{ + smLookup: &realSMClient{c: smClient}, + project: project, + domains: domains, + cacheTTL: defaultCacheTTL, + }, nil } -// Select discovers eligible MCs and DNS zones dynamically, then picks one of -// each using a round-robin counter. The candidates parameter is ignored. -func (s *DynamicSelector) Select(ctx context.Context, _ []Candidate) (mcName, baseDomain string, err error) { +// Select discovers eligible MCs dynamically, then picks one MC and one +// configured DNS domain using round-robin counters. +func (s *DynamicSelector) Select(ctx context.Context) (mcName, baseDomain string, err error) { eligible, err := s.eligibleMCs(ctx) if err != nil { return "", "", fmt.Errorf("discover eligible MCs: %w", err) } if len(eligible) == 0 { - return "", "", fmt.Errorf("no eligible management clusters found (check Secret Manager labels and Maestro consumers)") + return "", "", fmt.Errorf("no eligible management clusters found (check mc-registration secrets and mode=active)") } mc := eligible[s.mcCounter.Add(1)%uint64(len(eligible))] - - domains, err := s.hcDNSDomains(ctx) - if err != nil { - return "", "", fmt.Errorf("discover DNS domains: %w", err) - } - if len(domains) == 0 { - return "", "", fmt.Errorf("no HC DNS domains found in Secret Manager for project %s", s.project) - } - - domain := domains[s.domCounter.Add(1)%uint64(len(domains))] + domain := s.domains[s.domCounter.Add(1)%uint64(len(s.domains))] return mc, domain, nil } -// eligibleMCs returns MC names present in both Secret Manager and Maestro. +// eligibleMCs returns the cached eligible MC list if still valid, otherwise +// re-queries Secret Manager. Individual secret access/unmarshal errors are +// logged and skipped — only a listSecrets failure or zero healthy secrets +// after filtering produces an error. func (s *DynamicSelector) eligibleMCs(ctx context.Context) ([]string, error) { - smMCs, err := s.smMCNames(ctx) - if err != nil { - return nil, fmt.Errorf("secret manager MC lookup: %w", err) - } + s.cacheMu.Lock() + defer s.cacheMu.Unlock() - maestroMCs, err := s.maestroConsumerNames(ctx) - if err != nil { - return nil, fmt.Errorf("maestro consumer lookup: %w", err) + if len(s.cachedMCs) > 0 && time.Since(s.cachedAt) < s.cacheTTL { + return s.cachedMCs, nil } - maestroSet := make(map[string]bool, len(maestroMCs)) - for _, m := range maestroMCs { - maestroSet[m] = true + eligible, err := s.fetchEligibleMCs(ctx) + if err != nil { + return nil, err } - var eligible []string - for _, mc := range smMCs { - if maestroSet[mc] { - eligible = append(eligible, mc) - } - } + s.cachedMCs = eligible + s.cachedAt = time.Now() return eligible, nil } -// smMCNames lists secrets in the project with label maestro-consumer-name:* and -// returns the label values (MC names). -func (s *DynamicSelector) smMCNames(ctx context.Context) ([]string, error) { +// fetchEligibleMCs lists mc-registration-* secrets, reads each secret's latest +// version, and returns the projectId of every secret whose mode is "active". +// Errors reading or parsing individual secrets are logged and skipped. +func (s *DynamicSelector) fetchEligibleMCs(ctx context.Context) ([]string, error) { secrets, err := s.smLookup.listSecrets(ctx, fmt.Sprintf("projects/%s", s.project), - "labels.maestro-consumer-name:*", + mcRegistrationLabelFilter, ) if err != nil { - return nil, fmt.Errorf("list secrets: %w", err) + return nil, fmt.Errorf("list mc-registration secrets: %w", err) } - var names []string + var eligible []string for _, secret := range secrets { - if mc := secret.Labels["maestro-consumer-name"]; mc != "" { - names = append(names, mc) + data, err := s.smLookup.accessSecretVersion(ctx, secret.Name+"/versions/latest") + if err != nil { + slog.WarnContext(ctx, "skipping mc-registration secret: access failed", + "secret", secret.Name, "error", err) + continue } - } - return names, nil -} - -// maestroConsumerNames queries the Maestro HTTP API for registered consumers. -func (s *DynamicSelector) maestroConsumerNames(ctx context.Context) ([]string, error) { - url := strings.TrimRight(s.maestroURL, "/") + "/api/maestro/v1/consumers?size=100" - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return nil, fmt.Errorf("build request: %w", err) - } - req.Header.Set("Accept", "application/json") - - resp, err := s.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("GET consumers: %w", err) - } - defer resp.Body.Close() //nolint:errcheck - - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("read consumers response: %w", err) - } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("GET consumers returned %d: %s", resp.StatusCode, body) - } - var page struct { - Items []struct { - Name string `json:"name"` - } `json:"items"` - } - if err := json.Unmarshal(body, &page); err != nil { - return nil, fmt.Errorf("unmarshal consumers: %w", err) - } - - names := make([]string, 0, len(page.Items)) - for _, c := range page.Items { - if c.Name != "" { - names = append(names, c.Name) + var payload mcRegistrationPayload + if err := json.Unmarshal(data, &payload); err != nil { + slog.WarnContext(ctx, "skipping mc-registration secret: invalid JSON", + "secret", secret.Name, "error", err) + continue } - } - return names, nil -} -// hcDNSDomains reads the argocd-cluster region secret from Secret Manager and -// returns the comma-separated domains from its meta_hc_dns_domains field. -func (s *DynamicSelector) hcDNSDomains(ctx context.Context) ([]string, error) { - secrets, err := s.smLookup.listSecrets(ctx, - fmt.Sprintf("projects/%s", s.project), - `labels.infra-type:region name:argocd-cluster`, - ) - if err != nil { - return nil, fmt.Errorf("list argocd-cluster secrets: %w", err) - } - - var secretName string - for _, secret := range secrets { - secretName = secret.Name - break - } - - if secretName == "" { - return nil, fmt.Errorf("no secret matching name:argocd-cluster with labels.infra-type=region found in project %s", s.project) - } - - data, err := s.smLookup.accessSecretVersion(ctx, secretName+"/versions/latest") - if err != nil { - return nil, fmt.Errorf("access secret %s: %w", secretName, err) - } - - var payload struct { - MetaHCDNSDomains string `json:"meta_hc_dns_domains"` - } - if err := json.Unmarshal(data, &payload); err != nil { - return nil, fmt.Errorf("unmarshal secret payload: %w", err) - } - - var domains []string - for _, d := range strings.Split(payload.MetaHCDNSDomains, ",") { - if d = strings.TrimSpace(d); d != "" { - domains = append(domains, d) + if payload.Mode == "active" && payload.ProjectID != "" { + eligible = append(eligible, payload.ProjectID) } } - return domains, nil + return eligible, nil } diff --git a/controllers/placement/dynamic_selector_test.go b/controllers/placement/dynamic_selector_test.go index 0f2a8f18..f45907af 100644 --- a/controllers/placement/dynamic_selector_test.go +++ b/controllers/placement/dynamic_selector_test.go @@ -4,9 +4,8 @@ import ( "context" "encoding/json" "fmt" - "net/http" - "net/http/httptest" "testing" + "time" "cloud.google.com/go/secretmanager/apiv1/secretmanagerpb" "github.com/stretchr/testify/assert" @@ -17,13 +16,14 @@ import ( // mockSMLookup implements secretLookup for tests. // listResponses maps filter string to the secrets returned for that filter. +// accessResponses/accessErrs map a secret version name to its payload/error. // A single listErr short-circuits all listSecrets calls. type mockSMLookup struct { - listResponses map[string][]*secretmanagerpb.Secret - listErr error - secretData []byte - accessErr error - accessedName string // last name passed to accessSecretVersion + listResponses map[string][]*secretmanagerpb.Secret + listErr error + accessResponses map[string][]byte + accessErrs map[string]error + accessedNames []string // names passed to accessSecretVersion, in order } func (m *mockSMLookup) listSecrets(_ context.Context, _, filter string) ([]*secretmanagerpb.Secret, error) { @@ -34,394 +34,231 @@ func (m *mockSMLookup) listSecrets(_ context.Context, _, filter string) ([]*secr } func (m *mockSMLookup) accessSecretVersion(_ context.Context, name string) ([]byte, error) { - m.accessedName = name - if m.accessErr != nil { - return nil, m.accessErr + m.accessedNames = append(m.accessedNames, name) + if err, ok := m.accessErrs[name]; ok { + return nil, err } - return m.secretData, nil + return m.accessResponses[name], nil } // ─── helpers ────────────────────────────────────────────────────────────────── -const ( - filterMCNames = "labels.maestro-consumer-name:*" - filterArgoCD = `labels.infra-type:region name:argocd-cluster` - testProject = "my-project" -) +const testProject = "my-project" // smSecret builds a minimal secretmanagerpb.Secret with the given name and labels. func smSecret(name string, labels map[string]string) *secretmanagerpb.Secret { return &secretmanagerpb.Secret{Name: name, Labels: labels} } -// maestroResponse encodes a Maestro consumers API response body. -func maestroResponse(names ...string) []byte { - type item struct { - Name string `json:"name"` - } - type page struct { - Items []item `json:"items"` - } - items := make([]item, 0, len(names)) - for _, n := range names { - items = append(items, item{Name: n}) - } - b, _ := json.Marshal(page{Items: items}) - return b -} - -// dnsPayload encodes a Secret Manager payload with the given comma-separated domains. -func dnsPayload(domains string) []byte { - b, _ := json.Marshal(map[string]string{"meta_hc_dns_domains": domains}) +// mcRegistrationJSON encodes an mc-registration secret payload. +func mcRegistrationJSON(projectID, mode string) []byte { + b, _ := json.Marshal(mcRegistrationPayload{ProjectID: projectID, Mode: mode}) return b } // newSelector constructs a DynamicSelector using the given mock SM lookup and -// an HTTP client pointed at the provided test server (may be nil). -func newSelector(sm secretLookup, srv *httptest.Server) *DynamicSelector { - maestroURL := "" - httpClient := &http.Client{} - if srv != nil { - maestroURL = srv.URL - httpClient = srv.Client() - } +// static DNS domain list. Cache TTL is set to zero so every Select() call +// re-fetches unless the test overrides it. +func newSelector(sm secretLookup, domains []string) *DynamicSelector { return &DynamicSelector{ - smLookup: sm, - project: testProject, - maestroURL: maestroURL, - httpClient: httpClient, + smLookup: sm, + project: testProject, + domains: domains, + cacheTTL: 0, // no caching by default in tests } } -// maestroServer returns a test HTTP server that handles the consumers endpoint. -// If statusCode != 200, it returns that code with the given body. -// Otherwise it returns 200 with body. -func maestroServer(t *testing.T, statusCode int, body []byte) *httptest.Server { - t.Helper() - return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "/api/maestro/v1/consumers", r.URL.Path) - w.WriteHeader(statusCode) - _, _ = w.Write(body) - })) +// registrationSecret builds a secret + its accessSecretVersion registration +// for a given mc-registration secret name, project ID, and mode. +func registrationSecret(sm *mockSMLookup, name, projectID, mode string) *secretmanagerpb.Secret { + secret := smSecret(name, map[string]string{"mc-registration": "true"}) + if sm.accessResponses == nil { + sm.accessResponses = map[string][]byte{} + } + sm.accessResponses[name+"/versions/latest"] = mcRegistrationJSON(projectID, mode) + return secret } -// ─── maestroConsumerNames ───────────────────────────────────────────────────── - -func TestDynamicSelector_maestroConsumerNames(t *testing.T) { - ctx := context.Background() - - t.Run("returns consumer names from valid response", func(t *testing.T) { - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-a", "mc-b", "mc-c")) - defer srv.Close() - - s := newSelector(&mockSMLookup{}, srv) - names, err := s.maestroConsumerNames(ctx) - require.NoError(t, err) - assert.Equal(t, []string{"mc-a", "mc-b", "mc-c"}, names) - }) - - t.Run("filters out empty consumer names", func(t *testing.T) { - body := []byte(`{"items":[{"name":"mc-a"},{"name":""},{"name":"mc-b"}]}`) - srv := maestroServer(t, http.StatusOK, body) - defer srv.Close() +// ─── NewDynamicSelector ───────────────────────────────────────────────────────── - s := newSelector(&mockSMLookup{}, srv) - names, err := s.maestroConsumerNames(ctx) - require.NoError(t, err) - assert.Equal(t, []string{"mc-a", "mc-b"}, names) - }) - - t.Run("empty items array → empty slice, no error", func(t *testing.T) { - srv := maestroServer(t, http.StatusOK, maestroResponse()) - defer srv.Close() - - s := newSelector(&mockSMLookup{}, srv) - names, err := s.maestroConsumerNames(ctx) - require.NoError(t, err) - assert.Empty(t, names) - }) - - t.Run("non-200 status → error containing status code", func(t *testing.T) { - srv := maestroServer(t, http.StatusInternalServerError, []byte("oops")) - defer srv.Close() - - s := newSelector(&mockSMLookup{}, srv) - _, err := s.maestroConsumerNames(ctx) +func TestNewDynamicSelector(t *testing.T) { + t.Run("errors when domains empty", func(t *testing.T) { + _, err := NewDynamicSelector(nil, testProject, nil) require.Error(t, err) - assert.Contains(t, err.Error(), "500") }) - t.Run("invalid JSON body → error", func(t *testing.T) { - srv := maestroServer(t, http.StatusOK, []byte("not-json")) - defer srv.Close() - - s := newSelector(&mockSMLookup{}, srv) - _, err := s.maestroConsumerNames(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "unmarshal") - }) - - t.Run("unreachable server → HTTP error", func(t *testing.T) { - s := newSelector(&mockSMLookup{}, nil) - s.maestroURL = "http://127.0.0.1:0" // nothing listening - _, err := s.maestroConsumerNames(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "GET consumers") + t.Run("succeeds with domains", func(t *testing.T) { + s, err := NewDynamicSelector(nil, testProject, []string{"a.example.com"}) + require.NoError(t, err) + assert.Equal(t, []string{"a.example.com"}, s.domains) + assert.Equal(t, testProject, s.project) }) } -// ─── smMCNames ──────────────────────────────────────────────────────────────── +// ─── fetchEligibleMCs ───────────────────────────────────────────────────────── -func TestDynamicSelector_smMCNames(t *testing.T) { +func TestDynamicSelector_fetchEligibleMCs(t *testing.T) { ctx := context.Background() - t.Run("returns MC names from label values", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: { - smSecret("projects/p/secrets/s1", map[string]string{"maestro-consumer-name": "mc-a"}), - smSecret("projects/p/secrets/s2", map[string]string{"maestro-consumer-name": "mc-b"}), - }, - }, + t.Run("returns projectIds for secrets with mode=active", func(t *testing.T) { + sm := &mockSMLookup{} + s1 := registrationSecret(sm, "projects/p/secrets/mc-registration-a", "proj-a", "active") + s2 := registrationSecret(sm, "projects/p/secrets/mc-registration-b", "proj-b", "active") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {s1, s2}, } - s := newSelector(sm, nil) - names, err := s.smMCNames(ctx) + + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) require.NoError(t, err) - assert.Equal(t, []string{"mc-a", "mc-b"}, names) + assert.Equal(t, []string{"proj-a", "proj-b"}, eligible) }) - t.Run("filters secrets with empty label value", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: { - smSecret("projects/p/secrets/s1", map[string]string{"maestro-consumer-name": "mc-a"}), - smSecret("projects/p/secrets/s2", map[string]string{"maestro-consumer-name": ""}), - }, - }, + t.Run("filters out secrets with mode=maintenance", func(t *testing.T) { + sm := &mockSMLookup{} + s1 := registrationSecret(sm, "projects/p/secrets/mc-registration-a", "proj-a", "active") + s2 := registrationSecret(sm, "projects/p/secrets/mc-registration-b", "proj-b", "maintenance") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {s1, s2}, } - s := newSelector(sm, nil) - names, err := s.smMCNames(ctx) + + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) require.NoError(t, err) - assert.Equal(t, []string{"mc-a"}, names) + assert.Equal(t, []string{"proj-a"}, eligible) }) t.Run("no secrets → empty slice, no error", func(t *testing.T) { sm := &mockSMLookup{listResponses: map[string][]*secretmanagerpb.Secret{}} - s := newSelector(sm, nil) - names, err := s.smMCNames(ctx) + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) require.NoError(t, err) - assert.Empty(t, names) + assert.Empty(t, eligible) }) t.Run("listSecrets error → wrapped error", func(t *testing.T) { sm := &mockSMLookup{listErr: fmt.Errorf("gcp unavailable")} - s := newSelector(sm, nil) - _, err := s.smMCNames(ctx) + s := newSelector(sm, []string{"example.com"}) + _, err := s.fetchEligibleMCs(ctx) require.Error(t, err) - assert.Contains(t, err.Error(), "list secrets") + assert.Contains(t, err.Error(), "list mc-registration secrets") assert.Contains(t, err.Error(), "gcp unavailable") }) -} - -// ─── hcDNSDomains ───────────────────────────────────────────────────────────── - -func TestDynamicSelector_hcDNSDomains(t *testing.T) { - ctx := context.Background() - t.Run("parses single domain from secret payload", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, - }, - secretData: dnsPayload("us-central1.example.com"), + t.Run("accessSecretVersion error → skipped, remaining secrets returned", func(t *testing.T) { + sm := &mockSMLookup{} + sBad := smSecret("projects/p/secrets/mc-registration-bad", nil) + sGood := registrationSecret(sm, "projects/p/secrets/mc-registration-good", "proj-good", "active") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {sBad, sGood}, } - s := newSelector(sm, nil) - domains, err := s.hcDNSDomains(ctx) - require.NoError(t, err) - assert.Equal(t, []string{"us-central1.example.com"}, domains) - }) - - t.Run("splits comma-separated domains", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, - }, - secretData: dnsPayload("a.example.com,b.example.com,c.example.com"), + sm.accessErrs = map[string]error{ + "projects/p/secrets/mc-registration-bad/versions/latest": fmt.Errorf("permission denied"), } - s := newSelector(sm, nil) - domains, err := s.hcDNSDomains(ctx) - require.NoError(t, err) - assert.Equal(t, []string{"a.example.com", "b.example.com", "c.example.com"}, domains) - }) - t.Run("trims whitespace around domain entries", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, - }, - secretData: dnsPayload(" a.example.com , b.example.com "), - } - s := newSelector(sm, nil) - domains, err := s.hcDNSDomains(ctx) + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) require.NoError(t, err) - assert.Equal(t, []string{"a.example.com", "b.example.com"}, domains) + assert.Equal(t, []string{"proj-good"}, eligible) }) - t.Run("uses first matching secret when multiple returned", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: { - smSecret("projects/p/secrets/first", nil), - smSecret("projects/p/secrets/second", nil), - }, - }, - secretData: dnsPayload("first.example.com"), + t.Run("invalid JSON payload → skipped, remaining secrets returned", func(t *testing.T) { + sm := &mockSMLookup{} + sGood := registrationSecret(sm, "projects/p/secrets/mc-registration-good", "proj-good", "active") + sBad := smSecret("projects/p/secrets/mc-registration-bad", nil) + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {sGood, sBad}, } - s := newSelector(sm, nil) - _, err := s.hcDNSDomains(ctx) - require.NoError(t, err) - assert.Equal(t, "projects/p/secrets/first/versions/latest", sm.accessedName) - }) - - t.Run("no matching secret → error mentioning project", func(t *testing.T) { - sm := &mockSMLookup{listResponses: map[string][]*secretmanagerpb.Secret{}} - s := newSelector(sm, nil) - _, err := s.hcDNSDomains(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), testProject) - }) + if sm.accessResponses == nil { + sm.accessResponses = map[string][]byte{} + } + sm.accessResponses["projects/p/secrets/mc-registration-bad/versions/latest"] = []byte("not-json") - t.Run("listSecrets error → error propagated", func(t *testing.T) { - sm := &mockSMLookup{listErr: fmt.Errorf("gcp down")} - s := newSelector(sm, nil) - _, err := s.hcDNSDomains(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "list argocd-cluster secrets") + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) + require.NoError(t, err) + assert.Equal(t, []string{"proj-good"}, eligible) }) - t.Run("accessSecretVersion error → error propagated", func(t *testing.T) { + t.Run("all secrets broken → empty slice, no error", func(t *testing.T) { sm := &mockSMLookup{ listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, + mcRegistrationLabelFilter: {smSecret("projects/p/secrets/mc-registration-a", nil)}, }, - accessErr: fmt.Errorf("permission denied"), - } - s := newSelector(sm, nil) - _, err := s.hcDNSDomains(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "access secret") - assert.Contains(t, err.Error(), "permission denied") - }) - - t.Run("invalid JSON secret payload → error", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, + accessErrs: map[string]error{ + "projects/p/secrets/mc-registration-a/versions/latest": fmt.Errorf("permission denied"), }, - secretData: []byte("not-json"), } - s := newSelector(sm, nil) - _, err := s.hcDNSDomains(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "unmarshal") + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) + require.NoError(t, err) + assert.Empty(t, eligible) }) - t.Run("empty meta_hc_dns_domains field → empty slice", func(t *testing.T) { + t.Run("secret with empty projectId is excluded even if active", func(t *testing.T) { sm := &mockSMLookup{ listResponses: map[string][]*secretmanagerpb.Secret{ - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, + mcRegistrationLabelFilter: {smSecret("projects/p/secrets/mc-registration-a", nil)}, + }, + accessResponses: map[string][]byte{ + "projects/p/secrets/mc-registration-a/versions/latest": mcRegistrationJSON("", "active"), }, - secretData: dnsPayload(""), } - s := newSelector(sm, nil) - domains, err := s.hcDNSDomains(ctx) + s := newSelector(sm, []string{"example.com"}) + eligible, err := s.fetchEligibleMCs(ctx) require.NoError(t, err) - assert.Empty(t, domains) + assert.Empty(t, eligible) }) } -// ─── eligibleMCs ────────────────────────────────────────────────────────────── +// ─── cache ──────────────────────────────────────────────────────────────────── -func TestDynamicSelector_eligibleMCs(t *testing.T) { +func TestDynamicSelector_cache(t *testing.T) { ctx := context.Background() - makeSM := func(mcNames ...string) *mockSMLookup { - secrets := make([]*secretmanagerpb.Secret, 0, len(mcNames)) - for i, n := range mcNames { - secrets = append(secrets, smSecret( - fmt.Sprintf("projects/p/secrets/s%d", i), - map[string]string{"maestro-consumer-name": n}, - )) - } - return &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{filterMCNames: secrets}, + t.Run("second call within TTL uses cached result", func(t *testing.T) { + sm := &mockSMLookup{} + s1 := registrationSecret(sm, "projects/p/secrets/mc-registration-a", "proj-a", "active") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {s1}, } - } - t.Run("returns intersection of SM and Maestro sets", func(t *testing.T) { - sm := makeSM("mc-a", "mc-b", "mc-c") - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-b", "mc-c", "mc-d")) - defer srv.Close() + s := newSelector(sm, []string{"example.com"}) + s.cacheTTL = 1 * time.Minute - s := newSelector(sm, srv) - eligible, err := s.eligibleMCs(ctx) + // First call populates cache. + e1, err := s.eligibleMCs(ctx) require.NoError(t, err) - assert.ElementsMatch(t, []string{"mc-b", "mc-c"}, eligible) - }) + assert.Equal(t, []string{"proj-a"}, e1) + assert.Len(t, sm.accessedNames, 1) - t.Run("SM returns empty → no eligible MCs", func(t *testing.T) { - sm := makeSM() // no secrets - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-a")) - defer srv.Close() - - s := newSelector(sm, srv) - eligible, err := s.eligibleMCs(ctx) + // Second call should use cache — no new SM calls. + e2, err := s.eligibleMCs(ctx) require.NoError(t, err) - assert.Empty(t, eligible) - }) - - t.Run("Maestro returns empty → no eligible MCs", func(t *testing.T) { - sm := makeSM("mc-a", "mc-b") - srv := maestroServer(t, http.StatusOK, maestroResponse()) - defer srv.Close() - - s := newSelector(sm, srv) - eligible, err := s.eligibleMCs(ctx) - require.NoError(t, err) - assert.Empty(t, eligible) + assert.Equal(t, []string{"proj-a"}, e2) + assert.Len(t, sm.accessedNames, 1) // still 1 }) - t.Run("no overlap between SM and Maestro → empty", func(t *testing.T) { - sm := makeSM("mc-a", "mc-b") - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-c", "mc-d")) - defer srv.Close() - - s := newSelector(sm, srv) - eligible, err := s.eligibleMCs(ctx) - require.NoError(t, err) - assert.Empty(t, eligible) - }) + t.Run("call after TTL expiry re-fetches", func(t *testing.T) { + sm := &mockSMLookup{} + s1 := registrationSecret(sm, "projects/p/secrets/mc-registration-a", "proj-a", "active") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {s1}, + } - t.Run("SM error → error propagated", func(t *testing.T) { - sm := &mockSMLookup{listErr: fmt.Errorf("sm down")} - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-a")) - defer srv.Close() + s := newSelector(sm, []string{"example.com"}) + s.cacheTTL = 1 * time.Millisecond - s := newSelector(sm, srv) _, err := s.eligibleMCs(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "secret manager MC lookup") - }) + require.NoError(t, err) + assert.Len(t, sm.accessedNames, 1) - t.Run("Maestro error → error propagated", func(t *testing.T) { - sm := makeSM("mc-a") - srv := maestroServer(t, http.StatusInternalServerError, []byte("maestro down")) - defer srv.Close() + // Wait for cache to expire. + time.Sleep(5 * time.Millisecond) - s := newSelector(sm, srv) - _, err := s.eligibleMCs(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "maestro consumer lookup") + _, err = s.eligibleMCs(ctx) + require.NoError(t, err) + assert.Len(t, sm.accessedNames, 2) // re-fetched }) } @@ -430,60 +267,53 @@ func TestDynamicSelector_eligibleMCs(t *testing.T) { func TestDynamicSelector_Select(t *testing.T) { ctx := context.Background() - // buildFullMock wires up SM with the given MC names (in both SM and Maestro) - // and the given DNS domains, then returns the selector + test server. - buildFullMock := func(t *testing.T, mcNames []string, domains string) (*DynamicSelector, *httptest.Server) { - t.Helper() - secrets := make([]*secretmanagerpb.Secret, 0, len(mcNames)) - for i, n := range mcNames { - secrets = append(secrets, smSecret( - fmt.Sprintf("projects/p/secrets/mc%d", i), - map[string]string{"maestro-consumer-name": n}, - )) + // buildMock wires up SM with mc-registration secrets for the given + // (projectID, mode) pairs, all active unless mode is overridden. + buildMock := func(projectIDs ...string) *mockSMLookup { + sm := &mockSMLookup{} + secrets := make([]*secretmanagerpb.Secret, 0, len(projectIDs)) + for i, p := range projectIDs { + name := fmt.Sprintf("projects/p/secrets/mc-registration-%d", i) + secrets = append(secrets, registrationSecret(sm, name, p, "active")) } - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: secrets, - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, - }, - secretData: dnsPayload(domains), + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: secrets, } - srv := maestroServer(t, http.StatusOK, maestroResponse(mcNames...)) - return newSelector(sm, srv), srv + return sm } t.Run("happy path: returns single MC and domain", func(t *testing.T) { - s, srv := buildFullMock(t, []string{"mc-a"}, "us-central1.example.com") - defer srv.Close() + sm := buildMock("proj-a") + s := newSelector(sm, []string{"us-central1.example.com"}) - mc, domain, err := s.Select(ctx, nil) + mc, domain, err := s.Select(ctx) require.NoError(t, err) - assert.Equal(t, "mc-a", mc) + assert.Equal(t, "proj-a", mc) assert.Equal(t, "us-central1.example.com", domain) }) t.Run("round-robins across multiple eligible MCs", func(t *testing.T) { - s, srv := buildFullMock(t, []string{"mc-a", "mc-b", "mc-c"}, "example.com") - defer srv.Close() + sm := buildMock("proj-a", "proj-b", "proj-c") + s := newSelector(sm, []string{"example.com"}) seen := map[string]bool{} for i := 0; i < 6; i++ { - mc, _, err := s.Select(ctx, nil) + mc, _, err := s.Select(ctx) require.NoError(t, err) seen[mc] = true } - assert.True(t, seen["mc-a"], "mc-a should be selected at least once") - assert.True(t, seen["mc-b"], "mc-b should be selected at least once") - assert.True(t, seen["mc-c"], "mc-c should be selected at least once") + assert.True(t, seen["proj-a"], "proj-a should be selected at least once") + assert.True(t, seen["proj-b"], "proj-b should be selected at least once") + assert.True(t, seen["proj-c"], "proj-c should be selected at least once") }) t.Run("round-robins across multiple DNS domains independently", func(t *testing.T) { - s, srv := buildFullMock(t, []string{"mc-a"}, "zone1.example.com,zone2.example.com") - defer srv.Close() + sm := buildMock("proj-a") + s := newSelector(sm, []string{"zone1.example.com", "zone2.example.com"}) seen := map[string]bool{} for i := 0; i < 4; i++ { - _, domain, err := s.Select(ctx, nil) + _, domain, err := s.Select(ctx) require.NoError(t, err) seen[domain] = true } @@ -492,61 +322,50 @@ func TestDynamicSelector_Select(t *testing.T) { }) t.Run("no eligible MCs → error mentioning check hint", func(t *testing.T) { - // SM has mc-a but Maestro has none → intersection is empty - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: {smSecret("s1", map[string]string{"maestro-consumer-name": "mc-a"})}, - }, - } - srv := maestroServer(t, http.StatusOK, maestroResponse()) // no consumers - defer srv.Close() + sm := &mockSMLookup{listResponses: map[string][]*secretmanagerpb.Secret{}} + s := newSelector(sm, []string{"example.com"}) - s := newSelector(sm, srv) - _, _, err := s.Select(ctx, nil) + _, _, err := s.Select(ctx) require.Error(t, err) assert.Contains(t, err.Error(), "no eligible management clusters") }) - t.Run("no DNS domains in secret → error mentioning project", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: {smSecret("s1", map[string]string{"maestro-consumer-name": "mc-a"})}, - filterArgoCD: {smSecret("projects/p/secrets/argocd", nil)}, - }, - secretData: dnsPayload(""), // empty domains + t.Run("all MCs in maintenance → no eligible MCs error", func(t *testing.T) { + sm := &mockSMLookup{} + secret := registrationSecret(sm, "projects/p/secrets/mc-registration-a", "proj-a", "maintenance") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {secret}, } - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-a")) - defer srv.Close() + s := newSelector(sm, []string{"example.com"}) - s := newSelector(sm, srv) - _, _, err := s.Select(ctx, nil) + _, _, err := s.Select(ctx) require.Error(t, err) - assert.Contains(t, err.Error(), "no HC DNS domains") + assert.Contains(t, err.Error(), "no eligible management clusters") }) t.Run("SM error during eligible MC discovery → error propagated", func(t *testing.T) { sm := &mockSMLookup{listErr: fmt.Errorf("gcp outage")} - srv := maestroServer(t, http.StatusOK, maestroResponse("mc-a")) - defer srv.Close() + s := newSelector(sm, []string{"example.com"}) - s := newSelector(sm, srv) - _, _, err := s.Select(ctx, nil) + _, _, err := s.Select(ctx) require.Error(t, err) assert.Contains(t, err.Error(), "discover eligible MCs") }) - t.Run("Maestro error → error propagated through Select", func(t *testing.T) { - sm := &mockSMLookup{ - listResponses: map[string][]*secretmanagerpb.Secret{ - filterMCNames: {smSecret("s1", map[string]string{"maestro-consumer-name": "mc-a"})}, - }, + t.Run("one broken secret skipped, healthy one selected", func(t *testing.T) { + sm := &mockSMLookup{} + sBad := smSecret("projects/p/secrets/mc-registration-bad", nil) + sGood := registrationSecret(sm, "projects/p/secrets/mc-registration-good", "proj-good", "active") + sm.listResponses = map[string][]*secretmanagerpb.Secret{ + mcRegistrationLabelFilter: {sBad, sGood}, + } + sm.accessErrs = map[string]error{ + "projects/p/secrets/mc-registration-bad/versions/latest": fmt.Errorf("version disabled"), } - srv := maestroServer(t, http.StatusServiceUnavailable, []byte("down")) - defer srv.Close() - s := newSelector(sm, srv) - _, _, err := s.Select(ctx, nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "discover eligible MCs") + s := newSelector(sm, []string{"example.com"}) + mc, _, err := s.Select(ctx) + require.NoError(t, err) + assert.Equal(t, "proj-good", mc) }) } diff --git a/controllers/placement/placement_controller.go b/controllers/placement/placement_controller.go index da9aa4f7..97df8bde 100644 --- a/controllers/placement/placement_controller.go +++ b/controllers/placement/placement_controller.go @@ -23,19 +23,17 @@ const ( // Reconciler implements the placement controller reconcile loop. type Reconciler struct { - client client.Client - selector Selector - candidates []Candidate - log logger.Logger + client client.Client + selector Selector + log logger.Logger } // NewReconciler creates a new placement Reconciler. -func NewReconciler(selector Selector, candidates []Candidate, log logger.Logger, c client.Client) *Reconciler { +func NewReconciler(selector Selector, log logger.Logger, c client.Client) *Reconciler { return &Reconciler{ - selector: selector, - candidates: candidates, - log: log, - client: c, + selector: selector, + log: log, + client: c, } } @@ -62,7 +60,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } // Step 3: Select MC and DNS zone. - mc, domain, err := r.selector.Select(ctx, r.candidates) + mc, domain, err := r.selector.Select(ctx) if err != nil { return reconcile.Result{}, fmt.Errorf("placement: select MC for cluster %s: %w", clusterID, err) } diff --git a/controllers/placement/placement_controller_test.go b/controllers/placement/placement_controller_test.go index a2496c56..2a77b121 100644 --- a/controllers/placement/placement_controller_test.go +++ b/controllers/placement/placement_controller_test.go @@ -19,17 +19,6 @@ import ( "github.com/openshift-online/gecko/controllers/util/logger" ) -// mockSelector is a simple Selector implementation for tests. -type mockSelector struct { - mcName string - baseDomain string - err error -} - -func (m *mockSelector) Select(_ context.Context, _ []Candidate) (string, string, error) { - return m.mcName, m.baseDomain, m.err -} - // testLogger creates a logger for tests. func testLogger(t *testing.T) logger.Logger { t.Helper() @@ -44,19 +33,14 @@ func testLogger(t *testing.T) logger.Logger { return log } -// testCandidates returns a default candidate list for tests. -func testCandidates() []Candidate { - return []Candidate{ - {Name: "mc-us-c1", BaseDomains: []string{"hc-us-central1-abc.example.com"}}, - } -} - // mockStatusWriter is a minimal SubResourceWriter for status updates. type mockStatusWriter struct { - updateErr error + updateErr error + updateCalled bool } func (m *mockStatusWriter) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + m.updateCalled = true return m.updateErr } func (m *mockStatusWriter) Create(_ context.Context, _ client.Object, _ client.Object, _ ...client.SubResourceCreateOption) error { @@ -145,60 +129,85 @@ func buildCluster(id string, placed bool) *privatev1.Cluster { return c } +// mockSelector is a minimal Selector for reconciler tests. +type mockSelector struct { + mc string + domain string + err error +} + +func (m *mockSelector) Select(_ context.Context) (string, string, error) { + return m.mc, m.domain, m.err +} + +// workingSelector returns a Selector that always succeeds. +func workingSelector() Selector { + return &mockSelector{mc: "mc-us-c1", domain: "hc-us-central1-abc.example.com"} +} + +// failingSelector returns a Selector whose Select() always errors. +func failingSelector(errMsg string) Selector { + return &mockSelector{err: fmt.Errorf("%s", errMsg)} +} + func TestReconciler(t *testing.T) { tests := []struct { - name string - clusterID string - cluster *privatev1.Cluster // nil → NotFound - selector *mockSelector - expectUpdate bool - expectedResult reconcile.Result - expectError bool + name string + clusterID string + cluster *privatev1.Cluster // nil → NotFound + selector Selector + expectUpdate bool + expectStatusUpdate bool + expectedResult reconcile.Result + expectError bool }{ { - name: "happy path: selects MC and domain, updates status", - clusterID: "cluster-1", - cluster: buildCluster("cluster-1", false), - selector: &mockSelector{mcName: "mc-us-c1", baseDomain: "hc-us-central1-abc.example.com"}, - expectUpdate: false, - expectedResult: reconcile.Result{RequeueAfter: requeueStable}, + name: "happy path: selects MC and domain, updates status", + clusterID: "cluster-1", + cluster: buildCluster("cluster-1", false), + selector: workingSelector(), + expectUpdate: false, + expectStatusUpdate: true, + expectedResult: reconcile.Result{RequeueAfter: requeueStable}, }, { - name: "already placed: no update, empty result", - clusterID: "cluster-2", - cluster: buildCluster("cluster-2", true), - selector: &mockSelector{mcName: "mc-us-c1", baseDomain: "hc-us-central1-abc.example.com"}, - expectUpdate: false, - expectedResult: reconcile.Result{}, + name: "already placed: no update, empty result", + clusterID: "cluster-2", + cluster: buildCluster("cluster-2", true), + selector: workingSelector(), + expectUpdate: false, + expectStatusUpdate: false, + expectedResult: reconcile.Result{}, }, { - name: "cluster not found: return empty result, no error", - clusterID: "cluster-missing", - cluster: nil, // → NotFoundError - selector: &mockSelector{mcName: "mc-us-c1", baseDomain: "hc-us-central1-abc.example.com"}, - expectUpdate: false, - expectedResult: reconcile.Result{}, - expectError: false, + name: "cluster not found: return empty result, no error", + clusterID: "cluster-missing", + cluster: nil, // → NotFoundError + selector: workingSelector(), + expectUpdate: false, + expectStatusUpdate: false, + expectedResult: reconcile.Result{}, + expectError: false, }, { - name: "selector error: return error", - clusterID: "cluster-4", - cluster: buildCluster("cluster-4", false), - selector: &mockSelector{err: fmt.Errorf("no candidates available")}, - expectUpdate: false, - expectError: true, + name: "selector error: return error", + clusterID: "cluster-4", + cluster: buildCluster("cluster-4", false), + selector: failingSelector("no candidates available"), + expectUpdate: false, + expectStatusUpdate: false, + expectError: true, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - storeClient := &mockStoreClient{cluster: tc.cluster} + storeClient := &mockStoreClient{cluster: tc.cluster, statusWriter: &mockStatusWriter{}} reconciler := &Reconciler{ - client: storeClient, - selector: tc.selector, - candidates: testCandidates(), - log: testLogger(t), + client: storeClient, + selector: tc.selector, + log: testLogger(t), } req := reconcile.Request{ @@ -217,6 +226,7 @@ func TestReconciler(t *testing.T) { require.Equal(t, tc.expectedResult, result) require.Equal(t, tc.expectUpdate, storeClient.updateCalled, "Update called mismatch") + require.Equal(t, tc.expectStatusUpdate, storeClient.statusWriter.updateCalled, "Status().Update() called mismatch") }) } } @@ -251,45 +261,3 @@ func TestSetCondition(t *testing.T) { require.False(t, conds[0].LastTransitionTime.IsZero()) }) } - -func TestRoundRobinSelector(t *testing.T) { - t.Run("selects candidates in round-robin order", func(t *testing.T) { - candidates := []Candidate{ - {Name: "mc-a", BaseDomains: []string{"a.example.com"}}, - {Name: "mc-b", BaseDomains: []string{"b.example.com"}}, - } - - sel := NewRoundRobinSelector() - ctx := context.Background() - - mc1, domain1, err := sel.Select(ctx, candidates) - require.NoError(t, err) - require.Equal(t, "mc-a", mc1) - require.Equal(t, "a.example.com", domain1) - - mc2, domain2, err := sel.Select(ctx, candidates) - require.NoError(t, err) - require.Equal(t, "mc-b", mc2) - require.Equal(t, "b.example.com", domain2) - - // Wraps around. - mc3, _, err := sel.Select(ctx, candidates) - require.NoError(t, err) - require.Equal(t, "mc-a", mc3) - }) - - t.Run("returns error when no candidates", func(t *testing.T) { - sel := NewRoundRobinSelector() - _, _, err := sel.Select(context.Background(), nil) - require.Error(t, err) - }) - - t.Run("returns error when candidate has no base domains", func(t *testing.T) { - sel := NewRoundRobinSelector() - candidates := []Candidate{ - {Name: "mc-empty", BaseDomains: []string{}}, - } - _, _, err := sel.Select(context.Background(), candidates) - require.Error(t, err) - }) -} diff --git a/controllers/placement/selector.go b/controllers/placement/selector.go deleted file mode 100644 index dd5ea1bc..00000000 --- a/controllers/placement/selector.go +++ /dev/null @@ -1,49 +0,0 @@ -package placement - -import ( - "context" - "fmt" - "sync" -) - -// Candidate represents an eligible management cluster with its DNS capacity. -type Candidate struct { - Name string // Maestro consumer name / MC name - BaseDomains []string // available DNS zones/domains for this MC -} - -// Selector selects an MC and base domain from a list of candidates. -type Selector interface { - Select(ctx context.Context, candidates []Candidate) (mcName, baseDomain string, err error) -} - -// RoundRobinSelector selects MCs round-robin and picks the first available base domain. -type RoundRobinSelector struct { - mu sync.Mutex - counter int -} - -// NewRoundRobinSelector creates a new RoundRobinSelector. -func NewRoundRobinSelector() *RoundRobinSelector { - return &RoundRobinSelector{} -} - -// Select picks the next MC in round-robin order and returns its first base domain. -// Returns error if no candidates available or no base domains. -func (s *RoundRobinSelector) Select(_ context.Context, candidates []Candidate) (string, string, error) { - if len(candidates) == 0 { - return "", "", fmt.Errorf("placement: no candidates available") - } - - s.mu.Lock() - idx := s.counter % len(candidates) - s.counter++ - s.mu.Unlock() - - c := candidates[idx] - if len(c.BaseDomains) == 0 { - return "", "", fmt.Errorf("placement: candidate %q has no base domains", c.Name) - } - - return c.Name, c.BaseDomains[0], nil -} diff --git a/helm/charts/gecko-placement/templates/_helpers.tpl b/helm/charts/gecko-placement/templates/_helpers.tpl index f4b640a7..f89474c2 100644 --- a/helm/charts/gecko-placement/templates/_helpers.tpl +++ b/helm/charts/gecko-placement/templates/_helpers.tpl @@ -72,9 +72,10 @@ Validate required values. {{- if not (trim (toString .Values.image.tag)) -}} {{- fail "image.tag must be set (e.g. --set image.tag=abc1234)" -}} {{- end -}} -{{- if not .Values.secretManagerProject -}} -{{- if not .Values.candidates -}} -{{- fail "either secretManagerProject or candidates must be set" -}} +{{- if not (trim (toString .Values.secretManagerProject)) -}} +{{- fail "secretManagerProject must be set" -}} {{- end -}} +{{- if not (trim (toString .Values.hcDnsDomains)) -}} +{{- fail "hcDnsDomains must be set" -}} {{- end -}} {{- end }} diff --git a/helm/charts/gecko-placement/templates/deployment.yaml b/helm/charts/gecko-placement/templates/deployment.yaml index a9ab8487..85cf0253 100644 --- a/helm/charts/gecko-placement/templates/deployment.yaml +++ b/helm/charts/gecko-placement/templates/deployment.yaml @@ -45,17 +45,8 @@ spec: - --log-level={{ .Values.logLevel }} - --log-format={{ .Values.logFormat }} - --workers={{ .Values.workers }} - - --maestro-http-addr={{ .Values.maestro.httpAddr }} - {{- if .Values.secretManagerProject }} - --secretmanager-project={{ .Values.secretManagerProject }} - {{- else }} - {{- if .Values.candidates }} - - --candidates={{ join "," .Values.candidates }} - {{- if .Values.baseDomains }} - - --base-domains={{ join "," .Values.baseDomains }} - {{- end }} - {{- end }} - {{- end }} + - --hc-dns-domains={{ .Values.hcDnsDomains }} resources: {{- toYaml .Values.resources | nindent 12 }} {{- with .Values.nodeSelector }} diff --git a/helm/charts/gecko-placement/values.yaml b/helm/charts/gecko-placement/values.yaml index c08373ec..f5a82034 100644 --- a/helm/charts/gecko-placement/values.yaml +++ b/helm/charts/gecko-placement/values.yaml @@ -14,17 +14,13 @@ logLevel: info logFormat: json workers: 10 -maestro: - httpAddr: "http://maestro.hyperfleet.svc.cluster.local:8000" - -# Static list of management cluster names. Ignored when secretManagerProject is set. -candidates: [] -# Base domains paired with candidates (one per MC). -baseDomains: [] -# GCP project for Secret Manager MC/DNS discovery. Enables dynamic selector. +# GCP project for Secret Manager mc-registration discovery (required). # Can also be set at runtime via the SECRETMANAGER_PROJECT env var, but must -# be non-empty here (or candidates must be set) to pass Helm validation. +# be non-empty here to pass Helm validation. secretManagerProject: "" +# Comma-separated list of HC DNS zone domains to round-robin across +# (required). Injected by ArgoCD from Terraform via {{ .Values.hc_dns_domains }}. +hcDnsDomains: "" serviceAccount: create: true