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

[mailhog] Add extraPorts in Service and extraContainers in Deployment #627

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

jullianow
Copy link
Contributor

When deploying the mailhog graph using ingress GCE the health check of Google's Load Balancer fails to succeed if the service is configured with basic auth.

This is because the health check is not able to set authentication parameters so that a request gets a return code of 200.
Also, even though there is an smtp port, the load balancer health check only has the ability to communicate via HTTP, HTTPS and HTTP2 protocol.
Reference: https://cloud.google.com/kubernetes-engine/docs/how-to/ingress-features#direct_health

Therefore, the purpose of this change is to give the ability to add an extra container to be used only as a route for the Load Balancer life check to work, at the same time that mailhog basic authentication is enabled.

Signed-off-by: Julliano Goncalves jullianow@gmail.com

Signed-off-by: Julliano Goncalves <jullianow@gmail.com>
@jullianow jullianow requested a review from a team as a code owner July 29, 2022 19:36
charts/mailhog/README.md Outdated Show resolved Hide resolved
charts/mailhog/templates/deployment.yaml Outdated Show resolved Hide resolved
Signed-off-by: Julliano Goncalves <jullianow@gmail.com>
@grieshaber grieshaber merged commit 0fd6729 into codecentric:master Aug 1, 2022
@grieshaber
Copy link
Collaborator

@jullianow thanks for the PR. Will be release friday afternoon as a new feature release.

@jullianow
Copy link
Contributor Author

@grieshaber Very good.

@jullianow
Copy link
Contributor Author

jullianow commented Aug 2, 2022

@grieshaber I'm sending the message in this thread because it has to do with it.

Do you think a PR adding a route (E.g. /health) to the Mailhog service makes sense?
This route would not have configuration for basic authentication, so it could be used in the Ingress GCE health check.

That's because, even adding a specific container to be used in the health check (as I described in this PR) is not enough.
Google tries to ensure that all ports exposed by Ingress are returning 200, and therein lies the problem.
When Mailhog has basic authentication, the check fails.
image

@grieshaber
Copy link
Collaborator

grieshaber commented Aug 2, 2022

@jullianow tbh i'm not very familiar with the GCE native Load-Balancer stuff.
From the docs you mentioned, i see that the LB uses some defaults for the health check. But, alternativly, you can specify a backendService CR yourself that will be used for your Service and customize the healthcheck to point to your extra port:

apiVersion: cloud.google.com/v1
kind: BackendConfig
metadata:
  name: http-hc-config
spec:
  healthCheck:
    checkIntervalSec: 15
    port: 15020
    type: HTTPS
    requestPath: /healthz

It's not ideal, bc you need to handle the additional ressource though..

But, to understand you 2nd proposal further:
You want to add a valid /health endpoint to mailhog itself and use this for the readnessProbe (which google will then use aswell)?

grieshaber pushed a commit that referenced this pull request Aug 2, 2022
…oyment (#627)

* [mailhog] Add extraPorts in Service and extraContainers in Deployment

Signed-off-by: Julliano Goncalves <jullianow@gmail.com>

* Code review feedback

Signed-off-by: Julliano Goncalves <jullianow@gmail.com>
@jullianow
Copy link
Contributor Author

jullianow commented Aug 2, 2022

Yes, it is possible to do what you are talking about.
But the problem is that I can define which backendconfig should be used for a port.
But the point is that the BackendConfig (either mine or the default that exists naturally) it validates all ports that are specified in Ingress (which is the image I sent).

That is, this suggestion of yours was exactly what I did and that I verified that it doesn't work (after seeing the information block of the official documentation).

But, to understand you 2nd proposal further:
You want to add a valid /health endpoint to mailhog itself and use this for the readnessProbe (which google will then use aswell)?

Perfect. And this route will not suffer from the basic authentication configuration (The pod's readnessProbe would not change (it can remain as is).
So I can have a BackendConfig where the healthCheck will have a requestPath for this route and everything will work.

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

Successfully merging this pull request may close these issues.

2 participants