diff --git a/src/main/java/com/oneops/inv/Inventory.java b/src/main/java/com/oneops/inv/Inventory.java index 2fbb48e..5312474 100644 --- a/src/main/java/com/oneops/inv/Inventory.java +++ b/src/main/java/com/oneops/inv/Inventory.java @@ -325,7 +325,9 @@ private void gatherInstanceMapsByComponentName(Operation operation, CiResource e // Ugly - have to parse the name of the component to get the instance number. String instanceNum = extractInstanceNum(componentInstance); - instancesByInstanceNum.put(instanceNum, componentInstance); + if (instanceNum != null) { + instancesByInstanceNum.put(instanceNum, componentInstance); + } } } catch(OneOpsClientAPIException e) { @@ -528,7 +530,9 @@ private void generateHostvars(JSONObject meta) throws InventoryException { for( CiResource host : allHosts ) { JSONObject hostObj = new JSONObject(); String publicIp = generateHostJson(host, hostObj); - hostvars.put( publicIp, hostObj); + if (publicIp != null) { + hostvars.put( publicIp, hostObj); + } } } @@ -565,16 +569,18 @@ private String generateHostJson(CiResource host, JSONObject hostObj) throws Inve * something else, but it's painful. We should look at this. */ String metadataStr = (String) ciAddlProps.get("metadata"); - JSONObject metadata = new JSONObject( metadataStr ); - - hostObj.put("oo_organization", metadata.get("organization")); - hostObj.put("oo_assembly", metadata.get("assembly")); - hostObj.put("oo_environment", metadata.get("environment")); - hostObj.put("oo_platform", metadata.get("platform")); - hostObj.put("oo_owner", metadata.get("owner")); - hostObj.put("oo_mgmt_url", metadata.get("mgmt_url")); - hostObj.put("oo_component", metadata.get("component")); - hostObj.put("oo_instance", metadata.get("instance")); + // GCP instances return raw GCP instance metadata here (not OneOps metadata), + // which won't have the expected OneOps keys. Use has()/get() to avoid JSONException. + JSONObject metadata = (metadataStr != null) ? new JSONObject(metadataStr) : new JSONObject(); + + hostObj.put("oo_organization", metadata.has("organization") ? metadata.get("organization") : JSONObject.NULL); + hostObj.put("oo_assembly", metadata.has("assembly") ? metadata.get("assembly") : JSONObject.NULL); + hostObj.put("oo_environment", metadata.has("environment") ? metadata.get("environment") : JSONObject.NULL); + hostObj.put("oo_platform", metadata.has("platform") ? metadata.get("platform") : JSONObject.NULL); + hostObj.put("oo_owner", metadata.has("owner") ? metadata.get("owner") : JSONObject.NULL); + hostObj.put("oo_mgmt_url", metadata.has("mgmt_url") ? metadata.get("mgmt_url") : JSONObject.NULL); + hostObj.put("oo_component", metadata.has("component") ? metadata.get("component") : JSONObject.NULL); + hostObj.put("oo_instance", metadata.has("instance") ? metadata.get("instance") : JSONObject.NULL); /** * END I HATE EVERYTHING ABOUT THIS */ @@ -584,7 +590,8 @@ private String generateHostJson(CiResource host, JSONObject hostObj) throws Inve hostObj.put("oo_component_name", computeComponent.get( host ).getCiName()); // Adds the name of the cloud for this compute - hostObj.put("oo_cloud", ((Map) host.getAdditionalProperties().get("deployedTo")).get("ciName")); + Map deployedTo = (Map) host.getAdditionalProperties().get("deployedTo"); + hostObj.put("oo_cloud", deployedTo != null ? deployedTo.get("ciName") : JSONObject.NULL); hostObj.put("oo_host_id", ciAddlProps.get("host_id")); hostObj.put("oo_hypervisor", ciAddlProps.get("hypervisor")); @@ -600,12 +607,14 @@ private String generateHostJson(CiResource host, JSONObject hostObj) throws Inve // Get the instance number for this computer - we need this to jump to sibling instances for // hostname and os String instanceNum = extractInstanceNum(host); - String environmentName = metadata.get("environment").toString(); - String platformName = metadata.get("platform").toString(); + // metadata may not contain OneOps keys for non-Azure clouds (e.g. GCP returns raw GCP metadata) + String environmentName = metadata.has("environment") ? metadata.get("environment").toString() : null; + String platformName = metadata.has("platform") ? metadata.get("platform").toString() : null; - String compositeCiNameHostname = environmentName + ":" + platformName + ":hostname"; + String compositeCiNameHostname = instanceNum != null && environmentName != null && platformName != null + ? environmentName + ":" + platformName + ":hostname" : null; // If there is a hostname component, grab the hostnames for this compute instance. - if( instanceMapsByComponentName.containsKey(compositeCiNameHostname)) { + if( compositeCiNameHostname != null && instanceMapsByComponentName.containsKey(compositeCiNameHostname)) { Map hostnamesByInstanceNum = instanceMapsByComponentName.get(compositeCiNameHostname); // Retrieve related hostname component (if there is one) @@ -621,9 +630,10 @@ private String generateHostJson(CiResource host, JSONObject hostObj) throws Inve } - String compositeCiNameOs = environmentName + ":" + platformName + ":os"; + String compositeCiNameOs = instanceNum != null && environmentName != null && platformName != null + ? environmentName + ":" + platformName + ":os" : null; // If there is an OS, grab the OS name and type for this compute instance. - if( instanceMapsByComponentName.containsKey(compositeCiNameOs)) { + if( compositeCiNameOs != null && instanceMapsByComponentName.containsKey(compositeCiNameOs)) { Map osByInstanceNum = instanceMapsByComponentName.get(compositeCiNameOs); // Retrieve related os component (if there is one) @@ -653,7 +663,14 @@ private String generateHostJson(CiResource host, JSONObject hostObj) throws Inve */ private String extractInstanceNum(CiResource ciResource) { String ciName = ciResource.getCiName(); - return ciName.substring( ciName.indexOf('-') ); + if (ciName == null) { + return null; // GCP assemblies may not populate ciName + } + int dashIdx = ciName.indexOf('-'); + if (dashIdx == -1) { + return null; // ciName present but has no dash-based instance numbering + } + return ciName.substring(dashIdx); } /** @@ -678,15 +695,15 @@ private String computeHostId(CiResource compute) throws InventoryException { // hostname and os String instanceNum = extractInstanceNum(compute); - String metadataStr = (String) ciAddlProps.get("metadata"); - JSONObject metadata = new JSONObject( metadataStr ); - String environment = metadata.get("environment").toString(); - String platform = metadata.get("platform").toString(); + JSONObject metadata = (metadataStr != null) ? new JSONObject(metadataStr) : new JSONObject(); + String environment = metadata.has("environment") ? metadata.get("environment").toString() : null; + String platform = metadata.has("platform") ? metadata.get("platform").toString() : null; - String compositeCiNameHostname = environment + ":" + platform + ":hostname"; + String compositeCiNameHostname = instanceNum != null && environment != null && platform != null + ? environment + ":" + platform + ":hostname" : null; // If there is a hostname component, grab the hostnames for this compute instance. - if( instanceMapsByComponentName.containsKey(compositeCiNameHostname)) { + if( compositeCiNameHostname != null && instanceMapsByComponentName.containsKey(compositeCiNameHostname)) { Map hostnamesByInstanceNum = instanceMapsByComponentName.get(compositeCiNameHostname); // Retrieve related hostname component (if there is one) diff --git a/src/test/java/com/oneops/inv/InventoryGcpFixTest.java b/src/test/java/com/oneops/inv/InventoryGcpFixTest.java new file mode 100644 index 0000000..ddf2830 --- /dev/null +++ b/src/test/java/com/oneops/inv/InventoryGcpFixTest.java @@ -0,0 +1,415 @@ +package com.oneops.inv; + +import com.oneops.api.resource.model.CiAttributes; +import com.oneops.api.resource.model.CiResource; +import org.json.JSONObject; +import org.junit.Before; +import org.junit.Test; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.*; + +/** + * Unit tests for GCP-related fixes in {@link Inventory}. + * + * Covers: + * 1. extractInstanceNum() with null ciName (GCP NPE fix) + * 2. extractInstanceNum() with no-dash ciName (robustness) + * 3. extractInstanceNum() happy path (Azure-style name) + * 4. generateList() with a GCP compute (null ciName + raw GCP metadata) — no NPE/JSONException + * 5. generateList() with null metadata string — no NPE + * 6. generateList() with Azure compute — output unchanged after fix + * 7. generateList() with null public_ip (computeset type) — skips entry, no NPE + */ +public class InventoryGcpFixTest { + + private Inventory inventory; + private Method extractInstanceNum; + + // ── helpers ────────────────────────────────────────────────────────────── + + /** Create a minimal Inventory instance (no live API calls needed). */ + private Inventory newInventory() { + return new Inventory("org", "assembly", "env", "token", + "http://localhost:9999/", "public_ip"); + } + + /** Reflectively invoke private extractInstanceNum(CiResource). */ + private String callExtractInstanceNum(CiResource ci) throws Exception { + return (String) extractInstanceNum.invoke(inventory, ci); + } + + /** Build a minimal CiResource with just a ciName set. */ + private CiResource ciWithName(String ciName) { + CiResource ci = new CiResource(); + ci.setCiName(ciName); + ci.setCiClassName("bom.openstack.Compute"); + ci.setNsPath("/org/assembly/env/platform/1"); + ci.setCiAttributes(new CiAttributes()); + return ci; + } + + /** + * Build a compute CiResource suitable for generateList(). + * @param ciName null for GCP, "compute-1234567-1" for Azure + * @param metadataStr null or JSON string for the metadata attribute + * @param publicIp null or IP string + */ + private CiResource computeCi(String ciName, String metadataStr, String publicIp) { + CiResource ci = new CiResource(); + ci.setCiName(ciName); + ci.setCiClassName("bom.openstack.Compute"); + ci.setNsPath("/org/assembly/env/platform/1"); + + CiAttributes attrs = new CiAttributes(); + if (publicIp != null) attrs.setAdditionalProperty("public_ip", publicIp); + if (metadataStr != null) attrs.setAdditionalProperty("metadata", metadataStr); + attrs.setAdditionalProperty("private_ip", "192.168.0.1"); + attrs.setAdditionalProperty("instance_name", "node-01"); + attrs.setAdditionalProperty("instance_id", "i-001"); + attrs.setAdditionalProperty("host_id", "h-001"); + attrs.setAdditionalProperty("hypervisor", "kvm"); + attrs.setAdditionalProperty("availability_zone", "zone-1"); + attrs.setAdditionalProperty("size", "n1-standard-2"); + attrs.setAdditionalProperty("cores", "2"); + attrs.setAdditionalProperty("ram", "8192"); + attrs.setAdditionalProperty("server_image_name", "centos-7"); + attrs.setAdditionalProperty("server_image_id", "img-001"); + attrs.setAdditionalProperty("vm_state", "running"); + ci.setCiAttributes(attrs); + + // deployedTo in outer additionalProperties + Map deployedTo = new HashMap<>(); + deployedTo.put("ciName", "my-cloud"); + ci.setAdditionalProperty("deployedTo", deployedTo); + + return ci; + } + + /** Inject a value into a private field of inventory via reflection. */ + @SuppressWarnings("unchecked") + private void setField(String fieldName, T value) throws Exception { + Field f = Inventory.class.getDeclaredField(fieldName); + f.setAccessible(true); + f.set(inventory, value); + } + + /** Read a private field from inventory via reflection. */ + @SuppressWarnings("unchecked") + private T getField(String fieldName) throws Exception { + Field f = Inventory.class.getDeclaredField(fieldName); + f.setAccessible(true); + return (T) f.get(inventory); + } + + /** + * Populate inventory's internal collections so generateList() can run + * without hitting the network. + */ + private void populateInventory(CiResource compute, CiResource transitionCompute, + CiResource environment, CiResource platform) throws Exception { + // allHosts + List allHosts = getField("allHosts"); + allHosts.add(compute); + + // hostsById — keyed by public_ip (or null; generateHostvars skips null after fix) + Map hostsById = getField("hostsById"); + String ip = (String) compute.getCiAttributes().getAdditionalProperties().get("public_ip"); + if (ip != null) hostsById.put(ip, compute); + + // computeComponent — maps ops compute -> transition compute + Map computeComponent = getField("computeComponent"); + computeComponent.put(compute, transitionCompute); + + // envByHostMap + Map envByHostMap = getField("envByHostMap"); + envByHostMap.put(compute, environment); + + // platformByHostMap + Map platformByHostMap = getField("platformByHostMap"); + platformByHostMap.put(compute, platform); + + // envHosts + Map> envHosts = getField("envHosts"); + envHosts.put(environment, new ArrayList<>(java.util.Collections.singletonList(compute))); + + // platformHosts + Map> platformHosts = getField("platformHosts"); + platformHosts.put(platform, new ArrayList<>(java.util.Collections.singletonList(compute))); + + // globalVarsMap / platformVarsMap — empty lists + Map> globalVarsMap = getField("globalVarsMap"); + globalVarsMap.put(environment, new ArrayList<>()); + Map> platformVarsMap = getField("platformVarsMap"); + platformVarsMap.put(platform, new ArrayList<>()); + + // envByPlatformMap + Map envByPlatformMap = getField("envByPlatformMap"); + envByPlatformMap.put(platform, environment); + } + + private CiResource minimalEnvironment() { + CiResource env = new CiResource(); + env.setCiName("stg"); + env.setCiClassName("manifest.Environment"); + env.setNsPath("/org/assembly"); + env.setCiAttributes(new CiAttributes()); + return env; + } + + private CiResource minimalPlatform() { + CiResource plat = new CiResource(); + plat.setCiName("app-platform"); + plat.setCiClassName("manifest.Platform"); + plat.setNsPath("/org/assembly/stg"); + plat.setCiAttributes(new CiAttributes()); + return plat; + } + + private CiResource minimalTransitionCompute() { + CiResource tc = new CiResource(); + tc.setCiName("compute"); + tc.setCiClassName("manifest.openstack.Compute"); + tc.setNsPath("/org/assembly/stg/app-platform"); + tc.setCiAttributes(new CiAttributes()); + return tc; + } + + // ── setup ───────────────────────────────────────────────────────────────── + + @Before + public void setUp() throws Exception { + inventory = newInventory(); + extractInstanceNum = Inventory.class.getDeclaredMethod("extractInstanceNum", CiResource.class); + extractInstanceNum.setAccessible(true); + } + + // ── extractInstanceNum tests ────────────────────────────────────────────── + + /** + * BUG 1 (BEFORE FIX): ciName == null caused NPE at Inventory.java:656. + * AFTER FIX: returns null gracefully. + */ + @Test + public void extractInstanceNum_nullCiName_returnsNull() throws Exception { + CiResource ci = ciWithName(null); // GCP: ciName not populated + String result = callExtractInstanceNum(ci); + assertNull("GCP null ciName must return null, not throw NPE", result); + } + + /** + * BUG 1 (robustness): ciName with no dash (e.g. GCP naming without hyphens) + * caused StringIndexOutOfBoundsException. AFTER FIX: returns null. + */ + @Test + public void extractInstanceNum_noDashInCiName_returnsNull() throws Exception { + CiResource ci = ciWithName("gcpinstance001"); // no dash + String result = callExtractInstanceNum(ci); + assertNull("ciName with no dash must return null, not throw StringIndexOutOfBoundsException", result); + } + + /** + * Happy path: Azure-style ciName "compute-1234567-1" returns the suffix. + */ + @Test + public void extractInstanceNum_azureStyle_returnsCorrectSuffix() throws Exception { + CiResource ci = ciWithName("compute-1234567-1"); + String result = callExtractInstanceNum(ci); + assertEquals("-1234567-1", result); + } + + /** + * Happy path: minimal single-dash name still works. + */ + @Test + public void extractInstanceNum_singleDash_returnsFromDash() throws Exception { + CiResource ci = ciWithName("compute-1"); + String result = callExtractInstanceNum(ci); + assertEquals("-1", result); + } + + // ── generateList() integration tests ───────────────────────────────────── + + /** + * BUG 1+2+3+6 (BEFORE FIX): GCP compute with null ciName and raw GCP metadata + * caused NPE (extractInstanceNum) OR JSONException (metadata.get("organization")). + * AFTER FIX: generateList() succeeds and returns valid JSON with zero hosts + * (null publicIp is skipped). + */ + @Test + public void generateList_gcpComputeNullCiNameRawMetadata_noException() throws Exception { + // GCP raw metadata — no "organization", "assembly" etc. + String gcpMetadata = "{\"kind\":\"compute#instance\",\"name\":\"gcp-node-01\"," + + "\"labels\":{\"owner\":\"devteam\"},\"status\":\"RUNNING\"}"; + + CiResource compute = computeCi(null, gcpMetadata, null); // null ciName, null public_ip + populateInventory(compute, minimalTransitionCompute(), minimalEnvironment(), minimalPlatform()); + + JSONObject result = inventory.generateList(); + + assertNotNull("generateList() must return non-null JSON", result); + assertTrue("Result must have _meta key", result.has("_meta")); + JSONObject hostvars = result.getJSONObject("_meta").getJSONObject("hostvars"); + // null publicIp must be skipped — 0 hosts in hostvars + assertEquals("GCP compute with null IP must be skipped in hostvars", 0, hostvars.length()); + } + + /** + * BUG 2 (BEFORE FIX): GCP compute with valid ciName but raw GCP metadata + * (missing "organization" key) caused JSONException. + * AFTER FIX: generateList() succeeds; oo_organization is null in output. + */ + @Test + public void generateList_gcpComputeRawMetadataMissingOoKeys_noJsonException() throws Exception { + String gcpMetadata = "{\"kind\":\"compute#instance\",\"name\":\"gcp-node-01\"}"; + CiResource compute = computeCi("compute-2828046019-1", gcpMetadata, "10.35.185.16"); + populateInventory(compute, minimalTransitionCompute(), minimalEnvironment(), minimalPlatform()); + + JSONObject result = inventory.generateList(); + + assertNotNull(result); + JSONObject hostvars = result.getJSONObject("_meta").getJSONObject("hostvars"); + assertTrue("Host 10.35.185.16 must be present", hostvars.has("10.35.185.16")); + + JSONObject hostVars = hostvars.getJSONObject("10.35.185.16"); + // oo_organization should be null (JSONObject.NULL), not throw + assertTrue("oo_organization key must exist", hostVars.has("oo_organization")); + assertEquals("oo_organization must be null for GCP raw metadata", + JSONObject.NULL, hostVars.get("oo_organization")); + } + + /** + * BUG 5 (BEFORE FIX): null metadata string caused NPE in new JSONObject(null). + * AFTER FIX: treated as empty metadata, all oo_* keys set to null. + */ + @Test + public void generateList_nullMetadataString_noException() throws Exception { + CiResource compute = computeCi("compute-1234567-1", null, "10.0.0.1"); // no metadata at all + populateInventory(compute, minimalTransitionCompute(), minimalEnvironment(), minimalPlatform()); + + JSONObject result = inventory.generateList(); + + assertNotNull(result); + JSONObject hostvars = result.getJSONObject("_meta").getJSONObject("hostvars"); + assertTrue(hostvars.has("10.0.0.1")); + } + + /** + * Azure compute with correct OneOps metadata must still work correctly after the fix + * — verify oo_organization, oo_assembly etc. are populated from metadata. + */ + @Test + public void generateList_azureComputeCorrectMetadata_ooFieldsPopulated() throws Exception { + String azureMetadata = "{\"organization\":\"devtools\",\"assembly\":\"my-assembly\"," + + "\"environment\":\"stg\",\"platform\":\"app-platform\"," + + "\"owner\":\"devteam\",\"mgmt_url\":\"https://oneops.example.com\"," + + "\"component\":\"compute\",\"instance\":\"1\"}"; + + CiResource compute = computeCi("compute-1234567-1", azureMetadata, "10.122.248.174"); + populateInventory(compute, minimalTransitionCompute(), minimalEnvironment(), minimalPlatform()); + + JSONObject result = inventory.generateList(); + + assertNotNull(result); + JSONObject hostVars = result.getJSONObject("_meta") + .getJSONObject("hostvars") + .getJSONObject("10.122.248.174"); + + assertEquals("devtools", hostVars.get("oo_organization")); + assertEquals("my-assembly", hostVars.get("oo_assembly")); + assertEquals("stg", hostVars.get("oo_environment")); + assertEquals("app-platform", hostVars.get("oo_platform")); + assertEquals("devteam", hostVars.get("oo_owner")); + } + + /** + * BUG 6 (BEFORE FIX): compute with null public_ip caused JSONObject.put(null, hostObj) → NPE. + * AFTER FIX: entry is silently skipped, rest of JSON is valid. + */ + @Test + public void generateList_nullPublicIp_skipsEntryNoException() throws Exception { + String meta = "{\"organization\":\"devtools\",\"assembly\":\"a\",\"environment\":\"e\"," + + "\"platform\":\"p\",\"owner\":\"o\",\"mgmt_url\":\"u\",\"component\":\"c\",\"instance\":\"1\"}"; + CiResource compute = computeCi("compute-1234567-1", meta, null); // null public_ip + populateInventory(compute, minimalTransitionCompute(), minimalEnvironment(), minimalPlatform()); + + JSONObject result = inventory.generateList(); + + assertNotNull(result); + JSONObject hostvars = result.getJSONObject("_meta").getJSONObject("hostvars"); + assertEquals("Null-IP compute must be skipped from hostvars", 0, hostvars.length()); + } + + /** + * Mixed: two computes — one Azure (valid), one GCP (null ciName, raw metadata, valid IP). + * Both bugs fixed: Azure host is present, GCP host is present (IP exists), no exceptions. + */ + @Test + public void generateList_mixedAzureAndGcpComputes_bothHandledGracefully() throws Exception { + String azureMeta = "{\"organization\":\"devtools\",\"assembly\":\"a\",\"environment\":\"e\"," + + "\"platform\":\"p\",\"owner\":\"o\",\"mgmt_url\":\"u\",\"component\":\"c\",\"instance\":\"1\"}"; + String gcpMeta = "{\"kind\":\"compute#instance\",\"name\":\"gcp-node-01\"}"; + + CiResource azureCompute = computeCi("compute-1234567-1", azureMeta, "10.0.0.1"); + CiResource gcpCompute = computeCi(null, gcpMeta, "10.35.185.16"); + + CiResource env = minimalEnvironment(); + CiResource plat = minimalPlatform(); + CiResource tc = minimalTransitionCompute(); + + // populate both + List allHosts = getField("allHosts"); + allHosts.add(azureCompute); + allHosts.add(gcpCompute); + + Map hostsById = getField("hostsById"); + hostsById.put("10.0.0.1", azureCompute); + hostsById.put("10.35.185.16", gcpCompute); + + Map computeComponent = getField("computeComponent"); + computeComponent.put(azureCompute, tc); + computeComponent.put(gcpCompute, tc); + + Map envByHostMap = getField("envByHostMap"); + envByHostMap.put(azureCompute, env); + envByHostMap.put(gcpCompute, env); + + Map platformByHostMap = getField("platformByHostMap"); + platformByHostMap.put(azureCompute, plat); + platformByHostMap.put(gcpCompute, plat); + + List hostList = new ArrayList<>(); + hostList.add(azureCompute); + hostList.add(gcpCompute); + + Map> envHosts = getField("envHosts"); + envHosts.put(env, hostList); + Map> platformHosts = getField("platformHosts"); + platformHosts.put(plat, hostList); + Map> globalVarsMap = getField("globalVarsMap"); + globalVarsMap.put(env, new ArrayList<>()); + Map> platformVarsMap = getField("platformVarsMap"); + platformVarsMap.put(plat, new ArrayList<>()); + Map envByPlatformMap = getField("envByPlatformMap"); + envByPlatformMap.put(plat, env); + + JSONObject result = inventory.generateList(); + + assertNotNull(result); + JSONObject hostvars = result.getJSONObject("_meta").getJSONObject("hostvars"); + + assertTrue("Azure host must be in hostvars", hostvars.has("10.0.0.1")); + assertTrue("GCP host must be in hostvars", hostvars.has("10.35.185.16")); + + // Azure: oo_organization populated + assertEquals("devtools", hostvars.getJSONObject("10.0.0.1").get("oo_organization")); + // GCP: oo_organization is null (raw GCP metadata has no OneOps keys) + assertEquals(JSONObject.NULL, hostvars.getJSONObject("10.35.185.16").get("oo_organization")); + } +}