-
Notifications
You must be signed in to change notification settings - Fork 59
Rabbitmq vhost and user support #1052
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import ( | |
| "fmt" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| rabbitmqv1 "github.com/openstack-k8s-operators/infra-operator/apis/rabbitmq/v1beta1" | ||
| service "github.com/openstack-k8s-operators/lib-common/modules/common/service" | ||
| "github.com/robfig/cron/v3" | ||
|
|
||
|
|
@@ -88,6 +89,24 @@ func (spec *NovaSpecCore) Default() { | |
| spec.APITimeout = novaDefaults.APITimeout | ||
| } | ||
|
|
||
| // Default MessagingBus.Cluster from APIMessageBusInstance if not already set | ||
| if spec.MessagingBus.Cluster == "" { | ||
| spec.MessagingBus.Cluster = spec.APIMessageBusInstance | ||
| } | ||
|
|
||
| // Default NotificationsBus if NotificationsBusInstance is specified | ||
| if spec.NotificationsBusInstance != nil && *spec.NotificationsBusInstance != "" { | ||
| if spec.NotificationsBus == nil { | ||
| // Initialize empty NotificationsBus - credentials will be created dynamically | ||
| // to ensure separation from MessagingBus (RPC and notifications should never share credentials) | ||
| spec.NotificationsBus = &rabbitmqv1.RabbitMqConfig{} | ||
| } | ||
| // Default cluster name if not already set | ||
| if spec.NotificationsBus.Cluster == "" { | ||
| spec.NotificationsBus.Cluster = *spec.NotificationsBusInstance | ||
| } | ||
| } | ||
|
|
||
| for cellName, cellTemplate := range spec.CellTemplates { | ||
|
|
||
| if cellTemplate.MetadataServiceTemplate.Enabled == nil { | ||
|
|
@@ -106,6 +125,11 @@ func (spec *NovaSpecCore) Default() { | |
| } | ||
| } | ||
|
|
||
| // Default MessagingBus.Cluster from CellMessageBusInstance if not already set | ||
| if cellTemplate.MessagingBus.Cluster == "" { | ||
| cellTemplate.MessagingBus.Cluster = cellTemplate.CellMessageBusInstance | ||
| } | ||
|
|
||
| // "cellTemplate" is a by-value copy, so we need to re-inject the updated version of it into the map | ||
| spec.CellTemplates[cellName] = cellTemplate | ||
| } | ||
|
|
@@ -315,7 +339,34 @@ func (spec *NovaSpec) ValidateUpdate(old NovaSpec, basePath *field.Path, namespa | |
| // expected to be called by the validation webhook in the higher level meta | ||
| // operator | ||
| func (spec *NovaSpecCore) ValidateUpdate(old NovaSpecCore, basePath *field.Path, namespace string) field.ErrorList { | ||
| errors := spec.ValidateCellTemplates(basePath, namespace) | ||
| var errors field.ErrorList | ||
|
|
||
| // Reject changes to deprecated messagingBusInstance fields - users should use the new messagingBus fields instead | ||
| if spec.APIMessageBusInstance != old.APIMessageBusInstance { | ||
| errors = append(errors, field.Forbidden( | ||
| basePath.Child("apiMessageBusInstance"), | ||
| "apiMessageBusInstance is deprecated and cannot be changed. Please use messagingBus.cluster instead")) | ||
| } | ||
|
|
||
| if spec.NotificationsBusInstance != nil && old.NotificationsBusInstance != nil && | ||
| *spec.NotificationsBusInstance != *old.NotificationsBusInstance { | ||
| errors = append(errors, field.Forbidden( | ||
| basePath.Child("notificationsBusInstance"), | ||
| "notificationsBusInstance is deprecated and cannot be changed. Please use notificationsBus.cluster instead")) | ||
| } | ||
|
|
||
| // Check cell template changes | ||
| for cellName, cellTemplate := range spec.CellTemplates { | ||
| if oldCell, exists := old.CellTemplates[cellName]; exists { | ||
| if cellTemplate.CellMessageBusInstance != oldCell.CellMessageBusInstance { | ||
| errors = append(errors, field.Forbidden( | ||
| basePath.Child("cellTemplates").Key(cellName).Child("cellMessageBusInstance"), | ||
| "cellMessageBusInstance is deprecated and cannot be changed. Please use messagingBus.cluster instead")) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| errors = append(errors, spec.ValidateCellTemplates(basePath, namespace)...) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so i shoudl point out that explicit block 2 cells form sharing a rabbit instance since that is not supproted when not using seperat vhost per cell. we need to enhance ValidateCellTemplates to account for vhost and more imporatnly the new cellTemplate.MessagingBus.Cluster field. the quetion i have is do we want to enforce that you cant share the same rabbit cluster btween cells via the webhook or jsut document that as unsupproted in our product. three isnt a reasonable usecase for sharign beyond testing/dev by levrageing vhosts as cells only exist to shard your cloud to supprot scaling beyond the limitats fo a sincel db/rabbit cluster. my inclindation is to block 2 cells form using the same rabbit cluster in the webhook to ensure that unsupproted configuration are not possible rather then supproting it only for testing. |
||
| // Validate top-level TopologyRef | ||
| errors = append(errors, topologyv1.ValidateTopologyRef( | ||
| spec.TopologyRef, *basePath.Child("topologyRef"), namespace)...) | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we should allow them to null it out if its set
but we could reject other changes How that is a breaking api change so im not sold on doing that.
it defeats the reason for ahvaing a deprecation period
what woudl be more correct is to not allow setting both at the same time.
i.e you can either use the old field or new filed but not both.
i woudl condier it an error if
cellTemplate.MessagingBus.Cluster and APIMessageBusInstance were ever diffent unless APIMessageBusInstance was nil or ""
we should exicltiy check for an reject that.