Merge pull request #592 from vshn/feat/keycloak-nested-cnpg

Use CNPG as nested service for Keycloak

Author: github-actions[bot]

cmd/controller.go

@@ -196,6 +196,10 @@ func setupWebhooks(mgr manager.Manager, withQuota bool, withAppcatWebhooks bool,
 		if err != nil {
 			return err
 		}
+		err = webhooks.SetupXVSHNPostgreSQLWebhookHandlerWithManager(mgr)
+		if err != nil {
+			return err
+		}
 		err = webhooks.SetupRedisWebhookHandlerWithManager(mgr, withQuota)
 		if err != nil {
 			return err

config/controller/webhooks.yaml

@@ -324,3 +324,22 @@ webhooks:
     resources:
     - xobjectbuckets
   sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-vshn-appcat-vshn-io-v1-xvshnpostgresql
+  failurePolicy: Fail
+  name: xvshnpostgresql.vshn.appcat.vshn.io
+  rules:
+  - apiGroups:
+    - vshn.appcat.vshn.io
+    apiVersions:
+    - v1
+    operations:
+    - UPDATE
+    resources:
+    - xvshnpostgresqls
+  sideEffects: None

pkg/comp-functions/functions/common/postgresql.go

@@ -162,6 +162,20 @@ func (a *PostgreSQLDependencyBuilder) CreateDependency() (string, error) {
 	// and would therefore override any value we set before the merge.
 	params.Instances = a.comp.GetInstances()
 
+	// Preserve the compositionRef of an already-existing nested PostgreSQL composite.
+	compositionName := a.svc.Config.Data["defaultPGComposition"]
+	observedPg := &vshnv1.XVSHNPostgreSQL{}
+	err := a.svc.GetObservedComposedResource(observedPg, a.comp.GetName()+PgInstanceNameSuffix)
+	if err != nil && err != runtime.ErrNotFound {
+		return "", fmt.Errorf("couldn't read observed nested postgresql composite: %w", err)
+	}
+	if err == nil {
+		if observedPg.Spec.CompositionRef.Name == "" {
+			return "", fmt.Errorf("observed nested postgresql composite %q has no compositionRef, refusing to overwrite with default", observedPg.GetName())
+		}
+		compositionName = observedPg.Spec.CompositionRef.Name
+	}
+
 	// We have to ignore the providerconfig on the composite itself.
 	pg := &vshnv1.XVSHNPostgreSQL{
 		ObjectMeta: metav1.ObjectMeta{
@@ -173,7 +187,7 @@ func (a *PostgreSQLDependencyBuilder) CreateDependency() (string, error) {
 		Spec: vshnv1.XVSHNPostgreSQLSpec{
 			Parameters: *params,
 			CompositionRef: v1.CompositionReference{
-				Name: a.svc.Config.Data["defaultPGComposition"],
+				Name: compositionName,
 			},
 			ResourceSpec: xpv1.ResourceSpec{
 				WriteConnectionSecretToReference: &xpv1.SecretReference{
@@ -189,7 +203,7 @@ func (a *PostgreSQLDependencyBuilder) CreateDependency() (string, error) {
 		pg.Labels[runtime.ProviderConfigLabel] = v
 	}
 
-	err := CustomCreateNetworkPolicy([]string{a.comp.GetInstanceNamespace()}, pg.GetInstanceNamespace(), pg.GetName()+"-"+a.comp.GetServiceName(), "", false, a.svc)
+	err = CustomCreateNetworkPolicy([]string{a.comp.GetInstanceNamespace()}, pg.GetInstanceNamespace(), pg.GetName()+"-"+a.comp.GetServiceName(), "", false, a.svc)
 	if err != nil {
 		return "", err
 	}

pkg/comp-functions/functions/vshnpostgrescnpg/connection_details.go

@@ -40,6 +40,21 @@ func AddConnectionSecrets(ctx context.Context, comp *vshnv1.VSHNPostgreSQL, svc
 		return runtime.NewWarningResult(fmt.Sprintf("Couldn't set connection details: %v", err))
 	}
 
+	host := fmt.Sprintf("postgresql-rw.%s.svc.cluster.local", comp.GetInstanceNamespace())
+	svc.SetConnectionDetail(PostgresqlHost, []byte(host))
+
+	// CNPG's uri field uses a bare hostname and a wildcard '*' database, which most
+	// clients reject. Reconstruct the URL from the already-correct individual fields.
+	cd := svc.GetConnectionDetails()
+	user := string(cd[PostgresqlUser])
+	password := string(cd[PostgresqlPassword])
+	port := string(cd[PostgresqlPort])
+	db := string(cd[PostgresqlDb])
+	if user != "" && password != "" && port != "" {
+		svc.SetConnectionDetail(PostgresqlURL, []byte(fmt.Sprintf("postgresql://%s:%s@%s:%s/%s",
+			user, password, host, port, db)))
+	}
+
 	return nil
 }
 

pkg/controller/webhooks/postgresql.go

@@ -24,6 +24,9 @@ import (
 // See https://book.kubebuilder.io/reference/markers/webhook for docs
 //+kubebuilder:webhook:verbs=create;update;delete,path=/validate-vshn-appcat-vshn-io-v1-vshnpostgresql,mutating=false,failurePolicy=fail,groups=vshn.appcat.vshn.io,resources=vshnpostgresqls,versions=v1,name=postgresql.vshn.appcat.vshn.io,sideEffects=None,admissionReviewVersions=v1
 
+// Protect the XVSHNPostgreSQL composite from having its compositionRef changed once set.
+//+kubebuilder:webhook:verbs=update,path=/validate-vshn-appcat-vshn-io-v1-xvshnpostgresql,mutating=false,failurePolicy=fail,groups=vshn.appcat.vshn.io,resources=xvshnpostgresqls,versions=v1,name=xvshnpostgresql.vshn.appcat.vshn.io,sideEffects=None,admissionReviewVersions=v1
+
 //RBAC
 //+kubebuilder:rbac:groups=vshn.appcat.vshn.io,resources=xvshnpostgresqls,verbs=get;list;watch;patch;update
 //+kubebuilder:rbac:groups=vshn.appcat.vshn.io,resources=xvshnpostgresqls/status,verbs=get;list;watch;patch;update
@@ -38,10 +41,12 @@ const (
 )
 
 var (
-	pgGK = schema.GroupKind{Group: "vshn.appcat.vshn.io", Kind: "VSHNPostgreSQL"}
-	pgGR = schema.GroupResource{Group: pgGK.Group, Resource: "vshnpostgresqls"}
+	pgGK  = schema.GroupKind{Group: "vshn.appcat.vshn.io", Kind: "VSHNPostgreSQL"}
+	pgGR  = schema.GroupResource{Group: pgGK.Group, Resource: "vshnpostgresqls"}
+	xpgGK = schema.GroupKind{Group: "vshn.appcat.vshn.io", Kind: "XVSHNPostgreSQL"}
 
 	_ webhook.CustomValidator = &PostgreSQLWebhookHandler{}
+	_ webhook.CustomValidator = &XVSHNPostgreSQLWebhookHandler{}
 
 	blocklist = map[string]string{
 		"listen_addresses":      "",
@@ -84,6 +89,46 @@ func SetupPostgreSQLWebhookHandlerWithManager(mgr ctrl.Manager, withQuota bool)
 		Complete()
 }
 
+// XVSHNPostgreSQLWebhookHandler validates the nested XVSHNPostgreSQL composite resource.
+type XVSHNPostgreSQLWebhookHandler struct{}
+
+// SetupXVSHNPostgreSQLWebhookHandlerWithManager registers the XVSHNPostgreSQL validation webhook.
+func SetupXVSHNPostgreSQLWebhookHandlerWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&vshnv1.XVSHNPostgreSQL{}).
+		WithValidator(&XVSHNPostgreSQLWebhookHandler{}).
+		Complete()
+}
+
+func (x *XVSHNPostgreSQLWebhookHandler) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
+	return nil, nil
+}
+
+func (x *XVSHNPostgreSQLWebhookHandler) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	oldPg, ok := oldObj.(*vshnv1.XVSHNPostgreSQL)
+	if !ok {
+		return nil, fmt.Errorf("provided manifest is not a valid XVSHNPostgreSQL object")
+	}
+	newPg, ok := newObj.(*vshnv1.XVSHNPostgreSQL)
+	if !ok {
+		return nil, fmt.Errorf("provided manifest is not a valid XVSHNPostgreSQL object")
+	}
+
+	if newPg.DeletionTimestamp != nil {
+		return nil, nil
+	}
+
+	allErrs := newFielErrors(newPg.Name, xpgGK)
+	if err := validateCompositionRefImmutability(oldPg.Spec.CompositionRef.Name, newPg.Spec.CompositionRef.Name); err != nil {
+		allErrs.Add(err)
+	}
+	return nil, allErrs.Get()
+}
+
+func (x *XVSHNPostgreSQLWebhookHandler) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
+	return nil, nil
+}
+
 func (p *PostgreSQLWebhookHandler) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	return p.validatePostgreSQL(ctx, obj, nil, true)
 }
@@ -151,8 +196,8 @@ func (p *PostgreSQLWebhookHandler) validatePostgreSQL(ctx context.Context, newOb
 
 		// Do not allow changing compositionRef if it has been set previously.
 		// When creating a new VSHNPostgresQL, crossplane will automatically set this field if unset.
-		if oldPg.Spec.CompositionRef.Name != "" && newPg.Spec.CompositionRef.Name != oldPg.Spec.CompositionRef.Name {
-			allErrs.Add(field.Forbidden(field.NewPath("spec", "compositionRef"), "compositionRef is immutable"))
+		if err := validateCompositionRefImmutability(oldPg.Spec.CompositionRef.Name, newPg.Spec.CompositionRef.Name); err != nil {
+			allErrs.Add(err)
 		}
 
 		// Check for disk downsizing
@@ -389,6 +434,14 @@ func validatePostgreSQLEncryptionChanges(newEncryption, oldEncryption *vshnv1.VS
 	return nil
 }
 
+// validateCompositionRefImmutability returns a Forbidden error if the compositionRef name has changed after being set
+func validateCompositionRefImmutability(oldRef, newRef string) *field.Error {
+	if oldRef != "" && newRef != oldRef {
+		return field.Forbidden(field.NewPath("spec", "compositionRef"), "compositionRef is immutable")
+	}
+	return nil
+}
+
 // validatePinImageTag validates that pinImageTag's major version matches the specified majorVersion
 func validatePinImageTag(pinImageTag, majorVersion string) *field.Error {
 	if pinImageTag == "" {

pkg/controller/webhooks/postgresql_test.go

@@ -732,6 +732,97 @@ func TestPostgreSQLWebhookHandler_ValidateCreateWithPinImageTag(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestValidateCompositionRefImmutability(t *testing.T) {
+	tests := []struct {
+		name      string
+		oldRef    string
+		newRef    string
+		expectErr bool
+	}{
+		{
+			name:      "GivenBothEmpty_ThenNoError",
+			oldRef:    "",
+			newRef:    "",
+			expectErr: false,
+		},
+		{
+			name:      "GivenOldEmptyNewSet_ThenNoError",
+			oldRef:    "",
+			newRef:    "vshnpostgrescnpg.vshn.appcat.vshn.io",
+			expectErr: false,
+		},
+		{
+			name:      "GivenSameRef_ThenNoError",
+			oldRef:    "vshnpostgres.vshn.appcat.vshn.io",
+			newRef:    "vshnpostgres.vshn.appcat.vshn.io",
+			expectErr: false,
+		},
+		{
+			name:      "GivenOldSetNewDifferent_ThenError",
+			oldRef:    "vshnpostgres.vshn.appcat.vshn.io",
+			newRef:    "vshnpostgrescnpg.vshn.appcat.vshn.io",
+			expectErr: true,
+		},
+		{
+			name:      "GivenOldSetNewEmpty_ThenError",
+			oldRef:    "vshnpostgres.vshn.appcat.vshn.io",
+			newRef:    "",
+			expectErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateCompositionRefImmutability(tt.oldRef, tt.newRef)
+			if tt.expectErr {
+				assert.NotNil(t, err)
+				assert.Contains(t, err.Error(), "compositionRef is immutable")
+			} else {
+				assert.Nil(t, err)
+			}
+		})
+	}
+}
+
+func TestXVSHNPostgreSQLWebhookHandler_ValidateUpdate(t *testing.T) {
+	ctx := context.TODO()
+	handler := XVSHNPostgreSQLWebhookHandler{}
+
+	base := &vshnv1.XVSHNPostgreSQL{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "keycloak-pg",
+			Namespace: "vshn-keycloak-myinstance",
+		},
+	}
+
+	// Allow: compositionRef not yet set on either side
+	_, err := handler.ValidateUpdate(ctx, base, base.DeepCopy())
+	assert.NoError(t, err)
+
+	// Allow: first assignment (old empty, new set)
+	withRef := base.DeepCopy()
+	withRef.Spec.CompositionRef.Name = "vshnpostgrescnpg.vshn.appcat.vshn.io"
+	_, err = handler.ValidateUpdate(ctx, base, withRef)
+	assert.NoError(t, err)
+
+	// Allow: compositionRef unchanged
+	_, err = handler.ValidateUpdate(ctx, withRef, withRef.DeepCopy())
+	assert.NoError(t, err)
+
+	// Reject: compositionRef changed on existing instance
+	changedRef := base.DeepCopy()
+	changedRef.Spec.CompositionRef.Name = "vshnpostgres.vshn.appcat.vshn.io"
+	_, err = handler.ValidateUpdate(ctx, withRef, changedRef)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "compositionRef is immutable")
+
+	// Allow: object is being deleted even if compositionRef would change
+	deletingObj := changedRef.DeepCopy()
+	now := metav1.Now()
+	deletingObj.DeletionTimestamp = &now
+	_, err = handler.ValidateUpdate(ctx, withRef, deletingObj)
+	assert.NoError(t, err)
+}
+
 // disabling this temporarily
 // func TestPostgreSQLWebhookHandler_ValidateMajorVersionUpgrade(t *testing.T) {
 // 	tests := []struct {

pkg/maintenance/postgresqlcnpg.go

@@ -19,6 +19,7 @@ import (
 	cnpgv1 "github.com/vshn/appcat/v4/apis/cnpg/v1"
 	vshnv1 "github.com/vshn/appcat/v4/apis/vshn/v1"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -67,17 +68,17 @@ func (p *PostgreSQLCNPG) RunBackup(ctx context.Context) error {
 func (p *PostgreSQLCNPG) DoMaintenance(ctx context.Context) error {
 	p.log.Info("Starting maintenance on postgresql instance")
 
-	claim := &vshnv1.VSHNPostgreSQL{}
-	if err := p.k8sClient.Get(ctx, client.ObjectKey{Name: p.claimName, Namespace: p.claimNamespace}, claim); err != nil {
-		return fmt.Errorf("couldn't get claim: %w", err)
+	comp := &vshnv1.XVSHNPostgreSQL{}
+	if err := p.k8sClient.Get(ctx, client.ObjectKey{Name: p.compName}, comp); err != nil {
+		return fmt.Errorf("couldn't get composite: %w", err)
 	}
 
 	instanceCluster, err := p.getCompositeCluster(ctx)
 	if err != nil {
 		return fmt.Errorf("couldn't get instance cluster: %w", err)
 	}
 
-	version, err := strconv.Atoi(claim.Spec.Parameters.Service.MajorVersion)
+	version, err := strconv.Atoi(comp.Spec.Parameters.Service.MajorVersion)
 	if err != nil {
 		return fmt.Errorf("cannot parse postgresql major version: %w", err)
 	}
@@ -102,7 +103,6 @@ func (p *PostgreSQLCNPG) DoMaintenance(ctx context.Context) error {
 	// EOL handling
 
 	if isEol := p.isEOL(version, latestCatalog); isEol {
-		p.log.Info("Setting EOL on claim")
 		if err := p.setEOLStatus(ctx); err != nil {
 			return fmt.Errorf("couldn't set EOL status on claim: %w", err)
 		}
@@ -270,11 +270,15 @@ func (p *PostgreSQLCNPG) isEOL(currentVersion int, imageCatalog *cnpgv1.ImageCat
 func (p *PostgreSQLCNPG) setEOLStatus(ctx context.Context) error {
 	claim := &vshnv1.VSHNPostgreSQL{}
 	err := p.k8sClient.Get(ctx, client.ObjectKey{Name: p.claimName, Namespace: p.claimNamespace}, claim)
+	if apierrors.IsNotFound(err) {
+		// There's no claim for nested services
+		p.log.Info("VSHNPostgreSQL claim not found, skipping EOL status update")
+		return nil
+	}
 	if err != nil {
 		return err
 	}
 
 	claim.Status.IsEOL = true
-
-	return p.k8sClient.Update(ctx, claim)
+	return p.k8sClient.Status().Update(ctx, claim)
 }