Skip to content
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

Fixes icinga2 certs path for newer versions 2.8+ #50615

Closed

Conversation

Projects
None yet
4 participants
@ClaudiuPID
Copy link
Contributor

commented Nov 22, 2018

What does this PR do?

Bug fixing.

What issues does this PR fix or reference?

#45867

Previous Behavior

Icinga2 module and state worked only with versions prior to 2.8 for new deployments.
It also worked for versions newer than 2.8 but if upgraded from older versions without following the official documentation.

Tests written?

No

Commits signed with GPG?

No

Notes

My issues so far:

  1. Documentation refers to old certs path and now the path is selected based on icinga2 version. How should the docs be written?
  2. If the server was deployed with icinga2 prior to 2.8 and upgraded without following the documentation, this change will break compatibility. This scenario is a bit messy. If configuration files are not updated, certs path is kept like before but new certs will be deployed in new path. So icinga2 will fail to register new clients. Again this will happen only if the upgrade was done without following the documentation;
  3. I moved some common parts to salt/utils/icinga2.py so this is a new file. This is not a big change but since it is a bug fix I am not sure if all my changes are acceptable.
  4. I created this pull request for branch 2018.3.3 since this should be a bug fix, is it ok?

Tomorrow I will fully test all this changes on an older server following upgrade documentation.

log = logging.getLogger(__name__)


def execute(cmd, ret_code=False):

This comment has been minimized.

Copy link
@cachedout

cachedout Nov 22, 2018

Collaborator

WIth this, would it perhaps be better just to use Salt's cmd.run or cmd.retcode module functions that already exist?

This comment has been minimized.

Copy link
@ClaudiuPID

ClaudiuPID Nov 22, 2018

Author Contributor

It did not work last time I tested it.
Please see #38987

This comment has been minimized.

Copy link
@ClaudiuPID

ClaudiuPID Nov 23, 2018

Author Contributor

Will see if cmd.retcode works, I just realized I did not test that one.

This comment has been minimized.

Copy link
@cachedout

cachedout Nov 23, 2018

Collaborator

Thanks. If we can figure out the exact issue with the cmd module, it would be great to have an issue filed against that so we can get it fixed as well. Thanks.

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

This worked ok on my old server after I upgraded it.
One issue with debian clients, new certs path is not created by default so one should create it through salt.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Sure, will push tomorrow.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

@ClaudiuPID Thanks. Once those changes are made we should be able to get this in.

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Fixed.

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2018

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

I don't think those are caused by my pull request. I only changed icinga2.py files, none of those reported there.

@Ch3LL Ch3LL closed this Dec 3, 2018

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

@Ch3LL did I miss something for this to be merged?

@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

@ClaudiuPID the 2018.3.3 branch was deleted yesterday since its a release branch and its been released. It looks like github automatically closed it since I deleted the branches. Can you push to 2018.3 instead?

@ClaudiuPID

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

@Ch3LL for some reason when I try to create a new pull request on github it shows branch 2018.3 but not 2018.3.3.
When I try to enter that branch manually it shows:
Choose different branches or forks above to discuss and review changes.

Ideas?

@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

Yes you want to select 2018.3 not 2018.3.3. The 2018.3.3 branch is deleted because that release has been released. Please submit to the 2018.3 branch please.

@ClaudiuPID ClaudiuPID referenced this pull request Dec 5, 2018

Merged

Fix icinga2 cert path #50765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.