Reindex outbound policy backends when a service changes

[adleong]

If an HTTPRoute references a backend service that does not exist, the policy controller synthesizes a FailureInjector in the outbound policy so that requests to that backend will fail with a 500 status code. However, we do not update the policy when backend services are created or deleted, which can result in an outbound policy that synthesizes 500s for backends, even if the backend currently exists (or vice versa).

This is often papered over because when a backend service is created or deleted, this will trigger the HTTPRoute's ResolvedRef status condition to change which will cause a reindex of the HTTPRotue and a recomputation of the backends. However, depending on the specific order that these updates are processed, the outbound policy can still be left with the incorrect backend state.

In order to be able to update the backend of an outbound policy when backend services are created or deleted, we change the way these backends are represented in the index. Previously, we had represented backends which were services that did not exist as Backend::Invalid. However, this discards the necessary backend information necessary to recreate the backend if the service is created. Instead, we update this to represent these backends as a Backend::Service but with a new field exists set to false. This allows us to update this field as backend services are created or deleted.

[olix0r]

makes sense to me

[the-wondersmith]

Just out of curiosity, I assume there's effectively no better way to accomplish what needs doing here without nesting for loops this deeply, is there?

[adleong]

the way to avoid nesting for loops would be to do something like:

        for routes in self
            .service_port_routes
            .values_mut()
            .flat_map(|routes| routes.watches_by_ns.values_mut())
        {
            routes
                .routes
                .values_mut()
                .flat_map(|route| route.rules.iter_mut())
                .flat_map(|rule| rule.backends.iter_mut())
                .for_each(|backend| {
                    if let Backend::Service(svc) = backend {
                        let service_ref = ServiceRef {
                            name: svc.name.clone(),
                            namespace: svc.namespace.clone(),
                        };
                        svc.exists = service_info.contains_key(&service_ref);
                    }
                });
            routes.send_if_modified()
        }

but I don't think that's clearer than the nested for loops in this case.

[the-wondersmith]

You're certainly right about clarity 😂. Any idea if there's a meaningful difference in the compiled binary? It absolutely should't be a blocker to merging, by all means "looks good, ship it"-stamped. Just curious if it could be avoided or if maybe clang will take care of it either way.