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

Icinga Kubernetes Role and Icinga Kubernetes Web install #348

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gianmarco-mameli
Copy link
Contributor

Hello,
in the past month I tried the new Icinga Kubernetes daemon and module and, because my Icinga Infrastructure is outside of my K3S Cluster, I preferred to install the packages on the master nodes.
Using most of the excellent existent work, I copied the structures and implemented the installation of the daemon and db schema, in addition to the Icingaweb2 dedicated module.
The tasks are working and running correctly on my Icinga test installation (both existent and from scratch), so if you're interested I appreciate to check and merge my work.
I think I have covered most of the docs and files that needed update, but I certainly not a good expert to add the Molecule tests needed, so in that case maybe I need some help, but I'm already trying to fill the gaps

Thanks

PS. In my spare time I'm also implementing the same work for Dependencies and Fileshipper modules.

@cla-bot cla-bot bot added the cla/signed label Nov 14, 2024
@gianmarco-mameli gianmarco-mameli marked this pull request as ready for review November 14, 2024 08:10
@mkayontour
Copy link
Member

Hi, at the first glance this looks really good, thank you for the work. We'll review it as soon as possible and give you feedback.

Copy link
Collaborator

@Donien Donien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far.

The only thing is that I'm running into a bug where my apache web server fails completely for some reason a few minutes after having k3s, Icinga Web + Icinga-Kubernetes running (service okay, no errors in log, but 404 for every url).
This is likely unrelated though.

doc/role-icingaweb2/module-kubernetes.md Outdated Show resolved Hide resolved
doc/role-kubernetes/role-kubernetes.md Outdated Show resolved Hide resolved
roles/kubernetes/defaults/main.yml Outdated Show resolved Hide resolved
@Donien
Copy link
Collaborator

Donien commented Nov 18, 2024

One more thing (could not add to my review somehow).

https://github.com/gianmarco-mameli/ansible-collection-icinga/blob/kubernetes_role_and_module/roles/icingaweb2/vars/main.yml#L1-L6

   director: icinga-director
   x509: icinga-x509
   businessprocess: icinga-businessprocess
+  kubernetes: icinga-kubernetes-web

This is needed so the mapping from icingaweb2_modules.kubernetes to the appropriate package can be made.

@gianmarco-mameli
Copy link
Contributor Author

ok, I made the fix you suggested, thanks

Copy link
Collaborator

@Donien Donien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a changelog fragment.

changelogs/fragments/feature_kubernetes.yml:

major_changes:
  - Add a role for the installation and configuration of `Icinga for Kubernetes <https://icinga.com/docs/icinga-for-kubernetes/latest/>`_.
  - Add tasks to role :code:`icingaweb2` to install and configure `Icinga for Kubernetes Web <https://icinga.com/docs/icinga-kubernetes-web/latest/doc/02-Installation/>`_.

@mkayontour, do you want to have another look at this as well?

@gianmarco-mameli
Copy link
Contributor Author

changelog fragment ok

Copy link
Member

@mkayontour mkayontour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm good with the general code, I'm having a problem with the variable names which may overlap with other kubernetes roles and helps with visibility on your ansible code.
We should have a prefix like "icinga_"

And the override file should not be created as it is right now, see the comments on the file.

roles/kubernetes/templates/kubernetes.ini.j2 Outdated Show resolved Hide resolved
roles/kubernetes/templates/kubernetes.ini.j2 Outdated Show resolved Hide resolved
roles/kubernetes/tasks/manage_service.yml Outdated Show resolved Hide resolved
@gianmarco-mameli
Copy link
Contributor Author

Hi @mkayontour I've updated the role with your suggestions, I also modified the application of the service override only if is defined a custom path for the kubeconfig.
The name of the role is now icinga_kubernetes to permit adding the prefix on every variable and compliance with the linting rule of variable prefix name.
I think also doc is updated correctly, let me know

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants