From 9d411cf04a695e7a3f41036e8377b0aa544d754d Mon Sep 17 00:00:00 2001 From: rongtong Date: Thu, 11 May 2023 03:01:29 -0700 Subject: [PATCH] Make configPath unable to update at runtime (#6733) * Make configPath unable to update at runtime * Make configPath unable to update at runtime * Add ASF header * Add ASF header * Pass the check style * Polish the response code * Polish the response code --- .../processor/AdminBrokerProcessor.java | 10 ++- .../processor/AdminBrokerProcessorTest.java | 32 +++++++++ .../processor/ControllerRequestProcessor.java | 6 ++ .../ControllerRequestProcessorTest.java | 70 +++++++++++++++++++ .../processor/DefaultRequestProcessor.java | 6 ++ .../processor/RequestProcessorTest.java | 41 ++++++++++- 6 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java diff --git a/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java b/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java index 65e45e81780..be673b91619 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java @@ -775,13 +775,21 @@ private synchronized RemotingCommand updateBrokerConfig(ChannelHandlerContext ct String bodyStr = new String(body, MixAll.DEFAULT_CHARSET); Properties properties = MixAll.string2Properties(bodyStr); if (properties != null) { - LOGGER.info("updateBrokerConfig, new config: [{}] client: {} ", properties, ctx.channel().remoteAddress()); + LOGGER.info("updateBrokerConfig, new config: [{}] client: {} ", properties, callerAddress); + + if (properties.containsKey("brokerConfigPath")) { + response.setCode(ResponseCode.NO_PERMISSION); + response.setRemark("Can not update config path"); + return response; + } + this.brokerController.getConfiguration().update(properties); if (properties.containsKey("brokerPermission")) { long stateMachineVersion = brokerController.getMessageStore() != null ? brokerController.getMessageStore().getStateMachineVersion() : 0; this.brokerController.getTopicConfigManager().getDataVersion().nextVersion(stateMachineVersion); this.brokerController.registerBrokerAll(false, false, true); } + } else { LOGGER.error("string2Properties error"); response.setCode(ResponseCode.SYSTEM_ERROR); diff --git a/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java b/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java index 7af19aee46f..fa2d929b0c9 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java @@ -24,8 +24,10 @@ import java.net.SocketAddress; import java.net.UnknownHostException; import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; +import java.util.Properties; import java.util.Set; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.LongAdder; @@ -276,6 +278,36 @@ public void testGetBrokerConfig() throws Exception { assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); } + @Test + public void testProcessRequest_UpdateConfigPath() throws RemotingCommandException { + final RemotingCommand updateConfigRequest = RemotingCommand.createRequestCommand(RequestCode.UPDATE_BROKER_CONFIG, null); + Properties properties = new Properties(); + + ChannelHandlerContext ctx = mock(ChannelHandlerContext.class); + when(ctx.channel()).thenReturn(null); + + // Update allowed value + properties.setProperty("allAckInSyncStateSet", "true"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + RemotingCommand response = adminBrokerProcessor.processRequest(ctx, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); + + //update disallowed value + properties.clear(); + properties.setProperty("brokerConfigPath", "test/path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = adminBrokerProcessor.processRequest(ctx, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config path"); + + } + @Test public void testSearchOffsetByTimestamp() throws Exception { messageStore = mock(MessageStore.class); diff --git a/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java b/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java index e4789060a1c..6010f81eebc 100644 --- a/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java +++ b/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java @@ -241,6 +241,12 @@ private RemotingCommand handleUpdateControllerConfig(ChannelHandlerContext ctx, return response; } + if (properties.containsKey("configStorePath")) { + response.setCode(ResponseCode.NO_PERMISSION); + response.setRemark("Can not update config path"); + return response; + } + this.controllerManager.getConfiguration().update(properties); } diff --git a/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java b/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java new file mode 100644 index 00000000000..ede6ca36a49 --- /dev/null +++ b/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java @@ -0,0 +1,70 @@ +/* + * 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. + */ + +package org.apache.rocketmq.controller; + +import java.nio.charset.StandardCharsets; +import java.util.Properties; +import org.apache.rocketmq.common.ControllerConfig; +import org.apache.rocketmq.common.MixAll; +import org.apache.rocketmq.controller.processor.ControllerRequestProcessor; +import org.apache.rocketmq.remoting.netty.NettyClientConfig; +import org.apache.rocketmq.remoting.netty.NettyServerConfig; +import org.apache.rocketmq.remoting.protocol.RemotingCommand; +import org.apache.rocketmq.remoting.protocol.RequestCode; +import org.apache.rocketmq.remoting.protocol.ResponseCode; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ControllerRequestProcessorTest { + + private ControllerRequestProcessor controllerRequestProcessor; + + @Before + public void init() throws Exception { + controllerRequestProcessor = new ControllerRequestProcessor(new ControllerManager(new ControllerConfig(), new NettyServerConfig(), new NettyClientConfig())); + } + + @Test + public void testProcessRequest_UpdateConfigPath() throws Exception { + final RemotingCommand updateConfigRequest = RemotingCommand.createRequestCommand(RequestCode.UPDATE_CONTROLLER_CONFIG, null); + Properties properties = new Properties(); + + // Update allowed value + properties.setProperty("notifyBrokerRoleChanged", "true"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + RemotingCommand response = controllerRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); + + // Update disallowed value + properties.clear(); + properties.setProperty("configStorePath", "test/path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = controllerRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config path"); + + } +} diff --git a/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java b/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java index b6728c19734..8220507d25e 100644 --- a/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java +++ b/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java @@ -627,6 +627,12 @@ private RemotingCommand updateConfig(ChannelHandlerContext ctx, RemotingCommand return response; } + if (properties.containsKey("kvConfigPath") || properties.containsKey("configStorePathName")) { + response.setCode(ResponseCode.NO_PERMISSION); + response.setRemark("Can not update config path"); + return response; + } + this.namesrvController.getConfiguration().update(properties); } diff --git a/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java b/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java index 1b171959bae..8e7a21d98e7 100644 --- a/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java +++ b/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java @@ -21,10 +21,13 @@ import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.HashMap; import java.util.Map; +import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; +import org.apache.rocketmq.common.MixAll; import org.apache.rocketmq.common.TopicConfig; import org.apache.rocketmq.common.UtilAll; import org.apache.rocketmq.common.namesrv.NamesrvConfig; @@ -85,7 +88,6 @@ public void init() throws Exception { clientRequestProcessor = new ClientRequestProcessor(namesrvController); - registerRouteInfoManager(); logger = mock(Logger.class); @@ -178,6 +180,43 @@ public void testProcessRequest_UnSupportedRequest() throws RemotingCommandExcept assertThat(response.getCode()).isEqualTo(ResponseCode.REQUEST_CODE_NOT_SUPPORTED); } + @Test + public void testProcessRequest_UpdateConfigPath() throws RemotingCommandException { + final RemotingCommand updateConfigRequest = RemotingCommand.createRequestCommand(RequestCode.UPDATE_NAMESRV_CONFIG, null); + Properties properties = new Properties(); + + // Update allowed value + properties.setProperty("enableTopicList", "true"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + RemotingCommand response = defaultRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.SUCCESS); + + //update disallowed value + properties.clear(); + properties.setProperty("configStorePathName", "test/path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = defaultRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config path"); + + //update disallowed values + properties.clear(); + properties.setProperty("kvConfigPath", "test/path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = defaultRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config path"); + } + @Test public void testProcessRequest_RegisterBroker() throws RemotingCommandException, NoSuchFieldException, IllegalAccessException {