Update Ingress.status.loadBalancer.ingress when reconciling
#69
Conversation
UnstoppableMango
force-pushed
the
lb-status
branch
from
e67f8d8 to
c7ed1c5
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #69 +/- ##
==========================================
+ Coverage 25.91% 26.48% +0.57%
==========================================
Files 9 9
Lines 629 638 +9
==========================================
+ Hits 163 169 +6
- Misses 456 458 +2
- Partials 10 11 +1 ☔ View full report in Codecov by Sentry. |
UnstoppableMango
force-pushed
the
lb-status
branch
from
c7ed1c5 to
20742fe
Compare
There was a problem hiding this comment.
Hi @UnstoppableMango , thanks for your contributions! Before this PR get merged, there is something needs changes:
- the field to be updated is
ingress.Status.LoadBalancer.Ingress, notservice.Status.LoadBalancer.Ingress. ref: https://github.com/tailscale/tailscale/blob/1c3c3d6752314c802007fd8f223e0c8bbddb7984/cmd/k8s-operator/ingress.go#L270-L281 - we should update ingress after we actually put rules on cloudflare, that's really import for kubernetes controller pattern. In other words, the codes need to be inserte at here:
|
PTAL @UnstoppableMango, I would very much appreciate it if you could address these changes. 🥰🥰 |
|
You got it! That first one is a little embarrassing 😳 should be able to make the updates this afternoon! |
| @@ -99,6 +99,19 @@ func (i *IngressController) Reconcile(ctx context.Context, request reconcile.Req | |||
| } | |||
| } | |||
|
|
|||
| origin.Status.LoadBalancer.Ingress = append(origin.Status.LoadBalancer.Ingress, | |||
| networkingv1.IngressLoadBalancerIngress{ | |||
| Hostname: i.tunnelClient.TunnelDomain(), | |||
There was a problem hiding this comment.
In the original comment in #68 they mentioned using the cloudflare cname value, do you agree with this? Or should this use the ingress hostname?
Also, should https:// be appended to the beginning?
There was a problem hiding this comment.
Hi @UnstoppableMango , sorry for the late response.
we do not need https:// prefix.
|
Ach I apoligize that first attempt was really half-baked! I didn't see a great spot to move tests to, is there a place you think they should live? |
Missed some stuff
UnstoppableMango
force-pushed
the
lb-status
branch
from
8f9ff7e to
025da4a
Compare
UnstoppableMango
changed the title
Service.status.loadBalancer.ingress when reconcilingIngress.status.loadBalancer.ingress when reconciling
| @@ -99,6 +99,19 @@ func (i *IngressController) Reconcile(ctx context.Context, request reconcile.Req | |||
| } | |||
| } | |||
|
|
|||
| origin.Status.LoadBalancer.Ingress = append(origin.Status.LoadBalancer.Ingress, | |||
| networkingv1.IngressLoadBalancerIngress{ | |||
| Hostname: i.tunnelClient.TunnelDomain(), | |||
There was a problem hiding this comment.
Hi @UnstoppableMango , sorry for the late response.
we do not need https:// prefix.
Should resolve #68
A note about the implementation, I wasn't a fan of mutating theServiceinside ofFromIngressToExposurebut it was the most convenient place for it right now. I could move the mutation into the reconcile method directly, but then we would need a redundantclient.Get()call. We could move it somewhere else as well, but that would probably require a bit of refactoring.