Skip to content

Commit

Permalink
[BugFix] Fix match of source ip for resource group (backport #47732) (#…
Browse files Browse the repository at this point in the history
…48173)

CP from #47732.

## Why I'm doing:

## What I'm doing:

Fixes #47437.

1. Match
   `SubnetUtils::isInRange` doesn't consider the broadcast and network address as a usable endpoint by default. As a result, `127.0.0.1` cannot matched `127.0.0.1/32`.

2. Weight
The weight of `sourceIP` of a resource group classifier is `1 + cidrPrefixLength/64`. However, we calculate `cidrPrefixLength` by `Long.numberOfLeadingZeros(subnetUtils.getInfo().getAddressCountLong()) - 32`, which should be `Long.numberOfLeadingZeros(subnetUtils.getInfo().getAddressCountLong()) - 31`.

Signed-off-by: zihe.liu <[email protected]>
  • Loading branch information
ZiheLiu committed Jul 11, 2024
1 parent db50e29 commit 80b7ae8
Show file tree
Hide file tree
Showing 6 changed files with 158 additions and 21 deletions.
8 changes: 8 additions & 0 deletions fe/fe-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,14 @@ under the License.
<scope>test</scope>
</dependency>

<!-- https://mvnrepository.com/artifact/org.assertj/assertj-core -->
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.24.2</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.apache.iceberg</groupId>
<artifactId>iceberg-api</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
import com.google.gson.annotations.SerializedName;
import com.starrocks.common.io.Text;
import com.starrocks.common.io.Writable;
import com.starrocks.common.util.NetUtils;
import com.starrocks.persist.gson.GsonUtils;
import com.starrocks.server.GlobalStateMgr;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.net.util.SubnetUtils;

import java.io.DataInput;
import java.io.DataOutput;
Expand Down Expand Up @@ -132,7 +132,7 @@ public boolean isVisible(String user, String role, String sourceIp) {
return false;
}
if (this.sourceIp != null && sourceIp != null) {
return new SubnetUtils(this.sourceIp).getInfo().isInRange(sourceIp);
return NetUtils.isIPInSubnet(sourceIp, this.sourceIp);
}
return true;
}
Expand All @@ -149,8 +149,7 @@ public double weight() {
w += 1 + 0.1 / queryTypes.size();
}
if (sourceIp != null) {
SubnetUtils.SubnetInfo subnetInfo = new SubnetUtils(sourceIp).getInfo();
w += 1 + (Long.numberOfLeadingZeros(subnetInfo.getAddressCountLong() + 2) - 32) / 64.0;
w += 1 + NetUtils.getCidrPrefixLength(sourceIp) / 64.0;
}
if (CollectionUtils.isNotEmpty(databaseIds)) {
w += 10.0 * databaseIds.size();
Expand Down
20 changes: 20 additions & 0 deletions fe/fe-core/src/main/java/com/starrocks/common/util/NetUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import com.google.common.base.Strings;
import com.starrocks.common.Pair;
import org.apache.commons.net.util.SubnetUtils;
import org.apache.commons.validator.routines.InetAddressValidator;

import java.io.IOException;
Expand Down Expand Up @@ -99,4 +100,23 @@ public static boolean checkAccessibleForAllPorts(String host, List<Integer> port
}
return accessible;
}

public static boolean isIPInSubnet(String ip, String subnetCidr) {
SubnetUtils subnetUtils = new SubnetUtils(subnetCidr);
subnetUtils.setInclusiveHostCount(true);
return subnetUtils.getInfo().isInRange(ip);
}

/**
* Get the prefix length of the CIDR, that is the `y` part of `xxx.xxx.xxx.xxx/y` in CIDR, e.g. 16 for `192.168.0.1/16`.
* @param cidr The CIDR format address.
* @return The length of the prefix. The range is within [0, 32].
*/
public static int getCidrPrefixLength(String cidr) {
SubnetUtils subnetUtils = new SubnetUtils(cidr);
subnetUtils.setInclusiveHostCount(true);
// 2^(32 - prefixLength) = addressCount,
// so prefixLength = 32 - log2(addressCount) = 32 - (63 - leadingZeros(addressCount)) = leadingZeros(addressCount) - 31
return Long.numberOfLeadingZeros(subnetUtils.getInfo().getAddressCountLong()) - 31;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.google.common.collect.ImmutableSet;
import com.starrocks.catalog.ResourceGroupClassifier;
import com.starrocks.common.AnalysisException;
import com.starrocks.common.Config;
import com.starrocks.common.DdlException;
import com.starrocks.common.FeConstants;
import com.starrocks.qe.ConnectContext;
Expand All @@ -27,6 +28,7 @@
import java.util.stream.Collectors;

import static com.starrocks.common.ErrorCode.ERROR_NO_WG_ERROR;
import static org.assertj.core.api.Assertions.assertThat;

public class ResourceGroupStmtTest {
private static final Pattern idPattern = Pattern.compile("\\bid=(\\b\\d+\\b)");
Expand Down Expand Up @@ -194,18 +196,18 @@ public void testCreateResourceGroup() throws Exception {
List<List<String>> rows = starRocksAssert.executeResourceGroupShowSql("show resource groups all");
String result = rowsToString(rows);
String expect = "" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=4.459375, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=3.459375, user=rg1_user2, query_type in (SELECT), source_ip=192.168.3.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=2.359375, user=rg1_user3, source_ip=192.168.4.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=4.475, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=3.475, user=rg1_user2, query_type in (SELECT), source_ip=192.168.3.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=2.375, user=rg1_user3, source_ip=192.168.4.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=1.0, user=rg1_user4)\n" +
"rg2|30|50.0%|0|0|0|20|NORMAL|(weight=3.459375, role=rg2_role1, query_type in (SELECT), source_ip=192.168.5.1/24)\n" +
"rg2|30|50.0%|0|0|0|20|NORMAL|(weight=2.359375, role=rg2_role2, source_ip=192.168.6.1/24)\n" +
"rg2|30|50.0%|0|0|0|20|NORMAL|(weight=3.475, role=rg2_role1, query_type in (SELECT), source_ip=192.168.5.1/24)\n" +
"rg2|30|50.0%|0|0|0|20|NORMAL|(weight=2.375, role=rg2_role2, source_ip=192.168.6.1/24)\n" +
"rg2|30|50.0%|0|0|0|20|NORMAL|(weight=1.0, role=rg2_role3)\n" +
"rg3|32|80.0%|0|0|0|10|NORMAL|(weight=2.459375, query_type in (SELECT), source_ip=192.168.6.1/24)\n" +
"rg3|32|80.0%|0|0|0|10|NORMAL|(weight=2.475, query_type in (SELECT), source_ip=192.168.6.1/24)\n" +
"rg3|32|80.0%|0|0|0|10|NORMAL|(weight=1.1, query_type in (SELECT))\n" +
"rg4|25|80.0%|1024|1024|1024|10|NORMAL|(weight=1.359375, source_ip=192.168.7.1/24)\n" +
"rg4|25|80.0%|1024|1024|1024|10|NORMAL|(weight=1.375, source_ip=192.168.7.1/24)\n" +
"rg5|25|80.0%|0|0|0|10|NORMAL|(weight=10.0, db='db1')\n" +
"rg6|32|80.0%|0|0|0|10|NORMAL|(weight=2.459375, query_type in (INSERT), source_ip=192.168.6.1/24)\n" +
"rg6|32|80.0%|0|0|0|10|NORMAL|(weight=2.475, query_type in (INSERT), source_ip=192.168.6.1/24)\n" +
"rt_rg1|25|80.0%|0|0|0|10|SHORT_QUERY|(weight=1.0, user=rt_rg_user)";
Assert.assertEquals(result, expect);
dropResourceGroups();
Expand Down Expand Up @@ -529,7 +531,7 @@ public void testShowVisibleResourceGroups() throws Exception {
String result = rowsToString(rows);
String expect = "" +
"rg5|25|80.0%|0|0|0|10|NORMAL|(weight=10.0, db='db1')\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=4.459375, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg1|10|20.0%|0|0|0|11|NORMAL|(weight=4.475, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg3|32|80.0%|0|0|0|10|NORMAL|(weight=1.1, query_type in (SELECT))";
Assert.assertEquals(result, expect);
dropResourceGroups();
Expand Down Expand Up @@ -573,18 +575,18 @@ public void testAlterResourceGroupAlterProperties() throws Exception {
List<List<String>> rows = starRocksAssert.executeResourceGroupShowSql("SHOW RESOURCE GROUPS all");
String result = rowsToString(rows);
String expect = "" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=4.459375, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=3.459375, user=rg1_user2, query_type in (SELECT), source_ip=192.168.3.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=2.359375, user=rg1_user3, source_ip=192.168.4.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=4.475, user=rg1_user1, role=rg1_role1, query_type in (SELECT), source_ip=192.168.2.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=3.475, user=rg1_user2, query_type in (SELECT), source_ip=192.168.3.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=2.375, user=rg1_user3, source_ip=192.168.4.1/24)\n" +
"rg1|21|20.0%|0|0|0|11|NORMAL|(weight=1.0, user=rg1_user4)\n" +
"rg2|30|37.0%|0|0|0|20|NORMAL|(weight=3.459375, role=rg2_role1, query_type in (SELECT), source_ip=192.168.5.1/24)\n" +
"rg2|30|37.0%|0|0|0|20|NORMAL|(weight=2.359375, role=rg2_role2, source_ip=192.168.6.1/24)\n" +
"rg2|30|37.0%|0|0|0|20|NORMAL|(weight=3.475, role=rg2_role1, query_type in (SELECT), source_ip=192.168.5.1/24)\n" +
"rg2|30|37.0%|0|0|0|20|NORMAL|(weight=2.375, role=rg2_role2, source_ip=192.168.6.1/24)\n" +
"rg2|30|37.0%|0|0|0|20|NORMAL|(weight=1.0, role=rg2_role3)\n" +
"rg3|32|80.0%|0|0|0|23|NORMAL|(weight=2.459375, query_type in (SELECT), source_ip=192.168.6.1/24)\n" +
"rg3|32|80.0%|0|0|0|23|NORMAL|(weight=2.475, query_type in (SELECT), source_ip=192.168.6.1/24)\n" +
"rg3|32|80.0%|0|0|0|23|NORMAL|(weight=1.1, query_type in (SELECT))\n" +
"rg4|13|41.0%|1024|1024|1024|23|NORMAL|(weight=1.359375, source_ip=192.168.7.1/24)\n" +
"rg4|13|41.0%|1024|1024|1024|23|NORMAL|(weight=1.375, source_ip=192.168.7.1/24)\n" +
"rg5|25|80.0%|0|0|0|10|NORMAL|(weight=10.0, db='db1')\n" +
"rg6|32|80.0%|0|0|0|10|NORMAL|(weight=2.459375, query_type in (INSERT), source_ip=192.168.6.1/24)\n" +
"rg6|32|80.0%|0|0|0|10|NORMAL|(weight=2.475, query_type in (INSERT), source_ip=192.168.6.1/24)\n" +
"rt_rg1|25|80.0%|0|0|0|10|SHORT_QUERY|(weight=1.0, user=rt_rg_user)";
Assert.assertEquals(result, expect);
dropResourceGroups();
Expand Down Expand Up @@ -726,4 +728,21 @@ public void testChooseShortQueryResourceGroup() throws Exception {

dropResourceGroups();
}

@Test
public void testSourceIP() throws Exception {
String createSQL = "create resource group rg1\n" +
"to\n" +
" (source_ip='192.168.2.1/32')\n" +
"with (\n" +
" 'cpu_core_limit' = '10',\n" +
" 'mem_limit' = '20%'\n" +
");";
starRocksAssert.executeResourceGroupDdlSql(createSQL);
List<List<String>> rows = starRocksAssert.executeResourceGroupShowSql("show resource groups all");
assertThat(rowsToString(rows)).isEqualTo(
"rg1|10|20.0%|0|0|0|null|NORMAL|(weight=1.5, source_ip=192.168.2.1/32)");

starRocksAssert.executeResourceGroupDdlSql("DROP RESOURCE GROUP rg1");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright 2021-present StarRocks, Inc. All rights reserved.
//
// Licensed 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
//
// https://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 com.starrocks.catalog;

import org.junit.Test;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.within;

public class ResourceGroupClassifierTest {
@Test
public void testSourceIP() {
ResourceGroupClassifier classifier = new ResourceGroupClassifier();

classifier.setSourceIp("192.168.0.1/32");
assertThat(classifier.isVisible("user", null, "192.168.0.1")).isTrue();
assertThat(classifier.isVisible("user", null, "192.168.0.2")).isFalse();

classifier.setSourceIp("192.168.0.1/31");
assertThat(classifier.isVisible("user", null, "192.168.0.1")).isTrue();
assertThat(classifier.isVisible("user", null, "192.168.0.2")).isFalse();

classifier.setSourceIp("192.168.0.1/30");
assertThat(classifier.isVisible("user", null, "192.168.0.1")).isTrue();
assertThat(classifier.isVisible("user", null, "192.168.0.2")).isTrue();
assertThat(classifier.isVisible("user", null, "192.168.1.1")).isFalse();
}

@Test
public void testWeight() {
ResourceGroupClassifier classifier = new ResourceGroupClassifier();

for (int i = 0; i <= 32; i++) {
classifier.setSourceIp("192.168.0.1/" + i);
assertThat(classifier.weight()).isCloseTo(1 + i / 64., within(1e-5));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2021-present StarRocks, Inc. All rights reserved.
//
// Licensed 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
//
// https://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 com.starrocks.common.util;

import org.junit.Test;

import static org.assertj.core.api.Assertions.assertThat;

public class NetUtilsTest {
@Test
public void testGetCidrPrefixLength() {
for (int i = 0; i <= 32; i++) {
String addr = "192.168.0.1/" + i;
assertThat(NetUtils.getCidrPrefixLength(addr)).isEqualTo(i);
}
}

@Test
public void testIsIPInSubnet() {
assertThat(NetUtils.isIPInSubnet("192.168.0.1", "192.168.0.1/32")).isTrue();
assertThat(NetUtils.isIPInSubnet("192.168.0.1", "192.168.1.1/32")).isFalse();

assertThat(NetUtils.isIPInSubnet("192.168.0.1", "192.168.0.1/24")).isTrue();
assertThat(NetUtils.isIPInSubnet("192.168.0.1", "192.168.0.2/24")).isTrue();
assertThat(NetUtils.isIPInSubnet("192.168.0.1", "192.168.1.0/24")).isFalse();

assertThat(NetUtils.isIPInSubnet("192.168.0.1", "10.0.0.0/8")).isFalse();
}
}

0 comments on commit 80b7ae8

Please sign in to comment.