icinga2: address comment loading where host reference is not found

[ymartin-ovh]

address #9752: check if host reference is valid

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @ymartin-ovh

Details

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @ymartin-ovh

Details

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[bobapple]

@cla-bot check

[julianbrost]

@Al2Klimov Please explain what #7786 has to do with this/what exactly this is supposed to fix here.

Anyways, Comment::OnAllConfigLoaded() even states in the validation error it can throw that it's supposed to handle non-existent hosts ("Comment '...' references a host/service which doesn't exist."), but apparently has a bug there, so that null check there should definitely by done.

[ymartin-ovh]

Hello @julianbrost / @Al2Klimov

Should I rebase my diff with just null check in comment.cpp ?

Regards

[Al2Klimov]

a27c6cf is exactly what I wanted, but as a PR on its own into #7786. I.e. your new PR's spec:

• Base branch: bugfix/api-runtime-object-sync-order (not master)
• Commits: a27c6cf
• Also mention me there once done

[ymartin-ovh]

Hello @Al2Klimov

I can do that but the commit you want does not address the bug I submitted.
We have to deal with the null reference use.

Regards

[ymartin-ovh]

I rollback to my original contribution about null reference check.

[Al2Klimov]

that null check there should definitely by done

– #9861 (comment)

I agree.

[ymartin-ovh]

Hello

Do you plan to integrate this fix in 2.14.x branch too ? We have this issue in 2.14, I maintain my own build because of this.

Regards

[Al2Klimov]

• Colleagues, wanna get this little thing done, so that OP can test Satellite sync failure #9752 with snapshot packages?

[julianbrost]

#9861 (comment):

I maintain my own build because of this.

Sounds like it should already be tested?

[julianbrost]

Given that there's error handling for m_Checkable being nullptr following, it looks it host (which is a possible value for m_Checkable is expected to possibly be nullptr:

icinga2/lib/icinga/comment.cpp

Lines 73 to 74 in 9a000f3

	if (!m_Checkable)
	BOOST_THROW_EXCEPTION(ScriptError("Comment '" + GetName() + "' references a host/service which doesn't exist.", GetDebugInfo()));

So that check looks like a good idea indeed.

Side note: while quickly reading over the related issue, I noticed the following comment (#9752 (comment)):

I'm trying to understand why master does not send hostgroups configuration first.

Actually, the config object send order was tweaked by the just merged #10000, so that could also have an effect here. Though, as per what I wrote earlier in this comment, having a nullptr check here is a good idea nonetheless.

[Al2Klimov]

• Sorry, of course you're right – this PR is nice, but Fix broken downtime comment sync #10000 should do the thing by itself.