Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ spec:
size: 1
# nameServers is the [ip:port] list of name service
nameServers: ""
# rocketMQName is the rocketmq name, must equal to nameservice.spec.rocketMQName and and topictransfer.spec.rocketMQName
rocketMQName: "rocketmq_name"
# replicaPerGroup is the number of each broker cluster
replicaPerGroup: 1
# brokerImage is the customized docker image repo of the RocketMQ broker
Expand Down Expand Up @@ -245,6 +247,8 @@ spec:
nameServiceImage: apacherocketmq/rocketmq-nameserver:4.5.0-alpine-operator-0.3.0
# imagePullPolicy is the image pull policy
imagePullPolicy: Always
# rocketMQName is the rocketmq name, must equal to broker.spec.rocketMQName and and topictransfer.spec.rocketMQName
rocketMQName: "rocketmq_name"
# hostNetwork can be true or false
hostNetwork: true
# Set DNS policy for the pod.
Expand Down Expand Up @@ -440,6 +444,8 @@ spec:
sourceCluster: broker-0
# targetCluster defines the target cluster
targetCluster: broker-1
# rocketMQName is the rocketmq name, must equal to nameservice.spec.rocketMQName and and broker.spec.rocketMQName
rocketMQName: "rocketmq_name"
```

Then apply the ```TopicTransfer``` resource:
Expand Down
8 changes: 0 additions & 8 deletions cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/apache/rocketmq-operator/pkg/apis"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test changes detected alongside source modifications. Consider adding tests to cover the changes.

"github.com/apache/rocketmq-operator/pkg/controller"

"github.com/operator-framework/operator-sdk/pkg/k8sutil"
"github.com/operator-framework/operator-sdk/pkg/leader"
"github.com/operator-framework/operator-sdk/pkg/log/zap"
"github.com/operator-framework/operator-sdk/pkg/metrics"
Expand Down Expand Up @@ -76,12 +75,6 @@ func main() {

printVersion()

namespace, err := k8sutil.GetWatchNamespace()
if err != nil {
log.Error(err, "Failed to get watch namespace")
os.Exit(1)
}

// Get a config to talk to the apiserver
cfg, err := config.GetConfig()
if err != nil {
Expand All @@ -100,7 +93,6 @@ func main() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing Namespace from manager.Options changes the operator from namespace-scoped to cluster-scoped watching. This is intentional (given the new ClusterRole), but it is a significant behavioral change: the operator will now watch all namespaces, increasing API server load and requiring the new ClusterRole/ClusterRoleBinding to be applied. This should be explicitly documented in the PR and migration notes, and the old namespace-scoped Role/RoleBinding files should be deprecated or removed to avoid confusion.

// Create a new Cmd to provide shared dependencies and start components
mgr, err := manager.New(cfg, manager.Options{
Namespace: namespace,
MetricsBindAddress: fmt.Sprintf("%s:%d", metricsHost, metricsPort),
})
if err != nil {
Expand Down
72 changes: 72 additions & 0 deletions deploy/cluster_role.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You 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.
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
creationTimestamp: null
name: rocketmq-operator
rules:
- apiGroups:
- ""
resources:
- pods
- services
- endpoints
- persistentvolumeclaims
- events
- configmaps
- secrets
- pods/exec
verbs:
- '*'
- apiGroups:
- ""
resources:
- namespaces
verbs:
- get
- apiGroups:
- apps
resources:
- deployments
- daemonsets
- replicasets
- statefulsets
verbs:
- '*'
- apiGroups:
- monitoring.coreos.com
resources:
- servicemonitors
verbs:
- get
- create
- apiGroups:
- apps
resourceNames:
- rocketmq-operator
resources:
- deployments/finalizers
verbs:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rocketmq.apache.org ClusterRole rule lists both '*' (wildcard) and explicit resources brokers, pods/exec, topictransfers. The wildcard already covers everything, making the explicit entries redundant. More importantly, pods/exec is not a valid subresource of the rocketmq.apache.org API group — it belongs to the core "" group. This is a copy-paste error that should be removed to avoid confusion and potential future misinterpretation.

- update
- apiGroups:
- rocketmq.apache.org
resources:
- '*'
- brokers
- pods/exec
- topictransfers
verbs:
- '*'
27 changes: 27 additions & 0 deletions deploy/clusterrole_binding.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Licensed to the Apache Software Foundation (ASF) under one or more
# contributor license agreements. See the NOTICE file distributed with
# this work for additional information regarding copyright ownership.
# The ASF licenses this file to You 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.

kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: rocketmq-operator
subjects:
- kind: ServiceAccount
name: rocketmq-operator

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ClusterRoleBinding hardcodes namespace: default for the ServiceAccount subject. If the operator is deployed in a non-default namespace, this binding will be incorrect and the operator will lack permissions. This should be templated or documented clearly, since ClusterRole + hardcoded namespace is a common misconfiguration in multi-tenant deployments.

namespace: default
roleRef:
kind: ClusterRole
name: rocketmq-operator
apiGroup: rbac.authorization.k8s.io
4 changes: 4 additions & 0 deletions deploy/crds/rocketmq_v1alpha1_broker_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ spec:
items:
type: object
type: array
rocketMQName:
description: The name of rocketmq, and the broker and nameserver in the same cluster must be filled with the same name
type: string
required:
- size
- replicaPerGroup
Expand All @@ -93,6 +96,7 @@ spec:
- volumes

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rocketMQName is now a required field in the CRD. This is a breaking change for any existing Broker CR that does not have this field set. Existing clusters that upgrade to this operator version will have their Broker CRs fail validation. A defaulting webhook or a non-required field with a documented migration path is needed for safe upgrades.

- volumeClaimTemplates
- scalePodName
- rocketMQName
type: object
status:
properties:
Expand Down
4 changes: 4 additions & 0 deletions deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ spec:
items:
type: object
type: array
rocketMQName:
description: The name of rocketmq, and the broker and nameserver in the same cluster must be filled with the same name
type: string
required:
- size
- nameServiceImage
Expand All @@ -73,6 +76,7 @@ spec:
- storageMode

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same breaking change as the Broker CRD: making rocketMQName required on NameService CRD will invalidate all existing NameService CRs on upgrade. Needs a migration strategy.

- hostPath
- volumeClaimTemplates
- rocketMQName
type: object
status:
properties:
Expand Down
5 changes: 5 additions & 0 deletions deploy/crds/rocketmq_v1alpha1_topictransfer_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ spec:
topic:
description: Topic name
type: string
rocketMQName:
description: The name of rocketmq, and the broker and nameserver in the same cluster must be filled with the same name
type: string
required:
- rocketMQName
type: object
status:
type: object
Expand Down
2 changes: 2 additions & 0 deletions example/rocketmq_v1alpha1_broker_cr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ spec:
size: 1
# nameServers is the [ip:port] list of name service
nameServers: ""
# rocketMQName is the rocketmq name, must equal to nameserver.spec.rocketMQName and topictransfer.spec.rocketMQName

@liuruiyiyang liuruiyiyang Sep 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not ensure users set insistent name correctly, is there a better way to do this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because we don't have a rocketmq resource as the parent resource of the broker and nameserver. Without a common parent controller, we can only specify the connection of the child resource in the spec. So I hope to add a rocketmq api.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case should we add rocketmq higher level api before this PR?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so

rocketMQName: "rocketmq_name"
# replicaPerGroup is the number of each broker cluster
replicaPerGroup: 1
# brokerImage is the customized docker image repo of the RocketMQ broker
Expand Down
2 changes: 2 additions & 0 deletions example/rocketmq_v1alpha1_nameservice_cr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ spec:
nameServiceImage: apacherocketmq/rocketmq-nameserver:4.5.0-alpine-operator-0.3.0
# imagePullPolicy is the image pull policy
imagePullPolicy: Always
# rocketMQName is the rocketmq name, must equal to broker.spec.rocketMQName and topictransfer.spec.rocketMQName
rocketMQName: "rocketmq_name"
# hostNetwork can be true or false
hostNetwork: true
# Set DNS policy for the pod.
Expand Down
4 changes: 4 additions & 0 deletions example/rocketmq_v1alpha1_rocketmq_cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ spec:
size: 1
# nameServers is the [ip:port] list of name service
nameServers: ""
# rocketMQName is the rocketmq name, must equal to nameserver.spec.rocketMQName
rocketMQName: "rocketmq_name"
# replicaPerGroup is the number of each broker cluster
replicaPerGroup: 1
# brokerImage is the customized docker image repo of the RocketMQ broker
Expand Down Expand Up @@ -99,6 +101,8 @@ spec:
nameServiceImage: apacherocketmq/rocketmq-nameserver:4.5.0-alpine-operator-0.3.0
# imagePullPolicy is the image pull policy
imagePullPolicy: Always
# rocketMQName is the rocketmq name, must equal to broker.spec.rocketMQName and and topictransfer.spec.rocketMQName
rocketMQName: "rocketmq_name"
# hostNetwork can be true or false
hostNetwork: true
# Set DNS policy for the pod.
Expand Down
2 changes: 2 additions & 0 deletions example/rocketmq_v1alpha1_topictransfer_cr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ spec:
sourceCluster: broker-0
# targetCluster defines the target cluster
targetCluster: broker-1
# rocketMQName is the rocketmq name, must equal to nameserver.spec.rocketMQName and broker.spec.rocketMQName
rocketMQName: "rocketmq_name"
2 changes: 2 additions & 0 deletions pkg/apis/rocketmq/v1alpha1/broker_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ type BrokerSpec struct {
VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
// The name of pod where the metadata from
ScalePodName string `json:"scalePodName"`
// RocketMQ Name, the broker and nameserver in the same cluster must be filled with the same name
RocketMQName string `json:"rocketMQName"`
}

// BrokerStatus defines the observed state of Broker
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/rocketmq/v1alpha1/nameservice_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ type NameServiceSpec struct {
HostPath string `json:"hostPath"`
// VolumeClaimTemplates defines the StorageClass
VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates"`
// RocketMQ Name, the broker and nameserver in the same cluster must be filled with the same name
RocketMQName string `json:"rocketMQName"`
}

// NameServiceStatus defines the observed state of NameService
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/rocketmq/v1alpha1/topictransfer_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type TopicTransferSpec struct {
SourceCluster string `json:"sourceCluster,omitempty"`
// The cluster where the topic will be transferred to
TargetCluster string `json:"targetCluster,omitempty"`
// // RocketMQ Name, the broker and nameserver in the same cluster must be filled with the same name

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double comment marker on line 40: // // RocketMQ Name, ... — there is a tab and an extra // before the actual comment text. This is a copy-paste artifact and should be // RocketMQ Name, ....

RocketMQName string `json:"rocketMQName"`
}

// TopicTransferStatus defines the observed state of TopicTransfer
Expand Down
Loading