From f79a6d9bcf7ee939e903c89cdd8f0e7b0992d073 Mon Sep 17 00:00:00 2001 From: tstollin Date: Thu, 18 Jun 2026 10:49:43 +0200 Subject: [PATCH 1/2] feat: add support for multiple GitHub apps --- .github/copilot-instructions.md | 13 +- README.md | 55 ++++++++- .../api/v1alpha1/githubappconfig.go | 56 +++++++++ .../api/v1alpha1/organizationspec.go | 20 ++- api/v1alpha1/applyconfiguration/utils.go | 2 + api/v1alpha1/organization_methods.go | 23 ++++ api/v1alpha1/organization_types.go | 38 +++++- api/v1alpha1/zz_generated.deepcopy.go | 25 ++++ cmd/main.go | 23 ++-- .../github.interhyp.de_organizations.yaml | 33 ++++- docs/configuration/organization.md | 21 +++- docs/crds.md | 22 +++- .../controller/organization_controller.go | 29 +++-- .../organization_controller_test.go | 1 + .../controller/repository_controller_test.go | 1 + internal/controller/team_controller_test.go | 1 + internal/controller/test_helpers_test.go | 4 + internal/ghclient/factory.go | 115 ++++++++++-------- internal/reconciler/executor_test.go | 34 +++--- .../orgrec/rec_actions_settings_test.go | 18 +-- .../rec_code_security_configurations_test.go | 8 +- .../orgrec/rec_custom_properties_test.go | 4 +- internal/reconciler/orgrec/rec_org_test.go | 2 +- .../reconciler/orgrec/rec_rulesets_test.go | 4 +- internal/reconciler/orgrec/reconciler_test.go | 4 +- .../reconciler/reconcilerfactory/factory.go | 28 ++++- .../reconcilerfactory/factory_test.go | 59 +++------ internal/reconciler/reporec/rec_repo_test.go | 2 +- internal/reconciler/types.go | 3 +- .../webhook/v1alpha1/organization_webhook.go | 12 ++ .../v1alpha1/organization_webhook_test.go | 60 ++++++++- .../webhook/v1alpha1/repository_webhook.go | 20 ++- .../v1alpha1/repository_webhook_test.go | 8 +- .../webhook/v1alpha1/webhook_suite_test.go | 2 +- test/mock/ghclientmock/mock_factory.go | 5 +- 35 files changed, 572 insertions(+), 183 deletions(-) create mode 100644 api/v1alpha1/applyconfiguration/api/v1alpha1/githubappconfig.go diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index a6b07dd..1331cec 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -16,7 +16,11 @@ These instructions help AI agents work productively in this **Kubebuilder v4**-b - [internal/reconciler/orgrec](internal/reconciler/orgrec) for organizations - manages org settings, custom properties, rulesets, code security configurations, and actions settings - [internal/reconciler/reporec](internal/reconciler/reporec) for repositories - manages repo settings, webhooks, and rulesets - [internal/reconciler/teamrec](internal/reconciler/teamrec) for teams - manages team creation, membership, and multi-organization support -- GitHub integration via a cached client factory using GitHub App credentials from a Kubernetes Secret; see [internal/ghclient/factory.go](internal/ghclient/factory.go), [internal/ghclient/wrapper.go](internal/ghclient/wrapper.go), [internal/ghclient/interface.go](internal/ghclient/interface.go). +- GitHub integration via a cached client factory that supports **multiple GitHub Apps** — each organization can reference its own credentials secret via `spec.githubAppConfig`; see [internal/ghclient/factory.go](internal/ghclient/factory.go), [internal/ghclient/wrapper.go](internal/ghclient/wrapper.go), [internal/ghclient/interface.go](internal/ghclient/interface.go). + - `CachingGitHubClientFactory` caches credentials per secret name and clients per organization (`cacheKey`); rate-limit state is shared per GitHub App ID. + - `SecretProviderFunc` now takes a `secretName string` parameter so any secret in the credentials namespace can be fetched on demand. + - `GetClient(ctx, cacheKey, app v1alpha1.GitHubAppConfig)` and `GetGitHubClientAndCheckRateLimit(...)` accept a `GitHubAppConfig` struct (containing `InstallationId` and `CredentialsSecretName`). + - When `spec.githubAppConfig` is set on an `Organization`, it takes precedence; otherwise the operator falls back to `spec.githubAppInstallationId` (deprecated) combined with the default secret name from `--app-credentials-secret-name`. - Status conditions are set consistently via helpers; see [internal/conditions/conditions.go](internal/conditions/conditions.go). Finalizers gate deletion; see reconciler files. - **Validation-only webhooks** enforce spec rules (e.g., organization custom properties, repository references); see [internal/webhook/v1alpha1/organization_webhook.go](internal/webhook/v1alpha1/organization_webhook.go) and [internal/webhook/v1alpha1/repository_webhook.go](internal/webhook/v1alpha1/repository_webhook.go). **Mutating webhooks have been removed**; labels are now applied in reconcilers. **Note**: Team CRD has no webhook currently. @@ -164,7 +168,12 @@ Controllers implement continuous drift detection: ## Configuration & Secrets - The manager reads flags for secret location and TLS; see [cmd/main.go](cmd/main.go) for `--app-credentials-secret-namespace` (default `github-controller`) and `--app-credentials-secret-name` (default `git-hubby-app-credentials`). -- Required secret keys: `app-id`, `private-key` (PEM RSA); parsed in [internal/ghclient/factory.go](internal/ghclient/factory.go). The GitHub App Installation ID is provided per-organization via `Organization.Spec.GitHubAppInstallationId`. +- Required secret keys: `app-id`, `private-key` (PEM RSA); parsed in [internal/ghclient/factory.go](internal/ghclient/factory.go). +- **Multiple GitHub Apps**: Each `Organization` CR can reference a different credentials Secret via `spec.githubAppConfig.credentialsSecretName`. All referenced secrets must reside in the namespace set by `--app-credentials-secret-namespace`. The operator fetches credentials lazily on first use and caches them per secret name. + - `spec.githubAppConfig` (preferred): specify both `installationId` and `credentialsSecretName`. + - `spec.githubAppInstallationId` (deprecated): uses the default secret from `--app-credentials-secret-name`; still supported for backward compatibility. + - If both are set, `githubAppConfig` takes precedence. +- **Secret rotation**: Updating a credentials Secret does **not** automatically invalidate the in-memory client cache. A pod restart is required to pick up rotated credentials. - Metrics and webhook TLS can be configured via flags; HTTP/2 is disabled by default for security. - **Log level**: Configurable via `LOG_LEVEL` environment variable (accepts `debug`, `info`, `warn`, `error`; case-insensitive). Overrides the `--zap-log-level` CLI flag. Can also be set in `.env` file. diff --git a/README.md b/README.md index 8b6fcaf..314b450 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ A Kubernetes operator for managing GitHub organizations and repositories as code ### Key Features - **Declarative GitHub Management**: Define organizations and repositories as Kubernetes resources -- **GitHub App Integration**: Secure authentication using GitHub App credentials +- **Multiple GitHub Apps**: Each organization can reference its own GitHub App credentials secret, enabling multi-tenant and multi-app setups - **Multi-Plan Support**: Works with GitHub `free`, `team`, and `enterprise` plans — plan-gated features are automatically skipped when not available - **Advanced Features**: Manage repository rulesets, webhooks, organization custom properties, and code security configurations - **Rate Limit Awareness**: Built-in GitHub API rate limit handling with intelligent backoff @@ -91,13 +91,17 @@ The log output format can be configured via: ### GitHub App Credentials -The operator requires a Kubernetes Secret with GitHub App credentials: +The operator authenticates with GitHub using GitHub App credentials stored in Kubernetes Secrets. Each organization can reference its own credentials secret, enabling multiple GitHub Apps across organizations. + +#### Secret Format + +Create one or more Secrets, each containing credentials for a GitHub App: ```yaml apiVersion: v1 kind: Secret metadata: - name: git-hubby-app-credentials + name: git-hubby-app-credentials # default secret name namespace: github-controller type: Opaque stringData: @@ -108,11 +112,52 @@ stringData: -----END RSA PRIVATE KEY----- ``` -**Secret location** is configurable via flags: +All credential secrets must reside in the same namespace, configured via: - `--app-credentials-secret-namespace` (default: `github-controller`) + +The default secret name is configured via: - `--app-credentials-secret-name` (default: `git-hubby-app-credentials`) -**GitHub App Installation ID** is provided per-organization in the `Organization` resource spec. +#### Per-Organization App Configuration (recommended) + +Use `spec.githubAppConfig` on the `Organization` resource to specify both the installation ID and which credentials secret to use: + +```yaml +spec: + githubAppConfig: + installationId: 12345678 + credentialsSecretName: my-org-app-credentials # references a Secret in the credentials namespace +``` + +This is the preferred approach and supports multiple GitHub Apps across different organizations. + +#### Legacy: Single App via Installation ID (deprecated) + +The older `spec.githubAppInstallationId` field is still supported for backward compatibility. When set alone, it uses the default credentials secret configured via `--app-credentials-secret-name`: + +```yaml +spec: + githubAppInstallationId: 12345678 # deprecated; use githubAppConfig instead +``` + +If both `githubAppConfig` and `githubAppInstallationId` are set, `githubAppConfig` takes precedence. + +#### Secret Rotation + +Updating a credentials Secret in Kubernetes does **not** automatically invalidate the operator's in-memory client cache. To force the operator to pick up rotated credentials, restart the operator pod. + +For automated rotation workflows, we recommend [Stakater Reloader](https://github.com/stakater/Reloader). Add the reload annotation to the operator `Deployment` and list the Secret(s) that should trigger a restart: + +```yaml +apiVersion: apps/v1 +kind: Deployment +metadata: + name: git-hubby-controller-manager + annotations: + secret.reloader.stakater.com/reload: "git-hubby-app-credentials,my-org-app-credentials" +``` + +When Reloader detects a change in any of the listed Secrets, it rolls the Deployment, causing the operator to restart and re-fetch fresh credentials. ## Architecture Highlights diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/githubappconfig.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/githubappconfig.go new file mode 100644 index 0000000..e063432 --- /dev/null +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/githubappconfig.go @@ -0,0 +1,56 @@ +/* +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 + +// GitHubAppConfigApplyConfiguration represents a declarative configuration of the GitHubAppConfig type for use +// with apply. +// +// GitHubAppConfig defines the GitHub App configuration for an organization, referencing the +// Kubernetes Secret that holds the app credentials by name. The secret must reside in the +// namespace configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. +type GitHubAppConfigApplyConfiguration struct { + // InstallationId is the numeric ID of the GitHub App installation for this organization. + // You can find this ID in your GitHub App's installation settings or via the GitHub API. + InstallationId *int64 `json:"installationId,omitempty"` + // CredentialsSecretName is the name of the Kubernetes Secret containing the GitHub App credentials. + // The secret must contain the keys `app-id` and `private-key` and must reside in the namespace + // configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. + CredentialsSecretName *string `json:"credentialsSecretName,omitempty"` +} + +// GitHubAppConfigApplyConfiguration constructs a declarative configuration of the GitHubAppConfig type for use with +// apply. +func GitHubAppConfig() *GitHubAppConfigApplyConfiguration { + return &GitHubAppConfigApplyConfiguration{} +} + +// WithInstallationId sets the InstallationId 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 InstallationId field is set to the value of the last call. +func (b *GitHubAppConfigApplyConfiguration) WithInstallationId(value int64) *GitHubAppConfigApplyConfiguration { + b.InstallationId = &value + return b +} + +// WithCredentialsSecretName sets the CredentialsSecretName 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 CredentialsSecretName field is set to the value of the last call. +func (b *GitHubAppConfigApplyConfiguration) WithCredentialsSecretName(value string) *GitHubAppConfigApplyConfiguration { + b.CredentialsSecretName = &value + return b +} diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go index 0586838..4dbfd93 100644 --- a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go @@ -40,9 +40,17 @@ type OrganizationSpecApplyConfiguration struct { // At least one of Login or Name must be specified. Name *string `json:"name,omitempty"` // GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization. - // This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's - // installation settings or via the GitHub API. + // This field is deprecated. Use GitHubAppConfig instead, which also allows specifying which + // credential secret to use. When only this field is set, the operator falls back to the + // secret name configured via --app-credentials-secret-name. + // At least one of GitHubAppInstallationId or GitHubAppConfig must be set. + // If both are set, GitHubAppConfig takes precedence. GitHubAppInstallationId *int64 `json:"githubAppInstallationId,omitempty"` + // GitHubAppConfig specifies the GitHub App installation and credentials secret to use for + // authenticating API requests on behalf of this organization. + // At least one of GitHubAppConfig or GitHubAppInstallationId must be set. + // If both are set, GitHubAppConfig takes precedence. + GitHubAppConfig *GitHubAppConfigApplyConfiguration `json:"githubAppConfig,omitempty"` // CustomProperties defines custom metadata properties that can be assigned to repositories in the organization. // These properties allow you to categorize and add structured metadata to your repositories. // See: https://docs.github.com/en/rest/orgs/custom-properties @@ -103,6 +111,14 @@ func (b *OrganizationSpecApplyConfiguration) WithGitHubAppInstallationId(value i return b } +// WithGitHubAppConfig sets the GitHubAppConfig 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 GitHubAppConfig field is set to the value of the last call. +func (b *OrganizationSpecApplyConfiguration) WithGitHubAppConfig(value *GitHubAppConfigApplyConfiguration) *OrganizationSpecApplyConfiguration { + b.GitHubAppConfig = value + return b +} + // WithCustomProperties adds the given value to the CustomProperties 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 CustomProperties field. diff --git a/api/v1alpha1/applyconfiguration/utils.go b/api/v1alpha1/applyconfiguration/utils.go index 6aea365..547acd5 100644 --- a/api/v1alpha1/applyconfiguration/utils.go +++ b/api/v1alpha1/applyconfiguration/utils.go @@ -67,6 +67,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1alpha1.DependencyGraphAutosubmitActionOptionsApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("DeployKey"): return &apiv1alpha1.DeployKeyApplyConfiguration{} + case v1alpha1.SchemeGroupVersion.WithKind("GitHubAppConfig"): + return &apiv1alpha1.GitHubAppConfigApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("MergeStrategy"): return &apiv1alpha1.MergeStrategyApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("Organization"): diff --git a/api/v1alpha1/organization_methods.go b/api/v1alpha1/organization_methods.go index fa5cdec..9c5fb45 100644 --- a/api/v1alpha1/organization_methods.go +++ b/api/v1alpha1/organization_methods.go @@ -1,6 +1,8 @@ package v1alpha1 import ( + "fmt" + "github.com/Interhyp/git-hubby/internal/conditions" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -136,3 +138,24 @@ func (o *Organization) IsUsingLegacyNameField() bool { } return o.Spec.Login == "" && o.Spec.Name != "" } + +// ResolveGitHubAppConfig resolves the effective GitHubAppConfig for this Organization. +// If GitHubAppConfig is set, it is returned directly (it takes precedence). +// Otherwise, if the deprecated GitHubAppInstallationId field is set, a GitHubAppConfig is +// synthesised using the provided legacySecretName as the credentials secret. +// Returns an error if neither field is set. +func (in *Organization) ResolveGitHubAppConfig(legacySecretName string) (*GitHubAppConfig, error) { + if in == nil { + return nil, fmt.Errorf("organization is nil") + } + if in.Spec.GitHubAppConfig != nil { + return in.Spec.GitHubAppConfig, nil + } + if in.Spec.GitHubAppInstallationId != nil { + return &GitHubAppConfig{ + InstallationId: *in.Spec.GitHubAppInstallationId, + CredentialsSecretName: legacySecretName, + }, nil + } + return nil, fmt.Errorf("organization %s/%s has neither githubAppConfig nor githubAppInstallationId set", in.Namespace, in.Name) +} diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go index fbcc057..0bd18de 100644 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -22,6 +22,8 @@ import ( ) // GitHubAppCredentials defines the GitHub App credentials configuration for authenticating with the GitHub API. +// +// Deprecated: Use GitHubAppConfig instead, which allows per-organization credential secrets. type GitHubAppCredentials struct { // SecretRef is a reference to a Kubernetes Secret containing GitHub App credentials. // The secret must contain the following keys: @@ -30,6 +32,24 @@ type GitHubAppCredentials struct { SecretRef v1.LocalObjectReference `json:"secretRef"` } +// GitHubAppConfig defines the GitHub App configuration for an organization, referencing the +// Kubernetes Secret that holds the app credentials by name. The secret must reside in the +// namespace configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. +type GitHubAppConfig struct { + // InstallationId is the numeric ID of the GitHub App installation for this organization. + // You can find this ID in your GitHub App's installation settings or via the GitHub API. + // +kubebuilder:validation:Required + // +kubebuilder:validation:Minimum=1 + InstallationId int64 `json:"installationId"` + + // CredentialsSecretName is the name of the Kubernetes Secret containing the GitHub App credentials. + // The secret must contain the keys `app-id` and `private-key` and must reside in the namespace + // configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 + CredentialsSecretName string `json:"credentialsSecretName"` +} + // OrgCustomPropertyDefaultValue defines the default value for an organization custom property. // Either Value (for single values) or Values (for multi-select) must be set, but not both. // +kubebuilder:validation:ExactlyOneOf=value;values @@ -298,11 +318,21 @@ type OrganizationSpec struct { Name string `json:"name,omitempty"` // GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization. - // This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's - // installation settings or via the GitHub API. - // +kubebuilder:validation:Required + // This field is deprecated. Use GitHubAppConfig instead, which also allows specifying which + // credential secret to use. When only this field is set, the operator falls back to the + // secret name configured via --app-credentials-secret-name. + // At least one of GitHubAppInstallationId or GitHubAppConfig must be set. + // If both are set, GitHubAppConfig takes precedence. // +kubebuilder:validation:Minimum=1 - GitHubAppInstallationId int64 `json:"githubAppInstallationId"` + // +optional + GitHubAppInstallationId *int64 `json:"githubAppInstallationId,omitempty"` + + // GitHubAppConfig specifies the GitHub App installation and credentials secret to use for + // authenticating API requests on behalf of this organization. + // At least one of GitHubAppConfig or GitHubAppInstallationId must be set. + // If both are set, GitHubAppConfig takes precedence. + // +optional + GitHubAppConfig *GitHubAppConfig `json:"githubAppConfig,omitempty"` // CustomProperties defines custom metadata properties that can be assigned to repositories in the organization. // These properties allow you to categorize and add structured metadata to your repositories. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 83a04c1..d2db3a7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -620,6 +620,21 @@ func (in *DeployKey) DeepCopy() *DeployKey { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GitHubAppConfig) DeepCopyInto(out *GitHubAppConfig) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitHubAppConfig. +func (in *GitHubAppConfig) DeepCopy() *GitHubAppConfig { + if in == nil { + return nil + } + out := new(GitHubAppConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitHubAppCredentials) DeepCopyInto(out *GitHubAppCredentials) { *out = *in @@ -788,6 +803,16 @@ func (in *OrganizationRef) DeepCopy() *OrganizationRef { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OrganizationSpec) DeepCopyInto(out *OrganizationSpec) { *out = *in + if in.GitHubAppInstallationId != nil { + in, out := &in.GitHubAppInstallationId, &out.GitHubAppInstallationId + *out = new(int64) + **out = **in + } + if in.GitHubAppConfig != nil { + in, out := &in.GitHubAppConfig, &out.GitHubAppConfig + *out = new(GitHubAppConfig) + **out = **in + } if in.CustomProperties != nil { in, out := &in.CustomProperties, &out.CustomProperties *out = make([]OrgCustomProperty, len(*in)) diff --git a/cmd/main.go b/cmd/main.go index 78620db..7d953e6 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -360,20 +360,21 @@ func main() { os.Exit(1) } - fetchGitHubAppSecret := func(ctx context.Context) (*v1.Secret, error) { + // fetchSecret is a generic closure that fetches any secret by name from the credentials namespace. + fetchSecret := func(ctx context.Context, secretName string) (*v1.Secret, error) { log := ctrl.LoggerFrom(ctx) - var appCredentialsSecret v1.Secret - secretName := client.ObjectKey{ - Name: appCredentialsSecretName, + var secret v1.Secret + secretKey := client.ObjectKey{ + Name: secretName, Namespace: appCredentialsSecretNamespace, } - if fetchErr := directClient.Get(ctx, secretName, &appCredentialsSecret); fetchErr != nil { - log.Error(fetchErr, "Failed to fetch GitHub App credentials secret") + if fetchErr := directClient.Get(ctx, secretKey, &secret); fetchErr != nil { + log.Error(fetchErr, "Failed to fetch secret", "secretName", secretName) return nil, fetchErr } - return &appCredentialsSecret, nil + return &secret, nil } - clientManager, err := ghclient.NewGitHubCachingClientFactory(ghclient.DefaultClientConfig(), fetchGitHubAppSecret) + clientManager, err := ghclient.NewGitHubCachingClientFactory(ghclient.DefaultClientConfig(), fetchSecret) if err != nil { setupLog.Error(err, "failed to create GitHub client factory") os.Exit(1) @@ -389,6 +390,7 @@ func main() { ClientManager: clientManager, SpreadingManager: spreadingManager, K8sClient: mgr.GetClient(), + LegacySecretName: appCredentialsSecretName, } webhooksEnabled := os.Getenv("ENABLE_WEBHOOKS") != "false" @@ -404,6 +406,7 @@ func main() { ReconcilerFactory: reconcilerFactory, GithubRateLimiter: globalLimiter, SuccessRequeueInterval: spreadingManager.GetSpreadInterval(), + LegacySecretName: appCredentialsSecretName, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Organization") os.Exit(1) @@ -432,7 +435,9 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "Organization") os.Exit(1) } - if err := webhookv1alpha1.SetupRepositoryWebhookWithManager(mgr, clientManager); err != nil { + if err := webhookv1alpha1.SetupRepositoryWebhookWithManager( + mgr, clientManager, appCredentialsSecretName, + ); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Repository") os.Exit(1) } diff --git a/config/crd/bases/github.interhyp.de_organizations.yaml b/config/crd/bases/github.interhyp.de_organizations.yaml index ed33d20..3fddb74 100644 --- a/config/crd/bases/github.interhyp.de_organizations.yaml +++ b/config/crd/bases/github.interhyp.de_organizations.yaml @@ -317,11 +317,39 @@ spec: Description is a human-readable description of the organization. This appears on the organization's GitHub profile page. type: string + githubAppConfig: + description: |- + GitHubAppConfig specifies the GitHub App installation and credentials secret to use for + authenticating API requests on behalf of this organization. + At least one of GitHubAppConfig or GitHubAppInstallationId must be set. + If both are set, GitHubAppConfig takes precedence. + properties: + credentialsSecretName: + description: |- + CredentialsSecretName is the name of the Kubernetes Secret containing the GitHub App credentials. + The secret must contain the keys `app-id` and `private-key` and must reside in the namespace + configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. + minLength: 1 + type: string + installationId: + description: |- + InstallationId is the numeric ID of the GitHub App installation for this organization. + You can find this ID in your GitHub App's installation settings or via the GitHub API. + format: int64 + minimum: 1 + type: integer + required: + - credentialsSecretName + - installationId + type: object githubAppInstallationId: description: |- GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization. - This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's - installation settings or via the GitHub API. + This field is deprecated. Use GitHubAppConfig instead, which also allows specifying which + credential secret to use. When only this field is set, the operator falls back to the + secret name configured via --app-credentials-secret-name. + At least one of GitHubAppInstallationId or GitHubAppConfig must be set. + If both are set, GitHubAppConfig takes precedence. format: int64 minimum: 1 type: integer @@ -391,7 +419,6 @@ spec: - actionsSettings - codeSecurityConfigurations - description - - githubAppInstallationId type: object status: description: status defines the observed state of Organization diff --git a/docs/configuration/organization.md b/docs/configuration/organization.md index 35e2d93..6bc2c82 100644 --- a/docs/configuration/organization.md +++ b/docs/configuration/organization.md @@ -22,7 +22,9 @@ spec: website: "https://engineering.acme-corp.com" # --- Authentication --- - githubAppInstallationId: 12345678 # From GitHub App settings + githubAppConfig: + installationId: 12345678 # From GitHub App installation settings + credentialsSecretName: acme-corp-app-credentials # Secret in credentials namespace plan: enterprise # enterprise | team | free # --- Custom Properties --- @@ -102,6 +104,23 @@ spec: ## Key Concepts +### Authentication + +Each `Organization` resource must specify how the operator authenticates with GitHub. Use `spec.githubAppConfig` (recommended) to select both the installation ID and the credentials secret: + +```yaml +spec: + githubAppConfig: + installationId: 12345678 + credentialsSecretName: acme-corp-app-credentials # Secret in credentials namespace +``` + +This allows different organizations to use different GitHub Apps, enabling multi-tenant setups. + +The legacy `spec.githubAppInstallationId` field is still supported for backward compatibility; it falls back to the default credentials secret configured via `--app-credentials-secret-name`. If both fields are set, `githubAppConfig` takes precedence. + +See the [README](../../README.md#github-app-credentials) for details on creating the credentials Secret. + ### Custom Properties Custom properties let you attach structured metadata to all repositories in your organization. diff --git a/docs/crds.md b/docs/crds.md index a045eba..18a855b 100644 --- a/docs/crds.md +++ b/docs/crds.md @@ -452,6 +452,25 @@ _Appears in:_ | `readOnly` _boolean_ | ReadOnly determines the access level for this deploy key.
- true: Key can only read from the repository (cannot push)
- false: Key can read and write to the repository (can push commits) | true | Type: boolean
| +#### GitHubAppConfig + + + +GitHubAppConfig defines the GitHub App configuration for an organization, referencing the +Kubernetes Secret that holds the app credentials by name. The secret must reside in the +namespace configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. + + + +_Appears in:_ +- [OrganizationSpec](#organizationspec) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `installationId` _integer_ | InstallationId is the numeric ID of the GitHub App installation for this organization.
You can find this ID in your GitHub App's installation settings or via the GitHub API. | | Minimum: 1
Required: \{\}
| +| `credentialsSecretName` _string_ | CredentialsSecretName is the name of the Kubernetes Secret containing the GitHub App credentials.
The secret must contain the keys `app-id` and `private-key` and must reside in the namespace
configured via the APP_CREDENTIALS_SECRET_NAMESPACE environment variable. | | MinLength: 1
Required: \{\}
| + + #### MergeStrategy @@ -594,7 +613,8 @@ _Appears in:_ | --- | --- | --- | --- | | `login` _string_ | Login is the GitHub organization login (the unique, immutable identifier on GitHub).
This field is optional for backwards compatibility. If not specified, the Name field
will be used as both login and display name.
It is recommended to explicitly set this field to clearly separate login from display name. | | MaxLength: 39
MinLength: 1
Optional: \{\}
| | `name` _string_ | Name is the organization's display name shown on the GitHub profile.
If Login is not specified, this field will also be used as the organization login
for backwards compatibility.
At least one of Login or Name must be specified. | | MaxLength: 255
MinLength: 1
Optional: \{\}
| -| `githubAppInstallationId` _integer_ | GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization.
This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's
installation settings or via the GitHub API. | | Minimum: 1
Required: \{\}
| +| `githubAppInstallationId` _integer_ | GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization.
This field is deprecated. Use GitHubAppConfig instead, which also allows specifying which
credential secret to use. When only this field is set, the operator falls back to the
secret name configured via --app-credentials-secret-name.
At least one of GitHubAppInstallationId or GitHubAppConfig must be set.
If both are set, GitHubAppConfig takes precedence. | | Minimum: 1
Optional: \{\}
| +| `githubAppConfig` _[GitHubAppConfig](#githubappconfig)_ | GitHubAppConfig specifies the GitHub App installation and credentials secret to use for
authenticating API requests on behalf of this organization.
At least one of GitHubAppConfig or GitHubAppInstallationId must be set.
If both are set, GitHubAppConfig takes precedence. | | Optional: \{\}
| | `customProperties` _[OrgCustomProperty](#orgcustomproperty) array_ | CustomProperties defines custom metadata properties that can be assigned to repositories in the organization.
These properties allow you to categorize and add structured metadata to your repositories.
See: https://docs.github.com/en/rest/orgs/custom-properties | | MaxItems: 100
| | `actionsSettings` _[ActionsSettings](#actionssettings)_ | ActionsSettings configures GitHub Actions permissions and behavior for the organization.
This includes which repositories can use Actions, which actions are allowed, and runner group configurations.
See: https://docs.github.com/en/rest/actions/permissions | | | | `codeSecurityConfigurations` _[AttachableCodeSecurityConfigurationRef](#attachablecodesecurityconfigurationref) array_ | CodeSecurityConfigurations lists code security configurations to create and optionally attach to repositories.
Each configuration defines security features like dependency scanning, secret scanning, and code scanning.
See: https://docs.github.com/en/rest/code-security/configurations | | | diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go index ae70d1e..277930f 100644 --- a/internal/controller/organization_controller.go +++ b/internal/controller/organization_controller.go @@ -21,21 +21,21 @@ import ( "errors" "time" + githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/ratelimit" "github.com/Interhyp/git-hubby/internal/reconciler/orgrec" "github.com/Interhyp/git-hubby/internal/reconciler/reconcilerfactory" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" - - githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" - "k8s.io/apimachinery/pkg/runtime" - ctrl "sigs.k8s.io/controller-runtime" ) // OrganizationCtl reconciles a Organization object @@ -44,6 +44,9 @@ type OrganizationCtl struct { Scheme *runtime.Scheme ReconcilerFactory *reconcilerfactory.Factory SuccessRequeueInterval time.Duration + // LegacySecretName is the credential secret name used for Organizations that still + // reference the deprecated GitHubAppInstallationId field. + LegacySecretName string } // +kubebuilder:rbac:groups=github.interhyp.de,namespace=github-configuration,resources=organizations,verbs=get;list;watch;create;update;patch;delete @@ -54,13 +57,6 @@ type OrganizationCtl struct { // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the Organization object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.23.3/pkg/reconcile func (r *OrganizationCtl) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logf.FromContext(ctx) ctx, cancel := context.WithTimeout(ctx, 5*time.Minute) @@ -113,7 +109,11 @@ func (r *OrganizationCtl) SetupWithManager(mgr ctrl.Manager) error { return err } return ctrl.NewControllerManagedBy(mgr). - For(&githubv1alpha1.Organization{}). + // Apply the generation/annotation predicate only to the primary Organization resource, + // not to the secondary watches (CSC, RulesetPreset) so each can use its own filter. + For(&githubv1alpha1.Organization{}, + builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.AnnotationChangedPredicate{})), + ). Watches( // changes of csc should trigger reconciliation of orgs using it &githubv1alpha1.CodeSecurityConfiguration{}, handler.EnqueueRequestsFromMapFunc(r.findOrganizationsForCodeSecurityConfiguration), @@ -122,7 +122,10 @@ func (r *OrganizationCtl) SetupWithManager(mgr ctrl.Manager) error { &githubv1alpha1.RulesetPreset{}, handler.EnqueueRequestsFromMapFunc(r.findOrganizationsForRulesetPreset), ). - WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.AnnotationChangedPredicate{})). + // NOTE: credential secrets are fetched directly (non-cached) on each reconciliation; + // no Secret watch is registered so the credentials namespace does not need to be + // added to the manager's cache, avoiding RBAC errors in unrelated namespaces. + // A pod restart is required to pick up rotated credentials. WithOptions(controller.Options{ UsePriorityQueue: new(true), RateLimiter: workqueue.NewTypedMaxOfRateLimiter( diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go index ba6e1f6..926dc96 100644 --- a/internal/controller/organization_controller_test.go +++ b/internal/controller/organization_controller_test.go @@ -51,6 +51,7 @@ var _ = Describe("Organization Controller - Integration Tests", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, + LegacySecretName: secretName, } // Create test namespace and secret testEnv.CreateTestNamespace(namespaceName) diff --git a/internal/controller/repository_controller_test.go b/internal/controller/repository_controller_test.go index a21a7e5..02faa2d 100644 --- a/internal/controller/repository_controller_test.go +++ b/internal/controller/repository_controller_test.go @@ -58,6 +58,7 @@ var _ = Describe("Repository Controller - Integration Tests", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, + LegacySecretName: secretName, } testEnv.CreateTestNamespace(namespaceName) testEnv.CreateSecret(namespaceName, secretName) diff --git a/internal/controller/team_controller_test.go b/internal/controller/team_controller_test.go index f16b582..18b5509 100644 --- a/internal/controller/team_controller_test.go +++ b/internal/controller/team_controller_test.go @@ -53,6 +53,7 @@ var _ = Describe("TeamController", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, + LegacySecretName: "test-credentials", } testEnv.CreateTestNamespace(namespaceName) _ = testEnv.SetupOrganizationTest(nil, namespaceName, orgName) diff --git a/internal/controller/test_helpers_test.go b/internal/controller/test_helpers_test.go index 1237fff..f3a262d 100644 --- a/internal/controller/test_helpers_test.go +++ b/internal/controller/test_helpers_test.go @@ -159,6 +159,10 @@ func (te *TestEnvironment) SetupOrganizationTest(_ *testing.T, namespace, orgNam Spec: githubv1alpha1.OrganizationSpec{ Name: orgName, Description: "Test organization for unit tests", + GitHubAppConfig: &githubv1alpha1.GitHubAppConfig{ + InstallationId: 12345, + CredentialsSecretName: "test-credentials", + }, }, } diff --git a/internal/ghclient/factory.go b/internal/ghclient/factory.go index d0521d0..44e7244 100644 --- a/internal/ghclient/factory.go +++ b/internal/ghclient/factory.go @@ -12,6 +12,7 @@ import ( "sync" "time" + "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/PuerkitoBio/rehttp" "github.com/gofri/go-github-pagination/githubpagination" "github.com/gofri/go-github-ratelimit/v2/github_ratelimit" @@ -71,16 +72,19 @@ type ClientInfo struct { Client *GitHubClientWrapper InstallationID int64 CacheKey string + SecretName string } -// CachingGitHubClientFactory creates and caches GitHub clients with proper lifecycle and thread safety +// CachingGitHubClientFactory creates and caches GitHub clients with proper lifecycle and thread safety. +// Credentials are cached per secret name and clients are cached per organization (cacheKey). +// Rate limit state is shared per GitHub App ID so all installations of the same App share a quota bucket. type CachingGitHubClientFactory struct { - mu sync.RWMutex - clients map[string]*ClientInfo - config *ClientConfig - secretProvider SecretProviderFunc - credentials *AppCredentials - rateLimitState *github_primary_ratelimit.RateLimitState + mu sync.RWMutex + clients map[string]*ClientInfo + config *ClientConfig + secretProvider SecretProviderFunc + credentials map[string]*AppCredentials + rateLimitStates map[int64]*github_primary_ratelimit.RateLimitState } // AppCredentials holds parsed GitHub App credentials @@ -89,30 +93,36 @@ type AppCredentials struct { PrivateKey *rsa.PrivateKey } -type SecretProviderFunc = func(ctx context.Context) (*v1.Secret, error) +// SecretProviderFunc fetches a Kubernetes Secret by name. The factory calls this function +// lazily on first client creation for a given credential secret. +type SecretProviderFunc = func(ctx context.Context, secretName string) (*v1.Secret, error) // NewGitHubCachingClientFactory creates a new client cache with the given configuration. The necessary GitHub App -// credentials are taken from the given SecretProviderFunc upon first client creation. +// credentials are fetched lazily via the given SecretProviderFunc upon first client creation for each secret. func NewGitHubCachingClientFactory(config *ClientConfig, providerFunc SecretProviderFunc) (*CachingGitHubClientFactory, error) { if config == nil { config = DefaultClientConfig() } manager := &CachingGitHubClientFactory{ - clients: make(map[string]*ClientInfo), - config: config, - secretProvider: providerFunc, + clients: make(map[string]*ClientInfo), + credentials: make(map[string]*AppCredentials), + rateLimitStates: make(map[int64]*github_primary_ratelimit.RateLimitState), + config: config, + secretProvider: providerFunc, } return manager, nil } -// GetClient retrieves or creates a GitHub client for the given key -func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey string, appInstallationID int64) (GitHubClient, error) { +// GetClient retrieves or creates a GitHub client for the given organization (cacheKey). +// If a cached client exists for the cacheKey with matching credentials, it is returned directly. +// If the credentials secret changed, the old client is evicted and a new one is created. +func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig) (GitHubClient, error) { log := logf.FromContext(ctx, "function", "GetClient", ) - if c := m.getCachedClient(cacheKey); c != nil { + if c := m.getCachedClient(cacheKey, app.CredentialsSecretName); c != nil { return c, nil } @@ -120,9 +130,19 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str m.mu.Lock() defer m.mu.Unlock() + // Double-check after acquiring write lock + if info, exists := m.clients[cacheKey]; exists { + if info.SecretName == app.CredentialsSecretName { + return info.Client, nil + } + // Credentials secret changed – evict the stale client + delete(m.clients, cacheKey) + log.Info("Evicted stale GitHub client due to credential change", "cacheKey", cacheKey) + } + log.Info("Creating new GitHub client") - ghClient, err := m.createClient(ctx, appInstallationID) + ghClient, err := m.createClient(ctx, app) if err != nil { return nil, fmt.Errorf("failed to create GitHub client for key %s: %w", cacheKey, err) } @@ -132,16 +152,18 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str m.clients[cacheKey] = &ClientInfo{ Client: wrappedClient, - InstallationID: appInstallationID, + InstallationID: app.InstallationId, CacheKey: cacheKey, + SecretName: app.CredentialsSecretName, } - log.Info("Successfully created and cached GitHub client", "installationID", appInstallationID) + log.Info("Successfully created and cached GitHub client", "installationID", app.InstallationId) return wrappedClient, nil } -func (m *CachingGitHubClientFactory) GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, appInstallationID int64, rateLimitMinimum int) (GitHubClient, error) { - ghClient, err := m.GetClient(ctx, cacheKey, appInstallationID) +// GetGitHubClientAndCheckRateLimit retrieves a GitHub client and verifies the remaining rate limit. +func (m *CachingGitHubClientFactory) GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (GitHubClient, error) { + ghClient, err := m.GetClient(ctx, cacheKey, app) if err != nil { return nil, err } @@ -158,21 +180,13 @@ func (m *CachingGitHubClientFactory) GetGitHubClientAndCheckRateLimit(ctx contex return ghClient, nil } -// InvalidateClient removes a client from the cache, forcing recreation on next request -func (m *CachingGitHubClientFactory) InvalidateClient(cacheKey string) { - m.mu.Lock() - defer m.mu.Unlock() - - delete(m.clients, cacheKey) - logf.Log.Info("Invalidated GitHub client", "cacheKey", cacheKey) -} - -// getCachedClient returns an existing client without creating a new one -func (m *CachingGitHubClientFactory) getCachedClient(cacheKey string) *GitHubClientWrapper { +// getCachedClient returns an existing client without creating a new one, only if the cached +// client was created with the same credential secret. +func (m *CachingGitHubClientFactory) getCachedClient(cacheKey string, secretName string) *GitHubClientWrapper { m.mu.RLock() defer m.mu.RUnlock() - if info, exists := m.clients[cacheKey]; exists { + if info, exists := m.clients[cacheKey]; exists && info.SecretName == secretName { return info.Client } @@ -180,61 +194,64 @@ func (m *CachingGitHubClientFactory) getCachedClient(cacheKey string) *GitHubCli } // createClient creates a new GitHub client with proper middleware setup -func (m *CachingGitHubClientFactory) createClient(ctx context.Context, appInstallationID int64) (*github.Client, error) { +func (m *CachingGitHubClientFactory) createClient(ctx context.Context, app v1alpha1.GitHubAppConfig) (*github.Client, error) { log := logf.FromContext(ctx) log.Info("Creating GitHub client with middleware stack") - if m.credentials == nil { - // we store the credentials in the factory to avoid fetching the secret multiple times - secret, err := m.secretProvider(ctx) + creds, ok := m.credentials[app.CredentialsSecretName] + if !ok { + // Fetch and parse the secret on first use + secret, err := m.secretProvider(ctx, app.CredentialsSecretName) if err != nil { - log.Error(err, "failed to get GitHub app credentials secret") + log.Error(err, "failed to get GitHub app credentials secret", "secretName", app.CredentialsSecretName) return nil, err } if secret == nil { return nil, errors.New("GitHub app credentials secret cannot be nil") } - credentials, err := parseCredentials(*secret) + parsedCreds, err := parseCredentials(*secret) if err != nil { log.Error(err, "failed to prepare GitHub app credentials") return nil, err } - m.credentials = credentials + m.credentials[app.CredentialsSecretName] = parsedCreds + creds = parsedCreds } - ghClient := m.buildClientWithMiddleware(appInstallationID) + ghClient := m.buildClientWithMiddleware(app.InstallationId, creds) return ghClient, nil } // buildClientWithMiddleware creates a GitHub client with the full middleware stack -func (m *CachingGitHubClientFactory) buildClientWithMiddleware(appInstallationID int64) *github.Client { +func (m *CachingGitHubClientFactory) buildClientWithMiddleware(appInstallationID int64, creds *AppCredentials) *github.Client { clientName := fmt.Sprintf("github-%d", appInstallationID) client := github.NewClient(&http.Client{ - Transport: m.buildMiddlewareStack(clientName, m.credentials.AppID, appInstallationID, m.credentials.PrivateKey), + Transport: m.buildMiddlewareStack(clientName, creds, appInstallationID), Timeout: m.config.Timeout, }) client.DisableRateLimitCheck = true return client } -// buildMiddlewareStack constructs the HTTP transport middleware stack -func (m *CachingGitHubClientFactory) buildMiddlewareStack(clientName string, appID, appInstallationID int64, privateKey *rsa.PrivateKey) http.RoundTripper { +// buildMiddlewareStack constructs the HTTP transport middleware stack. +// Rate limit state is shared per GitHub App ID so installations of the same App share a quota bucket. +func (m *CachingGitHubClientFactory) buildMiddlewareStack(clientName string, creds *AppCredentials, appInstallationID int64) http.RoundTripper { // Start with the base transport rt := http.DefaultTransport - // Rate limiting (bottom layer) - if m.rateLimitState == nil { + // Rate limiting (bottom layer) – shared state per App ID so all installations are counted together + if state, exists := m.rateLimitStates[creds.AppID]; !exists { primary := github_ratelimit.NewPrimaryLimiter(rt) - m.rateLimitState = primary.GetState() + m.rateLimitStates[creds.AppID] = primary.GetState() rt = github_ratelimit.NewSecondaryLimiter(primary) } else { - rt = github_ratelimit.New(rt, github_primary_ratelimit.WithSharedState(m.rateLimitState)) + rt = github_ratelimit.New(rt, github_primary_ratelimit.WithSharedState(state)) } // Authentication - rt = AuthorizeGitHubAccess(rt, appID, appInstallationID, privateKey) + rt = AuthorizeGitHubAccess(rt, creds.AppID, appInstallationID, creds.PrivateKey) // Request logging (if enabled) - TODO: Implement when logging package is available // retry diff --git a/internal/reconciler/executor_test.go b/internal/reconciler/executor_test.go index 9644fae..5f3b4e8 100644 --- a/internal/reconciler/executor_test.go +++ b/internal/reconciler/executor_test.go @@ -101,7 +101,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -147,7 +147,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -194,7 +194,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -233,7 +233,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "my-github-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -275,7 +275,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "new-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -319,7 +319,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -363,7 +363,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "my-org-with-dashes", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -402,7 +402,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -458,7 +458,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -502,7 +502,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -575,7 +575,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -656,7 +656,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -730,7 +730,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -790,7 +790,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -831,7 +831,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, Status: githubv1alpha1.OrganizationStatus{ Conditions: []metav1.Condition{}, @@ -901,7 +901,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -952,7 +952,7 @@ var _ = Describe("OrganizationReconciler Labels", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } diff --git a/internal/reconciler/orgrec/rec_actions_settings_test.go b/internal/reconciler/orgrec/rec_actions_settings_test.go index a8bae9c..5760511 100644 --- a/internal/reconciler/orgrec/rec_actions_settings_test.go +++ b/internal/reconciler/orgrec/rec_actions_settings_test.go @@ -60,7 +60,7 @@ var _ = Describe("ReconcileActionsSettings", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -188,7 +188,7 @@ var _ = Describe("ReconcilePermissions", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -367,7 +367,7 @@ var _ = Describe("ReconcileRetention", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -533,7 +533,7 @@ var _ = Describe("ReconcileAllowedActions", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -804,7 +804,7 @@ var _ = Describe("ReconcileDefaultWorkflowPermissions", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -975,7 +975,7 @@ var _ = Describe("ReconcileSelfHostedRunnerSettings", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -1155,7 +1155,7 @@ var _ = Describe("ReconcileActionsEnabled", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: v1alpha1.ActionsSettings{ EnabledRepositories: new("all"), }, @@ -1771,7 +1771,7 @@ var _ = Describe("ReconcileRunnerGroups", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } @@ -2783,7 +2783,7 @@ var _ = Describe("ReconcilePermissions with selected repositories", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), ActionsSettings: actionsSettings, }, } diff --git a/internal/reconciler/orgrec/rec_code_security_configurations_test.go b/internal/reconciler/orgrec/rec_code_security_configurations_test.go index 308c06e..bea212e 100644 --- a/internal/reconciler/orgrec/rec_code_security_configurations_test.go +++ b/internal/reconciler/orgrec/rec_code_security_configurations_test.go @@ -53,7 +53,7 @@ var _ = Describe("ReconcileCodeSecurityConfigurations", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: testOrg, - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -674,7 +674,7 @@ var _ = Describe("UnsetObsoleteDefaults", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: testOrg, - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -831,7 +831,7 @@ var _ = Describe("ResolveBypassReviewerNames", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: testOrg, - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -1034,7 +1034,7 @@ var _ = Describe("AttachToRepos", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: testOrg, - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } diff --git a/internal/reconciler/orgrec/rec_custom_properties_test.go b/internal/reconciler/orgrec/rec_custom_properties_test.go index 09d9270..60e7502 100644 --- a/internal/reconciler/orgrec/rec_custom_properties_test.go +++ b/internal/reconciler/orgrec/rec_custom_properties_test.go @@ -58,7 +58,7 @@ var _ = Describe("ReconcileCustomProperties", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } @@ -685,7 +685,7 @@ var _ = Describe("getGitHubOrgCustomPropertiesByPropertyName", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } diff --git a/internal/reconciler/orgrec/rec_org_test.go b/internal/reconciler/orgrec/rec_org_test.go index a98a82a..66716ba 100644 --- a/internal/reconciler/orgrec/rec_org_test.go +++ b/internal/reconciler/orgrec/rec_org_test.go @@ -48,7 +48,7 @@ var _ = Describe("ReconcileOrganization", func() { Spec: v1alpha1.OrganizationSpec{ Name: "test-org", Description: "Test Organization", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } diff --git a/internal/reconciler/orgrec/rec_rulesets_test.go b/internal/reconciler/orgrec/rec_rulesets_test.go index c156dee..e03e1ee 100644 --- a/internal/reconciler/orgrec/rec_rulesets_test.go +++ b/internal/reconciler/orgrec/rec_rulesets_test.go @@ -51,7 +51,7 @@ var _ = Describe("ReconcileRulesetPresets", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), RulesetPresetList: []corev1.LocalObjectReference{}, }, } @@ -1018,7 +1018,7 @@ var _ = Describe("Helper Functions", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } diff --git a/internal/reconciler/orgrec/reconciler_test.go b/internal/reconciler/orgrec/reconciler_test.go index 794c3dd..6bcf61b 100644 --- a/internal/reconciler/orgrec/reconciler_test.go +++ b/internal/reconciler/orgrec/reconciler_test.go @@ -41,7 +41,7 @@ var _ = Describe("ReconcileDeletion", func() { Spec: v1alpha1.OrganizationSpec{ Name: "test-org", Description: "Test Organization", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } }) @@ -554,7 +554,7 @@ var _ = Describe("RequiredReconciliations", func() { Spec: v1alpha1.OrganizationSpec{ Name: "test-org", Description: "Test Organization", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: new(int64(12345)), }, } }) diff --git a/internal/reconciler/reconcilerfactory/factory.go b/internal/reconciler/reconcilerfactory/factory.go index 3c327e3..4f4d32a 100644 --- a/internal/reconciler/reconcilerfactory/factory.go +++ b/internal/reconciler/reconcilerfactory/factory.go @@ -20,6 +20,9 @@ type Factory struct { ClientManager reconciler.GitHubClientManager K8sClient client.Client SpreadingManager reconciler.SpreadManager + // LegacySecretName is the name of the credential secret used for Organizations that + // still rely on the deprecated GitHubAppInstallationId field without a GitHubAppConfig. + LegacySecretName string } const ( @@ -58,7 +61,13 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name return nil, requiresSpreadErr } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, orgRateLimitThreshold) + appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + if err != nil { + logPkg.FromContext(ctx).Error(err, "unable to resolve GitHub App config for Organization") + return nil, err + } + + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, orgRateLimitThreshold) if err != nil { return nil, err } @@ -106,7 +115,13 @@ func (f *Factory) CreateForRepo(ctx context.Context, repoName types.NamespacedNa return nil, err } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, repoRateLimitThreshold) + appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + if err != nil { + log.Error(err, "unable to resolve GitHub App config for Organization", "organization", org.GetLogin()) + return nil, err + } + + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, repoRateLimitThreshold) if err != nil { return nil, err } @@ -284,9 +299,14 @@ func buildGitHubOrgsSlice(ctx context.Context, f *Factory, team v1alpha1.Team, r } var githubOrgs []reconciler.GitHub[string] for _, org := range orgs { - ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, teamRateLimitThreshold) + appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + if err != nil { + log.Error(err, "unable to resolve GitHub App config for Organization", "organization", org.GetLogin()) + return nil, err + } + ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, teamRateLimitThreshold) if err != nil { - log.Error(err, "unable to get github client for installationId", "organization", org.GetLogin(), "installationId", org.Spec.GitHubAppInstallationId) + log.Error(err, "unable to get github client for installationId", "organization", org.GetLogin(), "installationId", appConfig.InstallationId) return nil, err } diff --git a/internal/reconciler/reconcilerfactory/factory_test.go b/internal/reconciler/reconcilerfactory/factory_test.go index 6f64e7c..910c337 100644 --- a/internal/reconciler/reconcilerfactory/factory_test.go +++ b/internal/reconciler/reconcilerfactory/factory_test.go @@ -100,7 +100,7 @@ var _ = Describe("Factory", func() { Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, Description: "Test Organization", - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } }) @@ -153,7 +153,7 @@ var _ = Describe("Factory", func() { It("should call GetGitHubClientAndCheckRateLimit with correct parameters", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockClientMgr.lastOrgName).To(Equal(defaultOrgName)) - Expect(mockClientMgr.lastAppID).To(Equal(defaultAppID)) + Expect(mockClientMgr.lastAppConfig.InstallationId).To(Equal(defaultAppID)) Expect(mockClientMgr.lastRateLimit).To(Equal(orgRateLimitThreshold)) }) }) @@ -228,7 +228,7 @@ var _ = Describe("Factory", func() { Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, Description: "Test Organization", - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } @@ -296,7 +296,7 @@ var _ = Describe("Factory", func() { It("should call GetGitHubClientAndCheckRateLimit with correct parameters", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockClientMgr.lastOrgName).To(Equal(defaultOrgName)) - Expect(mockClientMgr.lastAppID).To(Equal(defaultAppID)) + Expect(mockClientMgr.lastAppConfig.InstallationId).To(Equal(defaultAppID)) Expect(mockClientMgr.lastRateLimit).To(Equal(repoRateLimitThreshold)) }) }) @@ -405,7 +405,7 @@ var _ = Describe("Factory", func() { Spec: v1alpha1.OrganizationSpec{ Name: "org1", Description: "Test Organization 1", - GitHubAppInstallationId: 11111, + GitHubAppInstallationId: new(int64(11111)), }, } @@ -417,7 +417,7 @@ var _ = Describe("Factory", func() { Spec: v1alpha1.OrganizationSpec{ Name: "org2", Description: "Test Organization 2", - GitHubAppInstallationId: 22222, + GitHubAppInstallationId: new(int64(22222)), }, } @@ -688,7 +688,7 @@ var _ = Describe("Factory", func() { Spec: v1alpha1.OrganizationSpec{ Name: orgRef, Description: "Test Organization", - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } }) @@ -711,7 +711,7 @@ var _ = Describe("Factory", func() { It("should return organization with correct spec", func() { Expect(err).NotTo(HaveOccurred()) Expect(result.Spec.Name).To(Equal(orgRef)) - Expect(result.Spec.GitHubAppInstallationId).To(Equal(defaultAppID)) + Expect(*result.Spec.GitHubAppInstallationId).To(Equal(defaultAppID)) }) }) @@ -750,7 +750,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "org1", - GitHubAppInstallationId: 11111, + GitHubAppInstallationId: new(int64(11111)), }, } @@ -761,7 +761,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "org2", - GitHubAppInstallationId: 22222, + GitHubAppInstallationId: new(int64(22222)), }, } @@ -904,7 +904,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } Expect(k8sClient.Create(ctx, org)).To(Succeed()) @@ -960,7 +960,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } Expect(k8sClient.Create(ctx, org)).To(Succeed()) @@ -1030,7 +1030,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } Expect(k8sClient.Create(ctx, org)).To(Succeed()) @@ -1094,7 +1094,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } }) @@ -1566,7 +1566,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, RulesetPresetList: []v1.LocalObjectReference{ {Name: "integration-ruleset"}, }, @@ -1629,7 +1629,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } Expect(k8sClient.Create(ctx, org)).To(Succeed()) @@ -1745,7 +1745,7 @@ var _ = Describe("Factory", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: defaultOrgName, - GitHubAppInstallationId: defaultAppID, + GitHubAppInstallationId: &defaultAppID, }, } Expect(k8sClient.Create(ctx, org)).To(Succeed()) @@ -1813,33 +1813,14 @@ type mockGitHubClientManager struct { genericErr error callCount int lastOrgName string - lastAppID int64 + lastAppConfig v1alpha1.GitHubAppConfig lastRateLimit int } -func (m *mockGitHubClientManager) GetClient(_ context.Context, orgName string, appInstallationID int64) (ghclient.GitHubClient, error) { +func (m *mockGitHubClientManager) GetGitHubClientAndCheckRateLimit(_ context.Context, orgName string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) { m.callCount++ m.lastOrgName = orgName - m.lastAppID = appInstallationID - - if m.shouldFail { - return nil, m.genericErr - } - - // Return org-specific client if configured - if m.clientByOrg != nil { - if ghClient, ok := m.clientByOrg[orgName]; ok { - return ghClient, nil - } - } - - return m.client, nil -} - -func (m *mockGitHubClientManager) GetGitHubClientAndCheckRateLimit(_ context.Context, orgName string, appInstallationID int64, rateLimitMinimum int) (ghclient.GitHubClient, error) { - m.callCount++ - m.lastOrgName = orgName - m.lastAppID = appInstallationID + m.lastAppConfig = app m.lastRateLimit = rateLimitMinimum if m.shouldFailLimit { diff --git a/internal/reconciler/reporec/rec_repo_test.go b/internal/reconciler/reporec/rec_repo_test.go index 28f95f4..d7ac2db 100644 --- a/internal/reconciler/reporec/rec_repo_test.go +++ b/internal/reconciler/reporec/rec_repo_test.go @@ -278,7 +278,7 @@ var _ = Describe("ReconcileRepository", func() { }, Spec: v1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 123, + GitHubAppInstallationId: new(int64(123)), }, } }) diff --git a/internal/reconciler/types.go b/internal/reconciler/types.go index 49c9a3a..942f6f1 100644 --- a/internal/reconciler/types.go +++ b/internal/reconciler/types.go @@ -3,6 +3,7 @@ package reconciler import ( "context" + "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/conditions" "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/Interhyp/git-hubby/internal/reconciler/spreading" @@ -16,7 +17,7 @@ import ( const FieldOwner = client.FieldOwner("git-hubby") type GitHubClientManager interface { - GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, appInstallationID int64, rateLimitMinimum int) (ghclient.GitHubClient, error) + GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) } type SpreadManager interface { diff --git a/internal/webhook/v1alpha1/organization_webhook.go b/internal/webhook/v1alpha1/organization_webhook.go index a665f7a..1a830ee 100644 --- a/internal/webhook/v1alpha1/organization_webhook.go +++ b/internal/webhook/v1alpha1/organization_webhook.go @@ -90,6 +90,7 @@ func validateOrganization(organization *githubv1alpha1.Organization) error { allErrs = append(allErrs, validateCustomProperties(organization.Spec.CustomProperties, customPropertiesField)...) allErrs = append(allErrs, validatePlanFeatureCombinations(organization)...) + allErrs = append(allErrs, validateGitHubAppConfig(organization)...) if len(allErrs) == 0 { return nil @@ -99,6 +100,17 @@ func validateOrganization(organization *githubv1alpha1.Organization) error { organization.Name, allErrs) } +// validateGitHubAppConfig ensures that at least one of githubAppConfig or githubAppInstallationId is set. +func validateGitHubAppConfig(organization *githubv1alpha1.Organization) field.ErrorList { + if organization.Spec.GitHubAppConfig != nil || organization.Spec.GitHubAppInstallationId != nil { + return nil + } + specPath := field.NewPath("spec") + return field.ErrorList{ + field.Required(specPath, "at least one of githubAppConfig or githubAppInstallationId must be set"), + } +} + func validatePlanFeatureCombinations(organization *githubv1alpha1.Organization) field.ErrorList { plan := organization.Spec.Plan if plan == "" || plan == githubv1alpha1.PlanEnterprise { diff --git a/internal/webhook/v1alpha1/organization_webhook_test.go b/internal/webhook/v1alpha1/organization_webhook_test.go index 923de4d..2745a59 100644 --- a/internal/webhook/v1alpha1/organization_webhook_test.go +++ b/internal/webhook/v1alpha1/organization_webhook_test.go @@ -38,6 +38,8 @@ var _ = Describe("Organization Webhook", func() { validator OrganizationCustomValidator ) + installationID := int64(12345) + BeforeEach(func() { ctx = context.Background() obj = &githubv1alpha1.Organization{ @@ -47,7 +49,7 @@ var _ = Describe("Organization Webhook", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: &installationID, CustomProperties: []githubv1alpha1.OrgCustomProperty{}, }, } @@ -58,7 +60,7 @@ var _ = Describe("Organization Webhook", func() { }, Spec: githubv1alpha1.OrganizationSpec{ Name: "test-org", - GitHubAppInstallationId: 12345, + GitHubAppInstallationId: &installationID, CustomProperties: []githubv1alpha1.OrgCustomProperty{}, }, } @@ -970,4 +972,58 @@ var _ = Describe("Organization Webhook", func() { Expect(statusErr.Error()).To(ContainSubstring("rulesetPresets")) }) }) + + Context("When validating GitHub App config", func() { + It("Should reject when neither githubAppConfig nor githubAppInstallationId is set", func() { + obj.Spec.GitHubAppInstallationId = nil + obj.Spec.GitHubAppConfig = nil + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + var statusErr *errors.StatusError + Expect(stderrors.As(err, &statusErr)).To(BeTrue()) + Expect(statusErr.Error()).To(ContainSubstring("githubAppConfig")) + }) + + It("Should allow when only githubAppInstallationId is set", func() { + installID := int64(99999) + obj.Spec.GitHubAppInstallationId = &installID + obj.Spec.GitHubAppConfig = nil + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + }) + + It("Should allow when only githubAppConfig is set", func() { + obj.Spec.GitHubAppInstallationId = nil + obj.Spec.GitHubAppConfig = &githubv1alpha1.GitHubAppConfig{ + InstallationId: 42, + CredentialsSecretName: "my-app-secret", + } + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + }) + + It("Should allow when both githubAppConfig and githubAppInstallationId are set", func() { + installID := int64(12345) + obj.Spec.GitHubAppInstallationId = &installID + obj.Spec.GitHubAppConfig = &githubv1alpha1.GitHubAppConfig{ + InstallationId: 42, + CredentialsSecretName: "my-app-secret", + } + warnings, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + }) + + It("Should reject on update when neither githubAppConfig nor githubAppInstallationId is set", func() { + obj.Spec.GitHubAppInstallationId = nil + obj.Spec.GitHubAppConfig = nil + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + var statusErr *errors.StatusError + Expect(stderrors.As(err, &statusErr)).To(BeTrue()) + Expect(statusErr.Error()).To(ContainSubstring("githubAppConfig")) + }) + }) }) diff --git a/internal/webhook/v1alpha1/repository_webhook.go b/internal/webhook/v1alpha1/repository_webhook.go index 7a31209..17fa451 100644 --- a/internal/webhook/v1alpha1/repository_webhook.go +++ b/internal/webhook/v1alpha1/repository_webhook.go @@ -21,6 +21,7 @@ import ( baseerrors "errors" "fmt" + githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/google/go-github/v86/github" "k8s.io/apimachinery/pkg/api/errors" @@ -29,8 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" ) // nolint:unused @@ -38,17 +37,19 @@ import ( var repositorylog = logf.Log.WithName("repository-resource") // SetupRepositoryWebhookWithManager registers the webhook for Repository in the manager. -func SetupRepositoryWebhookWithManager(mgr ctrl.Manager, clientManager GitHubClientManager) error { +func SetupRepositoryWebhookWithManager(mgr ctrl.Manager, clientManager GitHubClientManager, legacySecretName string) error { return ctrl.NewWebhookManagedBy(mgr, &githubv1alpha1.Repository{}). WithValidator(&RepositoryCustomValidator{ K8sClient: mgr.GetClient(), GitHubClientManager: clientManager, + LegacySecretName: legacySecretName, }). Complete() } +// GitHubClientManager is the interface the repository webhook uses to obtain a GitHub client. type GitHubClientManager interface { - GetClient(ctx context.Context, orgName string, appInstallationID int64) (ghclient.GitHubClient, error) + GetClient(ctx context.Context, cacheKey string, app githubv1alpha1.GitHubAppConfig) (ghclient.GitHubClient, error) } // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. @@ -64,6 +65,9 @@ type RepositoryCustomValidator struct { // TODO fugly: find a way to validate without doing either k8s or github api calls K8sClient client.Client GitHubClientManager GitHubClientManager + // LegacySecretName is the credential secret name used when the Organization uses the + // deprecated GitHubAppInstallationId field instead of the new GitHubAppConfig. + LegacySecretName string } var _ admission.Validator[*githubv1alpha1.Repository] = &RepositoryCustomValidator{} @@ -107,7 +111,13 @@ func (v *RepositoryCustomValidator) validateRepository(ctx context.Context, repo if err := v.K8sClient.Get(ctx, client.ObjectKey{Name: repo.Spec.OrganizationRef.Name, Namespace: repo.Namespace}, &org); err != nil { return fmt.Errorf("failed to fetch organization during validation of repository %s: %w", repo.Name, err) } - githubClient, err := v.GitHubClientManager.GetClient(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId) + + appConfig, err := org.ResolveGitHubAppConfig(v.LegacySecretName) + if err != nil { + return fmt.Errorf("failed to resolve GitHub App config for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) + } + + githubClient, err := v.GitHubClientManager.GetClient(ctx, org.GetLogin(), *appConfig) if err != nil { return fmt.Errorf("failed to create GitHub client for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) } diff --git a/internal/webhook/v1alpha1/repository_webhook_test.go b/internal/webhook/v1alpha1/repository_webhook_test.go index b8af738..958054c 100644 --- a/internal/webhook/v1alpha1/repository_webhook_test.go +++ b/internal/webhook/v1alpha1/repository_webhook_test.go @@ -105,8 +105,11 @@ var _ = Describe("Repository Webhook", func() { Namespace: "default", }, Spec: githubv1alpha1.OrganizationSpec{ - Name: "test-org", - GitHubAppInstallationId: 12345, + Name: "test-org", + GitHubAppConfig: &githubv1alpha1.GitHubAppConfig{ + InstallationId: 12345, + CredentialsSecretName: "test-credentials", + }, }, } @@ -148,6 +151,7 @@ var _ = Describe("Repository Webhook", func() { validator = RepositoryCustomValidator{ K8sClient: mockK8s, GitHubClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), + LegacySecretName: "test-credentials", } Expect(validator).NotTo(BeNil(), "Expected validator to be initialized") diff --git a/internal/webhook/v1alpha1/webhook_suite_test.go b/internal/webhook/v1alpha1/webhook_suite_test.go index e79ee61..6db5f5f 100644 --- a/internal/webhook/v1alpha1/webhook_suite_test.go +++ b/internal/webhook/v1alpha1/webhook_suite_test.go @@ -115,7 +115,7 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) mockClient := ghclientmock.NewMockGitHubClientWrapper() - err = SetupRepositoryWebhookWithManager(mgr, ghclientmock.NewGitHubMockClientFactory(mockClient)) + err = SetupRepositoryWebhookWithManager(mgr, ghclientmock.NewGitHubMockClientFactory(mockClient), "test-credentials") Expect(err).NotTo(HaveOccurred()) // +kubebuilder:scaffold:webhook diff --git a/test/mock/ghclientmock/mock_factory.go b/test/mock/ghclientmock/mock_factory.go index a0493b5..f0f3ea9 100644 --- a/test/mock/ghclientmock/mock_factory.go +++ b/test/mock/ghclientmock/mock_factory.go @@ -6,6 +6,7 @@ import ( "errors" "sync" + "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/google/go-github/v86/github" ) @@ -20,14 +21,14 @@ func NewGitHubMockClientFactory(mockClient *MockGitHubClientWrapper) *GitHubMock } } -func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ int64) (ghclient.GitHubClient, error) { +func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ v1alpha1.GitHubAppConfig) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") } return m.mockClient, nil } -func (m *GitHubMockClientFactory) GetGitHubClientAndCheckRateLimit(_ context.Context, _ string, _ int64, _ int) (ghclient.GitHubClient, error) { +func (m *GitHubMockClientFactory) GetGitHubClientAndCheckRateLimit(_ context.Context, _ string, _ v1alpha1.GitHubAppConfig, _ int) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") } From 58d64ac975349df86ae7056ae9ce39eb14bd0748 Mon Sep 17 00:00:00 2001 From: tstollin Date: Thu, 18 Jun 2026 10:49:43 +0200 Subject: [PATCH 2/2] ci: split push and pull_request based workflows --- .github/workflows/ci.yml | 77 ---------------------- .github/workflows/pr.yml | 136 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 77 deletions(-) create mode 100644 .github/workflows/pr.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 758bdf3..ea02463 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,13 +3,9 @@ name: CI on: push: - pull_request: - # 'edited' is included so commitlint re-runs when PR title changes (used as squash commit message) - types: [opened, edited, synchronize, reopened] permissions: contents: read - pull-requests: write jobs: @@ -82,27 +78,6 @@ jobs: fi echo "Generated code is up to date." - commitlint: - name: Validate Commit Messages - runs-on: ubuntu-latest - if: github.event_name == 'pull_request' - steps: - - name: Checkout code - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - - name: Setup Node.js - uses: actions/setup-node@v4 - with: - node-version-file: .node-version - - - name: Install commitlint - run: npm ci - - - name: Lint commits - run: npx commitlint --from ${{ github.event.pull_request.base.sha }} --to ${{ github.event.pull_request.head.sha }} --verbose - test-e2e: name: Test E2E runs-on: ubuntu-latest @@ -129,55 +104,3 @@ jobs: run: | go mod tidy make test-e2e - - helm-chart-reminder: - name: Helm Chart Update Reminder - runs-on: ubuntu-latest - if: github.event_name == 'pull_request' - steps: - - name: Checkout code - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - - name: Check for Helm-relevant changes - id: changes - run: | - DIFF="$(git diff origin/${{ github.base_ref }}...HEAD)" - REASONS="" - if echo "$DIFF" | grep -qE '^\+.*\+kubebuilder:rbac'; then - REASONS="${REASONS}\n- RBAC markers (\`+kubebuilder:rbac\`) were added or modified → update RBAC template" - fi - if echo "$DIFF" | grep -qE '^\+.*\+kubebuilder:webhook'; then - REASONS="${REASONS}\n- Webhook markers (\`+kubebuilder:webhook\`) were added or modified → update webhook configuration template" - fi - if git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -q 'config/manager/manager.yaml'; then - REASONS="${REASONS}\n- \`config/manager/manager.yaml\` was modified → update deployment template (env vars, args, ports, volumes)" - fi - if [[ -n "$REASONS" ]]; then - echo "changed=true" >> "$GITHUB_OUTPUT" - # Use delimiter for multiline output - echo "reasons<> "$GITHUB_OUTPUT" - echo -e "$REASONS" >> "$GITHUB_OUTPUT" - echo "EOF" >> "$GITHUB_OUTPUT" - else - echo "changed=false" >> "$GITHUB_OUTPUT" - fi - - - name: Comment on PR about Helm chart changes - if: steps.changes.outputs.changed == 'true' - env: - GH_TOKEN: ${{ github.token }} - run: | - COMMENT_MARKER="" - EXISTING=$(gh pr view ${{ github.event.pull_request.number }} --json comments --jq '[.comments[].body | select(contains("'"$COMMENT_MARKER"'"))] | length') - if [[ "$EXISTING" == "0" ]]; then - gh pr comment ${{ github.event.pull_request.number }} --body "${COMMENT_MARKER} - ⚠️ **Helm Chart Update Required** - - This PR contains changes that likely require a matching update in [git-hubby-helm](https://github.com/Interhyp/git-hubby-helm): - - ${{ steps.changes.outputs.reasons }} - - After merging, run \`make manifests\` and compare the generated output in \`config/\` with the corresponding Helm chart templates." - fi diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml new file mode 100644 index 0000000..dd2f9a1 --- /dev/null +++ b/.github/workflows/pr.yml @@ -0,0 +1,136 @@ +name: PR Checks +# Jobs in this workflow are enforced as required status checks via branch protection on main. +# This file is intentionally separate from ci.yml so these jobs only ever trigger on +# pull_request events — preventing the double-run / skipped-status-check race that occurs +# when a force-push fires both a 'push' event and a 'pull_request: synchronize' event +# within the same workflow file. + +on: + pull_request: + # 'edited' is included so commitlint re-runs when PR title changes (used as squash commit message) + types: [opened, edited, synchronize, reopened] + +permissions: + contents: read + pull-requests: write + + +jobs: + commitlint: + name: Validate Commit Messages + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version-file: .node-version + + - name: Install commitlint + run: npm ci + + - name: Lint commits + run: npx commitlint --from ${{ github.event.pull_request.base.sha }} --to ${{ github.event.pull_request.head.sha }} --verbose + + helm-chart-reminder: + name: Helm Chart Update Reminder + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Check for Helm-relevant changes + id: changes + run: | + # Restrict to *.go so the workflow files themselves don't self-match + # (the grep patterns appear verbatim in this YAML and would otherwise trigger false positives) + GO_DIFF="$(git diff origin/${{ github.base_ref }}...HEAD -- '*.go')" + + echo "=== Changed .go files ===" + git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.go' + echo "=== All changed files ===" + git diff --name-only origin/${{ github.base_ref }}...HEAD + + REASONS="" + + RBAC_FILES="$(git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.go' \ + | while read -r f; do + if git diff origin/${{ github.base_ref }}...HEAD -- "$f" | grep -qE '^\+.*\+kubebuilder:rbac'; then + echo "$f" + fi + done)" + if [[ -n "$RBAC_FILES" ]]; then + echo "=== Files with new +kubebuilder:rbac markers ===" + echo "$RBAC_FILES" + FILE_LIST="$(echo "$RBAC_FILES" | sed 's/^/ - \`/' | sed 's/$/\`/')" + REASONS="${REASONS}\n- RBAC markers (\`+kubebuilder:rbac\`) were added or modified → update RBAC template\n${FILE_LIST}" + fi + + WEBHOOK_FILES="$(git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.go' \ + | while read -r f; do + if git diff origin/${{ github.base_ref }}...HEAD -- "$f" | grep -qE '^\+.*\+kubebuilder:webhook'; then + echo "$f" + fi + done)" + if [[ -n "$WEBHOOK_FILES" ]]; then + echo "=== Files with new +kubebuilder:webhook markers ===" + echo "$WEBHOOK_FILES" + FILE_LIST="$(echo "$WEBHOOK_FILES" | sed 's/^/ - \`/' | sed 's/$/\`/')" + REASONS="${REASONS}\n- Webhook markers (\`+kubebuilder:webhook\`) were added or modified → update webhook configuration template\n${FILE_LIST}" + fi + + if git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -q 'config/manager/manager.yaml'; then + echo "=== config/manager/manager.yaml was modified ===" + REASONS="${REASONS}\n- \`config/manager/manager.yaml\` was modified → update deployment template (env vars, args, ports, volumes)" + fi + + if [[ -n "$REASONS" ]]; then + echo "changed=true" >> "$GITHUB_OUTPUT" + # Use delimiter for multiline output + echo "reasons<> "$GITHUB_OUTPUT" + echo -e "$REASONS" >> "$GITHUB_OUTPUT" + echo "EOF" >> "$GITHUB_OUTPUT" + else + echo "No Helm-relevant changes detected" + echo "changed=false" >> "$GITHUB_OUTPUT" + fi + + - name: Comment on PR about Helm chart changes + env: + GH_TOKEN: ${{ github.token }} + run: | + COMMENT_MARKER="" + echo "=== Looking up existing reminder comment ===" + EXISTING_ID=$(gh api \ + "repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/comments" \ + --jq '[.[] | select(.body | contains("'"$COMMENT_MARKER"'"))] | first | .id // empty') + echo "EXISTING_ID=${EXISTING_ID}" + echo "changed=${{ steps.changes.outputs.changed }}" + if [[ "${{ steps.changes.outputs.changed }}" == "true" ]]; then + if [[ -z "$EXISTING_ID" ]]; then + echo "=== Posting reminder comment ===" + gh pr comment ${{ github.event.pull_request.number }} --body "${COMMENT_MARKER} + ⚠️ **Helm Chart Update Required** + + This PR contains changes that likely require a matching update in [git-hubby-helm](https://github.com/Interhyp/git-hubby-helm): + + ${{ steps.changes.outputs.reasons }} + + After merging, run \`make manifests\` and compare the generated output in \`config/\` with the corresponding Helm chart templates." + else + echo "=== Reminder comment already exists, skipping ===" + fi + else + if [[ -n "$EXISTING_ID" ]]; then + echo "=== Deleting stale reminder comment ===" + gh api -X DELETE "repos/${{ github.repository }}/issues/comments/${EXISTING_ID}" + else + echo "=== No reminder comment to delete ===" + fi + fi