connect components using watchable

[arkodg]

Signed-off-by: Arko Dasgupta arko@tetrate.io

[danehans]

Instead of passing in logger logr.Logger, consider using a context and then calling logr.FromContext(ctx), xref.

[arkodg]

I always prefer using function args to pass data around over context unless I absolutely need to ( and when the data is tied to a context rather than the package).
I can change the function signature and use a Config object instead to reduce the number of params passed to it

[danehans]

Each controller should have a unique name. This is typically used to tie logging messages back to a specific controller, xref.

[arkodg]

this ties to gwapiv1b1.GatewayController(name) / class controller, not the same of the sw controller/reconciler

[danehans]

The value of controller should continue to be exposed to users to support running multiple Envoy Gateways in a cluster.

[arkodg]

not sure what you are referring to here

[danehans]

@arkodg this is a great start! Other than a few nits, test coverage, and passing CI, this LGTM.

[LukeShu]

Yeah, this is basically how I think watchable should be used. I left a suggestion to simplify the simple cases. If we think we'll have a lot of non-map uses, adding a watchable.Value would be good.

[LukeShu]

Do we expect to track multiple sets of resources, and this is a short-term hack? Or is this a hack around not being able to have a single "watchable.Value" without wrapping it in a map?

[arkodg]

its the latter, wrapping it to return a single value embedded in a map (with a single key-value pair)

[youngnick]

Took a really quick look over this and had one naming thing, sorry to be picky, but I think it could really bite us down the line if we don't fix it now.

I'll have more of a look at the rest later.

[arkodg]

this PR is ready to be reviewed now. I'd prefer not to increase the size of this PR anymore and address other issues in a follow up PR

Two issue I'd like to highlight

• the provider fails while starting with this error

no matches for kind "HTTPRoute" in version "[gateway.networking.k8s.io/v1beta1](http://gateway.networking.k8s.io/v1beta1)"

from here
this issue is not tied to this PR but is likely introduced by
#194 and is present on main too. I have commented the snippet causing the issue and this can be resolved in a follow up PR after reopening #189
cc @chauhanshubham

• looks like there is always a watchable subscription notification during init
  I've circumvented it by skipping those messages

if msg == nil {
  continue
}

but im wondering if this is by design @LukeShu ?

[arkodg]

looks like the 1st error highlighted in #197 (comment) is not an issue, reverted the code, and installed the right version of GAPI CRDs(0.5.0) which installed the v1beta1 version for HTTPRoute which is used internally in the code, thank you @chauhanshubham for pointing this out 🙇

[danehans]

Please review my feedback. I'm not opposed to approving the PR with the suggested changes or filing issues to track the suggested changes.

[danehans]

The func name states it starts all the services but it's starting runners. I'm not going to block the PR on this but I prefer we use service instead of runner, e.g. providerService. I find it easier to build a mental model around service.

[arkodg]

I reverted all the code based on @youngnick's suggestion, I'll rename this function to setupRunners make this more logical. This name is not final and would prefer if we can revisit this in the near future.

[danehans]

This change is unneeded due to https://github.com/envoyproxy/gateway/blob/main/internal/provider/kubernetes/config/default/kustomization.yaml#L1-L2

[danehans]

Why are these changes required?

[arkodg]

these args were being appended to the envoy-gateway container, which I dont think is the intent

[danehans]

This change is unneeded due to https://github.com/envoyproxy/gateway/blob/main/internal/provider/kubernetes/config/default/kustomization.yaml#L1-L2

[arkodg]

uncommenting the above line changes the name of the envoy-gateway pod to envoy-gateway-envoy-gateway, do we want that ?

[danehans]

See my other comments in reference to changing this. Not sure why this change is needed.

[LukeShu]

Wait, is there no CI checking that make generate is up-to-date?

[arkodg]

Wait, is there no CI checking that make generate is up-to-date?

not yet, raised #213 to track it

plan on fixing additional issues in follow up PRs