Gateway subscribes to Validation notifications from Gloo #1820
Conversation
|
Issues linked to changelog: |
ilackarms
added
the
work in progress
|
/kick |
ilackarms
removed
the
work in progress
| time.Sleep(time.Second / 2) | ||
| Expect(getNotifications()).To(HaveLen(3)) |
There was a problem hiding this comment.
should this be an Eventually?
There was a problem hiding this comment.
not exactly. it should be a Consistently (because we want to make sure they value does not go over 3 once the ctx is cancelled)
| ) | ||
|
|
||
| func MakeNotificationChannel(ctx context.Context, stream validation.ProxyValidationService_NotifyOnResyncClient) <-chan struct{} { | ||
| notifications := make(chan struct{}, 10) |
There was a problem hiding this comment.
Why use a 10 buffered channel here. I get the need to make up for lost updates, but 10 seems like far too many. 1 or 2 would probably accomplish the same thing.
There was a problem hiding this comment.
i think this was a typo or copypaste error. i intended to have 1 notification (there's never a need to have more than one resync queued)
| message NotificationRequest { | ||
|
|
||
| } | ||
|
|
||
| message NotificationResponse { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
nit: NotifyOnResyncRequest/ NotifyOnResyncResponse.
In general with our grpc services we try to name the request and response after the rpc in this way. Since there is more than one now, I think if possible we should also rename the ValidateProxy request and response object as well. As more and more rpcs are added it makes managing them much easier.
| s.lock.Lock() | ||
| validator := s.validator | ||
| s.lock.Unlock() |
There was a problem hiding this comment.
create a locking get validator function?
Also, this can be a RWMutex
| if err := stream.Send(&validation.NotificationResponse{}); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Won't this cause an additional, potentially unecessary resync every time gloo starts up.
There was a problem hiding this comment.
no - this is part of the Notification "handshake". look where the gateway processes notifications, it waits to receive the first notification before starting the event loop. gateway will block on gloo starting up. currently, if gloo goes down/restarts without restarting the gateway, notifications will no longer be received by gateway.
i can add logic to make the gateway retry the connection if the notification stream gets cut, but i feel that's work for a follow-up if/when we need it (this PR is already pretty complicated).
relevant code lives in MakeNotificationChannel:
https://github.com/solo-io/gloo/pull/1820/files#diff-4aa11983835012a15b5c74cdff446732R11
| // only call within a lock | ||
| // notify all receivers |
There was a problem hiding this comment.
Rather than annotate this way, can you just lock the function itself with a lock, defer unlock
There was a problem hiding this comment.
this function needs to be called by another function that acquires a lock. locking here would cause a deadlock
| // only call within a lock | ||
| // should we notify on this snap update |
There was a problem hiding this comment.
Same comment about the lock comment, should just lock the function itself
| for _, receiver := range s.notifyResync { | ||
| receiver := receiver | ||
| go func() { | ||
| select { | ||
| // only write to channel if it's empty | ||
| case receiver <- struct{}{}: | ||
| default: | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Would it ever be necessary to send more than one notification from this function? These go functions can potentially back up and exist for a while if the channel fills up.
There was a problem hiding this comment.
the receiver only has a size of 1, so we never buffer more than 1 notification
|
/kick rate limit flake |
|
/kick |
Context
The Gateway needs to know when Gloo resyncs its own storage in order to retry validation of gateway resources (GW, VS, RT). See issue #1815 for a description of the issue.
Solution
This PR adds a
NotifyOnResyncmethod to Gloo's Validation grpc service. The gateway subscribes to this endpoint at boot (if validation is enabled) and wires it into itsApiEmitter, forcing the gateway translation loop to resync when a notification is received.This allows the Gateway to receive notifications when a Gloo resource changes (upstream, secret, configmap, proxy) without having to explicitly watch the resource itself.
Note that gloo notification server does not notify on
EDSchanges as they are not considered in validating proxy configuration.BOT NOTES:
resolves #1812