KYLIN-1502 When cube is not empty, only signature consistent cube desc updates are allowed
Project: http://git-wip-us.apache.org/repos/asf/kylin/repo Commit: http://git-wip-us.apache.org/repos/asf/kylin/commit/f5030b64 Tree: http://git-wip-us.apache.org/repos/asf/kylin/tree/f5030b64 Diff: http://git-wip-us.apache.org/repos/asf/kylin/diff/f5030b64 Branch: refs/heads/master Commit: f5030b6426708ebef687787bd21b2dce9795af50 Parents: 98ac225 Author: Hongbin Ma <[email protected]> Authored: Wed Mar 16 23:31:43 2016 +0800 Committer: Hongbin Ma <[email protected]> Committed: Wed Mar 16 23:42:24 2016 +0800 ---------------------------------------------------------------------- .../org/apache/kylin/cube/CubeDescManager.java | 2 +- .../org/apache/kylin/cube/model/CubeDesc.java | 13 ++++ .../kylin/rest/controller/CubeController.java | 69 ++++++++++---------- .../apache/kylin/rest/service/CubeService.java | 33 ++++------ .../kylin/rest/service/CubeServiceTest.java | 2 +- 5 files changed, 63 insertions(+), 56 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kylin/blob/f5030b64/core-cube/src/main/java/org/apache/kylin/cube/CubeDescManager.java ---------------------------------------------------------------------- diff --git a/core-cube/src/main/java/org/apache/kylin/cube/CubeDescManager.java b/core-cube/src/main/java/org/apache/kylin/cube/CubeDescManager.java index 56667b6..33a6830 100644 --- a/core-cube/src/main/java/org/apache/kylin/cube/CubeDescManager.java +++ b/core-cube/src/main/java/org/apache/kylin/cube/CubeDescManager.java @@ -58,7 +58,7 @@ public class CubeDescManager { if (r != null) { return r; } - + synchronized (CubeDescManager.class) { r = CACHE.get(config); if (r != null) { http://git-wip-us.apache.org/repos/asf/kylin/blob/f5030b64/core-cube/src/main/java/org/apache/kylin/cube/model/CubeDesc.java ---------------------------------------------------------------------- diff --git a/core-cube/src/main/java/org/apache/kylin/cube/model/CubeDesc.java b/core-cube/src/main/java/org/apache/kylin/cube/model/CubeDesc.java index 704af60..4a019ca 100644 --- a/core-cube/src/main/java/org/apache/kylin/cube/model/CubeDesc.java +++ b/core-cube/src/main/java/org/apache/kylin/cube/model/CubeDesc.java @@ -438,6 +438,13 @@ public class CubeDesc extends RootPersistentEntity { return "CubeDesc [name=" + name + "]"; } + /** + * this method is to prevent malicious metadata change by checking the saved signature + * with the calculated signature. + * + * if you're comparing two cube desc prefer to use consistentWith() + * @return + */ public boolean checkSignature() { if (KylinVersion.isCompatibleWith(getVersion()) && !KylinVersion.isSignatureCompatibleWith(getVersion())) { logger.info("checkSignature on {} is skipped as the its version is {} (not signature compatible but compatible) ", getName(), getVersion()); @@ -453,6 +460,12 @@ public class CubeDesc extends RootPersistentEntity { return calculated.equals(saved); } + public boolean consistentWith(CubeDesc another) { + if (another == null) + return false; + return this.calculateSignature().equals(another.calculateSignature()); + } + public String calculateSignature() { MessageDigest md; try { http://git-wip-us.apache.org/repos/asf/kylin/blob/f5030b64/server/src/main/java/org/apache/kylin/rest/controller/CubeController.java ---------------------------------------------------------------------- diff --git a/server/src/main/java/org/apache/kylin/rest/controller/CubeController.java b/server/src/main/java/org/apache/kylin/rest/controller/CubeController.java index bc00795..410b2bd 100644 --- a/server/src/main/java/org/apache/kylin/rest/controller/CubeController.java +++ b/server/src/main/java/org/apache/kylin/rest/controller/CubeController.java @@ -18,7 +18,7 @@ package org.apache.kylin.rest.controller; -import java.io.*; +import java.io.IOException; import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Iterator; @@ -92,13 +92,13 @@ public class CubeController extends BasicController { @Autowired private JobService jobService; - @RequestMapping(value = "", method = {RequestMethod.GET}) + @RequestMapping(value = "", method = { RequestMethod.GET }) @ResponseBody public List<CubeInstance> getCubes(@RequestParam(value = "cubeName", required = false) String cubeName, @RequestParam(value = "modelName", required = false) String modelName, @RequestParam(value = "projectName", required = false) String projectName, @RequestParam(value = "limit", required = false) Integer limit, @RequestParam(value = "offset", required = false) Integer offset) { return cubeService.getCubes(cubeName, projectName, modelName, limit, offset); } - @RequestMapping(value = "/{cubeName}", method = {RequestMethod.GET}) + @RequestMapping(value = "/{cubeName}", method = { RequestMethod.GET }) @ResponseBody public CubeInstance getCube(@PathVariable String cubeName) { CubeInstance cube = cubeService.getCubeManager().getCube(cubeName); @@ -108,7 +108,6 @@ public class CubeController extends BasicController { return cube; } - /** * Get hive SQL of the cube * @@ -117,7 +116,7 @@ public class CubeController extends BasicController { * @throws UnknownHostException * @throws IOException */ - @RequestMapping(value = "/{cubeName}/segs/{segmentName}/sql", method = {RequestMethod.GET}) + @RequestMapping(value = "/{cubeName}/segs/{segmentName}/sql", method = { RequestMethod.GET }) @ResponseBody public GeneralResponse getSql(@PathVariable String cubeName, @PathVariable String segmentName) { CubeInstance cube = cubeService.getCubeManager().getCube(cubeName); @@ -139,7 +138,7 @@ public class CubeController extends BasicController { * @param notifyList * @throws IOException */ - @RequestMapping(value = "/{cubeName}/notify_list", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/notify_list", method = { RequestMethod.PUT }) @ResponseBody public void updateNotifyList(@PathVariable String cubeName, @RequestBody List<String> notifyList) { CubeInstance cube = cubeService.getCubeManager().getCube(cubeName); @@ -157,7 +156,7 @@ public class CubeController extends BasicController { } - @RequestMapping(value = "/{cubeName}/cost", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/cost", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance updateCubeCost(@PathVariable String cubeName, @RequestParam(value = "cost") int cost) { try { @@ -169,7 +168,7 @@ public class CubeController extends BasicController { } } - @RequestMapping(value = "/{cubeName}/coprocessor", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/coprocessor", method = { RequestMethod.PUT }) @ResponseBody public Map<String, Boolean> updateCubeCoprocessor(@PathVariable String cubeName, @RequestParam(value = "force") String force) { try { @@ -187,7 +186,7 @@ public class CubeController extends BasicController { * * @throws IOException */ - @RequestMapping(value = "/{cubeName}/segs/{segmentName}/refresh_lookup", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/segs/{segmentName}/refresh_lookup", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance rebuildLookupSnapshot(@PathVariable String cubeName, @PathVariable String segmentName, @RequestParam(value = "lookupTable") String lookupTable) { try { @@ -205,7 +204,7 @@ public class CubeController extends BasicController { * @return * @throws IOException */ - @RequestMapping(value = "/{cubeName}/rebuild", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/rebuild", method = { RequestMethod.PUT }) @ResponseBody public JobInstance rebuild(@PathVariable String cubeName, @RequestBody JobBuildRequest jobBuildRequest) { try { @@ -222,7 +221,7 @@ public class CubeController extends BasicController { } } - @RequestMapping(value = "/{cubeName}/disable", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/disable", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance disableCube(@PathVariable String cubeName) { try { @@ -240,7 +239,7 @@ public class CubeController extends BasicController { } } - @RequestMapping(value = "/{cubeName}/purge", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/purge", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance purgeCube(@PathVariable String cubeName) { try { @@ -258,8 +257,7 @@ public class CubeController extends BasicController { } } - - @RequestMapping(value = "/{cubeName}/clone", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/clone", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance cloneCube(@PathVariable String cubeName, @RequestBody CubeRequest cubeRequest) { String newCubeName = cubeRequest.getCubeName(); @@ -271,10 +269,9 @@ public class CubeController extends BasicController { } CubeDesc cubeDesc = cube.getDescriptor(); CubeDesc newCubeDesc = CubeDesc.getCopyOf(cubeDesc); - newCubeDesc.setName(newCubeName); - CubeInstance newCube = null; + CubeInstance newCube; try { newCube = cubeService.createCubeAndDesc(newCubeName, project, newCubeDesc); @@ -288,8 +285,7 @@ public class CubeController extends BasicController { } - - @RequestMapping(value = "/{cubeName}/enable", method = {RequestMethod.PUT}) + @RequestMapping(value = "/{cubeName}/enable", method = { RequestMethod.PUT }) @ResponseBody public CubeInstance enableCube(@PathVariable String cubeName) { try { @@ -306,7 +302,7 @@ public class CubeController extends BasicController { } } - @RequestMapping(value = "/{cubeName}", method = {RequestMethod.DELETE}) + @RequestMapping(value = "/{cubeName}", method = { RequestMethod.DELETE }) @ResponseBody public void deleteCube(@PathVariable String cubeName) { CubeInstance cube = cubeService.getCubeManager().getCube(cubeName); @@ -330,7 +326,7 @@ public class CubeController extends BasicController { * @return Table metadata array * @throws IOException */ - @RequestMapping(value = "", method = {RequestMethod.POST}) + @RequestMapping(value = "", method = { RequestMethod.POST }) @ResponseBody public CubeRequest saveCubeDesc(@RequestBody CubeRequest cubeRequest) { @@ -368,37 +364,40 @@ public class CubeController extends BasicController { * @throws JsonProcessingException * @throws IOException */ - @RequestMapping(value = "", method = {RequestMethod.PUT}) + @RequestMapping(value = "", method = { RequestMethod.PUT }) @ResponseBody public CubeRequest updateCubeDesc(@RequestBody CubeRequest cubeRequest) throws JsonProcessingException { //update cube CubeDesc desc = deserializeCubeDesc(cubeRequest); - CubeDesc oldCubeDesc = null; + CubeDesc oldCubeDesc; + boolean isCubeDescFreeEditable; if (desc == null) { return cubeRequest; } // Check if the cube is editable - if (!cubeService.isCubeDescEditable(desc)) { - String error = "Purge the related cube before editing its desc. Desc name: " + desc.getName(); - updateRequest(cubeRequest, false, error); - return cubeRequest; - } + isCubeDescFreeEditable = cubeService.isCubeDescFreeEditable(desc); - //cube renaming: + //cube renaming is not allowed if (!cubeRequest.getCubeName().equalsIgnoreCase(CubeService.getCubeNameFromDesc(desc.getName()))) { - deleteCube(cubeRequest.getCubeName()); - saveCubeDesc(cubeRequest); + String error = "Cube Desc renaming is not allowed: " + desc.getName(); + updateRequest(cubeRequest, false, error); + return cubeRequest; } String projectName = (null == cubeRequest.getProject()) ? ProjectInstance.DEFAULT_PROJECT_NAME : cubeRequest.getProject(); try { CubeInstance cube = cubeService.getCubeManager().getCube(cubeRequest.getCubeName()); oldCubeDesc = cube.getDescriptor(); - desc = cubeService.updateCubeAndDesc(cube, desc, projectName); - + if (isCubeDescFreeEditable || oldCubeDesc.consistentWith(desc)) { + desc = cubeService.updateCubeAndDesc(cube, desc, projectName); + } else { + logger.warn("Won't update the cube desc due to inconsistency"); + updateRequest(cubeRequest, false, "CubeDesc " + desc.getName() + " is inconsistent with existing. Try purge that cube first or avoid updating key cube desc fields."); + return cubeRequest; + } } catch (AccessDeniedException accessDeniedException) { throw new ForbiddenException("You don't have right to update this cube."); } catch (Exception e) { @@ -424,7 +423,7 @@ public class CubeController extends BasicController { * @return true * @throws IOException */ - @RequestMapping(value = "/{cubeName}/hbase", method = {RequestMethod.GET}) + @RequestMapping(value = "/{cubeName}/hbase", method = { RequestMethod.GET }) @ResponseBody public List<HBaseResponse> getHBaseInfo(@PathVariable String cubeName) { List<HBaseResponse> hbase = new ArrayList<HBaseResponse>(); @@ -461,7 +460,7 @@ public class CubeController extends BasicController { return hbase; } - @RequestMapping(value = "/{cubeName}/segments", method = {RequestMethod.POST}) + @RequestMapping(value = "/{cubeName}/segments", method = { RequestMethod.POST }) @ResponseBody public CubeSegmentRequest appendSegment(@PathVariable String cubeName, @RequestBody CubeSegmentRequest cubeSegmentRequest) { CubeInstance cube = cubeService.getCubeManager().getCube(cubeName); @@ -567,7 +566,7 @@ public class CubeController extends BasicController { */ private String omitMessage(List<String> errors) { StringBuffer buffer = new StringBuffer(); - for (Iterator<String> iterator = errors.iterator(); iterator.hasNext(); ) { + for (Iterator<String> iterator = errors.iterator(); iterator.hasNext();) { String string = (String) iterator.next(); buffer.append(string); buffer.append("\n"); http://git-wip-us.apache.org/repos/asf/kylin/blob/f5030b64/server/src/main/java/org/apache/kylin/rest/service/CubeService.java ---------------------------------------------------------------------- diff --git a/server/src/main/java/org/apache/kylin/rest/service/CubeService.java b/server/src/main/java/org/apache/kylin/rest/service/CubeService.java index 0c57d00..6ef796c 100644 --- a/server/src/main/java/org/apache/kylin/rest/service/CubeService.java +++ b/server/src/main/java/org/apache/kylin/rest/service/CubeService.java @@ -171,27 +171,23 @@ public class CubeService extends BasicService { throw new InternalErrorException("The cube named " + cubeName + " already exists"); } + if (getCubeDescManager().getCubeDesc(desc.getName()) != null) { + throw new InternalErrorException("The cube desc named " + desc.getName() + " already exists"); + } + String owner = SecurityContextHolder.getContext().getAuthentication().getName(); - CubeDesc createdDesc = null; - CubeInstance createdCube = null; + CubeDesc createdDesc; + CubeInstance createdCube; - boolean isNew = false; - if (getCubeDescManager().getCubeDesc(desc.getName()) == null) { - createdDesc = getCubeDescManager().createCubeDesc(desc); - isNew = true; - } else { - createdDesc = getCubeDescManager().updateCubeDesc(desc); - } + createdDesc = getCubeDescManager().createCubeDesc(desc); if (!createdDesc.getError().isEmpty()) { - if (isNew) { - getCubeDescManager().removeCubeDesc(createdDesc); - } + getCubeDescManager().removeCubeDesc(createdDesc); throw new InternalErrorException(createdDesc.getError().get(0)); } try { - int cuboidCount = CuboidCLI.simulateCuboidGeneration(createdDesc,false); + int cuboidCount = CuboidCLI.simulateCuboidGeneration(createdDesc, false); logger.info("New cube " + cubeName + " has " + cuboidCount + " cuboids"); } catch (Exception e) { getCubeDescManager().removeCubeDesc(createdDesc); @@ -256,13 +252,12 @@ public class CubeService extends BasicService { } try { - if (!cube.getDescriptor().checkSignature()) { - logger.info("Releasing all segments due to cube desc conflict"); - this.releaseAllSegments(cube); + if (!cube.getDescriptor().consistentWith(desc)) { + throw new IllegalStateException("cube's desc is not consistent with the new desc"); } CubeDesc updatedCubeDesc = getCubeDescManager().updateCubeDesc(desc); - int cuboidCount = CuboidCLI.simulateCuboidGeneration(updatedCubeDesc,false); + int cuboidCount = CuboidCLI.simulateCuboidGeneration(updatedCubeDesc, false); logger.info("Updated cube " + cube.getName() + " has " + cuboidCount + " cuboids"); ProjectManager projectManager = getProjectManager(); @@ -290,7 +285,7 @@ public class CubeService extends BasicService { accessService.clean(cube, true); } - public boolean isCubeDescEditable(CubeDesc cd) { + public boolean isCubeDescFreeEditable(CubeDesc cd) { List<CubeInstance> cubes = getCubeManager().getCubesByDesc(cd.getName()); for (CubeInstance cube : cubes) { if (cube.getSegments().size() != 0) { @@ -300,6 +295,7 @@ public class CubeService extends BasicService { } return true; } + public static String getCubeDescNameFromCube(String cubeName) { return cubeName + DESC_SUFFIX; } @@ -312,7 +308,6 @@ public class CubeService extends BasicService { } } - /** * Stop all jobs belonging to this cube and clean out all segments * http://git-wip-us.apache.org/repos/asf/kylin/blob/f5030b64/server/src/test/java/org/apache/kylin/rest/service/CubeServiceTest.java ---------------------------------------------------------------------- diff --git a/server/src/test/java/org/apache/kylin/rest/service/CubeServiceTest.java b/server/src/test/java/org/apache/kylin/rest/service/CubeServiceTest.java index d86935e..f93c04f 100644 --- a/server/src/test/java/org/apache/kylin/rest/service/CubeServiceTest.java +++ b/server/src/test/java/org/apache/kylin/rest/service/CubeServiceTest.java @@ -51,7 +51,7 @@ public class CubeServiceTest extends ServiceTestBase { List<CubeInstance> cubes = cubeService.getCubes(null, null, null, null, null); Assert.assertNotNull(cubes); CubeInstance cube = cubes.get(0); - cubeService.isCubeDescEditable(cube.getDescriptor()); + cubeService.isCubeDescFreeEditable(cube.getDescriptor()); cubes = cubeService.getCubes(null, null, null, 1, 0); Assert.assertTrue(cubes.size() == 1);
