HDDS-10481. Use proto 3 in SCMRatisProtocol.proto (#9695)
diff --git a/dev-support/pmd/pmd-ruleset.xml b/dev-support/pmd/pmd-ruleset.xml index 6943837..897ae36 100644 --- a/dev-support/pmd/pmd-ruleset.xml +++ b/dev-support/pmd/pmd-ruleset.xml
@@ -52,5 +52,6 @@ </rule> <exclude-pattern>.*/generated-sources/.*</exclude-pattern> + <exclude-pattern>.*/generated-test-sources/.*</exclude-pattern> <exclude-pattern>.*/org/apache/hadoop/.*_/.*</exclude-pattern> </ruleset>
diff --git a/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto b/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto index 3ae9879..96afb36 100644 --- a/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto +++ b/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto
@@ -15,45 +15,50 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -syntax = "proto2"; +syntax = "proto3"; + option java_package = "org.apache.hadoop.hdds.protocol.proto"; option java_outer_classname = "SCMRatisProtocol"; option java_generate_equals_and_hash = true; enum RequestType { - PIPELINE = 1; - CONTAINER = 2; - BLOCK = 3; - SEQUENCE_ID = 4; - CERT_STORE = 5; - MOVE = 6; - STATEFUL_SERVICE_CONFIG = 7; - FINALIZE = 8; - SECRET_KEY = 9; - CERT_ROTATE = 10; + REQUEST_TYPE_UNSPECIFIED = 0; + + PIPELINE = 1; + CONTAINER = 2; + BLOCK = 3; + SEQUENCE_ID = 4; + CERT_STORE = 5; + MOVE = 6; + STATEFUL_SERVICE_CONFIG = 7; + FINALIZE = 8; + SECRET_KEY = 9; + CERT_ROTATE = 10; } message Method { - required string name = 1; - repeated MethodArgument args = 2; + optional string name = 1; + repeated MethodArgument args = 2; } message MethodArgument { - required string type = 1; - required bytes value = 2; + optional string type = 1; + optional bytes value = 2; } message ListArgument { - required string type = 1; - repeated bytes value = 2; + optional string type = 1; + repeated bytes value = 2; } message SCMRatisRequestProto { - required RequestType type = 1; - required Method method = 2; + optional RequestType type = 1; + optional Method method = 2; } message SCMRatisResponseProto { - required string type = 2; - required bytes value = 3; + reserved 1; + + optional string type = 2; + optional bytes value = 3; }
diff --git a/hadoop-hdds/server-scm/pom.xml b/hadoop-hdds/server-scm/pom.xml index d8cfb03..78c5124 100644 --- a/hadoop-hdds/server-scm/pom.xml +++ b/hadoop-hdds/server-scm/pom.xml
@@ -308,6 +308,22 @@ <excludeFilterFile>${basedir}/dev-support/findbugsExcludeFile.xml</excludeFilterFile> </configuration> </plugin> + <plugin> + <groupId>org.xolstice.maven.plugins</groupId> + <artifactId>protobuf-maven-plugin</artifactId> + <configuration> + <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact> + </configuration> + <executions> + <execution> + <id>compile-test-protobuf</id> + <goals> + <goal>test-compile</goal> + </goals> + <phase>generate-test-sources</phase> + </execution> + </executions> + </plugin> </plugins> </build> </project>
diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java index 67db93d..28fca97 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java
@@ -123,11 +123,33 @@ public static SCMRatisRequest decode(Message message) throws InvalidProtocolBufferException { final SCMRatisRequestProto requestProto = SCMRatisRequestProto.parseFrom(message.getContent().toByteArray()); + + // proto2 required-equivalent checks + if (!requestProto.hasType()) { + throw new InvalidProtocolBufferException("Missing request type"); + } + if (!requestProto.hasMethod()) { + throw new InvalidProtocolBufferException("Missing method"); + } + final Method method = requestProto.getMethod(); + + // proto2 required-equivalent checks + if (!method.hasName()) { + throw new InvalidProtocolBufferException("Missing method name"); + } + List<Object> args = new ArrayList<>(); Class<?>[] parameterTypes = new Class[method.getArgsCount()]; int paramCounter = 0; for (MethodArgument argument : method.getArgsList()) { + // proto2 required-equivalent checks + if (!argument.hasType()) { + throw new InvalidProtocolBufferException("Missing argument type"); + } + if (!argument.hasValue()) { + throw new InvalidProtocolBufferException("Missing argument value"); + } try { final Class<?> clazz = ReflectionUtil.getClass(argument.getType()); parameterTypes[paramCounter++] = clazz;
diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java index af4f0bb..34e5bea 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java
@@ -94,6 +94,14 @@ public static SCMRatisResponse decode(RaftClientReply reply) final SCMRatisResponseProto responseProto = SCMRatisResponseProto.parseFrom(response.toByteArray()); + // proto2 required-equivalent checks + if (!responseProto.hasType()) { + throw new InvalidProtocolBufferException("Missing response type"); + } + if (!responseProto.hasValue()) { + throw new InvalidProtocolBufferException("Missing response value"); + } + try { final Class<?> type = ReflectionUtil.getClass(responseProto.getType()); return new SCMRatisResponse(CodecFactory.getCodec(type)
diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java index 6602bd8..591fb17 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java
@@ -59,6 +59,12 @@ public Object deserialize(Class<?> type, ByteString value) final ListArgument listArgs = (ListArgument) ReflectionUtil .getMethod(ListArgument.class, "parseFrom", byte[].class) .invoke(null, (Object) value.toByteArray()); + + // proto2 required-equivalent check + if (!listArgs.hasType()) { + throw new InvalidProtocolBufferException("Missing ListArgument.type"); + } + final Class<?> dataType = ReflectionUtil.getClass(listArgs.getType()); for (ByteString element : listArgs.getValueList()) { result.add(CodecFactory.getCodec(dataType)
diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java new file mode 100644 index 0000000..ba6d7b2 --- /dev/null +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java
@@ -0,0 +1,98 @@ +/* + * 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.hadoop.hdds.scm.ha; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.protobuf.ByteString; +import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; +import org.apache.hadoop.hdds.protocol.proto.testing.Proto2SCMRatisProtocolForTesting; +import org.junit.jupiter.api.Test; + +/** + * Tests proto2 to proto3 compatibility for SCMRatisProtocol. + */ +public class TestSCMRatisProtocolCompatibility { + + /** + * Verifies that messages encoded with the legacy proto2 schema + * can be parsed by the current proto3 schema. + */ + @Test + public void testProto2RequestCanBeParsedByProto3() throws Exception { + // Build request using proto2 (test-only schema) + Proto2SCMRatisProtocolForTesting.MethodArgument arg = + Proto2SCMRatisProtocolForTesting.MethodArgument.newBuilder() + .setType("java.lang.String") + .setValue(ByteString.copyFromUtf8("v")) + .build(); + + Proto2SCMRatisProtocolForTesting.Method method = + Proto2SCMRatisProtocolForTesting.Method.newBuilder() + .setName("testOp") + .addArgs(arg) + .build(); + + Proto2SCMRatisProtocolForTesting.SCMRatisRequestProto proto2 = + Proto2SCMRatisProtocolForTesting.SCMRatisRequestProto.newBuilder() + .setType(Proto2SCMRatisProtocolForTesting.RequestType.PIPELINE) + .setMethod(method) + .build(); + + byte[] bytes = proto2.toByteArray(); + + // Parse using proto3 + SCMRatisProtocol.SCMRatisRequestProto proto3 = + SCMRatisProtocol.SCMRatisRequestProto.parseFrom(bytes); + + // Presence + value checks (proto3 optional fields) + assertTrue(proto3.hasType(), "proto3 should see type presence from proto2 bytes"); + assertTrue(proto3.hasMethod(), "proto3 should see method presence from proto2 bytes"); + assertEquals(SCMRatisProtocol.RequestType.PIPELINE, proto3.getType()); + assertEquals("testOp", proto3.getMethod().getName()); + assertEquals(1, proto3.getMethod().getArgsCount()); + assertEquals("java.lang.String", proto3.getMethod().getArgs(0).getType()); + assertEquals(ByteString.copyFromUtf8("v"), proto3.getMethod().getArgs(0).getValue()); + } + + /** + * Verifies that responses encoded with the legacy proto2 schema + * can be parsed by the current proto3 schema. + */ + @Test + public void testProto2ResponseCanBeParsedByProto3() throws Exception { + // Build response using proto2 + Proto2SCMRatisProtocolForTesting.SCMRatisResponseProto proto2 = + Proto2SCMRatisProtocolForTesting.SCMRatisResponseProto.newBuilder() + .setType("java.lang.String") + .setValue(ByteString.copyFromUtf8("ok")) + .build(); + + byte[] bytes = proto2.toByteArray(); + + // Parse using proto3 (production schema) + SCMRatisProtocol.SCMRatisResponseProto proto3 = + SCMRatisProtocol.SCMRatisResponseProto.parseFrom(bytes); + + assertTrue(proto3.hasType(), "proto3 should see type presence from proto2 bytes"); + assertTrue(proto3.hasValue(), "proto3 should see value presence from proto2 bytes"); + assertEquals("java.lang.String", proto3.getType()); + assertEquals(ByteString.copyFromUtf8("ok"), proto3.getValue()); + } +}
diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java index cca59c4..baf5d37 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java
@@ -18,13 +18,17 @@ package org.apache.hadoop.hdds.scm.ha; import static org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol.RequestType.PIPELINE; +import static org.apache.ratis.util.Preconditions.assertTrue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import com.google.protobuf.ByteString; import com.google.protobuf.InvalidProtocolBufferException; import java.util.ArrayList; import java.util.List; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; +import org.apache.hadoop.hdds.scm.ha.io.ListCodec; import org.apache.hadoop.hdds.scm.pipeline.PipelineID; import org.apache.ratis.protocol.Message; import org.junit.jupiter.api.Test; @@ -89,4 +93,139 @@ public void testEncodeAndDecodeOfLong() throws Exception { assertEquals(operation, SCMRatisRequest.decode(request.encode()).getOperation()); assertEquals(value, SCMRatisRequest.decode(request.encode()).getArguments()[0]); } + + @Test + public void testDecodeMissingRequestTypeShouldFail() throws Exception { + SCMRatisProtocol.SCMRatisRequestProto proto = + SCMRatisProtocol.SCMRatisRequestProto.newBuilder() + // no type + .setMethod(SCMRatisProtocol.Method.newBuilder() + .setName("test") + .build()) + .build(); + + Message msg = Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray())); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisRequest.decode(msg)); + + assertTrue(ex.getMessage().contains("Missing request type")); + } + + @Test + public void testDecodeMissingMethodShouldFail() throws Exception { + SCMRatisProtocol.SCMRatisRequestProto proto = + SCMRatisProtocol.SCMRatisRequestProto.newBuilder() + .setType(SCMRatisProtocol.RequestType.PIPELINE) + // no method + .build(); + + Message msg = Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray())); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisRequest.decode(msg)); + + assertTrue(ex.getMessage().contains("Missing method")); + } + + @Test + public void testDecodeMissingMethodNameShouldFail() throws Exception { + SCMRatisProtocol.SCMRatisRequestProto proto = + SCMRatisProtocol.SCMRatisRequestProto.newBuilder() + .setType(SCMRatisProtocol.RequestType.PIPELINE) + .setMethod(SCMRatisProtocol.Method.newBuilder() + // no name + .build()) + .build(); + + Message msg = Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray())); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisRequest.decode(msg)); + + assertTrue(ex.getMessage().contains("Missing method name")); + } + + @Test + public void testDecodeMissingArgumentTypeShouldFail() throws Exception { + SCMRatisProtocol.MethodArgument arg = + SCMRatisProtocol.MethodArgument.newBuilder() + // no type + .setValue(ByteString.copyFromUtf8("v")) + .build(); + + SCMRatisProtocol.SCMRatisRequestProto proto = + SCMRatisProtocol.SCMRatisRequestProto.newBuilder() + .setType(SCMRatisProtocol.RequestType.PIPELINE) + .setMethod(SCMRatisProtocol.Method.newBuilder() + .setName("test") + .addArgs(arg) + .build()) + .build(); + + Message msg = Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray())); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisRequest.decode(msg)); + + assertTrue(ex.getMessage().contains("Missing argument type")); + } + + @Test + public void testDecodeMissingArgumentValueShouldFail() throws Exception { + SCMRatisProtocol.MethodArgument arg = + SCMRatisProtocol.MethodArgument.newBuilder() + .setType("java.lang.String") + // no value + .build(); + + SCMRatisProtocol.SCMRatisRequestProto proto = + SCMRatisProtocol.SCMRatisRequestProto.newBuilder() + .setType(SCMRatisProtocol.RequestType.PIPELINE) + .setMethod(SCMRatisProtocol.Method.newBuilder() + .setName("test") + .addArgs(arg) + .build()) + .build(); + + Message msg = Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray())); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisRequest.decode(msg)); + + assertTrue(ex.getMessage().contains("Missing argument value")); + } + + @Test + public void testListDecodeMissingTypeShouldFail() throws Exception { + // ListArgument without type + SCMRatisProtocol.ListArgument listArg = + SCMRatisProtocol.ListArgument.newBuilder() + // no type + .addValue(ByteString.copyFromUtf8("x")) + .build(); + + ListCodec codec = new ListCodec(); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> codec.deserialize(List.class, listArg.toByteString())); + + assertTrue(ex.getMessage().contains("Missing ListArgument.type")); + } }
diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java index 7eaf75e..b0c67a1 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java
@@ -23,8 +23,12 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import com.google.protobuf.ByteString; import com.google.protobuf.InvalidProtocolBufferException; +import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; import org.apache.ratis.protocol.ClientId; import org.apache.ratis.protocol.Message; import org.apache.ratis.protocol.RaftClientReply; @@ -91,4 +95,48 @@ public void testEncodeFailureWithNonProto() { assertThrows(InvalidProtocolBufferException.class, () -> SCMRatisResponse.encode(message)); } + + @Test + public void testResponseDecodeMissingTypeShouldFail() throws Exception { + // Build response proto missing type + SCMRatisProtocol.SCMRatisResponseProto proto = + SCMRatisProtocol.SCMRatisResponseProto.newBuilder() + // no type + .setValue(ByteString.copyFromUtf8("v")) + .build(); + + RaftClientReply reply = mock(RaftClientReply.class); + when(reply.isSuccess()).thenReturn(true); + when(reply.getMessage()).thenReturn(Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray()))); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisResponse.decode(reply)); + + assertTrue(ex.getMessage().contains("Missing response type")); + } + + @Test + public void testResponseDecodeMissingValueShouldFail() throws Exception { + // Build response proto missing value + SCMRatisProtocol.SCMRatisResponseProto proto = + SCMRatisProtocol.SCMRatisResponseProto.newBuilder() + .setType("java.lang.String") + // no value + .build(); + + RaftClientReply reply = mock(RaftClientReply.class); + when(reply.isSuccess()).thenReturn(true); + when(reply.getMessage()).thenReturn(Message.valueOf( + org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom( + proto.toByteArray()))); + + InvalidProtocolBufferException ex = assertThrows( + InvalidProtocolBufferException.class, + () -> SCMRatisResponse.decode(reply)); + + assertTrue(ex.getMessage().contains("Missing response value")); + } }
diff --git a/hadoop-hdds/server-scm/src/test/proto/Proto2SCMRatisProtocolForTesting.proto b/hadoop-hdds/server-scm/src/test/proto/Proto2SCMRatisProtocolForTesting.proto new file mode 100644 index 0000000..06461dd --- /dev/null +++ b/hadoop-hdds/server-scm/src/test/proto/Proto2SCMRatisProtocolForTesting.proto
@@ -0,0 +1,59 @@ +/** + * 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. + */ +syntax = "proto2"; +option java_package = "org.apache.hadoop.hdds.protocol.proto.testing"; +option java_outer_classname = "Proto2SCMRatisProtocolForTesting"; +option java_generate_equals_and_hash = true; + +enum RequestType { + PIPELINE = 1; + CONTAINER = 2; + BLOCK = 3; + SEQUENCE_ID = 4; + CERT_STORE = 5; + MOVE = 6; + STATEFUL_SERVICE_CONFIG = 7; + FINALIZE = 8; + SECRET_KEY = 9; + CERT_ROTATE = 10; +} + +message Method { + required string name = 1; + repeated MethodArgument args = 2; +} + +message MethodArgument { + required string type = 1; + required bytes value = 2; +} + +message ListArgument { + required string type = 1; + repeated bytes value = 2; +} + +message SCMRatisRequestProto { + required RequestType type = 1; + required Method method = 2; +} + +message SCMRatisResponseProto { + required string type = 2; + required bytes value = 3; +}