diff --git a/cmd/operator/main.go b/cmd/operator/main.go index dddb97f..81a4be7 100644 --- a/cmd/operator/main.go +++ b/cmd/operator/main.go @@ -212,6 +212,7 @@ func main() { // Initialize provider with config - credentials will be loaded from secret when needed keycloakProvider := &providers.KeycloakProvider{ Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), Config: authConfig.Keycloak, } oidcProviders[constants.ProviderKeycloak] = keycloakProvider diff --git a/internal/controller/nebariapp_controller.go b/internal/controller/nebariapp_controller.go index b701f09..a7ee926 100644 --- a/internal/controller/nebariapp_controller.go +++ b/internal/controller/nebariapp_controller.go @@ -22,6 +22,7 @@ import ( "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + egv1alpha1 "github.com/envoyproxy/gateway/api/v1alpha1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -30,11 +31,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" 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/controllerutil" "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" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" appsv1 "github.com/nebari-dev/nebari-operator/api/v1" @@ -382,22 +386,70 @@ func (r *NebariAppReconciler) cleanup(ctx context.Context, nebariApp *appsv1.Neb // SetupWithManager sets up the controller with the Manager. func (r *NebariAppReconciler) SetupWithManager(mgr ctrl.Manager) error { - builder := ctrl.NewControllerManagedBy(mgr). + b := ctrl.NewControllerManagedBy(mgr). For(&appsv1.NebariApp{}). - Named("nebariapp") + Named("nebariapp"). + // Watch the resources the operator owns so that external edits or + // deletions trigger prompt reconciliation (re-creation / drift repair) + // instead of waiting up to the periodic requeue interval. Owned objects + // are mapped back to their NebariApp via controller owner references: + // HTTPRoutes and SecurityPolicies set them at construction; the OIDC + // client Secret now sets one in storeClientSecret. + Owns(&gatewayv1.HTTPRoute{}). + Owns(&egv1alpha1.SecurityPolicy{}). + Owns(&corev1.Secret{}) // Watch cert-manager Certificates so that Certificate readiness transitions // trigger NebariApp reconciliation without waiting for the periodic requeue. // Certificates are matched to NebariApps via the nebari.dev/nebariapp-name // and nebari.dev/nebariapp-namespace labels. if r.TLSReconciler != nil { - builder = builder.Watches( + b = b.Watches( &certmanagerv1.Certificate{}, handler.EnqueueRequestsFromMapFunc(r.certificateToNebariApp), ) } - return builder.Complete(r) + // Watch user-provided TLS secrets in the Gateway namespace. When a NebariApp + // references routing.tls.secretName for a secret that does not yet exist (or + // has the wrong type), TLSReady stays False; creating or fixing the secret + // should flip it promptly rather than on the next periodic requeue. The + // predicate restricts the watch to the Gateway namespace, the only place a + // user-provided secret is read from. The OIDC client Secret in app namespaces + // is handled by Owns above; this watch is for the unowned, user-managed secret. + b = b.Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(r.secretToNebariApp), + builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetNamespace() == constants.GatewayNamespace + })), + ) + + return b.Complete(r) +} + +// secretToNebariApp maps a user-provided TLS Secret in the Gateway namespace to +// any NebariApp that references it via routing.tls.secretName. The NebariApp list +// is served from the controller cache, so the per-event cost is negligible. +func (r *NebariAppReconciler) secretToNebariApp(ctx context.Context, obj client.Object) []reconcile.Request { + var apps appsv1.NebariAppList + if err := r.List(ctx, &apps); err != nil { + return nil + } + + var requests []reconcile.Request + for i := range apps.Items { + app := &apps.Items[i] + if app.Spec.Routing == nil || app.Spec.Routing.TLS == nil { + continue + } + if app.Spec.Routing.TLS.SecretName == obj.GetName() { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: app.Name, Namespace: app.Namespace}, + }) + } + } + return requests } // certificateToNebariApp maps a cert-manager Certificate to the NebariApp that owns it diff --git a/internal/controller/reconcilers/auth/providers/keycloak.go b/internal/controller/reconcilers/auth/providers/keycloak.go index 71fba4e..5ccbf5d 100644 --- a/internal/controller/reconcilers/auth/providers/keycloak.go +++ b/internal/controller/reconcilers/auth/providers/keycloak.go @@ -37,14 +37,17 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" ) // KeycloakProvider implements the OIDCProvider interface for Keycloak. type KeycloakProvider struct { Client client.Client + Scheme *runtime.Scheme Config config.KeycloakConfig } @@ -728,6 +731,17 @@ func (p *KeycloakProvider) storeClientSecret(ctx context.Context, nebariApp *app Data: secretData, } + // Set the NebariApp as the controller owner so the secret is garbage-collected + // with the NebariApp and so the controller's Owns(&corev1.Secret{}) watch maps + // secret events back to this NebariApp. The secret lives in the NebariApp's own + // namespace, so the owner reference is valid. Best-effort: a nil Scheme (e.g. in + // tests that don't wire one) skips the owner reference rather than failing. + if p.Scheme != nil { + if err := controllerutil.SetControllerReference(nebariApp, secret, p.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference on OIDC client secret: %w", err) + } + } + // Check if secret exists existingSecret := &corev1.Secret{} err := p.Client.Get(ctx, types.NamespacedName{Name: secretName, Namespace: nebariApp.Namespace}, existingSecret) @@ -739,8 +753,10 @@ func (p *KeycloakProvider) storeClientSecret(ctx context.Context, nebariApp *app return fmt.Errorf("failed to check for existing secret: %w", err) } - // Update existing secret + // Update existing secret data and ensure the owner reference is set (so secrets + // created before owner references were applied get adopted on the next sync). existingSecret.Data = secret.Data + existingSecret.OwnerReferences = secret.OwnerReferences return p.Client.Update(ctx, existingSecret) } diff --git a/internal/controller/reconcilers/auth/providers/keycloak_test.go b/internal/controller/reconcilers/auth/providers/keycloak_test.go index 51f6bdd..c4301bc 100644 --- a/internal/controller/reconcilers/auth/providers/keycloak_test.go +++ b/internal/controller/reconcilers/auth/providers/keycloak_test.go @@ -511,6 +511,47 @@ func TestKeycloakProvider_StoreClientSecret(t *testing.T) { } } +// TestKeycloakProvider_StoreClientSecret_SetsOwnerReference asserts the OIDC +// client secret is created with a controller owner reference to the NebariApp, +// so it is garbage-collected with the app and matched by the controller's +// Owns(&corev1.Secret{}) watch (see #30). +func TestKeycloakProvider_StoreClientSecret_SetsOwnerReference(t *testing.T) { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = appsv1.AddToScheme(scheme) + + nebariApp := &appsv1.NebariApp{ + ObjectMeta: metav1.ObjectMeta{Name: "test-app", Namespace: "default", UID: "test-uid"}, + } + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(nebariApp).Build() + + provider := &KeycloakProvider{ + Config: config.KeycloakConfig{}, + Client: k8sClient, + Scheme: scheme, + } + + if err := provider.storeClientSecret(context.Background(), nebariApp, "default-test-app", "secret-value", "", "", ""); err != nil { + t.Fatalf("storeClientSecret returned error: %v", err) + } + + secret := &corev1.Secret{} + if err := k8sClient.Get(context.Background(), types.NamespacedName{ + Name: naming.ClientSecretName(nebariApp), + Namespace: nebariApp.Namespace, + }, secret); err != nil { + t.Fatalf("failed to get secret: %v", err) + } + + owner := metav1.GetControllerOf(secret) + if owner == nil { + t.Fatal("expected a controller owner reference on the OIDC client secret, got none") + } + if owner.Kind != "NebariApp" || owner.Name != nebariApp.Name { + t.Errorf("expected controller owner NebariApp/%s, got %s/%s", nebariApp.Name, owner.Kind, owner.Name) + } +} + func TestKeycloakProvider_LoadCredentials(t *testing.T) { scheme := runtime.NewScheme() _ = corev1.AddToScheme(scheme) diff --git a/internal/controller/watches_test.go b/internal/controller/watches_test.go new file mode 100644 index 0000000..e40b012 --- /dev/null +++ b/internal/controller/watches_test.go @@ -0,0 +1,93 @@ +/* +Copyright 2026, OpenTeams. + +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. +*/ + +package controller + +import ( + "context" + "reflect" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + appsv1 "github.com/nebari-dev/nebari-operator/api/v1" + "github.com/nebari-dev/nebari-operator/internal/controller/utils/constants" +) + +// TestSecretToNebariApp covers the map function backing the user-provided TLS +// secret watch (see #115): a secret enqueues only the NebariApp(s) that +// reference it via routing.tls.secretName, and unrelated secrets enqueue +// nothing. Namespace filtering is handled by the watch predicate, not the map +// function, so it is not exercised here. +func TestSecretToNebariApp(t *testing.T) { + scheme := runtime.NewScheme() + _ = appsv1.AddToScheme(scheme) + _ = corev1.AddToScheme(scheme) + + appWithSecret := &appsv1.NebariApp{ + ObjectMeta: metav1.ObjectMeta{Name: "app-a", Namespace: "ns-a"}, + Spec: appsv1.NebariAppSpec{ + Routing: &appsv1.RoutingConfig{TLS: &appsv1.RoutingTLSConfig{SecretName: "my-tls"}}, + }, + } + appNoSecretName := &appsv1.NebariApp{ + ObjectMeta: metav1.ObjectMeta{Name: "app-b", Namespace: "ns-b"}, + Spec: appsv1.NebariAppSpec{ + Routing: &appsv1.RoutingConfig{TLS: &appsv1.RoutingTLSConfig{}}, + }, + } + appNoRouting := &appsv1.NebariApp{ + ObjectMeta: metav1.ObjectMeta{Name: "app-c", Namespace: "ns-c"}, + } + + c := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(appWithSecret, appNoSecretName, appNoRouting).Build() + r := &NebariAppReconciler{Client: c} + + tests := []struct { + name string + secretName string + want []reconcile.Request + }{ + { + name: "secret referenced by a NebariApp enqueues that app", + secretName: "my-tls", + want: []reconcile.Request{{NamespacedName: types.NamespacedName{Name: "app-a", Namespace: "ns-a"}}}, + }, + { + name: "unrelated secret enqueues nothing", + secretName: "other-tls", + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: constants.GatewayNamespace}, + } + got := r.secretToNebariApp(context.Background(), secret) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("secretToNebariApp(%q) = %v, want %v", tt.secretName, got, tt.want) + } + }) + } +}