New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render Icinga config helpers with safe globals access #1654

Merged
merged 1 commit into from Sep 27, 2018

Conversation

Projects
None yet
2 participants
@dnsmichi
Member

dnsmichi commented Sep 27, 2018

globals[key] works, although it wasn't documented how it
behaves in the case when this field doesn't exist.

Previously this has been a dictionary which silently returns
null if the key doesn't exist. With v2.10, globals turned
into a namespace which throws an error if not existing (which is
the correct behaviour).

In order to avoid configuration errors with 2.10, this is changed
into contains() which has been sitting in dictionaries since 2014.
So this patch ensures compatibility with older versions as well.

Render Icinga config helpers with safe globals access
globals[key] works, although it wasn't documented how it
behaves in the case when this field doesn't exist.

Previously this has been a dictionary which silently returns
null if the key doesn't exist. With v2.10, globals turned
into a namespace which throws an error if not existing (which is
the correct behaviour).

In order to avoid configuration errors with 2.10, this is changed
into `contains()` which has been sitting in dictionaries since 2014.
So this patch ensures compatibility with older versions as well.

@dnsmichi dnsmichi requested a review from lazyfrosch Sep 27, 2018

@Thomas-Gelf

This comment has been minimized.

Contributor

Thomas-Gelf commented Sep 27, 2018

Looks good to me, thanks for the pull request and the related details. 2014 means that Agents running 2.6 are safe I guess. @lazyfrosch: if you have the chance to give 2.6 a quick test this would be cool, otherwise I'd blindly trust @dnsmichi and merge to master.

@dnsmichi

This comment has been minimized.

Member

dnsmichi commented Sep 27, 2018

Anything since 2.4 should work here, contains() as dictionary method was available when globals was introduced the first time (December 2014, or so).

@Thomas-Gelf Thomas-Gelf merged commit bb81de8 into master Sep 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Thomas-Gelf Thomas-Gelf deleted the bugfix/director-basics-globals branch Sep 27, 2018

@Thomas-Gelf

This comment has been minimized.

Contributor

Thomas-Gelf commented Sep 27, 2018

Thanks again!

@dnsmichi

This comment has been minimized.

Member

dnsmichi commented Oct 11, 2018

Since this problem comes out more often, I've discussed it again with @lippserd ... the patch for the Director is safe, still we need to take care about existing deployments where this would stop with 2.10. The patch for the Director is fine and safe, but we don't need a release for it now. I'll like a PR for 2.10 which solves the access problem, and allows for smooth upgrading.

dnsmichi added a commit to Icinga/icinga2 that referenced this pull request Oct 11, 2018

Don't throw an error when namespace indexers don't find a valid key
Examples:

```
globals["abc"]
globals.def
```

The patch for the Icinga Director unfortunately only solves the
master, and as discussed with @lippserd we need to ensure that
satellites and clients with 2.10 can be restarted without any errors
from deployed configuration.

refs #6509
refs Icinga/icingaweb2-module-director#1654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment