RockteMQ-AI commented on code in PR #61:
URL: https://github.com/apache/rocketmq-operator/pull/61#discussion_r3902480013
##########
deploy/crds/rocketmq_v1alpha1_broker_crd.yaml:
##########
@@ -79,6 +79,31 @@ spec:
items:
type: object
type: array
+ affinity:
+ description: Affinity and anti-affinity scheduling
+ type: object
Review Comment:
The `affinity` field is declared as `type: object` with no structural
schema. This allows any arbitrary YAML to pass CRD validation, deferring errors
to the kube-scheduler at runtime. Consider using
`x-kubernetes-preserve-unknown-fields: true` with a more specific schema, or at
minimum document the expected structure. Same issue applies to
`securityContext`, `nodeSelector`, and `podAnnotations` (all `type: object`),
and to `imagePullSecrets`/`tolerations` (`items: type: object`). While this
pattern is common in early-stage operators, it provides zero admission-time
validation.
##########
example/rocketmq_v1alpha1_rocketmq_feature_cluster.yaml:
##########
@@ -0,0 +1,180 @@
+# 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: v1
+kind: ConfigMap
+metadata:
+ name: broker-config
+data:
+ # BROKER_MEM sets the broker JVM, if set to "" then Xms = Xmx = max(min(1/2
ram, 1024MB), min(1/4 ram, 8GB))
+ BROKER_MEM: " -Xms2g -Xmx2g -Xmn1g "
+ broker-common.conf: |
+ # brokerClusterName, brokerName, brokerId are automatically generated by
the operator and do not set it manually!!!
+ deleteWhen=04
+ fileReservedTime=48
+ flushDiskType=ASYNC_FLUSH
+ # set brokerRole to ASYNC_MASTER or SYNC_MASTER. DO NOT set to SLAVE
because the replica instance will automatically be set!!!
+ brokerRole=ASYNC_MASTER
+
+---
+apiVersion: rocketmq.apache.org/v1alpha1
+kind: Broker
+metadata:
+ # name of broker cluster
+ name: broker
+spec:
+ # size is the number of the broker cluster, each broker cluster contains a
master broker and [replicaPerGroup] replica brokers.
+ size: 1
+ # nameServers is the [ip:port] list of name service
+ nameServers: ""
+ # replicaPerGroup is the number of each broker cluster
+ replicaPerGroup: 0
+ # brokerImage is the customized docker image repo of the RocketMQ broker
+ brokerImage: apacherocketmq/rocketmq-broker:4.5.0-alpine-operator-0.3.0
+ # imagePullPolicy is the image pull policy
+ imagePullPolicy: Always
+ # resources describes the compute resource requirements and limits
+ resources:
+ requests:
+ memory: "2048Mi"
+ cpu: "250m"
+ limits:
+ memory: "12288Mi"
+ cpu: "500m"
+ # allowRestart defines whether allow pod restart
+ allowRestart: true
+ # storageMode can be EmptyDir, HostPath, StorageClass
+ storageMode: EmptyDir
+ # hostPath is the local path to store data
+ hostPath: /tmp/data/rocketmq/broker
+ # scalePodName is [Broker name]-[broker group number]-master-0
+ scalePodName: broker-0-master-0
+ # env defines custom env, e.g. BROKER_MEM
+ env:
+ - name: BROKER_MEM
+ valueFrom:
+ configMapKeyRef:
+ name: broker-config
+ key: BROKER_MEM
+ # volumes defines the broker.conf
+ volumes:
+ - name: broker-config
+ configMap:
+ name: broker-config
+ items:
+ - key: broker-common.conf
+ path: broker-common.conf
+ # volumeClaimTemplates defines the storageClass
+ volumeClaimTemplates:
+ - metadata:
+ name: broker-storage
+ spec:
+ accessModes:
+ - ReadWriteOnce
+ resources:
+ requests:
+ storage: 8Gi
+ selector:
+ matchLabels:
+ app: broker-storage-pv
+---
+apiVersion: rocketmq.apache.org/v1alpha1
+kind: NameService
+metadata:
+ name: name-service
+spec:
+ # size is the the name service instance number of the name service cluster
+ size: 1
+ # nameServiceImage is the customized docker image repo of the RocketMQ name
service
+ nameServiceImage:
apacherocketmq/rocketmq-nameserver:4.5.0-alpine-operator-0.3.0
+ # imagePullPolicy is the image pull policy
+ imagePullPolicy: Always
+ # hostNetwork can be true or false
+ hostNetwork: true
+ podAnnotations:
+ prometheus.io/path: /metrics
+ prometheus.io/port: "5557"
+ prometheus.io/scrape: "true"
+ securityContext:
+ allowPrivilegeEscalation: true
+ runAsUser: 0
+ runAsGroup: 0
+ affinity:
+ nodeAffinity:
+ requiredDuringSchedulingIgnoredDuringExecution:
+ nodeSelectorTerms:
+ - matchExpressions:
+ - key: kubernetes.io/hostname
+ operator: In
+ values:
+ - azure-k8s-7
+ - azure-k8s-1
+ - azure-k8s-3
+ - azure-k8s-2
+ podAntiAffinity:
+ preferredDuringSchedulingIgnoredDuringExecution:
+ - weight: 100
Review Comment:
The example sets `allowPrivilegeEscalation: true` and `runAsUser: 0` /
`runAsGroup: 0`, which runs the NameService container as root with privilege
escalation enabled. This is a security anti-pattern and should not be in an
example file that users will copy. If root is truly required for RocketMQ, add
a comment explaining why and recommend dropping to a non-root user where
possible. At minimum, set `allowPrivilegeEscalation: false`.
##########
deploy/crds/rocketmq_v1alpha1_nameservice_crd.yaml:
##########
@@ -63,6 +63,31 @@ spec:
items:
type: object
type: array
+ affinity:
+ description: Affinity and anti-affinity scheduling
+ type: object
Review Comment:
Same unstructured schema issue as the broker CRD — `affinity`,
`securityContext`, `nodeSelector`, `podAnnotations` all accept arbitrary
objects. Since these fields are duplicated across two CRDs, consider extracting
a shared validation schema (or a shared struct with code-generated OpenAPI) to
keep them in sync and avoid drift.
##########
pkg/controller/broker/broker_controller.go:
##########
@@ -401,8 +401,15 @@ func (r *ReconcileBroker) getBrokerStatefulSet(broker
*rocketmqv1alpha1.Broker,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: ls,
+ Annotations: broker.Spec.PodAnnotations,
Review Comment:
`Annotations: broker.Spec.PodAnnotations` directly assigns the spec map to
the pod template. If a user later removes all annotations from the spec
(setting it to `nil` or `{}`), the existing annotations on the live
StatefulSet's pod template won't be cleared during an update unless the
controller explicitly handles this in its update/reconciliation path. Verify
that the update logic in `updateBrokerStatefulSet` (or equivalent) properly
propagates annotation removal, not just addition.
##########
pkg/apis/rocketmq/v1alpha1/broker_types.go:
##########
@@ -56,6 +56,21 @@ type BrokerSpec struct {
VolumeClaimTemplates []corev1.PersistentVolumeClaim
`json:"volumeClaimTemplates"`
// The name of pod where the metadata from
ScalePodName string `json:"scalePodName"`
+ // Affinity, affinity and anti-affinity scheduling
+ Affinity *corev1.Affinity `json:"affinity,omitempty"`
+ // SecurityContext defines privilege and access control settings for a
Pod or Container.
+ SecurityContext *corev1.PodSecurityContext
`json:"securityContext,omitempty"`
+ // ImagePullSecrets pull an image from a private registry
+ ImagePullSecrets []corev1.LocalObjectReference
`json:"imagePullSecrets,omitempty"`
+ // Tolerations taints and tolerations work together to ensure that pods
are not scheduled onto inappropriate nodes
+ Tolerations []corev1.Toleration
`json:"tolerations,omitempty"`
+ // NodeSelector is the simplest recommended form of node selection
constraint.
+ NodeSelector map[string]string
`json:"nodeSelector,omitempty"`
+ // PodAnnotations you can use annotations to attach arbitrary
non-identifying metadata to objects.
+ PodAnnotations map[string]string
`json:"podAnnotations,omitempty"`
+ // PriorityClassName defines priority class's name
+ PriorityClassName string
`json:"priorityClassName,omitempty"`
Review Comment:
No test coverage is added for the new fields. The broker and nameservice
controller tests should verify that: (1) a CR with these fields produces a
StatefulSet with the correct PodSpec, (2) nil/empty values produce the same
StatefulSet as before (backward compat), and (3) updates to these fields
trigger a rolling update. Without tests, regressions in the reconciliation
logic are likely to go undetected.
##########
pkg/controller/broker/broker_controller.go:
##########
@@ -401,8 +401,15 @@ func (r *ReconcileBroker) getBrokerStatefulSet(broker
*rocketmqv1alpha1.Broker,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: ls,
+ Annotations: broker.Spec.PodAnnotations,
},
Review Comment:
All six new PodSpec fields (`Affinity`, `SecurityContext`,
`ImagePullSecrets`, `Tolerations`, `NodeSelector`, `PriorityClassName`) are
assigned directly from the CR spec with no nil-guard. This is safe for the
Kubernetes API (nil/empty values are valid), but it means any change to these
fields will trigger a StatefulSet rolling update. For fields like
`nodeSelector` or `tolerations` that an operator might want to change
frequently, consider documenting this behavior so users understand the pod
churn implications.
##########
pkg/controller/nameservice/nameservice_controller.go:
##########
@@ -316,10 +316,17 @@ func (r *ReconcileNameService)
statefulSetForNameService(nameService *rocketmqv1
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: ls,
+ Annotations:
nameService.Spec.PodAnnotations,
Review Comment:
Same annotation propagation concern as the broker controller: `Annotations:
nameService.Spec.PodAnnotations` is a direct assignment. If the existing
StatefulSet's pod template has annotations set by a mutating webhook or by a
previous spec version, they will be silently overwritten on reconciliation.
Consider merging annotations (existing + spec) rather than replacing, or
document that spec annotations are authoritative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
