[Fleet] Fix saved object import missing reference

[nchaulet]

Summary

Resolve #127664

We had some special case to retry when we have missing reference in kibana assets but we were considering that other assets were correctly imported, according to the doc

Objects are only created when all resolvable errors are addressed, including conflicts and missing references. For information on how to resolve errors, refer to the examples.

That PR fix by including all the saved object in the retry call for missing reference.

How to test?

You can try to install the AWS integration in the 1.13.0 version before that fix after install you should see only one dashboard in the assets tab of the integration after you should see 24.

[elasticmachine]

Pinging @elastic/fleet (Team:Fleet)

[nchaulet]

@elasticmachine merge upstream

[jen-huang]

@nchaulet Do you know if this might affect 7.17 too?

[nchaulet]

@nchaulet Do you know if this might affect 7.17 too?

I need to test, but I guess it's probably good to backport it there too.

the index template metricbeat-* was existing in 7.x and removed in 8.0 so for the AWS package this will not be a problem, but a package could still be malformed at some point and we get the same issue.

[nchaulet]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 7494db2
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

✅ unchanged

History

• 💔 Build #30510 failed bccbadf
• 💔 Build #30485 failed 741ae4a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @nchaulet

[joshdover]

@nchaulet Do you know if this might affect 7.17 too?

I need to test, but I guess it's probably good to backport it there too.

the index template metricbeat-* was existing in 7.x and removed in 8.0 so for the AWS package this will not be a problem, but a package could still be malformed at some point and we get the same issue.

We didn't use the import API in 7.17, we made that switch in 8.0 so I don't believe this would impact the 7.x series. @hop-dev does that match your understanding as well?

[joshdover]

A couple questions before this PR is merged:

• Why did the package install not fail completely when a missing package reference is found?
• Should we really allow missing references at all? IMO this sounds like a broken package and Kibana should fail to install it. That said, we should probably have some validation in the package-spec for this to help avoid this situation.

[nchaulet]

@joshdover

Why did the package install not fail completely when a missing package reference is found?
Should we really allow missing references at all? IMO this sounds like a broken package and Kibana should fail to install it. That said, we should probably have some validation in the package-spec for this to help avoid this situation.

We had a mechanism to retry with allow missing reference, I agree that we could probably simplify that by removing the retry on missing reference and throw like we do for other errors. This will be a breaking change as it will result in existing package the AWS 1.13.0 one to not be installable.

[joshdover]

We had a mechanism to retry with allow missing reference, I agree that we could probably simplify that by removing the retry on missing reference and throw like we do for other errors. This will be a breaking change as it will result in existing package the AWS 1.13.0 one to not be installable.

The AWS package is being fixed here: elastic/integrations#2826 so we may be able to move towards failing in the way you describe.

My concern is that there may be other packages that are broken that we don't know about. Should we have a CI test that attempts to install the latest version of every package in the repository? It might be useful to throw up a draft PR now that changes the logic to fail on missing references and adds this test to see if there others we should fix.

[nchaulet]

My concern is that there may be other packages that are broken that we don't know about. Should we have a CI test that attempts to install the latest version of every package in the repository? It might be useful to throw up a draft PR now that changes the logic to fail on missing references and adds this test to see if there others we should fix.

Yes it will be usefull, I can create that PR

[hop-dev]

Regarding allowing reference errors, pre 8.x we didn't check for (or throw) reference errors, so when we moved to import I decided to keep the behaviour the same to prevent any issues installing historic packages etc.

Taking a harder stance on the reference errors is appealing but in the AWS example I believe the dashboards do still work (if they are installed 😓 ) regardless of the error, so not broken to the user.

[joshdover]

Regarding allowing reference errors, pre 8.x we didn't check for (or throw) reference errors, so when we moved to import I decided to keep the behaviour the same to prevent any issues installing historic packages etc.

Taking a harder stance on the reference errors is appealing but in the AWS example I believe the dashboards do still work (if they are installed 😓 ) regardless of the error, so not broken to the user.

Thanks for that info, Mark, that makes sense. In that case, I'm comfortable moving forward with merging this and investigating the overall problem separately as we make packages more rock solid.

@nchaulet I'm going to merge this now so we can make the next 8.1.1 BC (there was a blocker so there should be another build). Would you mind opening an issue to investigate this problem more broadly?

[hop-dev]

I do have a hacked together script for installing the latest versions of each package from when I was developing this if its any use

install_all_packages.js

import fetch from "node-fetch";

const SKIP_PACKAGES = ["kibana", "logstash", "microsoft", "fleet_server", "elastic_agent"]
async function installPackage(name, version) {
    const start = Date.now()
    const res = await fetch(`http://localhost:5601/mark/api/fleet/epm/packages/${name}-${version}`, {
        "headers": {
            "accept": "*/*",
            "content-type": "application/json",
            "kbn-xsrf": "xyz",
            'Authorization': 'Basic ' + Buffer.from("elastic:changeme").toString('base64')
        },
        "body": null,
        "method": "POST"
    });
    const end = Date.now()

    const body = await res.json()

    return { body, status: res.status, took : (end - start) / 1000 };
}

async function deletePackage(name, version) {
    const res = await fetch(`http://localhost:5601/mark/api/fleet/epm/packages/${name}-${version}`, {
        "headers": {
            "accept": "*/*",
            "content-type": "application/json",
            "kbn-xsrf": "xyz",
            'Authorization': 'Basic ' + Buffer.from("elastic:changeme").toString('base64')
        },
        "body": null,
        "method": "DELETE"
    });

    const body = await res.json()

    return { body, status: res.status };
}

async function getAllPackages() {
const res  = await fetch("https://epr-snapshot.elastic.co/search", {
  "headers": {
    "accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9",
  },
  "body": null,
  "method": "GET"
});
const body = await res.json()
    return body;
}

function logResult(pkg, result){
    const pre = `${pkg.name}-${pkg.version} ${result.took ? ` took ${result.took}s` : ""} : `;
    if (result.status !== 200) {
        console.log(pre + JSON.stringify(result))
    } else {
        console.log(pre + 200)
    }
}

async function go() {
    const allPackages = await getAllPackages()

    console.log("INSTALLING ------");

    for (const pkg of allPackages) {
        if (SKIP_PACKAGES.includes(pkg.name)) {
            console.log(`Skipping ${pkg.name}`)
            continue;
        }
        const result = await installPackage(pkg.name, pkg.version);

        logResult(pkg,result)
    }

    const deletePackages = process.argv.includes("--delete")

    if(!deletePackages) return;
    
    console.log("DELETING ------");
    for (const pkg of allPackages) {
        if (SKIP_PACKAGES.includes(pkg.name)) {
            console.log(`Skipping ${pkg.name}`)
            continue;
        }
        const result = await deletePackage(pkg.name, pkg.version);

        logResult(pkg,result)
    }
}

go()

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation