-
2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago2 years ago1 year ago1 year ago1 year ago1 year ago1 year ago1 year ago1 year ago1 year ago2 weeks ago
-
Due to the fact that this PR contains a lot of changes (from a different branch? or maybe from a different origin), I think it would be hard to merge.
I've reviewed changes in Killbill chart and those look OK. The only question is, if we should expose this new sidecar endpoint in the helm chart? Right now, we have sidecar container and it accepts HTTP requests from inside the cluster, but it is not exposed outside using any service or ingress. Shouldn't we add this as well? or we will be calling it only from inside of the k8s cluster?
-
1 week ago
-
Indeed the inventory checker must be exposed outside the K8s cluster. The purpose is for it to be monitored by Uptime Kuma (which is outside the cluster). Having the service be visible within the cluster alone would service no purpose.
I had forgotten to add the service definition in the first rev of the PR. PR is now updated.
PS: However for better security, I want to narrow the visibility of the service to the IP of the Uptime Kuma alone; may be I can add authentication so the abuse of the inventory checker may be prevented. I can add it as in a follow up PR.
-
I checked out the laster and create a new feature branch. The only changes I added are:
- the definition of sidecar template in templates/inventory-checker-service.yaml
- insert sidecar instance in container section of deployment.yaml
- parameter values in values.yaml
I have no other changes. Should I redo this PR?
-
Now that the inventory checker service is exposed outside the cluster, I want to lock down the access to the service only to service-status.tigase.net, as a measure of security.
I can add the required network policy to this PR or to a follow up PR after this PR is merged. What is your suggestion?
-
1 week ago
-
Thank you for the changes and rebasing onto
master
branch. That helped readability.Adding a lockdown (I suppose it is related to
whitelist-source-range
annotation) is a good idea. However, I would move this to the helm values, so that it could be easily changed on the deployment of the helm chart.Could you also check if "ports" are configured properly? I see that we can set port for service, but later on ingress that should be connecting to this port uses 8080. I would skip option to set this port or we should point ingress to connect to this port from value.
-
1 week ago
-
1 week ago
-
1 week ago
-
1 week ago
-
I have squashed all commits into the first PR, viz., https://tigase.dev/tigase/helm-charts/~pulls/1
Hence merge this PR alone. PR2 and PR3 can be disregarded.
Submitter | rk@tigase.net |
Target | master |
Source | feature/sidecar-container1 |
This is the definition of inventory checker sidecar container. The changes entail augmenting the killbill deployment container and the attendant changes to values.yaml.
Testing:
The changes were tested by template instantiation: