Skip to content

Commit b47e14a

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #604 from lmiccini/instanceha-rotation-grace-period
Skip NodeSet sync for controlplane-only credential rotation
2 parents 8bde9f9 + ff70f08 commit b47e14a

4 files changed

Lines changed: 307 additions & 9 deletions

File tree

‎apis/rabbitmq/v1beta1/conditions.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,29 @@ const (
6161
// The TransportURL controller sets this label after verifying NodeSet deployment
6262
// completion, so the user controller can safely auto-delete the CR on sight.
6363
RabbitMQUserOrphanedLabel = "rabbitmq.openstack.org/orphaned"
64+
65+
// EDPMServiceAnnotation overrides the automatic owner-based EDPM detection
66+
// for a TransportURL. When set to "true", the controller always uses the
67+
// two-phase NodeSet sync check. When "false", it releases the old user
68+
// immediately. When unset, the controller infers EDPM status from the
69+
// ownerReference Kind (Nova and NeutronAPI are EDPM; everything else is not).
70+
EDPMServiceAnnotation = "rabbitmq.openstack.org/edpm-service"
6471
)
6572

73+
// edpmOwnerKinds lists the owner CR Kinds whose TransportURLs serve
74+
// EDPM-deployed agents and therefore require NodeSet hash-sync gating
75+
// during credential rotation.
76+
var edpmOwnerKinds = map[string]bool{
77+
"Nova": true,
78+
"NeutronAPI": true,
79+
}
80+
81+
// IsEDPMOwnerKind reports whether the given owner Kind serves
82+
// EDPM-deployed agents that require NodeSet hash-sync gating.
83+
func IsEDPMOwnerKind(kind string) bool {
84+
return edpmOwnerKinds[kind]
85+
}
86+
6687
// TransportURLFinalizerFor returns the per-consumer finalizer for a TransportURL.
6788
// If the name fits within Kubernetes' 63-char name segment limit, it is used directly
6889
// (preserving human readability and reverse mapping). For longer names, the suffix

‎internal/controller/rabbitmq/rabbitmquser_controller.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,6 @@ const userFinalizer = "rabbitmquser.openstack.org/finalizer"
5555
// credentialSecretNameField is the field index for the credential secret
5656
const credentialSecretNameField = ".spec.secret"
5757

58-
// ConnectionCheckInterval is the polling interval for pending user release checks
59-
// in the TransportURL controller, used as a fallback alongside the NodeSet watch.
60-
const ConnectionCheckInterval = 5 * time.Minute
61-
6258
// generatePassword generates a random password
6359
func generatePassword(length int) (string, error) {
6460
bytes := make([]byte, length)

‎internal/controller/rabbitmq/transporturl_controller.go‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ import (
4949
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
5050
)
5151

52+
// PendingReleaseCheckInterval is the requeue interval while waiting for a
53+
// pending user release.
54+
const PendingReleaseCheckInterval = 1 * time.Minute
55+
5256
// GetClient -
5357
func (r *TransportURLReconciler) GetClient() client.Client {
5458
return r.Client
@@ -560,7 +564,7 @@ func (r *TransportURLReconciler) reconcileNormal(ctx context.Context, instance *
560564
if !released {
561565
Log.Info("Pending user release waiting for deployment",
562566
"pendingUser", instance.Status.PreviousRabbitmqUserRef)
563-
return ctrl.Result{RequeueAfter: ConnectionCheckInterval}, nil
567+
return ctrl.Result{RequeueAfter: PendingReleaseCheckInterval}, nil
564568
}
565569
}
566570

@@ -888,6 +892,25 @@ func (r *TransportURLReconciler) tryReleasePendingUser(ctx context.Context, inst
888892
Log := r.GetLogger(ctx)
889893
pendingRef := instance.Status.PreviousRabbitmqUserRef
890894

895+
// Determine whether this TransportURL serves an EDPM-deployed service.
896+
// Explicit annotation wins; otherwise infer from the ownerReference Kind.
897+
edpmService := false
898+
if v, ok := instance.Annotations[rabbitmqv1.EDPMServiceAnnotation]; ok {
899+
edpmService = v == "true"
900+
} else {
901+
for _, ref := range instance.OwnerReferences {
902+
if rabbitmqv1.IsEDPMOwnerKind(ref.Kind) {
903+
edpmService = true
904+
break
905+
}
906+
}
907+
}
908+
if !edpmService {
909+
Log.Info("Non-EDPM service, releasing old user immediately",
910+
"pendingUser", pendingRef)
911+
return r.releasePendingUser(ctx, instance)
912+
}
913+
891914
// No NodeSets with secretHashes -> no computes affected, release immediately
892915
haveNS, err := edpm.HaveNodeSets(ctx, r.Client, instance.Namespace)
893916
if err != nil {

‎test/functional/transporturl_controller_test.go‎

Lines changed: 262 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1208,11 +1208,27 @@ var _ = Describe("TransportURL controller", func() {
12081208
rabbitmq := CreateRabbitMQ(rabbitmqName, spec)
12091209
DeferCleanup(th.DeleteInstance, rabbitmq)
12101210

1211-
tuSpec := map[string]any{
1212-
"rabbitmqClusterName": rabbitmqName.Name,
1213-
"username": "phase-user-a",
1211+
// Create TransportURL with a Nova ownerReference (EDPM service)
1212+
tu := &rabbitmqv1.TransportURL{
1213+
ObjectMeta: metav1.ObjectMeta{
1214+
Name: turlName.Name,
1215+
Namespace: turlName.Namespace,
1216+
OwnerReferences: []metav1.OwnerReference{
1217+
{
1218+
APIVersion: "nova.openstack.org/v1beta1",
1219+
Kind: "Nova",
1220+
Name: "nova",
1221+
UID: "fake-nova-uid",
1222+
},
1223+
},
1224+
},
1225+
Spec: rabbitmqv1.TransportURLSpec{
1226+
RabbitmqClusterName: rabbitmqName.Name,
1227+
Username: "phase-user-a",
1228+
},
12141229
}
1215-
DeferCleanup(th.DeleteInstance, CreateTransportURL(turlName, tuSpec))
1230+
Expect(k8sClient.Create(ctx, tu)).Should(Succeed())
1231+
DeferCleanup(th.DeleteInstance, tu)
12161232

12171233
CreateNodeSet(nodesetName)
12181234
DeferCleanup(DeleteNodeSet, nodesetName)
@@ -1333,6 +1349,248 @@ var _ = Describe("TransportURL controller", func() {
13331349
})
13341350
})
13351351

1352+
When("credential rotation for controlplane-only owner (non-EDPM)", func() {
1353+
var rabbitmqName types.NamespacedName
1354+
var turlName types.NamespacedName
1355+
var nodesetName types.NamespacedName
1356+
1357+
BeforeEach(func() {
1358+
rabbitmqName = types.NamespacedName{
1359+
Name: "rabbitmq-cp-only",
1360+
Namespace: namespace,
1361+
}
1362+
turlName = types.NamespacedName{
1363+
Name: "turl-cp-only",
1364+
Namespace: namespace,
1365+
}
1366+
nodesetName = types.NamespacedName{
1367+
Name: "compute-cp-only",
1368+
Namespace: namespace,
1369+
}
1370+
1371+
SetupMockRabbitMQAPI()
1372+
DeferCleanup(StopMockRabbitMQAPI)
1373+
1374+
CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false))
1375+
DeferCleanup(DeleteRabbitMQCluster, rabbitmqName)
1376+
1377+
spec := GetDefaultRabbitMQSpec()
1378+
rabbitmq := CreateRabbitMQ(rabbitmqName, spec)
1379+
DeferCleanup(th.DeleteInstance, rabbitmq)
1380+
1381+
// Create TransportURL with a non-EDPM ownerReference (simulating Cinder)
1382+
tu := &rabbitmqv1.TransportURL{
1383+
ObjectMeta: metav1.ObjectMeta{
1384+
Name: turlName.Name,
1385+
Namespace: turlName.Namespace,
1386+
OwnerReferences: []metav1.OwnerReference{
1387+
{
1388+
APIVersion: "cinder.openstack.org/v1beta1",
1389+
Kind: "Cinder",
1390+
Name: "cinder",
1391+
UID: "fake-cinder-uid",
1392+
},
1393+
},
1394+
},
1395+
Spec: rabbitmqv1.TransportURLSpec{
1396+
RabbitmqClusterName: rabbitmqName.Name,
1397+
Username: "cp-only-user-a",
1398+
},
1399+
}
1400+
Expect(k8sClient.Create(ctx, tu)).Should(Succeed())
1401+
DeferCleanup(th.DeleteInstance, tu)
1402+
1403+
CreateNodeSet(nodesetName)
1404+
DeferCleanup(DeleteNodeSet, nodesetName)
1405+
})
1406+
1407+
It("should release old user immediately without waiting for NodeSet sync", func() {
1408+
SimulateRabbitMQClusterReady(rabbitmqName)
1409+
1410+
userACRName := types.NamespacedName{
1411+
Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "cp-only-user-a"),
1412+
Namespace: namespace,
1413+
}
1414+
Eventually(func(g Gomega) {
1415+
user := &rabbitmqv1.RabbitMQUser{}
1416+
g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed())
1417+
}, timeout, interval).Should(Succeed())
1418+
1419+
SimulateRabbitMQUserReady(userACRName, "/")
1420+
1421+
transportURLSecretName := types.NamespacedName{
1422+
Name: "rabbitmq-transport-url-" + turlName.Name,
1423+
Namespace: namespace,
1424+
}
1425+
Eventually(func(g Gomega) {
1426+
tr := th.GetTransportURL(turlName)
1427+
g.Expect(tr.Status.RabbitmqUsername).To(Equal("cp-only-user-a"))
1428+
s := &corev1.Secret{}
1429+
g.Expect(k8sClient.Get(ctx, transportURLSecretName, s)).Should(Succeed())
1430+
}, timeout, interval).Should(Succeed())
1431+
1432+
initialHash := GetSecretHash(transportURLSecretName)
1433+
SetNodeSetSecretHashes(nodesetName, map[string]string{
1434+
transportURLSecretName.Name: initialHash,
1435+
})
1436+
1437+
// --- Trigger credential rotation ---
1438+
Eventually(func(g Gomega) {
1439+
tr := th.GetTransportURL(turlName)
1440+
tr.Spec.Username = "cp-only-user-b"
1441+
g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed())
1442+
}, timeout, interval).Should(Succeed())
1443+
1444+
userBCRName := types.NamespacedName{
1445+
Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "cp-only-user-b"),
1446+
Namespace: namespace,
1447+
}
1448+
Eventually(func(g Gomega) {
1449+
user := &rabbitmqv1.RabbitMQUser{}
1450+
g.Expect(k8sClient.Get(ctx, userBCRName, user)).Should(Succeed())
1451+
}, timeout, interval).Should(Succeed())
1452+
1453+
SimulateRabbitMQUserReady(userBCRName, "/")
1454+
1455+
// Old user should be released immediately — owner is Cinder
1456+
// (not in EDPMOwnerKinds), so no NodeSet sync needed
1457+
Eventually(func(g Gomega) {
1458+
user := &rabbitmqv1.RabbitMQUser{}
1459+
err := k8sClient.Get(ctx, userACRName, user)
1460+
g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(),
1461+
"Old user should be deleted immediately for non-EDPM owner")
1462+
}, timeout, interval).Should(Succeed())
1463+
1464+
// Verify cleanup
1465+
Eventually(func(g Gomega) {
1466+
tr := th.GetTransportURL(turlName)
1467+
g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty())
1468+
}, timeout, interval).Should(Succeed())
1469+
})
1470+
})
1471+
1472+
When("annotation overrides owner-based EDPM detection", func() {
1473+
var rabbitmqName types.NamespacedName
1474+
var turlName types.NamespacedName
1475+
var nodesetName types.NamespacedName
1476+
1477+
BeforeEach(func() {
1478+
rabbitmqName = types.NamespacedName{
1479+
Name: "rabbitmq-anno-override",
1480+
Namespace: namespace,
1481+
}
1482+
turlName = types.NamespacedName{
1483+
Name: "turl-anno-override",
1484+
Namespace: namespace,
1485+
}
1486+
nodesetName = types.NamespacedName{
1487+
Name: "compute-anno-override",
1488+
Namespace: namespace,
1489+
}
1490+
1491+
SetupMockRabbitMQAPI()
1492+
DeferCleanup(StopMockRabbitMQAPI)
1493+
1494+
CreateRabbitMQCluster(rabbitmqName, GetDefaultRabbitMQClusterSpec(false))
1495+
DeferCleanup(DeleteRabbitMQCluster, rabbitmqName)
1496+
1497+
spec := GetDefaultRabbitMQSpec()
1498+
rabbitmq := CreateRabbitMQ(rabbitmqName, spec)
1499+
DeferCleanup(th.DeleteInstance, rabbitmq)
1500+
1501+
// Nova-owned TransportURL with edpm-service=false annotation override
1502+
tu := &rabbitmqv1.TransportURL{
1503+
ObjectMeta: metav1.ObjectMeta{
1504+
Name: turlName.Name,
1505+
Namespace: turlName.Namespace,
1506+
Annotations: map[string]string{
1507+
rabbitmqv1.EDPMServiceAnnotation: "false",
1508+
},
1509+
OwnerReferences: []metav1.OwnerReference{
1510+
{
1511+
APIVersion: "nova.openstack.org/v1beta1",
1512+
Kind: "Nova",
1513+
Name: "nova",
1514+
UID: "fake-nova-uid-2",
1515+
},
1516+
},
1517+
},
1518+
Spec: rabbitmqv1.TransportURLSpec{
1519+
RabbitmqClusterName: rabbitmqName.Name,
1520+
Username: "anno-user-a",
1521+
},
1522+
}
1523+
Expect(k8sClient.Create(ctx, tu)).Should(Succeed())
1524+
DeferCleanup(th.DeleteInstance, tu)
1525+
1526+
CreateNodeSet(nodesetName)
1527+
DeferCleanup(DeleteNodeSet, nodesetName)
1528+
})
1529+
1530+
It("should release immediately when annotation says false despite Nova owner", func() {
1531+
SimulateRabbitMQClusterReady(rabbitmqName)
1532+
1533+
userACRName := types.NamespacedName{
1534+
Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "anno-user-a"),
1535+
Namespace: namespace,
1536+
}
1537+
Eventually(func(g Gomega) {
1538+
user := &rabbitmqv1.RabbitMQUser{}
1539+
g.Expect(k8sClient.Get(ctx, userACRName, user)).Should(Succeed())
1540+
}, timeout, interval).Should(Succeed())
1541+
1542+
SimulateRabbitMQUserReady(userACRName, "/")
1543+
1544+
transportURLSecretName := types.NamespacedName{
1545+
Name: "rabbitmq-transport-url-" + turlName.Name,
1546+
Namespace: namespace,
1547+
}
1548+
Eventually(func(g Gomega) {
1549+
tr := th.GetTransportURL(turlName)
1550+
g.Expect(tr.Status.RabbitmqUsername).To(Equal("anno-user-a"))
1551+
s := &corev1.Secret{}
1552+
g.Expect(k8sClient.Get(ctx, transportURLSecretName, s)).Should(Succeed())
1553+
}, timeout, interval).Should(Succeed())
1554+
1555+
initialHash := GetSecretHash(transportURLSecretName)
1556+
SetNodeSetSecretHashes(nodesetName, map[string]string{
1557+
transportURLSecretName.Name: initialHash,
1558+
})
1559+
1560+
// Trigger credential rotation
1561+
Eventually(func(g Gomega) {
1562+
tr := th.GetTransportURL(turlName)
1563+
tr.Spec.Username = "anno-user-b"
1564+
g.Expect(k8sClient.Update(ctx, tr)).Should(Succeed())
1565+
}, timeout, interval).Should(Succeed())
1566+
1567+
userBCRName := types.NamespacedName{
1568+
Name: rabbitmqv1.CanonicalUserName(rabbitmqName.Name, "/", "anno-user-b"),
1569+
Namespace: namespace,
1570+
}
1571+
Eventually(func(g Gomega) {
1572+
user := &rabbitmqv1.RabbitMQUser{}
1573+
g.Expect(k8sClient.Get(ctx, userBCRName, user)).Should(Succeed())
1574+
}, timeout, interval).Should(Succeed())
1575+
1576+
SimulateRabbitMQUserReady(userBCRName, "/")
1577+
1578+
// Annotation override: despite Nova owner, edpm-service=false
1579+
// should cause immediate release
1580+
Eventually(func(g Gomega) {
1581+
user := &rabbitmqv1.RabbitMQUser{}
1582+
err := k8sClient.Get(ctx, userACRName, user)
1583+
g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(),
1584+
"Annotation override should bypass owner-based EDPM detection")
1585+
}, timeout, interval).Should(Succeed())
1586+
1587+
Eventually(func(g Gomega) {
1588+
tr := th.GetTransportURL(turlName)
1589+
g.Expect(tr.Status.PreviousRabbitmqUserRef).To(BeEmpty())
1590+
}, timeout, interval).Should(Succeed())
1591+
})
1592+
})
1593+
13361594
When("username is changed for standalone TransportURL without owner", func() {
13371595
var rabbitmqName types.NamespacedName
13381596
var transportURLName types.NamespacedName

0 commit comments

Comments
 (0)