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

[ENHANCE] forecastle should read the ingress host from the status instead of the spec #440

Open
visheyra opened this issue Sep 14, 2024 · 4 comments · May be fixed by #446
Open

[ENHANCE] forecastle should read the ingress host from the status instead of the spec #440

visheyra opened this issue Sep 14, 2024 · 4 comments · May be fixed by #446
Labels
good-first-issue A good starting issue for new commers kind/enhancement kind/help wanted

Comments

@visheyra
Copy link

Is your feature request related to a problem? Please describe.
When using specific ingress controllers (like the tailscale one), the address discovered, is the one defined in the ingress and not the one set in the status. This leads to incorrect url being discovered, while reading from the status field should always grant the correct url as the ingress controller is responsible of managing that field. Kubectl (when using the command kubectl get ing -o wide) is reading address from the status field as well.

Describe the solution you'd like
Discover the url from the status field instead of the host / tls field.

Describe alternatives you've considered
The current alternative solution is to set the url manually

Additional context
Adding screenshot from my kubectl output and my forecastle display

Screenshot from 2024-09-14 14-27-12
Screenshot from 2024-09-14 14-26-38

@bnallapeta
Copy link

@visheyra

Upon checking, we found that not all Ingress Controllers populate the same way. Some populate status.ip and some status.hostname and some none.

I think a reliable way to do this would be to first rely on the status and then fall back to spec if status isn't available.

Thoughts?

@aslafy-z
Copy link
Contributor

aslafy-z commented Sep 30, 2024

ingress-nginx publishes by default the service LoadBalancer IP or hostname to the ingresses statuses. This is most of the time, not the value we want forecastle to use. I would do the other way around, default on ingresses hosts, or if unset fallback to status.loadbalancer.hostname, then ip.

@MuneebAijaz
Copy link
Contributor

ingress-nginx publishes by default the service LoadBalancer IP or hostname to the ingresses statuses. This is most of the time, not the value we want forecastle to use. I would do the other way around, default on ingresses hosts, or if unset fallback to status.loadbalancer.hostname, then ip.

PRs are welcome for this change if this is the finalized workflow

@MuneebAijaz MuneebAijaz added kind/help wanted good-first-issue A good starting issue for new commers labels Oct 2, 2024
@aslafy-z aslafy-z linked a pull request Oct 2, 2024 that will close this issue
@aslafy-z
Copy link
Contributor

aslafy-z commented Nov 9, 2024

@visheyra #446 may solve your issue. As no TLS host will be found, Forecastle with default with the http:// protocol. Does that fit your use case? Or we may want to add an additional annotation like forecastle.stakater.com/protocol which would allow forcing the protocol to https:// for eg.

aslafy-z added a commit to aslafy-z/Forecastle that referenced this issue Dec 20, 2024
Update `GetURL` method in `IngressWrapper` class to check for protocol annotation and use specified protocol if present

* Add `tryGetStatusHost` method to read ingress host from status field
* Update `GetURL` method to use `tryGetStatusHost` method to get host from status field if not found in spec
* Add unit tests for `tryGetStatusHost` method and update existing tests for `GetURL` method
* Add helper functions to create ingress objects with status fields populated for testing
* Add `DefaultProtocol` field in `Config` struct
* Update `getURL` function to include logic for reading ingress host from status field
* Modify `CustomApp` struct to include `Protocol` field and update `convertCustomAppsToForecastleApps` function to use specified protocol

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/stakater/Forecastle/pull/446?shareId=XXXX-XXXX-XXXX-XXXX).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue A good starting issue for new commers kind/enhancement kind/help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants