From 18b987516df7a194c169156621c217b02717a1e9 Mon Sep 17 00:00:00 2001 From: tstollin Date: Tue, 29 Sep 2026 11:50:46 +0200 Subject: [PATCH 1/2] add graphql client based on https://github.com/shurcooL/githubv4 --- docs/rest-vs-graphql-api.md | 138 +++++++++++++++ go.mod | 2 + go.sum | 4 + internal/config/config.go | 4 + internal/ghclient/factory.go | 166 +++++++++++++++--- internal/ghclient/graphql_factory_test.go | 96 ++++++++++ internal/ghclient/graphql_interface.go | 31 ++++ internal/ghclient/graphql_wrapper.go | 31 ++++ internal/ghclient/graphql_wrapper_test.go | 106 +++++++++++ internal/ghclient/transport.go | 22 ++- internal/ratelimit/org_registry.go | 12 +- .../reconcilerfactory/factory_test.go | 17 ++ internal/reconciler/types.go | 6 +- test/mock/ghclientmock/mock_factory.go | 16 +- test/mock/ghclientmock/mock_graphql.go | 56 ++++++ 15 files changed, 672 insertions(+), 35 deletions(-) create mode 100644 docs/rest-vs-graphql-api.md create mode 100644 internal/ghclient/graphql_factory_test.go create mode 100644 internal/ghclient/graphql_interface.go create mode 100644 internal/ghclient/graphql_wrapper.go create mode 100644 internal/ghclient/graphql_wrapper_test.go create mode 100644 test/mock/ghclientmock/mock_graphql.go diff --git a/docs/rest-vs-graphql-api.md b/docs/rest-vs-graphql-api.md new file mode 100644 index 0000000..7bd658e --- /dev/null +++ b/docs/rest-vs-graphql-api.md @@ -0,0 +1,138 @@ +# REST vs. GraphQL: what would change if git-hubby also used GraphQL + +git-hubby is currently 100% REST, built on `github.com/google/go-github/v92`. There is no GraphQL +client dependency anywhere in the codebase (no `shurcooL/githubv4`, no hand-rolled GraphQL calls). +That said, `internal/ratelimit` already reserves a `CategoryGraphQL` bucket, and `internal/ghclient` +concentrates every GitHub call behind a single `GitHubClientWrapper`, so the codebase is structurally +ready to add a second transport if it were ever justified. + +This document captures the trade-off in both directions: + +1. What GraphQL would give us that REST cannot do at all today. +2. What we would lose if we replaced REST with GraphQL instead of adding it alongside. + +It also notes where the pain we already have in the codebase (documented in code comments) lines up +with GraphQL's usual strengths, and where our own REST-limitation workarounds are for capabilities +GraphQL doesn't have either — i.e. no API layer would fix them. + +## Where we stand today (evidence from the codebase) + +All REST calls happen through `internal/ghclient/wrapper.go`, grouped as: + +- **Organizations**: profile, custom properties (org-level schema), rulesets, code security + configurations, organization roles, membership, installations. +- **Repositories**: CRUD, custom property values, topics, autolinks, deploy keys, webhooks, + rulesets, Actions access level. +- **Teams**: CRUD, membership, external-group (IdP) sync. +- **Actions**: org-level permissions, artifact/log retention, allowed-actions, default workflow + permissions, self-hosted runner settings, runner groups, per-repo enablement. + +Several code comments show REST-specific friction that has already cost engineering effort: + +- `internal/webhook/v1alpha1/repository_webhook.go:89` — every repository admission-webhook + invocation re-fetches the parent org + its custom-property schema from GitHub (a classic N+1 + pattern GraphQL nesting is designed to eliminate). +- `internal/ghclient/wrapper.go:443-460` / `:543-560` — `ListInstallations` and + `ListEnabledReposInOrg` need a hand-rolled pagination loop because the pagination middleware + produces EOF errors on those two endpoints. +- `internal/reconciler/orgrec/rec_code_security_configurations.go:220-256` — to compute "all + public repos" or "all private/internal repos" for a security-configuration scope, the operator + lists *every* repository in the org via REST and filters by `Visibility` client-side, because + there's no server-side filtered query. +- `internal/reconciler/orgrec/rec_code_security_configurations.go:258-326` — attaching a code + security configuration to repositories is asynchronous (`202 Accepted`); the operator polls a + list endpoint every 5s for up to 2 minutes waiting for per-repo status strings to settle. +- `api/v1alpha1/organization_types.go:260-270` — explicit doc-comment limitation: *"GitHub's API + does not provide a way to retrieve the current attachment scope type... there is no reliable + way to determine which repositories should be included"* for the `all_without_configurations` + scope — worked around by unconditional reattachment. +- `internal/reconciler/orgrec/rec_actions_settings.go:32-39` — Actions settings are reconciled via + six separate sequential REST calls per organization (permissions, retention, allowed actions, + default workflow permissions, self-hosted runner settings, runner groups) instead of one + combined read/write. +- `internal/reconciler/orgrec/rec_actions_settings.go:73-75` — runner groups are matched by name, + not ID, because the ID is only known after creation, so a rename is treated as delete+recreate + (losing/reassigning runners). +- `internal/config/config.go:17-23` — the whole "startup spreading" mechanism exists specifically + to avoid exhausting REST per-category rate limits when many CRs reconcile at once. + +These are exactly the shapes of problem GraphQL is usually pitched to solve (N+1 fetches, no +server-side filtering, no combined queries) — see the next section for which of them GraphQL +would actually fix, and which are really about missing *mutations*, not missing *query +flexibility*, and therefore wouldn't be solved by GraphQL either. + +## 1. Capabilities GraphQL adds that REST doesn't have at all + +These are things the REST API has **no endpoint for**, confirmed against the public GraphQL +schema reference (`docs.github.com/en/graphql/reference`). + +| Capability | GraphQL type/mutation | Notes | +|---|---|---| +| Organization/Enterprise **IP allow lists** | `IpAllowListEntry`, `createIpAllowListEntry`, `deleteIpAllowListEntry`, `ipAllowListEnabledSetting`, `ipAllowListForInstalledAppsEnabledSetting` | No REST equivalent exists at all. Natural fit for the `Organization` CRD (`spec.ipAllowList`) if this is ever needed. | +| Classic branch protection **"required deployments before merge"** | `BranchProtectionRule.requiresDeployments` / `requiredDeploymentEnvironments` (in `createBranchProtectionRule`/`updateBranchProtectionRule`) | Only relevant if git-hubby manages classic branch protection rules; the modern Rulesets API (which we use) doesn't have this rule type either, so it's a real gap either way. | +| Repository **Discussions categories** | Discussion category CRUD | REST only has the `has_discussions` on/off toggle; category management (format, emoji, description, answerable flag) is GraphQL-only. | +| **Projects (v2)** | `projectsV2`, `ProjectV2`, related mutations | Projects Classic is REST-and-GraphQL but deprecated (removal 2025-04-01 UTC per docs). The new Projects experience (fields, views, workflows, item linking) has essentially no REST support. | +| **Enterprise-wide policy settings** | `Enterprise`/`EnterpriseOwnerInfo` fields, e.g. `defaultRepositoryPermissionSetting`, `membersCanCreateRepositoriesSetting` (+ public/private/internal variants), `membersCanDeleteRepositoriesSetting`, `membersCanDeleteIssuesSetting`, `membersCanInviteCollaboratorsSetting`, `membersCanChangeRepositoryVisibilitySetting`, `allowPrivateRepositoryForkingSetting`, `announcementBanner` | No REST endpoints for these at all. This is the single biggest capability delta, but it's a new axis for git-hubby — an `Enterprise` CRD, which doesn't exist today. | +| `Repository.codeownersErrors` | read-only query | Validates CODEOWNERS syntax/paths; useful only if git-hubby starts managing/linting CODEOWNERS as code. | + +Everything git-hubby currently manages via REST (rulesets, webhooks, custom properties, code +security configurations, teams, Actions settings, deploy keys) is fully covered by REST — none of +it requires GraphQL. Adding GraphQL today would be additive (new capabilities), not a fix for an +existing gap in what we manage. + +## 2. Capabilities we would lose by moving to GraphQL-only + +This is the more important question for git-hubby specifically, since **most of the operator's +actual REST surface has no GraphQL equivalent for writes**. A GraphQL-only client would be a +regression, not a lateral move. Concretely, mapped to what `internal/ghclient/wrapper.go` uses +today: + +| Feature area used by git-hubby | REST support | GraphQL support | +|---|---|---| +| **Repository deploy keys** (`ListKeys`/`CreateKey`/`DeleteKey`) | Yes | **None.** No `DeployKey` mutation exists in the GraphQL schema. | +| **Webhooks** — repo and org (`ListHooks`/`CreateHook`/`DeleteHook`) | Yes | **None.** No mutations to create/list/delete repository or organization webhooks. | +| **GitHub Actions settings** — org-level permissions, allowed-actions, artifact/log retention, default workflow permissions, self-hosted runner settings, runner groups, per-repo enablement | Yes (all of `ActionsSettings` in the `Organization` CRD) | **None.** There is no GraphQL surface for any Actions administration settings. This is roughly half of the `Organization` CRD's functionality. | +| **Code Security Configurations** — dependency graph, Dependabot alerts/updates, code scanning default setup, secret scanning (+push protection, validity checks, delegated bypass), attach-to-repos | Yes (entire `CodeSecurityConfiguration` CRD) | **None.** This whole 2024-era API has no GraphQL equivalent for writes. | +| **Repository Rulesets — writes** (`CreateRuleset`/`UpdateRuleset`/`DeleteRuleset`, org and repo level) | Yes | **Read-only.** GraphQL exposes `Organization.ruleset(s)`/`Repository.rulesets` as *queries*, but there is no `createRepositoryRuleset`/`updateRepositoryRuleset` mutation. This is the biggest CRD in the repo (`RulesetPreset`, ~600 lines) and would be entirely unmanageable via GraphQL alone. | +| **Custom properties — writes** (org-level schema `CreateOrUpdateCustomProperties`, repo-level values) | Yes | **Read-only.** GraphQL exposes `repositoryCustomProperties`/`repositoryCustomProperty` as queries only; no mutation to define or set them. | +| **Repository topics / autolinks** (`ReplaceAllTopics`, `ListAutolinks`/`CreateAutolink`/`DeleteAutolink`) | Yes | No GraphQL mutations for repository autolinks. Topic management via GraphQL is likewise not part of the standard mutation set. | +| **Team ↔ IdP external-group sync** (`ListExternalGroups`, `UpdateConnectedExternalGroup`) | Yes | **None.** SCIM/IdP group connection for teams is REST-only. This is used by the `Team` CRD's IDP-group-sync feature, which is Enterprise-plan-gated functionality git-hubby explicitly supports. | +| **Organization roles → team assignment** (`AssignOrgRoleToTeam`/`RemoveOrgRoleFromTeam`) | Yes | No equivalent GraphQL mutation. | +| **GitHub App installation listing** (`ListInstallations`) | Yes | Not modeled the same way in GraphQL's viewer-centric schema; this is how git-hubby's own auth/installation bookkeeping works. | +| **Organization/repository audit log** | Yes | Being actively **removed**: every `*AuditEntry` type and field in the GraphQL schema is now marked *"The GraphQL audit-log is deprecated. Please use the REST API instead. Removal on 2026-04-01 UTC."* GitHub itself is moving this capability GraphQL → REST, the opposite direction — a GraphQL-only strategy would be actively walking into a deprecation. | + +**Net effect:** if git-hubby switched to GraphQL-only, it would lose the ability to manage deploy +keys, webhooks, all of Actions settings, all of Code Security Configurations, and — critically — +**writing** rulesets and custom properties (only reading them would still work). That covers the +majority of what the operator exists to do. GraphQL could still be used for read-heavy paths +(fetching org + repos + custom properties + rulesets in one round trip to reduce the N+1 problem +in the admission webhook, for example) but essentially all *mutations* would still need to go +through REST. + +## 3. Secondary considerations + +- **Rate limiting model**: REST limits are simple request-count based, tracked per category in + `internal/ratelimit` today. GraphQL uses a point-based "query cost" model that's harder to + predict statically (cost depends on requested connection sizes/nesting) and would need its own + budgeting logic layered on top of the existing `rateLimitTrackerTransport`. +- **Tooling and type safety**: `go-github` gives typed request/response structs matching REST 1:1, + which is what `GitHubClientWrapper` relies on for compile-time safety and mocking in tests. + There isn't an equivalently complete, actively maintained typed Go client for GitHub's GraphQL + mutations covering the areas above — using GraphQL would mean hand-written query/mutation + strings (fragile, string-typed) for a large fraction of new code, increasing maintenance risk in + a codebase that currently keeps its entire GitHub surface behind one interface. +- **Two auth/transport stacks**: adding GraphQL alongside REST (rather than replacing it) means + maintaining two HTTP clients, two token/auth code paths (though both can share the same GitHub + App installation token), and two rate-limit categories to reconcile against the existing + `RateLimitedError`/stall-and-requeue logic. + +## Conclusion + +- **GraphQL-in-addition-to-REST** is worth it only for the genuinely REST-less features above (IP + allow lists, Discussions categories, Projects v2, and — if git-hubby ever gets an `Enterprise` + CRD — the large set of enterprise-wide policy settings). None of it is needed for what + git-hubby manages today. +- **GraphQL-instead-of-REST** is not viable: git-hubby's core value (rulesets, webhooks, Actions + settings, code security configurations, deploy keys, custom properties, IdP-synced teams) is + either REST-only or REST-write/GraphQL-read-only. A GraphQL-only client would regress the + operator to a fraction of its current functionality. diff --git a/go.mod b/go.mod index f6a812a..b9486f1 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/google/go-github/v92 v92.0.0 github.com/google/uuid v1.6.0 github.com/joho/godotenv v1.5.1 + github.com/shurcooL/githubv4 v0.0.0-20260209031235-2402fdf4a9ed go.elastic.co/ecszap v1.0.3 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.71.0 go.uber.org/zap v1.28.0 @@ -81,6 +82,7 @@ require ( github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.70.0 // indirect github.com/prometheus/procfs v0.21.1 // indirect + github.com/shurcooL/graphql v0.0.0-20240915155400-7ee5256398cf // indirect github.com/spf13/cobra v1.10.2 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/x448/float16 v0.8.4 // indirect diff --git a/go.sum b/go.sum index 852ad55..7e6701a 100644 --- a/go.sum +++ b/go.sum @@ -171,6 +171,10 @@ github.com/prometheus/procfs v0.21.1/go.mod h1:aB55Cww9pdSJVHk0hUf0inxWyyjPogFIj github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/shurcooL/githubv4 v0.0.0-20260209031235-2402fdf4a9ed h1:KT7hI8vYXgU0s2qaMkrfq9tCA1w/iEPgfredVP+4Tzw= +github.com/shurcooL/githubv4 v0.0.0-20260209031235-2402fdf4a9ed/go.mod h1:zqMwyHmnN/eDOZOdiTohqIUKUrTFX62PNlu7IJdu0q8= +github.com/shurcooL/graphql v0.0.0-20240915155400-7ee5256398cf h1:o1uxfymjZ7jZ4MsgCErcwWGtVKSiNAXtS59Lhs6uI/g= +github.com/shurcooL/graphql v0.0.0-20240915155400-7ee5256398cf/go.mod h1:9dIRpgIY7hVhoqfe0/FcYp0bpInZaT7dc3BYOprrIUE= github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= diff --git a/internal/config/config.go b/internal/config/config.go index 2415176..504f7a3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,6 +24,10 @@ type Features struct { type RateLimitConfig struct { // StallThresholdCore is the minimum core API calls remaining before stalling an org. StallThresholdCore int `env:"RATE_LIMIT_STALL_THRESHOLD_CORE" envDefault:"100"` + // StallThresholdGraphQL is the minimum GraphQL API points remaining before stalling an org. + // A value of 0 means GraphQL usage is tracked but never causes a stall (the default), which + // preserves prior behaviour while GraphQL adoption ramps up. + StallThresholdGraphQL int `env:"RATE_LIMIT_STALL_THRESHOLD_GRAPHQL" envDefault:"0"` // ResetGracePeriod is added to the GitHub reset time before allowing reconciliation to resume. ResetGracePeriod int `env:"RATE_LIMIT_RESET_GRACE_PERIOD_SECONDS" envDefault:"10"` // StalenessThresholdMinutes is how many minutes old registry data can be before refreshing. diff --git a/internal/ghclient/factory.go b/internal/ghclient/factory.go index 5fb6473..b18aeda 100644 --- a/internal/ghclient/factory.go +++ b/internal/ghclient/factory.go @@ -18,6 +18,7 @@ import ( "github.com/gofri/go-github-ratelimit/v2/github_ratelimit" "github.com/gofri/go-github-ratelimit/v2/github_ratelimit/github_primary_ratelimit" "github.com/google/go-github/v92/github" + "github.com/shurcooL/githubv4" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" v1 "k8s.io/api/core/v1" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -83,6 +84,7 @@ func DefaultClientConfig() *ClientConfig { // ClientInfo holds metadata about a cached client type ClientInfo struct { Client GitHubClient + GraphQLClient GraphQLClient InstallationID int64 CacheKey string SecretName string @@ -182,8 +184,15 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str clientToCache = NewCachingClient(clientToCache, m.config.ResponseCacheTTL) } + graphqlClient, err := m.createGraphQLClient(ctx, app.InstallationID, secretName, cacheKey) + if err != nil { + return nil, fmt.Errorf("failed to create GitHub GraphQL client for key %s: %w", cacheKey, err) + } + var graphqlToCache GraphQLClient = NewGraphQLClientWrapper(graphqlClient) + m.clients[cacheKey] = &ClientInfo{ Client: clientToCache, + GraphQLClient: graphqlToCache, InstallationID: app.InstallationID, CacheKey: cacheKey, SecretName: secretName, @@ -238,6 +247,47 @@ func (m *CachingGitHubClientFactory) getCachedClient(cacheKey string, secretName return nil } +// getCachedGraphQLClient returns the cached GraphQL client for the given cacheKey, only if the +// cached ClientInfo was created with the same credential secret. +func (m *CachingGitHubClientFactory) getCachedGraphQLClient(cacheKey string, secretName string) GraphQLClient { + m.mu.RLock() + defer m.mu.RUnlock() + + if info, exists := m.clients[cacheKey]; exists && info.SecretName == secretName { + return info.GraphQLClient + } + + return nil +} + +// GetGraphQLClient retrieves or creates a GitHub GraphQL client for the given cacheKey and +// AppConfig, enforcing the same per-org rate-limit gating as GetClient before returning it. +// +// REST and GraphQL clients for an org are created together and cached in the same ClientInfo, +// sharing credentials and rate-limit state. This method delegates client creation and the +// stall check to GetClient (which is idempotent for an already-cached org), then returns the +// GraphQL sibling from the cache. A rate-limit stall on the org therefore blocks GraphQL calls +// exactly as it blocks REST calls. +func (m *CachingGitHubClientFactory) GetGraphQLClient(ctx context.Context, cacheKey string, app AppConfig) (GraphQLClient, error) { + secretName := app.CredentialsSecretName + if secretName == "" { + secretName = m.legacySecretName + } + + // GetClient handles cache lookup, creation (which also builds the GraphQL sibling), + // credential-change eviction, and rate-limit gating. We reuse it to avoid duplicating + // that logic, then fetch the cached GraphQL client it created. + if _, err := m.GetClient(ctx, cacheKey, app); err != nil { + return nil, err + } + + graphqlClient := m.getCachedGraphQLClient(cacheKey, secretName) + if graphqlClient == nil { + return nil, fmt.Errorf("GraphQL client unexpectedly missing for key %s", cacheKey) + } + return graphqlClient, nil +} + // SetOrgRateLimitRegistry attaches a registry to an already-constructed factory. // This allows the registry to be created after the factory (e.g., depending on a feature flag) // and injected before the first client is created. @@ -246,31 +296,16 @@ func (m *CachingGitHubClientFactory) SetOrgRateLimitRegistry(registry *ratelimit m.orgRegistry = registry } -// createClient creates a new GitHub client with proper middleware setup. +// createClient creates a new GitHub REST client with proper middleware setup. // orgLogin is used to label the rate limit tracker transport so the registry // can attribute response headers to the correct organization. func (m *CachingGitHubClientFactory) createClient(ctx context.Context, installationID int64, secretName string, orgLogin string) (*github.Client, error) { log := logf.FromContext(ctx) log.Info("Creating GitHub client with middleware stack") - creds, ok := m.credentials[secretName] - if !ok { - // Fetch and parse the secret on first use - secret, err := m.secretProvider(ctx, secretName) - if err != nil { - log.Error(err, "failed to get GitHub app credentials secret", "secretName", secretName) - return nil, err - } - if secret == nil { - return nil, errors.New("GitHub app credentials secret cannot be nil") - } - parsedCreds, err := parseCredentials(*secret) - if err != nil { - log.Error(err, "failed to prepare GitHub app credentials") - return nil, err - } - m.credentials[secretName] = parsedCreds - creds = parsedCreds + creds, err := m.resolveCredentials(ctx, secretName) + if err != nil { + return nil, err } ghClient, err := m.buildClientWithMiddleware(installationID, creds, orgLogin) @@ -281,6 +316,49 @@ func (m *CachingGitHubClientFactory) createClient(ctx context.Context, installat return ghClient, nil } +// createGraphQLClient creates a new GitHub GraphQL client sharing the same credentials +// and middleware behaviour as the REST client for the same org. +func (m *CachingGitHubClientFactory) createGraphQLClient(ctx context.Context, installationID int64, secretName string, orgLogin string) (*githubv4.Client, error) { + log := logf.FromContext(ctx) + log.Info("Creating GitHub GraphQL client with middleware stack") + + creds, err := m.resolveCredentials(ctx, secretName) + if err != nil { + return nil, err + } + + return m.buildGraphQLClientWithMiddleware(installationID, creds, orgLogin), nil +} + +// resolveCredentials returns the parsed GitHub App credentials for the given secret name, +// fetching and parsing the Kubernetes secret on first use and caching the result. It is the +// shared credential resolution used by both the REST and GraphQL client creation paths. +// Callers must hold the factory write lock (it is only invoked from the create path). +func (m *CachingGitHubClientFactory) resolveCredentials(ctx context.Context, secretName string) (*AppCredentials, error) { + log := logf.FromContext(ctx) + + if creds, ok := m.credentials[secretName]; ok { + return creds, nil + } + + // Fetch and parse the secret on first use + secret, err := m.secretProvider(ctx, secretName) + if err != nil { + log.Error(err, "failed to get GitHub app credentials secret", "secretName", secretName) + return nil, err + } + if secret == nil { + return nil, errors.New("GitHub app credentials secret cannot be nil") + } + parsedCreds, err := parseCredentials(*secret) + if err != nil { + log.Error(err, "failed to prepare GitHub app credentials") + return nil, err + } + m.credentials[secretName] = parsedCreds + return parsedCreds, nil +} + // buildClientWithMiddleware creates a GitHub client with the full middleware stack func (m *CachingGitHubClientFactory) buildClientWithMiddleware(appInstallationID int64, creds *AppCredentials, orgLogin string) (*github.Client, error) { clientName := fmt.Sprintf("github-%d", appInstallationID) @@ -294,10 +372,53 @@ func (m *CachingGitHubClientFactory) buildClientWithMiddleware(appInstallationID ) } -// buildMiddlewareStack constructs the HTTP transport middleware stack. +// buildGraphQLClientWithMiddleware creates a githubv4 GraphQL client that shares the REST +// client's authentication and rate-limiting behaviour via the common middleware stack. +// The githubv4 client is configured with an *http.Client whose transport is the GraphQL +// middleware stack, mirroring how the REST client is wired. +func (m *CachingGitHubClientFactory) buildGraphQLClientWithMiddleware(appInstallationID int64, creds *AppCredentials, orgLogin string) *githubv4.Client { + clientName := fmt.Sprintf("github-graphql-%d", appInstallationID) + + httpClient := &http.Client{ + Transport: m.buildGraphQLMiddlewareStack(clientName, creds, appInstallationID, orgLogin), + Timeout: m.config.Timeout, + } + return githubv4.NewClient(httpClient) +} + +// buildMiddlewareStack constructs the HTTP transport middleware stack for the REST client. // Rate limit state is shared per GitHub App ID so installations of the same App share a quota bucket. // If the factory has an OrgRateLimitRegistry, a tracker transport is inserted to record response headers. func (m *CachingGitHubClientFactory) buildMiddlewareStack(clientName string, creds *AppCredentials, appInstallationID int64, orgLogin string) http.RoundTripper { + // Shared layers: rate limiting, auth, per-org tracking, retry (identical to GraphQL). + rt := m.buildSharedMiddleware(creds, appInstallationID, orgLogin) + + // Pagination handling — REST only. The GraphQL API does not use the Link-header based + // pagination that this transport implements, so it is intentionally omitted there. + rt = githubpagination.New(rt, githubpagination.WithPerPage(30)) + // OpenTelemetry instrumentation (top layer) + rt = otelhttp.NewTransport(rt, otelhttp.WithServerName(clientName)) + + return rt +} + +// buildGraphQLMiddlewareStack constructs the HTTP transport middleware stack for the GraphQL client. +// It reuses exactly the same shared layers as the REST stack (rate limiting shared per App ID, +// GitHub App installation auth, per-org rate limit header tracking, and retry), differing only in +// that it omits the REST Link-header pagination transport. The per-org tracker still classifies +// responses correctly because GitHub returns X-RateLimit-Resource: graphql for /graphql requests. +func (m *CachingGitHubClientFactory) buildGraphQLMiddlewareStack(clientName string, creds *AppCredentials, appInstallationID int64, orgLogin string) http.RoundTripper { + rt := m.buildSharedMiddleware(creds, appInstallationID, orgLogin) + // OpenTelemetry instrumentation (top layer) + rt = otelhttp.NewTransport(rt, otelhttp.WithServerName(clientName)) + return rt +} + +// buildSharedMiddleware builds the transport layers common to both the REST and GraphQL clients, +// from the base transport upwards: rate limiting (shared per App ID), GitHub App authentication, +// per-org rate limit header tracking, and retry. Both clients build their protocol-specific +// layers (pagination, tracing) on top of the returned transport. +func (m *CachingGitHubClientFactory) buildSharedMiddleware(creds *AppCredentials, appInstallationID int64, orgLogin string) http.RoundTripper { // Start with the base transport rt := http.DefaultTransport @@ -329,11 +450,6 @@ func (m *CachingGitHubClientFactory) buildMiddlewareStack(clientName string, cre delayFn := rehttp.ExpJitterDelay(5*time.Second, 30*time.Second) rt = rehttp.NewTransport(rt, retryFn, delayFn) - // Pagination handling - rt = githubpagination.New(rt, githubpagination.WithPerPage(30)) - // OpenTelemetry instrumentation (top layer) - rt = otelhttp.NewTransport(rt, otelhttp.WithServerName(clientName)) - return rt } diff --git a/internal/ghclient/graphql_factory_test.go b/internal/ghclient/graphql_factory_test.go new file mode 100644 index 0000000..94b7df3 --- /dev/null +++ b/internal/ghclient/graphql_factory_test.go @@ -0,0 +1,96 @@ +package ghclient + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" +) + +// testAppSecret builds a Kubernetes secret containing a freshly generated 2048-bit RSA key, +// suitable for driving client creation in the factory without touching the network. +func testAppSecret() *corev1.Secret { + key, err := rsa.GenerateKey(rand.Reader, 2048) + Expect(err).NotTo(HaveOccurred()) + der := x509.MarshalPKCS1PrivateKey(key) + pemBytes := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: der}) + return &corev1.Secret{ + Data: map[string][]byte{ + "app-id": []byte("12345"), + "private-key": pemBytes, + }, + } +} + +var _ = Describe("GraphQL client factory", func() { + const cacheKey = "my-org" + app := AppConfig{InstallationID: 67890, CredentialsSecretName: "creds"} + + It("creates, caches and returns a GraphQL client", func() { + callCount := 0 + provider := func(_ context.Context, _ string) (*corev1.Secret, error) { + callCount++ + return testAppSecret(), nil + } + factory, err := NewGitHubCachingClientFactory(DefaultClientConfig(), provider, "legacy", nil) + Expect(err).NotTo(HaveOccurred()) + + gql, err := factory.GetGraphQLClient(context.Background(), cacheKey, app) + Expect(err).NotTo(HaveOccurred()) + Expect(gql).NotTo(BeNil()) + + // Second call returns the same cached instance without re-fetching the secret. + gql2, err := factory.GetGraphQLClient(context.Background(), cacheKey, app) + Expect(err).NotTo(HaveOccurred()) + Expect(gql2).To(BeIdenticalTo(gql)) + Expect(callCount).To(Equal(1), "secret should be fetched only once and reused") + }) + + It("shares the same cached ClientInfo between REST and GraphQL clients", func() { + provider := func(_ context.Context, _ string) (*corev1.Secret, error) { + return testAppSecret(), nil + } + factory, err := NewGitHubCachingClientFactory(DefaultClientConfig(), provider, "legacy", nil) + Expect(err).NotTo(HaveOccurred()) + + restClient, err := factory.GetClient(context.Background(), cacheKey, app) + Expect(err).NotTo(HaveOccurred()) + Expect(restClient).NotTo(BeNil()) + + // GraphQL client must have been created alongside the REST client. + gql := factory.getCachedGraphQLClient(cacheKey, "creds") + Expect(gql).NotTo(BeNil()) + }) + + It("propagates secret provider errors", func() { + provider := func(_ context.Context, _ string) (*corev1.Secret, error) { + return nil, errors.New("boom") + } + factory, err := NewGitHubCachingClientFactory(DefaultClientConfig(), provider, "legacy", nil) + Expect(err).NotTo(HaveOccurred()) + + _, err = factory.GetGraphQLClient(context.Background(), cacheKey, app) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("boom")) + }) + + It("falls back to the legacy secret name when none is configured", func() { + var requestedSecret string + provider := func(_ context.Context, name string) (*corev1.Secret, error) { + requestedSecret = name + return testAppSecret(), nil + } + factory, err := NewGitHubCachingClientFactory(DefaultClientConfig(), provider, "legacy-secret", nil) + Expect(err).NotTo(HaveOccurred()) + + _, err = factory.GetGraphQLClient(context.Background(), cacheKey, AppConfig{InstallationID: 1}) + Expect(err).NotTo(HaveOccurred()) + Expect(requestedSecret).To(Equal("legacy-secret")) + }) +}) diff --git a/internal/ghclient/graphql_interface.go b/internal/ghclient/graphql_interface.go new file mode 100644 index 0000000..5555e4f --- /dev/null +++ b/internal/ghclient/graphql_interface.go @@ -0,0 +1,31 @@ +package ghclient + +import ( + "context" + + "github.com/shurcooL/githubv4" +) + +// GraphQLClient defines the interface for GitHub GraphQL API operations used by reconcilers. +// +// It intentionally mirrors the shape of the shurcooL/githubv4 client (Query/Mutate) rather +// than exposing one method per business operation the way the REST GitHubClient does. GraphQL +// queries are defined by the caller as tagged Go structs, so a thin, generic surface keeps the +// interface stable while still allowing it to be mocked in tests. +// +// RATE LIMIT CATEGORY: All calls made through this interface hit the GitHub GraphQL API, which +// GitHub accounts for under the dedicated "graphql" rate limit category (5000 points/hour by +// default), separate from the REST "core" category. The HTTP transport built for this client in +// the factory records those responses in the OrgRateLimitRegistry under CategoryGraphQL, so the +// same per-org stall gating that protects REST calls also protects GraphQL calls. If you wire a +// non-zero RATE_LIMIT_STALL_THRESHOLD_GRAPHQL, GraphQL exhaustion will requeue reconciliations +// just like core exhaustion does. +type GraphQLClient interface { + // Query executes a GraphQL query. q must be a pointer to a struct that uses githubv4/graphql + // struct tags. variables may be nil when the query takes no arguments. + Query(ctx context.Context, q any, variables map[string]any) error + + // Mutate executes a GraphQL mutation. m must be a pointer to a struct describing the mutation + // payload, input is the mutation's input object, and variables may be nil. + Mutate(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error +} diff --git a/internal/ghclient/graphql_wrapper.go b/internal/ghclient/graphql_wrapper.go new file mode 100644 index 0000000..f698482 --- /dev/null +++ b/internal/ghclient/graphql_wrapper.go @@ -0,0 +1,31 @@ +package ghclient + +import ( + "context" + + "github.com/shurcooL/githubv4" +) + +// GraphQLClientWrapper is the production implementation of GraphQLClient. It wraps a +// *githubv4.Client and delegates directly to it. Error handling is left to the githubv4 +// client, which already surfaces GraphQL and HTTP transport errors; the HTTP-level concerns +// (auth, rate limiting, retry, tracing) are handled by the transport stack the factory +// installs on the underlying *http.Client, exactly as for the REST wrapper. +type GraphQLClientWrapper struct { + client *githubv4.Client +} + +// NewGraphQLClientWrapper wraps a githubv4 client with the GraphQLClient interface. +func NewGraphQLClientWrapper(client *githubv4.Client) *GraphQLClientWrapper { + return &GraphQLClientWrapper{client: client} +} + +// Query executes a GraphQL query against the GitHub GraphQL API. +func (g *GraphQLClientWrapper) Query(ctx context.Context, q any, variables map[string]any) error { + return g.client.Query(ctx, q, variables) +} + +// Mutate executes a GraphQL mutation against the GitHub GraphQL API. +func (g *GraphQLClientWrapper) Mutate(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error { + return g.client.Mutate(ctx, m, input, variables) +} diff --git a/internal/ghclient/graphql_wrapper_test.go b/internal/ghclient/graphql_wrapper_test.go new file mode 100644 index 0000000..2823ea8 --- /dev/null +++ b/internal/ghclient/graphql_wrapper_test.go @@ -0,0 +1,106 @@ +package ghclient + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/shurcooL/githubv4" +) + +var _ = Describe("GraphQLClientWrapper", func() { + var ( + server *httptest.Server + client *GraphQLClientWrapper + ) + + AfterEach(func() { + if server != nil { + server.Close() + } + }) + + // newWrapperAgainst spins up a test server returning the given raw GraphQL JSON body and + // wires a GraphQLClientWrapper (via githubv4.NewEnterpriseClient) to talk to it. + newWrapperAgainst := func(handler http.HandlerFunc) *GraphQLClientWrapper { + server = httptest.NewServer(handler) + v4 := githubv4.NewEnterpriseClient(server.URL, server.Client()) + return NewGraphQLClientWrapper(v4) + } + + Describe("Query", func() { + It("executes a query and decodes the response into the provided struct", func() { + client = newWrapperAgainst(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"viewer":{"login":"octocat"}}}`)) + }) + + var q struct { + Viewer struct { + Login githubv4.String + } + } + err := client.Query(context.Background(), &q, nil) + Expect(err).NotTo(HaveOccurred()) + Expect(string(q.Viewer.Login)).To(Equal("octocat")) + }) + + It("surfaces GraphQL errors returned by the API", func() { + client = newWrapperAgainst(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"errors":[{"message":"boom"}]}`)) + }) + + var q struct { + Viewer struct { + Login githubv4.String + } + } + err := client.Query(context.Background(), &q, nil) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("boom")) + }) + + It("propagates transport-level HTTP errors", func() { + client = newWrapperAgainst(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + + var q struct { + Viewer struct { + Login githubv4.String + } + } + err := client.Query(context.Background(), &q, nil) + Expect(err).To(HaveOccurred()) + }) + }) + + Describe("Mutate", func() { + It("executes a mutation and forwards the input", func() { + var body map[string]any + client = newWrapperAgainst(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"addComment":{"clientMutationId":"1"}}}`)) + }) + + var m struct { + AddComment struct { + ClientMutationID githubv4.String + } `graphql:"addComment(input: $input)"` + } + input := githubv4.AddCommentInput{ + SubjectID: "subject-id", + Body: "hello", + } + err := client.Mutate(context.Background(), &m, input, nil) + Expect(err).NotTo(HaveOccurred()) + Expect(body).To(HaveKey("query")) + Expect(body).To(HaveKey("variables")) + }) + }) +}) diff --git a/internal/ghclient/transport.go b/internal/ghclient/transport.go index 58b9760..a1944eb 100644 --- a/internal/ghclient/transport.go +++ b/internal/ghclient/transport.go @@ -7,6 +7,23 @@ import ( "github.com/bradleyfalzon/ghinstallation/v2" ) +// newInstallationTransport builds the shared GitHub App installation auth transport. +// +// It is the single source of truth for turning an App ID + installation ID + RSA private +// key into an http.RoundTripper that signs requests with an installation access token. +// Both the REST client and the GraphQL client build their auth on top of this so that the +// App/installation authentication logic lives in exactly one place. +// +// The returned *ghinstallation.Transport caches installation tokens internally and refreshes +// them before expiry, so it is intended to be constructed once per client and reused across +// requests (unlike wrapping it fresh on every RoundTrip). +func newInstallationTransport(rt http.RoundTripper, appID int64, appInstallationID int64, privateKey *rsa.PrivateKey) *ghinstallation.Transport { + appsTransport := ghinstallation.NewAppsTransportFromPrivateKey(rt, appID, privateKey) + return ghinstallation.NewFromAppsTransport(appsTransport, appInstallationID) +} + +// AuthorizeGitHubAccessOptions is an http.RoundTripper that authenticates requests as a +// GitHub App installation. It delegates to the shared newInstallationTransport builder. type AuthorizeGitHubAccessOptions struct { http.RoundTripper @@ -26,7 +43,6 @@ func AuthorizeGitHubAccess(rt http.RoundTripper, appID int64, appInstallationID // RoundTrip implements http.RoundTripper interface. func (t *AuthorizeGitHubAccessOptions) RoundTrip(req *http.Request) (*http.Response, error) { - rt1 := ghinstallation.NewAppsTransportFromPrivateKey(t.RoundTripper, t.appID, t.privateKey) - rt2 := ghinstallation.NewFromAppsTransport(rt1, t.appInstallationID) - return rt2.RoundTrip(req) + rt := newInstallationTransport(t.RoundTripper, t.appID, t.appInstallationID, t.privateKey) + return rt.RoundTrip(req) } diff --git a/internal/ratelimit/org_registry.go b/internal/ratelimit/org_registry.go index b1bb51b..42f5744 100644 --- a/internal/ratelimit/org_registry.go +++ b/internal/ratelimit/org_registry.go @@ -90,13 +90,15 @@ func DefaultOrgRegistryConfig() OrgRegistryConfig { func ConfiguredThresholds(cfg config.Config) map[Category]int { return map[Category]int{ CategoryCore: cfg.RateLimitConfig.StallThresholdCore, - // Non-core categories are monitored (tracked in the registry and warned about at - // runtime) but have no env-configurable threshold yet. When the operator starts - // using search/graphql endpoints, add their threshold fields to config.RateLimitConfig - // and wire them here. + // GraphQL has a dedicated, env-configurable threshold. It defaults to 0 (track only, + // never stall) until GraphQL usage is significant enough to warrant a stall guard. + CategoryGraphQL: cfg.RateLimitConfig.StallThresholdGraphQL, + // The remaining non-core categories are monitored (tracked in the registry and warned + // about at runtime) but have no env-configurable threshold yet. When the operator starts + // using search endpoints, add their threshold fields to config.RateLimitConfig and wire + // them here. CategorySearch: 0, CategoryCodeSearch: 0, - CategoryGraphQL: 0, } } diff --git a/internal/reconciler/reconcilerfactory/factory_test.go b/internal/reconciler/reconcilerfactory/factory_test.go index 33e2537..ab978dd 100644 --- a/internal/reconciler/reconcilerfactory/factory_test.go +++ b/internal/reconciler/reconcilerfactory/factory_test.go @@ -1803,6 +1803,7 @@ func (m *mockSpreadManager) Spread(_ context.Context, _ spreading.SpreadableReso // mockGitHubClientManager is a mock implementation of reconciler.GitHubClientManager for testing type mockGitHubClientManager struct { client *ghclientmock.MockGitHubClientWrapper + graphqlClient ghclient.GraphQLClient clientByOrg map[string]*ghclientmock.MockGitHubClientWrapper shouldFailLimit bool shouldFail bool @@ -1833,3 +1834,19 @@ func (m *mockGitHubClientManager) GetClient(_ context.Context, orgName string, a return m.client, nil } + +func (m *mockGitHubClientManager) GetGraphQLClient(_ context.Context, orgName string, app ghclient.AppConfig) (ghclient.GraphQLClient, error) { + m.callCount++ + m.lastOrgName = orgName + m.lastAppConfig = app + + if m.shouldFailLimit { + return nil, m.rateLimitErr + } + + if m.shouldFail { + return nil, m.genericErr + } + + return m.graphqlClient, nil +} diff --git a/internal/reconciler/types.go b/internal/reconciler/types.go index b309d7a..94ba027 100644 --- a/internal/reconciler/types.go +++ b/internal/reconciler/types.go @@ -16,9 +16,13 @@ import ( const FieldOwner = client.FieldOwner("git-hubby") type GitHubClientManager interface { - // GetClient returns a rate-limit-checked GitHub client for the given org. + // GetClient returns a rate-limit-checked GitHub REST client for the given org. // It enforces per-org stall thresholds via the OrgRateLimitRegistry when configured. GetClient(ctx context.Context, cacheKey string, app ghclient.AppConfig) (ghclient.GitHubClient, error) + // GetGraphQLClient returns a rate-limit-checked GitHub GraphQL client for the given org. + // It shares credentials and rate-limit state with the REST client and enforces the same + // per-org stall thresholds via the OrgRateLimitRegistry when configured. + GetGraphQLClient(ctx context.Context, cacheKey string, app ghclient.AppConfig) (ghclient.GraphQLClient, error) } type SpreadManager interface { diff --git a/test/mock/ghclientmock/mock_factory.go b/test/mock/ghclientmock/mock_factory.go index f78bdf0..4140e44 100644 --- a/test/mock/ghclientmock/mock_factory.go +++ b/test/mock/ghclientmock/mock_factory.go @@ -11,7 +11,8 @@ import ( ) type GitHubMockClientFactory struct { - mockClient ghclient.GitHubClient + mockClient ghclient.GitHubClient + mockGraphQLClient ghclient.GraphQLClient } func NewGitHubMockClientFactory(mockClient *MockGitHubClientWrapper) *GitHubMockClientFactory { @@ -20,6 +21,12 @@ func NewGitHubMockClientFactory(mockClient *MockGitHubClientWrapper) *GitHubMock } } +// SetGraphQLClient sets the GraphQL client returned by GetGraphQLClient. It is optional; when +// unset, GetGraphQLClient returns an error, mirroring the unset REST client behaviour. +func (m *GitHubMockClientFactory) SetGraphQLClient(mockGraphQLClient ghclient.GraphQLClient) { + m.mockGraphQLClient = mockGraphQLClient +} + func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ ghclient.AppConfig) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") @@ -27,6 +34,13 @@ func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ ghcli return m.mockClient, nil } +func (m *GitHubMockClientFactory) GetGraphQLClient(_ context.Context, _ string, _ ghclient.AppConfig) (ghclient.GraphQLClient, error) { + if m.mockGraphQLClient == nil { + return nil, errors.New("mock GitHub GraphQL client not set") + } + return m.mockGraphQLClient, nil +} + func (m *GitHubMockClientFactory) GetGitHubClientAndCheckRateLimit(_ context.Context, _ string, _ ghclient.AppConfig, _ int) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") diff --git a/test/mock/ghclientmock/mock_graphql.go b/test/mock/ghclientmock/mock_graphql.go new file mode 100644 index 0000000..03724ae --- /dev/null +++ b/test/mock/ghclientmock/mock_graphql.go @@ -0,0 +1,56 @@ +//nolint:lll +package ghclientmock + +import ( + "context" + "sync" + + "github.com/shurcooL/githubv4" +) + +// GraphQLCall records a single invocation of the mock GraphQL client for assertions in tests. +type GraphQLCall struct { + Method string // "Query" or "Mutate" +} + +// MockGraphQLClient is a hand-written, function-field mock of ghclient.GraphQLClient, following +// the same pattern as MockGitHubClientWrapper: each method records the call and delegates to an +// injected Func if set, otherwise returns a sensible default (nil error / no-op). +type MockGraphQLClient struct { + QueryFunc func(ctx context.Context, q any, variables map[string]any) error + MutateFunc func(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error + + mu sync.Mutex + GraphQLCalls []GraphQLCall +} + +// NewMockGraphQLClient creates a new mock GraphQL client with empty call tracking. +func NewMockGraphQLClient() *MockGraphQLClient { + return &MockGraphQLClient{ + GraphQLCalls: make([]GraphQLCall, 0), + } +} + +func (m *MockGraphQLClient) recordCall(call GraphQLCall) { + m.mu.Lock() + defer m.mu.Unlock() + m.GraphQLCalls = append(m.GraphQLCalls, call) +} + +// Query records the call and delegates to QueryFunc if configured. +func (m *MockGraphQLClient) Query(ctx context.Context, q any, variables map[string]any) error { + m.recordCall(GraphQLCall{Method: "Query"}) + if m.QueryFunc != nil { + return m.QueryFunc(ctx, q, variables) + } + return nil +} + +// Mutate records the call and delegates to MutateFunc if configured. +func (m *MockGraphQLClient) Mutate(ctx context.Context, mutation any, input githubv4.Input, variables map[string]any) error { + m.recordCall(GraphQLCall{Method: "Mutate"}) + if m.MutateFunc != nil { + return m.MutateFunc(ctx, mutation, input, variables) + } + return nil +} From 988a2f6682adf99fa841863f370528324366e5f7 Mon Sep 17 00:00:00 2001 From: tstollin Date: Tue, 29 Sep 2026 13:15:21 +0200 Subject: [PATCH 2/2] support ip allow list for orgs --- .../api/v1alpha1/ipallowlistentry.go | 66 ++++++ .../api/v1alpha1/ipallowlistsettings.go | 85 +++++++ .../api/v1alpha1/organizationspec.go | 14 ++ .../applyconfiguration/internal/internal.go | 35 +++ api/v1alpha1/applyconfiguration/utils.go | 4 + api/v1alpha1/organization_types.go | 70 ++++++ api/v1alpha1/zz_generated.deepcopy.go | 57 +++++ .../github.interhyp.de_organizations.yaml | 62 +++++ docs/crds.md | 50 ++++ internal/conditions/conditions.go | 2 + internal/ghclient/graphql_interface.go | 75 +++++- internal/ghclient/graphql_ip_allow_list.go | 178 +++++++++++++++ .../ghclient/graphql_ip_allow_list_test.go | 148 ++++++++++++ internal/ghclient/graphql_wrapper.go | 6 +- .../mapper/github_ip_allow_list_mapper.go | 122 ++++++++++ .../github_ip_allow_list_mapper_test.go | 123 ++++++++++ .../reconciler/orgrec/rec_ip_allow_list.go | 140 ++++++++++++ .../orgrec/rec_ip_allow_list_test.go | 213 ++++++++++++++++++ internal/reconciler/orgrec/reconciler.go | 1 + internal/reconciler/orgrec/reconciler_test.go | 2 +- .../reconciler/reconcilerfactory/factory.go | 10 +- internal/reconciler/types.go | 5 +- schemas/organization_v1alpha1.json | 51 +++++ test/mock/ghclientmock/mock_factory.go | 7 +- test/mock/ghclientmock/mock_graphql.go | 80 ++++++- 25 files changed, 1588 insertions(+), 18 deletions(-) create mode 100644 api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistentry.go create mode 100644 api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistsettings.go create mode 100644 internal/ghclient/graphql_ip_allow_list.go create mode 100644 internal/ghclient/graphql_ip_allow_list_test.go create mode 100644 internal/mapper/github_ip_allow_list_mapper.go create mode 100644 internal/mapper/github_ip_allow_list_mapper_test.go create mode 100644 internal/reconciler/orgrec/rec_ip_allow_list.go create mode 100644 internal/reconciler/orgrec/rec_ip_allow_list_test.go diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistentry.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistentry.go new file mode 100644 index 0000000..b8a9f1a --- /dev/null +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistentry.go @@ -0,0 +1,66 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by controller-gen. DO NOT EDIT. + +package v1alpha1 + +// IpAllowListEntryApplyConfiguration represents a declarative configuration of the IpAllowListEntry type for use +// with apply. +// +// IpAllowListEntry defines a single entry in an organization's IP allow list. +// Each entry allows access from a single IP address or a range of addresses in CIDR notation. +// See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization +type IpAllowListEntryApplyConfiguration struct { + // AllowListValue is an IP address or a range of addresses in CIDR notation (e.g. "192.0.2.1" or "192.0.2.0/24"). + // A range covering the entire address space (such as "0.0.0.0/0" or "::/0") is rejected by GitHub; + // to allow access from anywhere, disable the allow list via Enabled=false instead. + AllowListValue *string `json:"allowListValue,omitempty"` + // Name is an optional human-readable description of the entry, shown in the GitHub UI. + Name *string `json:"name,omitempty"` + // IsActive determines whether this entry is enforced while the IP allow list is enabled. + // Inactive entries remain configured but are ignored until activated. + IsActive *bool `json:"isActive,omitempty"` +} + +// IpAllowListEntryApplyConfiguration constructs a declarative configuration of the IpAllowListEntry type for use with +// apply. +func IpAllowListEntry() *IpAllowListEntryApplyConfiguration { + return &IpAllowListEntryApplyConfiguration{} +} + +// WithAllowListValue sets the AllowListValue field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the AllowListValue field is set to the value of the last call. +func (b *IpAllowListEntryApplyConfiguration) WithAllowListValue(value string) *IpAllowListEntryApplyConfiguration { + b.AllowListValue = &value + return b +} + +// WithName sets the Name field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Name field is set to the value of the last call. +func (b *IpAllowListEntryApplyConfiguration) WithName(value string) *IpAllowListEntryApplyConfiguration { + b.Name = &value + return b +} + +// WithIsActive sets the IsActive field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the IsActive field is set to the value of the last call. +func (b *IpAllowListEntryApplyConfiguration) WithIsActive(value bool) *IpAllowListEntryApplyConfiguration { + b.IsActive = &value + return b +} diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistsettings.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistsettings.go new file mode 100644 index 0000000..e16f4dc --- /dev/null +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/ipallowlistsettings.go @@ -0,0 +1,85 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by controller-gen. DO NOT EDIT. + +package v1alpha1 + +// IpAllowListSettingsApplyConfiguration represents a declarative configuration of the IpAllowListSettings type for use +// with apply. +// +// IpAllowListSettings configures the organization-level IP allow list. +// +// Only the organization-owned portion of the effective allow list is managed here. Entries +// inherited from the enterprise account and entries automatically managed by installed GitHub +// Apps (described as "Managed by the GitHub App") are read-only and are never modified, +// deleted, or reported as drift by the reconciler. +// +// IP allow lists are only available on GitHub Enterprise Cloud organizations. If the enterprise +// delegates its allow list to an identity provider (Enterprise Managed Users with Entra ID and +// OIDC), GitHub deactivates the organization IP allow list GraphQL APIs; in that case the +// reconciler surfaces the condition but does not treat it as a hard failure. +// See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization +type IpAllowListSettingsApplyConfiguration struct { + // Enabled determines whether the IP allow list is enforced for the organization. + // Entries are always reconciled first, so that enabling enforcement never locks out the + // currently configured (active) addresses. + Enabled *bool `json:"enabled,omitempty"` + // EnabledForInstalledApps determines whether IP addresses configured for installed GitHub Apps + // are automatically added to the allow list. This takes effect independently of Enabled. + // The resulting App-managed entries are read-only and are not managed via the Entries field. + EnabledForInstalledApps *bool `json:"enabledForInstalledApps,omitempty"` + // Entries is the desired set of organization-owned IP allow list entries, keyed by their + // AllowListValue. Entries present in GitHub but not listed here are deleted (except read-only + // enterprise-inherited and App-managed entries, which are always preserved). Omitting this + // field (nil) manages only the enabled settings and leaves organization-owned entries untouched; + // provide an empty list to explicitly remove all organization-owned entries. + Entries []IpAllowListEntryApplyConfiguration `json:"entries,omitempty"` +} + +// IpAllowListSettingsApplyConfiguration constructs a declarative configuration of the IpAllowListSettings type for use with +// apply. +func IpAllowListSettings() *IpAllowListSettingsApplyConfiguration { + return &IpAllowListSettingsApplyConfiguration{} +} + +// WithEnabled sets the Enabled field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Enabled field is set to the value of the last call. +func (b *IpAllowListSettingsApplyConfiguration) WithEnabled(value bool) *IpAllowListSettingsApplyConfiguration { + b.Enabled = &value + return b +} + +// WithEnabledForInstalledApps sets the EnabledForInstalledApps field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the EnabledForInstalledApps field is set to the value of the last call. +func (b *IpAllowListSettingsApplyConfiguration) WithEnabledForInstalledApps(value bool) *IpAllowListSettingsApplyConfiguration { + b.EnabledForInstalledApps = &value + return b +} + +// WithEntries adds the given value to the Entries field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the Entries field. +func (b *IpAllowListSettingsApplyConfiguration) WithEntries(values ...*IpAllowListEntryApplyConfiguration) *IpAllowListSettingsApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithEntries") + } + b.Entries = append(b.Entries, *values[i]) + } + return b +} diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go index 7fba454..551ca16 100644 --- a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go @@ -86,6 +86,12 @@ type OrganizationSpecApplyConfiguration struct { // MemberPrivileges configures the privileges and default permissions for members of this organization. // Configurability may be restricted by Enterprise policies. MemberPrivileges *OrganizationMemberPrivilegesApplyConfiguration `json:"memberPrivileges,omitempty"` + // IpAllowList configures the organization-level IP allow list (GitHub Enterprise Cloud only). + // When nil, IP allow list reconciliation is skipped entirely, leaving all settings and entries + // untouched. Only organization-owned entries and settings are managed; enterprise-inherited and + // GitHub App-managed entries are always preserved. + // See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + IpAllowList *IpAllowListSettingsApplyConfiguration `json:"ipAllowList,omitempty"` } // OrganizationSpecApplyConfiguration constructs a declarative configuration of the OrganizationSpec type for use with @@ -217,3 +223,11 @@ func (b *OrganizationSpecApplyConfiguration) WithMemberPrivileges(value *Organiz b.MemberPrivileges = value return b } + +// WithIpAllowList sets the IpAllowList field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the IpAllowList field is set to the value of the last call. +func (b *OrganizationSpecApplyConfiguration) WithIpAllowList(value *IpAllowListSettingsApplyConfiguration) *OrganizationSpecApplyConfiguration { + b.IpAllowList = value + return b +} diff --git a/api/v1alpha1/applyconfiguration/internal/internal.go b/api/v1alpha1/applyconfiguration/internal/internal.go index e03031b..d591503 100644 --- a/api/v1alpha1/applyconfiguration/internal/internal.go +++ b/api/v1alpha1/applyconfiguration/internal/internal.go @@ -342,6 +342,38 @@ var schemaYAML = typed.YAMLObject(`types: - name: installationId type: scalar: numeric +- name: com.github.Interhyp.git-hubby.api.v1alpha1.IpAllowListEntry + map: + fields: + - name: allowListValue + type: + scalar: string + - name: isActive + type: + scalar: boolean + default: true + - name: name + type: + scalar: string +- name: com.github.Interhyp.git-hubby.api.v1alpha1.IpAllowListSettings + map: + fields: + - name: enabled + type: + scalar: boolean + default: false + - name: enabledForInstalledApps + type: + scalar: boolean + default: false + - name: entries + type: + list: + elementType: + namedType: com.github.Interhyp.git-hubby.api.v1alpha1.IpAllowListEntry + elementRelationship: associative + keys: + - allowListValue - name: com.github.Interhyp.git-hubby.api.v1alpha1.MergeStrategy map: fields: @@ -467,6 +499,9 @@ var schemaYAML = typed.YAMLObject(`types: - name: githubAppInstallationId type: scalar: numeric + - name: ipAllowList + type: + namedType: com.github.Interhyp.git-hubby.api.v1alpha1.IpAllowListSettings - name: location type: scalar: string diff --git a/api/v1alpha1/applyconfiguration/utils.go b/api/v1alpha1/applyconfiguration/utils.go index 172a2f1..d436445 100644 --- a/api/v1alpha1/applyconfiguration/utils.go +++ b/api/v1alpha1/applyconfiguration/utils.go @@ -69,6 +69,10 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1alpha1.DeployKeyApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("GitHubAppConfig"): return &apiv1alpha1.GitHubAppConfigApplyConfiguration{} + case v1alpha1.SchemeGroupVersion.WithKind("IpAllowListEntry"): + return &apiv1alpha1.IpAllowListEntryApplyConfiguration{} + case v1alpha1.SchemeGroupVersion.WithKind("IpAllowListSettings"): + return &apiv1alpha1.IpAllowListSettingsApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("MergeStrategy"): return &apiv1alpha1.MergeStrategyApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("Organization"): diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go index 2d24e79..9f18ee7 100644 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -324,6 +324,68 @@ type OrganizationMemberPrivileges struct { MembersCanForkPrivateRepositories *bool `json:"membersCanForkPrivateRepositories,omitempty"` } +// IpAllowListEntry defines a single entry in an organization's IP allow list. +// Each entry allows access from a single IP address or a range of addresses in CIDR notation. +// See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization +type IpAllowListEntry struct { + // AllowListValue is an IP address or a range of addresses in CIDR notation (e.g. "192.0.2.1" or "192.0.2.0/24"). + // A range covering the entire address space (such as "0.0.0.0/0" or "::/0") is rejected by GitHub; + // to allow access from anywhere, disable the allow list via Enabled=false instead. + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=43 + AllowListValue string `json:"allowListValue"` + + // Name is an optional human-readable description of the entry, shown in the GitHub UI. + // +kubebuilder:validation:MaxLength=255 + // +optional + Name string `json:"name,omitempty"` + + // IsActive determines whether this entry is enforced while the IP allow list is enabled. + // Inactive entries remain configured but are ignored until activated. + // +kubebuilder:default=true + // +optional + IsActive *bool `json:"isActive,omitempty"` +} + +// IpAllowListSettings configures the organization-level IP allow list. +// +// Only the organization-owned portion of the effective allow list is managed here. Entries +// inherited from the enterprise account and entries automatically managed by installed GitHub +// Apps (described as "Managed by the GitHub App") are read-only and are never modified, +// deleted, or reported as drift by the reconciler. +// +// IP allow lists are only available on GitHub Enterprise Cloud organizations. If the enterprise +// delegates its allow list to an identity provider (Enterprise Managed Users with Entra ID and +// OIDC), GitHub deactivates the organization IP allow list GraphQL APIs; in that case the +// reconciler surfaces the condition but does not treat it as a hard failure. +// See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization +type IpAllowListSettings struct { + // Enabled determines whether the IP allow list is enforced for the organization. + // Entries are always reconciled first, so that enabling enforcement never locks out the + // currently configured (active) addresses. + // +kubebuilder:default=false + // +optional + Enabled *bool `json:"enabled,omitempty"` + + // EnabledForInstalledApps determines whether IP addresses configured for installed GitHub Apps + // are automatically added to the allow list. This takes effect independently of Enabled. + // The resulting App-managed entries are read-only and are not managed via the Entries field. + // +kubebuilder:default=false + // +optional + EnabledForInstalledApps *bool `json:"enabledForInstalledApps,omitempty"` + + // Entries is the desired set of organization-owned IP allow list entries, keyed by their + // AllowListValue. Entries present in GitHub but not listed here are deleted (except read-only + // enterprise-inherited and App-managed entries, which are always preserved). Omitting this + // field (nil) manages only the enabled settings and leaves organization-owned entries untouched; + // provide an empty list to explicitly remove all organization-owned entries. + // +optional + // +listType=map + // +listMapKey=allowListValue + Entries []IpAllowListEntry `json:"entries,omitempty"` +} + // OrganizationSpec defines the desired state of Organization. // An Organization represents a GitHub organization and its configuration including custom properties, // rulesets, code security settings, and Actions permissions. @@ -425,6 +487,14 @@ type OrganizationSpec struct { // Configurability may be restricted by Enterprise policies. // +optional MemberPrivileges *OrganizationMemberPrivileges `json:"memberPrivileges,omitempty"` + + // IpAllowList configures the organization-level IP allow list (GitHub Enterprise Cloud only). + // When nil, IP allow list reconciliation is skipped entirely, leaving all settings and entries + // untouched. Only organization-owned entries and settings are managed; enterprise-inherited and + // GitHub App-managed entries are always preserved. + // See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + // +optional + IpAllowList *IpAllowListSettings `json:"ipAllowList,omitempty"` } // OrganizationStatus defines the observed state of Organization. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 3c2e11b..1a4b5da 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -651,6 +651,58 @@ func (in *GitHubAppCredentials) DeepCopy() *GitHubAppCredentials { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *IpAllowListEntry) DeepCopyInto(out *IpAllowListEntry) { + *out = *in + if in.IsActive != nil { + in, out := &in.IsActive, &out.IsActive + *out = new(bool) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IpAllowListEntry. +func (in *IpAllowListEntry) DeepCopy() *IpAllowListEntry { + if in == nil { + return nil + } + out := new(IpAllowListEntry) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *IpAllowListSettings) DeepCopyInto(out *IpAllowListSettings) { + *out = *in + if in.Enabled != nil { + in, out := &in.Enabled, &out.Enabled + *out = new(bool) + **out = **in + } + if in.EnabledForInstalledApps != nil { + in, out := &in.EnabledForInstalledApps, &out.EnabledForInstalledApps + *out = new(bool) + **out = **in + } + if in.Entries != nil { + in, out := &in.Entries, &out.Entries + *out = make([]IpAllowListEntry, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IpAllowListSettings. +func (in *IpAllowListSettings) DeepCopy() *IpAllowListSettings { + if in == nil { + return nil + } + out := new(IpAllowListSettings) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MergeStrategy) DeepCopyInto(out *MergeStrategy) { *out = *in @@ -893,6 +945,11 @@ func (in *OrganizationSpec) DeepCopyInto(out *OrganizationSpec) { *out = new(OrganizationMemberPrivileges) (*in).DeepCopyInto(*out) } + if in.IpAllowList != nil { + in, out := &in.IpAllowList, &out.IpAllowList + *out = new(IpAllowListSettings) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OrganizationSpec. diff --git a/config/crd/bases/github.interhyp.de_organizations.yaml b/config/crd/bases/github.interhyp.de_organizations.yaml index 0d8bf5c..e24ac7f 100644 --- a/config/crd/bases/github.interhyp.de_organizations.yaml +++ b/config/crd/bases/github.interhyp.de_organizations.yaml @@ -352,6 +352,68 @@ spec: format: int64 minimum: 1 type: integer + ipAllowList: + description: |- + IpAllowList configures the organization-level IP allow list (GitHub Enterprise Cloud only). + When nil, IP allow list reconciliation is skipped entirely, leaving all settings and entries + untouched. Only organization-owned entries and settings are managed; enterprise-inherited and + GitHub App-managed entries are always preserved. + See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + properties: + enabled: + default: false + description: |- + Enabled determines whether the IP allow list is enforced for the organization. + Entries are always reconciled first, so that enabling enforcement never locks out the + currently configured (active) addresses. + type: boolean + enabledForInstalledApps: + default: false + description: |- + EnabledForInstalledApps determines whether IP addresses configured for installed GitHub Apps + are automatically added to the allow list. This takes effect independently of Enabled. + The resulting App-managed entries are read-only and are not managed via the Entries field. + type: boolean + entries: + description: |- + Entries is the desired set of organization-owned IP allow list entries, keyed by their + AllowListValue. Entries present in GitHub but not listed here are deleted (except read-only + enterprise-inherited and App-managed entries, which are always preserved). Omitting this + field (nil) manages only the enabled settings and leaves organization-owned entries untouched; + provide an empty list to explicitly remove all organization-owned entries. + items: + description: |- + IpAllowListEntry defines a single entry in an organization's IP allow list. + Each entry allows access from a single IP address or a range of addresses in CIDR notation. + See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + properties: + allowListValue: + description: |- + AllowListValue is an IP address or a range of addresses in CIDR notation (e.g. "192.0.2.1" or "192.0.2.0/24"). + A range covering the entire address space (such as "0.0.0.0/0" or "::/0") is rejected by GitHub; + to allow access from anywhere, disable the allow list via Enabled=false instead. + maxLength: 43 + minLength: 1 + type: string + isActive: + default: true + description: |- + IsActive determines whether this entry is enforced while the IP allow list is enabled. + Inactive entries remain configured but are ignored until activated. + type: boolean + name: + description: Name is an optional human-readable description + of the entry, shown in the GitHub UI. + maxLength: 255 + type: string + required: + - allowListValue + type: object + type: array + x-kubernetes-list-map-keys: + - allowListValue + x-kubernetes-list-type: map + type: object location: description: |- Location is the organization's location (e.g., "Munich, Germany"). diff --git a/docs/crds.md b/docs/crds.md index 9a028a1..7953158 100644 --- a/docs/crds.md +++ b/docs/crds.md @@ -460,6 +460,55 @@ _Appears in:_ +#### IpAllowListEntry + + + +IpAllowListEntry defines a single entry in an organization's IP allow list. +Each entry allows access from a single IP address or a range of addresses in CIDR notation. +See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + + + +_Appears in:_ +- [IpAllowListSettings](#ipallowlistsettings) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `allowListValue` _string_ | AllowListValue is an IP address or a range of addresses in CIDR notation (e.g. "192.0.2.1" or "192.0.2.0/24").
A range covering the entire address space (such as "0.0.0.0/0" or "::/0") is rejected by GitHub;
to allow access from anywhere, disable the allow list via Enabled=false instead. | | MaxLength: 43
MinLength: 1
Required: \{\}
| +| `name` _string_ | Name is an optional human-readable description of the entry, shown in the GitHub UI. | | MaxLength: 255
Optional: \{\}
| +| `isActive` _boolean_ | IsActive determines whether this entry is enforced while the IP allow list is enabled.
Inactive entries remain configured but are ignored until activated. | true | Optional: \{\}
| + + +#### IpAllowListSettings + + + +IpAllowListSettings configures the organization-level IP allow list. + +Only the organization-owned portion of the effective allow list is managed here. Entries +inherited from the enterprise account and entries automatically managed by installed GitHub +Apps (described as "Managed by the GitHub App") are read-only and are never modified, +deleted, or reported as drift by the reconciler. + +IP allow lists are only available on GitHub Enterprise Cloud organizations. If the enterprise +delegates its allow list to an identity provider (Enterprise Managed Users with Entra ID and +OIDC), GitHub deactivates the organization IP allow list GraphQL APIs; in that case the +reconciler surfaces the condition but does not treat it as a hard failure. +See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization + + + +_Appears in:_ +- [OrganizationSpec](#organizationspec) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `enabled` _boolean_ | Enabled determines whether the IP allow list is enforced for the organization.
Entries are always reconciled first, so that enabling enforcement never locks out the
currently configured (active) addresses. | false | Optional: \{\}
| +| `enabledForInstalledApps` _boolean_ | EnabledForInstalledApps determines whether IP addresses configured for installed GitHub Apps
are automatically added to the allow list. This takes effect independently of Enabled.
The resulting App-managed entries are read-only and are not managed via the Entries field. | false | Optional: \{\}
| +| `entries` _[IpAllowListEntry](#ipallowlistentry) array_ | Entries is the desired set of organization-owned IP allow list entries, keyed by their
AllowListValue. Entries present in GitHub but not listed here are deleted (except read-only
enterprise-inherited and App-managed entries, which are always preserved). Omitting this
field (nil) manages only the enabled settings and leaves organization-owned entries untouched;
provide an empty list to explicitly remove all organization-owned entries. | | Optional: \{\}
| + + #### MergeStrategy @@ -639,6 +688,7 @@ _Appears in:_ | `website` _string_ | Website is the organization's website URL.
This appears on the organization's GitHub profile page as a clickable link. | | MaxLength: 255
Optional: \{\}
| | `plan` _string_ | Plan indicates the GitHub plan tier for this organization (enterprise, team, or free).
Determines whether Enterprise-only features (e.g., custom properties, runner groups) are reconciled or skipped. | enterprise | Enum: [enterprise team free]
Optional: \{\}
| | `memberPrivileges` _[OrganizationMemberPrivileges](#organizationmemberprivileges)_ | MemberPrivileges configures the privileges and default permissions for members of this organization.
Configurability may be restricted by Enterprise policies. | | Optional: \{\}
| +| `ipAllowList` _[IpAllowListSettings](#ipallowlistsettings)_ | IpAllowList configures the organization-level IP allow list (GitHub Enterprise Cloud only).
When nil, IP allow list reconciliation is skipped entirely, leaving all settings and entries
untouched. Only organization-owned entries and settings are managed; enterprise-inherited and
GitHub App-managed entries are always preserved.
See: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization | | Optional: \{\}
| #### OrganizationStatus diff --git a/internal/conditions/conditions.go b/internal/conditions/conditions.go index 4485763..2e1b458 100644 --- a/internal/conditions/conditions.go +++ b/internal/conditions/conditions.go @@ -23,6 +23,8 @@ const ( TypeCustomPropertyDefinitionsSynced ConditionType = "CustomPropertyDefinitionsSynced" // CustomPropertyDefinitionsSynced indicates that organization custom properties are synced TypeCodeSecurityConfigurationsSynced ConditionType = "CodeSecurityConfigurationsSynced" + // TypeIpAllowListSynced indicates that the organization IP allow list settings and entries are synced + TypeIpAllowListSynced ConditionType = "IpAllowListSynced" ) // Repository-specific condition types diff --git a/internal/ghclient/graphql_interface.go b/internal/ghclient/graphql_interface.go index 5555e4f..cd1c480 100644 --- a/internal/ghclient/graphql_interface.go +++ b/internal/ghclient/graphql_interface.go @@ -2,16 +2,59 @@ package ghclient import ( "context" + "time" +) + +// IPAllowListEnabledSetting mirrors the GitHub GraphQL IpAllowListEnabledSettingValue / +// IpAllowListForInstalledAppsEnabledSettingValue enum. Reading these settings can also yield +// values that are neither ENABLED nor DISABLED on some owner types; callers should treat any +// value other than IPAllowListEnabled as "not enabled". +type IPAllowListEnabledSetting string - "github.com/shurcooL/githubv4" +const ( + // IPAllowListEnabled indicates the setting is enabled for the owner. + IPAllowListEnabled IPAllowListEnabledSetting = "ENABLED" + // IPAllowListDisabled indicates the setting is disabled for the owner. + IPAllowListDisabled IPAllowListEnabledSetting = "DISABLED" ) +// IPAllowListEntry is a single entry returned from GitHub's IP allow list for an owner. +// +// Origin distinguishes entries the organization owns (and git-hubby may manage) from read-only +// entries that must never be modified: those inherited from the enterprise account and those +// automatically managed by installed GitHub Apps. +type IPAllowListEntry struct { + // ID is the GraphQL node ID of the entry, required for update/delete mutations. + ID string + // Name is the optional human-readable description of the entry. + Name string + // AllowListValue is the IP address or CIDR range. + AllowListValue string + // IsActive reports whether the entry is enforced while the allow list is enabled. + IsActive bool + // CreatedAt / UpdatedAt are informational timestamps. + CreatedAt time.Time + UpdatedAt time.Time +} + +// OrgIPAllowListConfig is the current organization-level IP allow list configuration read from GitHub. +type OrgIPAllowListConfig struct { + // OwnerID is the GraphQL node ID of the organization, required as the owner for create/enable mutations. + OwnerID string + // EnabledSetting is the current enforcement setting. + EnabledSetting IPAllowListEnabledSetting + // InstalledAppsEnabledSetting is the current "configure for installed GitHub Apps" setting. + InstalledAppsEnabledSetting IPAllowListEnabledSetting + // Entries is the full list of entries (organization-owned, enterprise-inherited, and App-managed). + Entries []IPAllowListEntry +} + // GraphQLClient defines the interface for GitHub GraphQL API operations used by reconcilers. // -// It intentionally mirrors the shape of the shurcooL/githubv4 client (Query/Mutate) rather -// than exposing one method per business operation the way the REST GitHubClient does. GraphQL -// queries are defined by the caller as tagged Go structs, so a thin, generic surface keeps the -// interface stable while still allowing it to be mocked in tests. +// It exposes the generic githubv4 Query/Mutate surface (kept thin so it is stable and easy to +// mock) plus a small set of higher-level helpers for features that are only available via +// GraphQL, such as organization IP allow list management. The helpers keep GraphQL query/struct +// construction out of the reconcilers. // // RATE LIMIT CATEGORY: All calls made through this interface hit the GitHub GraphQL API, which // GitHub accounts for under the dedicated "graphql" rate limit category (5000 points/hour by @@ -27,5 +70,25 @@ type GraphQLClient interface { // Mutate executes a GraphQL mutation. m must be a pointer to a struct describing the mutation // payload, input is the mutation's input object, and variables may be nil. - Mutate(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error + Mutate(ctx context.Context, m any, input GraphQLInput, variables map[string]any) error + + // GetOrgIPAllowList reads the organization's IP allow list configuration (owner ID, both + // enabled settings, and all entries). It paginates over all entries. + GetOrgIPAllowList(ctx context.Context, orgLogin string) (*OrgIPAllowListConfig, error) + + // CreateOrgIPAllowListEntry adds a new organization-owned entry for the given owner (organization) ID. + CreateOrgIPAllowListEntry(ctx context.Context, ownerID string, value string, name string, isActive bool) error + + // UpdateOrgIPAllowListEntry updates an existing entry identified by its GraphQL node ID. + UpdateOrgIPAllowListEntry(ctx context.Context, entryID string, value string, name string, isActive bool) error + + // DeleteOrgIPAllowListEntry deletes an entry identified by its GraphQL node ID. + DeleteOrgIPAllowListEntry(ctx context.Context, entryID string) error + + // SetOrgIPAllowListEnabled sets the IP allow list enforcement setting for the given owner (organization) ID. + SetOrgIPAllowListEnabled(ctx context.Context, ownerID string, enabled bool) error + + // SetOrgIPAllowListForInstalledAppsEnabled sets the "configure for installed GitHub Apps" + // setting for the given owner (organization) ID. + SetOrgIPAllowListForInstalledAppsEnabled(ctx context.Context, ownerID string, enabled bool) error } diff --git a/internal/ghclient/graphql_ip_allow_list.go b/internal/ghclient/graphql_ip_allow_list.go new file mode 100644 index 0000000..e031961 --- /dev/null +++ b/internal/ghclient/graphql_ip_allow_list.go @@ -0,0 +1,178 @@ +package ghclient + +import ( + "context" + "fmt" + + "github.com/shurcooL/githubv4" +) + +// ipAllowListEntriesPageSize is the number of IP allow list entries fetched per GraphQL page. +const ipAllowListEntriesPageSize = 100 + +// ipAllowListEntryNode is the shared GraphQL selection for a single IP allow list entry. +type ipAllowListEntryNode struct { + ID githubv4.ID + Name githubv4.String + AllowListValue githubv4.String + IsActive githubv4.Boolean + CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime +} + +func (n ipAllowListEntryNode) toDomain() IPAllowListEntry { + return IPAllowListEntry{ + ID: fmt.Sprintf("%v", n.ID), + Name: string(n.Name), + AllowListValue: string(n.AllowListValue), + IsActive: bool(n.IsActive), + CreatedAt: n.CreatedAt.Time, + UpdatedAt: n.UpdatedAt.Time, + } +} + +// GetOrgIPAllowList reads the organization's IP allow list configuration, paginating over all entries. +func (g *GraphQLClientWrapper) GetOrgIPAllowList(ctx context.Context, orgLogin string) (*OrgIPAllowListConfig, error) { + var query struct { + Organization struct { + ID githubv4.ID + IPAllowListEnabledSetting githubv4.String + IPAllowListForInstalledAppsEnabledSetting githubv4.String + IPAllowListEntries struct { + Nodes []ipAllowListEntryNode + PageInfo struct { + HasNextPage githubv4.Boolean + EndCursor githubv4.String + } + } `graphql:"ipAllowListEntries(first: $first, after: $after)"` + } `graphql:"organization(login: $login)"` + } + + variables := map[string]any{ + "login": githubv4.String(orgLogin), + "first": githubv4.Int(ipAllowListEntriesPageSize), + "after": (*githubv4.String)(nil), + } + + config := &OrgIPAllowListConfig{} + entries := make([]IPAllowListEntry, 0) + for { + if err := g.client.Query(ctx, &query, variables); err != nil { + return nil, err + } + + // Owner-level fields are identical on every page; capture them from the first response. + config.OwnerID = fmt.Sprintf("%v", query.Organization.ID) + config.EnabledSetting = IPAllowListEnabledSetting(query.Organization.IPAllowListEnabledSetting) + config.InstalledAppsEnabledSetting = IPAllowListEnabledSetting(query.Organization.IPAllowListForInstalledAppsEnabledSetting) + + for _, node := range query.Organization.IPAllowListEntries.Nodes { + entries = append(entries, node.toDomain()) + } + + if !bool(query.Organization.IPAllowListEntries.PageInfo.HasNextPage) { + break + } + variables["after"] = new(query.Organization.IPAllowListEntries.PageInfo.EndCursor) + } + + config.Entries = entries + return config, nil +} + +// CreateOrgIPAllowListEntry adds a new organization-owned entry. +func (g *GraphQLClientWrapper) CreateOrgIPAllowListEntry(ctx context.Context, ownerID string, value string, name string, isActive bool) error { + var mutation struct { + CreateIPAllowListEntry struct { + IPAllowListEntry struct { + ID githubv4.ID + } + } `graphql:"createIpAllowListEntry(input: $input)"` + } + input := githubv4.CreateIpAllowListEntryInput{ + OwnerID: githubv4.ID(ownerID), + AllowListValue: githubv4.String(value), + IsActive: githubv4.Boolean(isActive), + Name: new(githubv4.String(name)), + } + return g.client.Mutate(ctx, &mutation, input, nil) +} + +// UpdateOrgIPAllowListEntry updates an existing entry by its GraphQL node ID. +func (g *GraphQLClientWrapper) UpdateOrgIPAllowListEntry(ctx context.Context, entryID string, value string, name string, isActive bool) error { + var mutation struct { + UpdateIPAllowListEntry struct { + IPAllowListEntry struct { + ID githubv4.ID + } + } `graphql:"updateIpAllowListEntry(input: $input)"` + } + input := githubv4.UpdateIpAllowListEntryInput{ + IPAllowListEntryID: githubv4.ID(entryID), + AllowListValue: githubv4.String(value), + IsActive: githubv4.Boolean(isActive), + Name: new(githubv4.String(name)), + } + return g.client.Mutate(ctx, &mutation, input, nil) +} + +// DeleteOrgIPAllowListEntry deletes an entry by its GraphQL node ID. +func (g *GraphQLClientWrapper) DeleteOrgIPAllowListEntry(ctx context.Context, entryID string) error { + var mutation struct { + DeleteIPAllowListEntry struct { + IPAllowListEntry struct { + ID githubv4.ID + } + } `graphql:"deleteIpAllowListEntry(input: $input)"` + } + input := githubv4.DeleteIpAllowListEntryInput{ + IPAllowListEntryID: githubv4.ID(entryID), + } + return g.client.Mutate(ctx, &mutation, input, nil) +} + +// SetOrgIPAllowListEnabled sets the enforcement setting for the organization. +func (g *GraphQLClientWrapper) SetOrgIPAllowListEnabled(ctx context.Context, ownerID string, enabled bool) error { + var mutation struct { + UpdateIPAllowListEnabledSetting struct { + Owner struct { + ID githubv4.ID `graphql:"id"` + } `graphql:"owner"` + } `graphql:"updateIpAllowListEnabledSetting(input: $input)"` + } + input := githubv4.UpdateIpAllowListEnabledSettingInput{ + OwnerID: githubv4.ID(ownerID), + SettingValue: boolToEnabledSetting(enabled), + } + return g.client.Mutate(ctx, &mutation, input, nil) +} + +// SetOrgIPAllowListForInstalledAppsEnabled sets the "configure for installed GitHub Apps" setting. +func (g *GraphQLClientWrapper) SetOrgIPAllowListForInstalledAppsEnabled(ctx context.Context, ownerID string, enabled bool) error { + var mutation struct { + UpdateIPAllowListForInstalledAppsEnabledSetting struct { + Owner struct { + ID githubv4.ID `graphql:"id"` + } `graphql:"owner"` + } `graphql:"updateIpAllowListForInstalledAppsEnabledSetting(input: $input)"` + } + input := githubv4.UpdateIpAllowListForInstalledAppsEnabledSettingInput{ + OwnerID: githubv4.ID(ownerID), + SettingValue: boolToInstalledAppsSetting(enabled), + } + return g.client.Mutate(ctx, &mutation, input, nil) +} + +func boolToEnabledSetting(enabled bool) githubv4.IpAllowListEnabledSettingValue { + if enabled { + return githubv4.IpAllowListEnabledSettingValueEnabled + } + return githubv4.IpAllowListEnabledSettingValueDisabled +} + +func boolToInstalledAppsSetting(enabled bool) githubv4.IpAllowListForInstalledAppsEnabledSettingValue { + if enabled { + return githubv4.IpAllowListForInstalledAppsEnabledSettingValueEnabled + } + return githubv4.IpAllowListForInstalledAppsEnabledSettingValueDisabled +} diff --git a/internal/ghclient/graphql_ip_allow_list_test.go b/internal/ghclient/graphql_ip_allow_list_test.go new file mode 100644 index 0000000..3c57c6e --- /dev/null +++ b/internal/ghclient/graphql_ip_allow_list_test.go @@ -0,0 +1,148 @@ +package ghclient + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/shurcooL/githubv4" +) + +var _ = Describe("GraphQLClientWrapper IP allow list", func() { + var server *httptest.Server + + AfterEach(func() { + if server != nil { + server.Close() + } + }) + + // newWrapper wires a GraphQLClientWrapper to a test server using the given handler. + newWrapper := func(handler http.HandlerFunc) *GraphQLClientWrapper { + server = httptest.NewServer(handler) + v4 := githubv4.NewEnterpriseClient(server.URL, server.Client()) + return NewGraphQLClientWrapper(v4) + } + + Describe("GetOrgIPAllowList", func() { + It("reads settings and entries, paginating over all pages", func() { + page := 0 + client := newWrapper(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + if page == 0 { + page++ + _, _ = w.Write([]byte(`{"data":{"organization":{ + "id":"org-1", + "ipAllowListEnabledSetting":"ENABLED", + "ipAllowListForInstalledAppsEnabledSetting":"DISABLED", + "ipAllowListEntries":{ + "nodes":[{"id":"e1","name":"Office","allowListValue":"192.0.2.0/24","isActive":true,"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z"}], + "pageInfo":{"hasNextPage":true,"endCursor":"CURSOR1"} + } + }}}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"organization":{ + "id":"org-1", + "ipAllowListEnabledSetting":"ENABLED", + "ipAllowListForInstalledAppsEnabledSetting":"DISABLED", + "ipAllowListEntries":{ + "nodes":[{"id":"e2","name":"VPN","allowListValue":"203.0.113.0/24","isActive":false,"createdAt":"2024-01-02T00:00:00Z","updatedAt":"2024-01-02T00:00:00Z"}], + "pageInfo":{"hasNextPage":false,"endCursor":"CURSOR2"} + } + }}}`)) + }) + + cfg, err := client.GetOrgIPAllowList(context.Background(), "my-org") + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.OwnerID).To(Equal("org-1")) + Expect(cfg.EnabledSetting).To(Equal(IPAllowListEnabled)) + Expect(cfg.InstalledAppsEnabledSetting).To(Equal(IPAllowListDisabled)) + Expect(cfg.Entries).To(HaveLen(2)) + Expect(cfg.Entries[0].ID).To(Equal("e1")) + Expect(cfg.Entries[0].AllowListValue).To(Equal("192.0.2.0/24")) + Expect(cfg.Entries[0].IsActive).To(BeTrue()) + Expect(cfg.Entries[1].ID).To(Equal("e2")) + Expect(cfg.Entries[1].IsActive).To(BeFalse()) + }) + + It("propagates GraphQL errors", func() { + client := newWrapper(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"errors":[{"message":"IP allow list is not available for this organization"}]}`)) + }) + _, err := client.GetOrgIPAllowList(context.Background(), "my-org") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("not available")) + }) + }) + + Describe("mutations", func() { + It("CreateOrgIPAllowListEntry sends the expected input", func() { + var body map[string]any + client := newWrapper(func(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"createIpAllowListEntry":{"ipAllowListEntry":{"id":"new-1"}}}}`)) + }) + err := client.CreateOrgIPAllowListEntry(context.Background(), "org-1", "192.0.2.1", "Office", true) + Expect(err).NotTo(HaveOccurred()) + vars, _ := body["variables"].(map[string]any) + input, _ := vars["input"].(map[string]any) + Expect(input["ownerId"]).To(Equal("org-1")) + Expect(input["allowListValue"]).To(Equal("192.0.2.1")) + Expect(input["isActive"]).To(BeTrue()) + Expect(input["name"]).To(Equal("Office")) + }) + + It("SetOrgIPAllowListEnabled maps true to ENABLED", func() { + var body map[string]any + client := newWrapper(func(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"updateIpAllowListEnabledSetting":{"owner":{"id":"org-1"}}}}`)) + }) + err := client.SetOrgIPAllowListEnabled(context.Background(), "org-1", true) + Expect(err).NotTo(HaveOccurred()) + vars, _ := body["variables"].(map[string]any) + input, _ := vars["input"].(map[string]any) + Expect(input["settingValue"]).To(Equal("ENABLED")) + }) + + It("SetOrgIPAllowListForInstalledAppsEnabled maps false to DISABLED", func() { + var body map[string]any + client := newWrapper(func(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"updateIpAllowListForInstalledAppsEnabledSetting":{"owner":{"id":"org-1"}}}}`)) + }) + err := client.SetOrgIPAllowListForInstalledAppsEnabled(context.Background(), "org-1", false) + Expect(err).NotTo(HaveOccurred()) + vars, _ := body["variables"].(map[string]any) + input, _ := vars["input"].(map[string]any) + Expect(input["settingValue"]).To(Equal("DISABLED")) + }) + + It("DeleteOrgIPAllowListEntry sends the entry id", func() { + var body map[string]any + client := newWrapper(func(w http.ResponseWriter, r *http.Request) { + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":{"deleteIpAllowListEntry":{"ipAllowListEntry":{"id":"e1"}}}}`)) + }) + err := client.DeleteOrgIPAllowListEntry(context.Background(), "e1") + Expect(err).NotTo(HaveOccurred()) + vars, _ := body["variables"].(map[string]any) + input, _ := vars["input"].(map[string]any) + Expect(input["ipAllowListEntryId"]).To(Equal("e1")) + }) + }) +}) diff --git a/internal/ghclient/graphql_wrapper.go b/internal/ghclient/graphql_wrapper.go index f698482..0815897 100644 --- a/internal/ghclient/graphql_wrapper.go +++ b/internal/ghclient/graphql_wrapper.go @@ -6,6 +6,10 @@ import ( "github.com/shurcooL/githubv4" ) +// GraphQLInput is the input type for GraphQL mutations. It aliases githubv4.Input so callers and +// mocks do not need to import githubv4 directly for the generic Mutate method. +type GraphQLInput = githubv4.Input + // GraphQLClientWrapper is the production implementation of GraphQLClient. It wraps a // *githubv4.Client and delegates directly to it. Error handling is left to the githubv4 // client, which already surfaces GraphQL and HTTP transport errors; the HTTP-level concerns @@ -26,6 +30,6 @@ func (g *GraphQLClientWrapper) Query(ctx context.Context, q any, variables map[s } // Mutate executes a GraphQL mutation against the GitHub GraphQL API. -func (g *GraphQLClientWrapper) Mutate(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error { +func (g *GraphQLClientWrapper) Mutate(ctx context.Context, m any, input GraphQLInput, variables map[string]any) error { return g.client.Mutate(ctx, m, input, variables) } diff --git a/internal/mapper/github_ip_allow_list_mapper.go b/internal/mapper/github_ip_allow_list_mapper.go new file mode 100644 index 0000000..0062fed --- /dev/null +++ b/internal/mapper/github_ip_allow_list_mapper.go @@ -0,0 +1,122 @@ +package mapper + +import ( + "strings" + + "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/ghclient" + "github.com/Interhyp/git-hubby/internal/utils" +) + +// appManagedDescriptionPrefix is the prefix GitHub uses for the description of IP allow list +// entries that are automatically managed by an installed GitHub App +// ("Managed by the GitHub App."). Such entries are read-only and must never be modified +// or deleted by git-hubby. +const appManagedDescriptionPrefix = "Managed by the " + +// appManagedDescriptionSuffix together with the prefix identifies App-managed entries. +const appManagedDescriptionSuffix = " GitHub App." + +// IsAppManagedIPAllowListEntry reports whether the given entry is automatically managed by an +// installed GitHub App and is therefore read-only. These entries are surfaced on the organization +// IP allow list but cannot be edited, deleted, or disabled by an organization owner. +func IsAppManagedIPAllowListEntry(entry ghclient.IPAllowListEntry) bool { + name := entry.Name + return strings.HasPrefix(name, appManagedDescriptionPrefix) && strings.HasSuffix(name, appManagedDescriptionSuffix) +} + +// IPAllowListEntryCreate describes an organization-owned entry that must be created. +type IPAllowListEntryCreate struct { + AllowListValue string + Name string + IsActive bool +} + +// IPAllowListEntryUpdate describes an existing organization-owned entry that must be updated. +type IPAllowListEntryUpdate struct { + EntryID string + AllowListValue string + Name string + IsActive bool +} + +// IPAllowListEntryDelete describes an organization-owned entry that must be deleted. +type IPAllowListEntryDelete struct { + EntryID string + AllowListValue string +} + +// IPAllowListEntryPlan is the set of mutations required to bring the organization-owned portion of +// the IP allow list into the desired state. Read-only entries (App-managed and enterprise-inherited) +// are never included. +type IPAllowListEntryPlan struct { + Create []IPAllowListEntryCreate + Update []IPAllowListEntryUpdate + Delete []IPAllowListEntryDelete +} + +// IsEmpty reports whether the plan requires no mutations. +func (p IPAllowListEntryPlan) IsEmpty() bool { + return len(p.Create) == 0 && len(p.Update) == 0 && len(p.Delete) == 0 +} + +// DiffIPAllowListEntries computes the create/update/delete plan for the organization-owned IP allow +// list entries, matching entries by their AllowListValue (CIDR). App-managed entries in the current +// state are ignored entirely so they are neither updated nor deleted. +// +// desired is the list from the Organization spec. current is the full list read from GitHub +// (which may include App-managed entries). Matching is keyed on the normalized allow list value. +func DiffIPAllowListEntries(desired []v1alpha1.IpAllowListEntry, current []ghclient.IPAllowListEntry) IPAllowListEntryPlan { + plan := IPAllowListEntryPlan{} + + currentByValue := make(map[string]ghclient.IPAllowListEntry, len(current)) + for _, entry := range current { + if IsAppManagedIPAllowListEntry(entry) { + // Read-only: never manage App-managed entries. + continue + } + currentByValue[entry.AllowListValue] = entry + } + + desiredSeen := make(map[string]struct{}, len(desired)) + for _, want := range desired { + value := want.AllowListValue + desiredSeen[value] = struct{}{} + wantActive := utils.WithDefault(want.IsActive, true) + + existing, found := currentByValue[value] + if !found { + plan.Create = append(plan.Create, IPAllowListEntryCreate{ + AllowListValue: value, + Name: want.Name, + IsActive: wantActive, + }) + continue + } + if existing.Name != want.Name || existing.IsActive != wantActive { + plan.Update = append(plan.Update, IPAllowListEntryUpdate{ + EntryID: existing.ID, + AllowListValue: value, + Name: want.Name, + IsActive: wantActive, + }) + } + } + + for value, existing := range currentByValue { + if _, keep := desiredSeen[value]; !keep { + plan.Delete = append(plan.Delete, IPAllowListEntryDelete{ + EntryID: existing.ID, + AllowListValue: value, + }) + } + } + + return plan +} + +// IPAllowListEnabledSettingDiffers reports whether the desired enabled state differs from the +// current GitHub setting. A current value other than ENABLED is treated as "not enabled". +func IPAllowListEnabledSettingDiffers(desiredEnabled bool, current ghclient.IPAllowListEnabledSetting) bool { + return desiredEnabled != (current == ghclient.IPAllowListEnabled) +} diff --git a/internal/mapper/github_ip_allow_list_mapper_test.go b/internal/mapper/github_ip_allow_list_mapper_test.go new file mode 100644 index 0000000..a06e4a3 --- /dev/null +++ b/internal/mapper/github_ip_allow_list_mapper_test.go @@ -0,0 +1,123 @@ +package mapper + +import ( + "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/ghclient" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("GitHub IP Allow List Mapper", func() { + Describe("IsAppManagedIPAllowListEntry", func() { + It("recognizes App-managed entries by their description", func() { + Expect(IsAppManagedIPAllowListEntry(ghclient.IPAllowListEntry{ + Name: "Managed by the Acme GitHub App.", + })).To(BeTrue()) + }) + + It("does not flag organization-owned entries", func() { + Expect(IsAppManagedIPAllowListEntry(ghclient.IPAllowListEntry{ + Name: "Office network", + })).To(BeFalse()) + Expect(IsAppManagedIPAllowListEntry(ghclient.IPAllowListEntry{ + Name: "", + })).To(BeFalse()) + }) + }) + + Describe("IPAllowListEnabledSettingDiffers", func() { + It("treats ENABLED as enabled", func() { + Expect(IPAllowListEnabledSettingDiffers(true, ghclient.IPAllowListEnabled)).To(BeFalse()) + Expect(IPAllowListEnabledSettingDiffers(false, ghclient.IPAllowListEnabled)).To(BeTrue()) + }) + + It("treats DISABLED and unknown values as not enabled", func() { + Expect(IPAllowListEnabledSettingDiffers(false, ghclient.IPAllowListDisabled)).To(BeFalse()) + Expect(IPAllowListEnabledSettingDiffers(true, ghclient.IPAllowListDisabled)).To(BeTrue()) + Expect(IPAllowListEnabledSettingDiffers(false, ghclient.IPAllowListEnabledSetting("UNKNOWN"))).To(BeFalse()) + }) + }) + + Describe("DiffIPAllowListEntries", func() { + It("creates entries that are desired but absent", func() { + desired := []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: new(true)}, + } + plan := DiffIPAllowListEntries(desired, nil) + + Expect(plan.Update).To(BeEmpty()) + Expect(plan.Delete).To(BeEmpty()) + Expect(plan.Create).To(HaveLen(1)) + Expect(plan.Create[0].AllowListValue).To(Equal("192.0.2.0/24")) + Expect(plan.Create[0].Name).To(Equal("Office")) + Expect(plan.Create[0].IsActive).To(BeTrue()) + }) + + It("defaults IsActive to true when unset", func() { + desired := []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.1"}, + } + plan := DiffIPAllowListEntries(desired, nil) + Expect(plan.Create).To(HaveLen(1)) + Expect(plan.Create[0].IsActive).To(BeTrue()) + }) + + It("updates entries whose name or active state changed", func() { + desired := []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.0/24", Name: "New name", IsActive: new(false)}, + } + current := []ghclient.IPAllowListEntry{ + {ID: "id-1", AllowListValue: "192.0.2.0/24", Name: "Old name", IsActive: true}, + } + plan := DiffIPAllowListEntries(desired, current) + + Expect(plan.Create).To(BeEmpty()) + Expect(plan.Delete).To(BeEmpty()) + Expect(plan.Update).To(HaveLen(1)) + Expect(plan.Update[0].EntryID).To(Equal("id-1")) + Expect(plan.Update[0].Name).To(Equal("New name")) + Expect(plan.Update[0].IsActive).To(BeFalse()) + }) + + It("does not update entries that already match", func() { + desired := []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: new(true)}, + } + current := []ghclient.IPAllowListEntry{ + {ID: "id-1", AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: true}, + } + plan := DiffIPAllowListEntries(desired, current) + Expect(plan.IsEmpty()).To(BeTrue()) + }) + + It("deletes organization-owned entries that are no longer desired", func() { + current := []ghclient.IPAllowListEntry{ + {ID: "id-1", AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: true}, + } + plan := DiffIPAllowListEntries(nil, current) + + Expect(plan.Create).To(BeEmpty()) + Expect(plan.Update).To(BeEmpty()) + Expect(plan.Delete).To(HaveLen(1)) + Expect(plan.Delete[0].EntryID).To(Equal("id-1")) + }) + + It("never updates or deletes App-managed entries", func() { + desired := []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: new(true)}, + } + current := []ghclient.IPAllowListEntry{ + {ID: "app-1", AllowListValue: "203.0.113.5", Name: "Managed by the Acme GitHub App.", IsActive: true}, + {ID: "app-2", AllowListValue: "192.0.2.0/24", Name: "Managed by the Beta GitHub App.", IsActive: true}, + } + plan := DiffIPAllowListEntries(desired, current) + + // The App-managed entry sharing the desired value must NOT be treated as the current + // entry: it is ignored, so the desired value is created fresh. + Expect(plan.Delete).To(BeEmpty()) + Expect(plan.Update).To(BeEmpty()) + Expect(plan.Create).To(HaveLen(1)) + Expect(plan.Create[0].AllowListValue).To(Equal("192.0.2.0/24")) + }) + }) +}) diff --git a/internal/reconciler/orgrec/rec_ip_allow_list.go b/internal/reconciler/orgrec/rec_ip_allow_list.go new file mode 100644 index 0000000..792a75a --- /dev/null +++ b/internal/reconciler/orgrec/rec_ip_allow_list.go @@ -0,0 +1,140 @@ +package orgrec + +import ( + "context" + "strings" + + "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/ghclient" + "github.com/Interhyp/git-hubby/internal/mapper" + "github.com/Interhyp/git-hubby/internal/utils" + logPkg "sigs.k8s.io/controller-runtime/pkg/log" +) + +// ipAllowListUnavailableMarkers are lowercase substrings that identify GitHub GraphQL errors +// indicating that organization IP allow list management is unavailable for this owner. This +// happens on non-Enterprise-Cloud organizations and when the enterprise delegates its allow list +// to an identity provider (Enterprise Managed Users with Entra ID and OIDC), which deactivates the +// organization IP allow list GraphQL APIs. In those cases the feature genuinely cannot be managed, +// so the reconciler surfaces the situation and stops without treating it as a transient failure. +var ipAllowListUnavailableMarkers = []string{ + "ip allow list is not available", + "ip allow list is disabled", + "ip allow list management", + "not available for this", + "ip allow lists are not available", +} + +// reconcileIpAllowList reconciles the organization-level IP allow list (settings and +// organization-owned entries) via the GitHub GraphQL API. +// +// It is a no-op when the spec does not configure an IP allow list (spec.IpAllowList == nil), +// leaving all existing settings and entries untouched. +// +// Only the organization-owned portion of the list is managed. Entries inherited from the +// enterprise account are not returned by the organization GraphQL connection and are therefore +// never touched. Entries automatically managed by installed GitHub Apps are read-only and are +// filtered out of the diff. +// +// Entries are always reconciled before the enabled setting, so enabling enforcement can never +// lock out the currently configured (active) addresses. +func (o *GitHubOrgReconciler) reconcileIpAllowList(ctx context.Context) error { + log := logPkg.FromContext(ctx) + + spec := o.Kubernetes.Resource.Spec.IpAllowList + if spec == nil { + log.V(1).Info("No IP allow list configuration in spec, skipping reconciliation") + return nil + } + + log.V(1).Info("Reconciling organization IP allow list on GitHub") + + current, err := o.GitHub.GraphQLClient.GetOrgIPAllowList(ctx, o.GitHub.Resource) + if err != nil { + if isIpAllowListUnavailable(err) { + // Terminal, non-transient: the feature cannot be managed for this owner. Log and + // stop without returning an error so the reconciliation is not retried in a loop. + log.Info("IP allow list management is unavailable for this organization, skipping reconciliation. "+ + "This is expected on non-Enterprise-Cloud orgs or when the enterprise delegates its allow list to an identity provider", + "organization", o.GitHub.Resource, "reason", err.Error()) + return nil + } + return err + } + + // 1. Reconcile entries first (create/update/delete) so enforcement never precedes the addresses. + if spec.Entries != nil { + if err := o.reconcileIpAllowListEntries(ctx, current, spec.Entries); err != nil { + return err + } + } + + // 2. Reconcile the "configure for installed GitHub Apps" setting. + desiredInstalledApps := utils.WithDefault(spec.EnabledForInstalledApps, false) + if mapper.IPAllowListEnabledSettingDiffers(desiredInstalledApps, current.InstalledAppsEnabledSetting) { + if err := o.GitHub.GraphQLClient.SetOrgIPAllowListForInstalledAppsEnabled(ctx, current.OwnerID, desiredInstalledApps); err != nil { + return err + } + } + + // 3. Reconcile the enforcement setting last. + desiredEnabled := utils.WithDefault(spec.Enabled, false) + if mapper.IPAllowListEnabledSettingDiffers(desiredEnabled, current.EnabledSetting) { + if err := o.GitHub.GraphQLClient.SetOrgIPAllowListEnabled(ctx, current.OwnerID, desiredEnabled); err != nil { + return err + } + } + + log.V(1).Info("Successfully reconciled organization IP allow list on GitHub") + return nil +} + +// reconcileIpAllowListEntries applies the create/update/delete plan for organization-owned entries. +// Enterprise-inherited entries are never returned by the organization GraphQL connection, and +// App-managed entries are filtered out by the mapper, so only organization-owned entries are +// affected. +func (o *GitHubOrgReconciler) reconcileIpAllowListEntries(ctx context.Context, current *ghclient.OrgIPAllowListConfig, desired []v1alpha1.IpAllowListEntry) error { + log := logPkg.FromContext(ctx) + plan := mapper.DiffIPAllowListEntries(desired, current.Entries) + if plan.IsEmpty() { + return nil + } + + // Create and update desired entries before deleting obsolete ones, so that a currently active + // address is never briefly absent from the list while enforcement is (or may be) enabled. + for _, create := range plan.Create { + log.V(1).Info("Creating IP allow list entry", "value", create.AllowListValue, "isActive", create.IsActive) + if err := o.GitHub.GraphQLClient.CreateOrgIPAllowListEntry(ctx, current.OwnerID, create.AllowListValue, create.Name, create.IsActive); err != nil { + return err + } + } + for _, update := range plan.Update { + log.V(1).Info("Updating IP allow list entry", "value", update.AllowListValue, "isActive", update.IsActive) + if err := o.GitHub.GraphQLClient.UpdateOrgIPAllowListEntry(ctx, update.EntryID, update.AllowListValue, update.Name, update.IsActive); err != nil { + return err + } + } + for _, del := range plan.Delete { + log.V(1).Info("Deleting IP allow list entry", "value", del.AllowListValue) + if err := o.GitHub.GraphQLClient.DeleteOrgIPAllowListEntry(ctx, del.EntryID); err != nil { + return err + } + } + + return nil +} + +// isIpAllowListUnavailable reports whether the given error indicates the IP allow list feature is +// unavailable for the organization (as opposed to a transient error worth retrying). +func isIpAllowListUnavailable(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + for _, marker := range ipAllowListUnavailableMarkers { + if strings.Contains(msg, marker) { + return true + } + } + return false +} diff --git a/internal/reconciler/orgrec/rec_ip_allow_list_test.go b/internal/reconciler/orgrec/rec_ip_allow_list_test.go new file mode 100644 index 0000000..273d9dd --- /dev/null +++ b/internal/reconciler/orgrec/rec_ip_allow_list_test.go @@ -0,0 +1,213 @@ +package orgrec + +import ( + "context" + "errors" + + "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/ghclient" + "github.com/Interhyp/git-hubby/internal/reconciler" + "github.com/Interhyp/git-hubby/test/mock/ghclientmock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// callsOfMethod returns the recorded GraphQL calls with the given method name. +func callsOfMethod(calls []ghclientmock.GraphQLCall, method string) []ghclientmock.GraphQLCall { + var out []ghclientmock.GraphQLCall + for _, c := range calls { + if c.Method == method { + out = append(out, c) + } + } + return out +} + +var _ = Describe("ReconcileIpAllowList", func() { + var ( + ctx context.Context + gqlClient *ghclientmock.MockGraphQLClient + k8sClient client.Client + rec *GitHubOrgReconciler + scheme *runtime.Scheme + org *v1alpha1.Organization + ipSpec *v1alpha1.IpAllowListSettings + current *ghclient.OrgIPAllowListConfig + currentErr error + err error + ) + + BeforeEach(func() { + ctx = context.Background() + gqlClient = ghclientmock.NewMockGraphQLClient() + + scheme = runtime.NewScheme() + Expect(v1alpha1.AddToScheme(scheme)).To(Succeed()) + + // Default current GitHub state: enterprise-cloud org, list disabled, no entries. + current = &ghclient.OrgIPAllowListConfig{ + OwnerID: "org-node-id", + EnabledSetting: ghclient.IPAllowListDisabled, + InstalledAppsEnabledSetting: ghclient.IPAllowListDisabled, + Entries: nil, + } + currentErr = nil + ipSpec = nil + }) + + JustBeforeEach(func() { + gqlClient.GetOrgIPAllowListFunc = func(_ context.Context, _ string) (*ghclient.OrgIPAllowListConfig, error) { + return current, currentErr + } + + org = &v1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{Name: "test-org", Namespace: "default"}, + Spec: v1alpha1.OrganizationSpec{ + Name: "test-org", + GitHubAppInstallationId: new(int64(12345)), + IpAllowList: ipSpec, + }, + } + + k8sClient = fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(org). + WithStatusSubresource(org). + Build() + + rec = &GitHubOrgReconciler{ + GitHub: reconciler.GitHub[string]{ + GraphQLClient: gqlClient, + Resource: "test-org", + }, + Kubernetes: reconciler.Kubernetes[*v1alpha1.Organization]{ + Client: k8sClient, + Resource: org, + }, + } + + err = rec.reconcileIpAllowList(ctx) + }) + + Context("when the spec has no IP allow list configuration", func() { + BeforeEach(func() { ipSpec = nil }) + + It("skips reconciliation entirely without querying GitHub", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(gqlClient.GraphQLCalls).To(BeEmpty()) + }) + }) + + Context("when IP allow list management is unavailable (enterprise IdP mode / non-GHEC)", func() { + BeforeEach(func() { + ipSpec = &v1alpha1.IpAllowListSettings{Enabled: new(true)} + currentErr = errors.New("IP allow list is not available for this organization") + }) + + It("does not return an error and performs no mutations", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "SetOrgIPAllowListEnabled")).To(BeEmpty()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "CreateOrgIPAllowListEntry")).To(BeEmpty()) + }) + }) + + Context("when the read fails with a transient error", func() { + BeforeEach(func() { + ipSpec = &v1alpha1.IpAllowListSettings{Enabled: new(true)} + currentErr = errors.New("something transient went wrong") + }) + + It("propagates the error", func() { + Expect(err).To(HaveOccurred()) + }) + }) + + Context("when enabling enforcement with new entries", func() { + BeforeEach(func() { + ipSpec = &v1alpha1.IpAllowListSettings{ + Enabled: new(true), + Entries: []v1alpha1.IpAllowListEntry{ + {AllowListValue: "192.0.2.0/24", Name: "Office", IsActive: new(true)}, + }, + } + }) + + It("creates the entry before enabling the setting", func() { + Expect(err).NotTo(HaveOccurred()) + + creates := callsOfMethod(gqlClient.GraphQLCalls, "CreateOrgIPAllowListEntry") + enables := callsOfMethod(gqlClient.GraphQLCalls, "SetOrgIPAllowListEnabled") + Expect(creates).To(HaveLen(1)) + Expect(creates[0].Value).To(Equal("192.0.2.0/24")) + Expect(creates[0].OwnerID).To(Equal("org-node-id")) + Expect(enables).To(HaveLen(1)) + Expect(enables[0].Enabled).To(BeTrue()) + + // Ordering: the create call must be recorded before the enable call. + var createIdx, enableIdx int + for i, c := range gqlClient.GraphQLCalls { + switch c.Method { + case "CreateOrgIPAllowListEntry": + createIdx = i + case "SetOrgIPAllowListEnabled": + enableIdx = i + } + } + Expect(createIdx).To(BeNumerically("<", enableIdx)) + }) + }) + + Context("when the settings already match", func() { + BeforeEach(func() { + current.EnabledSetting = ghclient.IPAllowListEnabled + current.InstalledAppsEnabledSetting = ghclient.IPAllowListDisabled + ipSpec = &v1alpha1.IpAllowListSettings{ + Enabled: new(true), + EnabledForInstalledApps: new(false), + } + }) + + It("does not call any setting mutations", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "SetOrgIPAllowListEnabled")).To(BeEmpty()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "SetOrgIPAllowListForInstalledAppsEnabled")).To(BeEmpty()) + }) + }) + + Context("when entries is nil", func() { + BeforeEach(func() { + current.Entries = []ghclient.IPAllowListEntry{ + {ID: "id-1", AllowListValue: "203.0.113.1", Name: "Existing", IsActive: true}, + } + ipSpec = &v1alpha1.IpAllowListSettings{Enabled: new(true)} // Entries omitted + }) + + It("leaves existing entries untouched", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "CreateOrgIPAllowListEntry")).To(BeEmpty()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "UpdateOrgIPAllowListEntry")).To(BeEmpty()) + Expect(callsOfMethod(gqlClient.GraphQLCalls, "DeleteOrgIPAllowListEntry")).To(BeEmpty()) + }) + }) + + Context("when an empty entries list is given", func() { + BeforeEach(func() { + current.Entries = []ghclient.IPAllowListEntry{ + {ID: "id-1", AllowListValue: "203.0.113.1", Name: "Existing", IsActive: true}, + {ID: "app-1", AllowListValue: "203.0.113.9", Name: "Managed by the Acme GitHub App.", IsActive: true}, + } + ipSpec = &v1alpha1.IpAllowListSettings{Entries: []v1alpha1.IpAllowListEntry{}} + }) + + It("deletes organization-owned entries but preserves App-managed ones", func() { + Expect(err).NotTo(HaveOccurred()) + deletes := callsOfMethod(gqlClient.GraphQLCalls, "DeleteOrgIPAllowListEntry") + Expect(deletes).To(HaveLen(1)) + Expect(deletes[0].EntryID).To(Equal("id-1")) + }) + }) +}) diff --git a/internal/reconciler/orgrec/reconciler.go b/internal/reconciler/orgrec/reconciler.go index 9a81672..134551b 100644 --- a/internal/reconciler/orgrec/reconciler.go +++ b/internal/reconciler/orgrec/reconciler.go @@ -62,6 +62,7 @@ func (o *GitHubOrgReconciler) RequiredReconciliations() []reconciler.ParallelRec {Function: o.reconcileRulesetPresets, Condition: conditions.TypeRulesetsSynced}, {Function: o.reconcileCodeSecurityConfigurations, Condition: conditions.TypeCodeSecurityConfigurationsSynced}, {Function: o.reconcileActionsSettings, Condition: conditions.TypeActionsConfigurationSynced}, + {Function: o.reconcileIpAllowList, Condition: conditions.TypeIpAllowListSynced}, }, } } diff --git a/internal/reconciler/orgrec/reconciler_test.go b/internal/reconciler/orgrec/reconciler_test.go index 6bcf61b..acefcdc 100644 --- a/internal/reconciler/orgrec/reconciler_test.go +++ b/internal/reconciler/orgrec/reconciler_test.go @@ -571,6 +571,6 @@ var _ = Describe("RequiredReconciliations", func() { groups := rec.RequiredReconciliations() Expect(groups).To(HaveLen(1)) // All reconcilers run in parallel; plan-based checks are handled within each reconciler - Expect(groups[0]).To(HaveLen(5)) + Expect(groups[0]).To(HaveLen(6)) }) }) diff --git a/internal/reconciler/reconcilerfactory/factory.go b/internal/reconciler/reconcilerfactory/factory.go index 5ca766a..9c8fa55 100644 --- a/internal/reconciler/reconcilerfactory/factory.go +++ b/internal/reconciler/reconcilerfactory/factory.go @@ -65,6 +65,11 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name return nil, err } + ghGraphQLClient, err := f.ClientManager.GetGraphQLClient(ctx, org.GetLogin(), appConfig) + if err != nil { + return nil, err + } + nameResolverOrg, err := reconciler.NewGitHubIDResolver(ctx, ghClient, org.GetLogin()) if err != nil { log.Error(err, "Failed to warm name resolver for Organization", "organization", org.GetLogin()) @@ -79,8 +84,9 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name CurrentSubResourceGenerations: subResourceGenerations, }, GitHub: reconciler.GitHub[string]{ - Client: ghClient, - Resource: org.GetLogin(), + Client: ghClient, + GraphQLClient: ghGraphQLClient, + Resource: org.GetLogin(), }, Features: f.Config.Features, IdResolver: nameResolverOrg, diff --git a/internal/reconciler/types.go b/internal/reconciler/types.go index 94ba027..71511f3 100644 --- a/internal/reconciler/types.go +++ b/internal/reconciler/types.go @@ -30,8 +30,9 @@ type SpreadManager interface { } type GitHub[T any] struct { - Client ghclient.GitHubClient - Resource T + Client ghclient.GitHubClient + GraphQLClient ghclient.GraphQLClient + Resource T } type GitHubTeamIdentifier struct { diff --git a/schemas/organization_v1alpha1.json b/schemas/organization_v1alpha1.json index 52bc73e..f4479c7 100644 --- a/schemas/organization_v1alpha1.json +++ b/schemas/organization_v1alpha1.json @@ -272,6 +272,57 @@ "minimum": 1, "type": "integer" }, + "ipAllowList": { + "description": "IpAllowList configures the organization-level IP allow list (GitHub Enterprise Cloud only).\nWhen nil, IP allow list reconciliation is skipped entirely, leaving all settings and entries\nuntouched. Only organization-owned entries and settings are managed; enterprise-inherited and\nGitHub App-managed entries are always preserved.\nSee: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization", + "properties": { + "enabled": { + "default": false, + "description": "Enabled determines whether the IP allow list is enforced for the organization.\nEntries are always reconciled first, so that enabling enforcement never locks out the\ncurrently configured (active) addresses.", + "type": "boolean" + }, + "enabledForInstalledApps": { + "default": false, + "description": "EnabledForInstalledApps determines whether IP addresses configured for installed GitHub Apps\nare automatically added to the allow list. This takes effect independently of Enabled.\nThe resulting App-managed entries are read-only and are not managed via the Entries field.", + "type": "boolean" + }, + "entries": { + "description": "Entries is the desired set of organization-owned IP allow list entries, keyed by their\nAllowListValue. Entries present in GitHub but not listed here are deleted (except read-only\nenterprise-inherited and App-managed entries, which are always preserved). Omitting this\nfield (nil) manages only the enabled settings and leaves organization-owned entries untouched;\nprovide an empty list to explicitly remove all organization-owned entries.", + "items": { + "description": "IpAllowListEntry defines a single entry in an organization's IP allow list.\nEach entry allows access from a single IP address or a range of addresses in CIDR notation.\nSee: https://docs.github.com/en/organizations/keeping-your-organization-secure/managing-security-settings-for-your-organization/managing-allowed-ip-addresses-for-your-organization", + "properties": { + "allowListValue": { + "description": "AllowListValue is an IP address or a range of addresses in CIDR notation (e.g. \"192.0.2.1\" or \"192.0.2.0/24\").\nA range covering the entire address space (such as \"0.0.0.0/0\" or \"::/0\") is rejected by GitHub;\nto allow access from anywhere, disable the allow list via Enabled=false instead.", + "maxLength": 43, + "minLength": 1, + "type": "string" + }, + "isActive": { + "default": true, + "description": "IsActive determines whether this entry is enforced while the IP allow list is enabled.\nInactive entries remain configured but are ignored until activated.", + "type": "boolean" + }, + "name": { + "description": "Name is an optional human-readable description of the entry, shown in the GitHub UI.", + "maxLength": 255, + "type": "string" + } + }, + "required": [ + "allowListValue" + ], + "type": "object", + "additionalProperties": false + }, + "type": "array", + "x-kubernetes-list-map-keys": [ + "allowListValue" + ], + "x-kubernetes-list-type": "map" + } + }, + "type": "object", + "additionalProperties": false + }, "location": { "description": "Location is the organization's location (e.g., \"Munich, Germany\").\nThis appears on the organization's GitHub profile page.", "maxLength": 100, diff --git a/test/mock/ghclientmock/mock_factory.go b/test/mock/ghclientmock/mock_factory.go index 4140e44..6fceac7 100644 --- a/test/mock/ghclientmock/mock_factory.go +++ b/test/mock/ghclientmock/mock_factory.go @@ -18,11 +18,14 @@ type GitHubMockClientFactory struct { func NewGitHubMockClientFactory(mockClient *MockGitHubClientWrapper) *GitHubMockClientFactory { return &GitHubMockClientFactory{ mockClient: mockClient, + // Provide a default no-op GraphQL client so callers that only exercise the REST path + // (the common case) do not need to set one explicitly. Override via SetGraphQLClient. + mockGraphQLClient: NewMockGraphQLClient(), } } -// SetGraphQLClient sets the GraphQL client returned by GetGraphQLClient. It is optional; when -// unset, GetGraphQLClient returns an error, mirroring the unset REST client behaviour. +// SetGraphQLClient overrides the GraphQL client returned by GetGraphQLClient. By default the +// factory already provides a no-op MockGraphQLClient; use this to inject a configured one. func (m *GitHubMockClientFactory) SetGraphQLClient(mockGraphQLClient ghclient.GraphQLClient) { m.mockGraphQLClient = mockGraphQLClient } diff --git a/test/mock/ghclientmock/mock_graphql.go b/test/mock/ghclientmock/mock_graphql.go index 03724ae..d161b01 100644 --- a/test/mock/ghclientmock/mock_graphql.go +++ b/test/mock/ghclientmock/mock_graphql.go @@ -5,12 +5,23 @@ import ( "context" "sync" - "github.com/shurcooL/githubv4" + "github.com/Interhyp/git-hubby/internal/ghclient" ) // GraphQLCall records a single invocation of the mock GraphQL client for assertions in tests. type GraphQLCall struct { - Method string // "Query" or "Mutate" + // Method is the invoked method name, e.g. "Query", "Mutate", + // "CreateOrgIPAllowListEntry", "SetOrgIPAllowListEnabled". + Method string + // The following fields carry the salient arguments for IP allow list methods; unused fields + // remain zero-valued for other calls. + OrgLogin string + OwnerID string + EntryID string + Value string + Name string + IsActive bool + Enabled bool } // MockGraphQLClient is a hand-written, function-field mock of ghclient.GraphQLClient, following @@ -18,7 +29,14 @@ type GraphQLCall struct { // injected Func if set, otherwise returns a sensible default (nil error / no-op). type MockGraphQLClient struct { QueryFunc func(ctx context.Context, q any, variables map[string]any) error - MutateFunc func(ctx context.Context, m any, input githubv4.Input, variables map[string]any) error + MutateFunc func(ctx context.Context, m any, input ghclient.GraphQLInput, variables map[string]any) error + + GetOrgIPAllowListFunc func(ctx context.Context, orgLogin string) (*ghclient.OrgIPAllowListConfig, error) + CreateOrgIPAllowListEntryFunc func(ctx context.Context, ownerID string, value string, name string, isActive bool) error + UpdateOrgIPAllowListEntryFunc func(ctx context.Context, entryID string, value string, name string, isActive bool) error + DeleteOrgIPAllowListEntryFunc func(ctx context.Context, entryID string) error + SetOrgIPAllowListEnabledFunc func(ctx context.Context, ownerID string, enabled bool) error + SetOrgIPAllowListForInstalledAppsEnabledFunc func(ctx context.Context, ownerID string, enabled bool) error mu sync.Mutex GraphQLCalls []GraphQLCall @@ -47,10 +65,64 @@ func (m *MockGraphQLClient) Query(ctx context.Context, q any, variables map[stri } // Mutate records the call and delegates to MutateFunc if configured. -func (m *MockGraphQLClient) Mutate(ctx context.Context, mutation any, input githubv4.Input, variables map[string]any) error { +func (m *MockGraphQLClient) Mutate(ctx context.Context, mutation any, input ghclient.GraphQLInput, variables map[string]any) error { m.recordCall(GraphQLCall{Method: "Mutate"}) if m.MutateFunc != nil { return m.MutateFunc(ctx, mutation, input, variables) } return nil } + +// GetOrgIPAllowList records the call and delegates to GetOrgIPAllowListFunc if configured. +func (m *MockGraphQLClient) GetOrgIPAllowList(ctx context.Context, orgLogin string) (*ghclient.OrgIPAllowListConfig, error) { + m.recordCall(GraphQLCall{Method: "GetOrgIPAllowList", OrgLogin: orgLogin}) + if m.GetOrgIPAllowListFunc != nil { + return m.GetOrgIPAllowListFunc(ctx, orgLogin) + } + return &ghclient.OrgIPAllowListConfig{}, nil +} + +// CreateOrgIPAllowListEntry records the call and delegates to CreateOrgIPAllowListEntryFunc if configured. +func (m *MockGraphQLClient) CreateOrgIPAllowListEntry(ctx context.Context, ownerID string, value string, name string, isActive bool) error { + m.recordCall(GraphQLCall{Method: "CreateOrgIPAllowListEntry", OwnerID: ownerID, Value: value, Name: name, IsActive: isActive}) + if m.CreateOrgIPAllowListEntryFunc != nil { + return m.CreateOrgIPAllowListEntryFunc(ctx, ownerID, value, name, isActive) + } + return nil +} + +// UpdateOrgIPAllowListEntry records the call and delegates to UpdateOrgIPAllowListEntryFunc if configured. +func (m *MockGraphQLClient) UpdateOrgIPAllowListEntry(ctx context.Context, entryID string, value string, name string, isActive bool) error { + m.recordCall(GraphQLCall{Method: "UpdateOrgIPAllowListEntry", EntryID: entryID, Value: value, Name: name, IsActive: isActive}) + if m.UpdateOrgIPAllowListEntryFunc != nil { + return m.UpdateOrgIPAllowListEntryFunc(ctx, entryID, value, name, isActive) + } + return nil +} + +// DeleteOrgIPAllowListEntry records the call and delegates to DeleteOrgIPAllowListEntryFunc if configured. +func (m *MockGraphQLClient) DeleteOrgIPAllowListEntry(ctx context.Context, entryID string) error { + m.recordCall(GraphQLCall{Method: "DeleteOrgIPAllowListEntry", EntryID: entryID}) + if m.DeleteOrgIPAllowListEntryFunc != nil { + return m.DeleteOrgIPAllowListEntryFunc(ctx, entryID) + } + return nil +} + +// SetOrgIPAllowListEnabled records the call and delegates to SetOrgIPAllowListEnabledFunc if configured. +func (m *MockGraphQLClient) SetOrgIPAllowListEnabled(ctx context.Context, ownerID string, enabled bool) error { + m.recordCall(GraphQLCall{Method: "SetOrgIPAllowListEnabled", OwnerID: ownerID, Enabled: enabled}) + if m.SetOrgIPAllowListEnabledFunc != nil { + return m.SetOrgIPAllowListEnabledFunc(ctx, ownerID, enabled) + } + return nil +} + +// SetOrgIPAllowListForInstalledAppsEnabled records the call and delegates to the configured func if set. +func (m *MockGraphQLClient) SetOrgIPAllowListForInstalledAppsEnabled(ctx context.Context, ownerID string, enabled bool) error { + m.recordCall(GraphQLCall{Method: "SetOrgIPAllowListForInstalledAppsEnabled", OwnerID: ownerID, Enabled: enabled}) + if m.SetOrgIPAllowListForInstalledAppsEnabledFunc != nil { + return m.SetOrgIPAllowListForInstalledAppsEnabledFunc(ctx, ownerID, enabled) + } + return nil +}