Feat/service import backend support

[tanujd11]

What type of PR is this?

Feature

What this PR does / why we need it:
Adds serviceImport Backend for Routes.

Which issue(s) this PR fixes:

Fixes #9

[tanujd11]

Can someone re-trigger the CI? Looks like the upload to codecov broke in the run.

[arkodg]

/retest

[arkodg]

thanks for picking this feature up @tanujd11 ! would be great if this land in v0.6
quick q - isn't this feature blocked on #1494 which is adding EndpointSlice support ?

[codecov]

Codecov Report

Merging #1705 (865aa0e) into main (3fbaa78) will decrease coverage by 0.18%.
The diff coverage is 63.07%.

@@            Coverage Diff             @@
##             main    #1705      +/-   ##
==========================================
- Coverage   65.22%   65.04%   -0.18%     
==========================================
  Files          86       86              
  Lines       12318    12452     +134     
==========================================
+ Hits         8034     8099      +65     
- Misses       3772     3832      +60     
- Partials      512      521       +9     

Files Changed	Coverage Δ
internal/gatewayapi/translator.go	97.33% <ø> (ø)
internal/gatewayapi/zz_generated.deepcopy.go	0.00% <0.00%> (ø)
internal/provider/kubernetes/helpers.go	78.52% <0.00%> (-1.35%)	⬇️
internal/provider/kubernetes/predicates.go	53.94% <40.90%> (-2.39%)	⬇️
internal/provider/kubernetes/controller.go	48.17% <59.04%> (-1.08%)	⬇️
internal/provider/kubernetes/routes.go	43.72% <66.66%> (+0.65%)	⬆️
internal/gatewayapi/validate.go	90.19% <80.00%> (-1.29%)	⬇️
internal/gatewayapi/helpers.go	81.13% <100.00%> (+0.74%)	⬆️
internal/gatewayapi/resource.go	57.50% <100.00%> (+7.50%)	⬆️
internal/gatewayapi/route.go	88.37% <100.00%> (+0.08%)	⬆️

[tanujd11]

@arkodg , I think this can go in with using ClusterSetIP type which will have VIPs in multi cluster service import. EndpointSlice can be used once that PR gets in. It will be similar to service backend.

[arkodg]

Rethinking, you're right @tanujd11, this PR can go in by itself.
Will review this soon once we start working on v0.6 features.

[arkodg]

did a first pass, it looks good ! added some minor comments

[Xunzhuo]

This is awesome, but there are some conflcts : )

It would be a nice feat in v0.6.0.

[arkodg]

ptal @envoyproxy/gateway-maintainers, a decision also needs be made here on whether we want the ServiceImport API to be opt in or not

[tanujd11]

@arkodg, if we are removing the serviceImport CRD from EG chart, then it automatically becomes the a Opt-in feature where user needs to install the CRD to use this feature. I can add a validation to check if the CRD exist before someone uses serviceImport as part of the backend. for example if the user has mentioned serviceImport as backendRef and the CRD does not exist, we can reject the config with status backendRef kind does not exist

[tanujd11]

/retest

[arkodg]

@arkodg, if we are removing the serviceImport CRD from EG chart, then it automatically becomes the a Opt-in feature where user needs to install the CRD to use this feature. I can add a validation to check if the CRD exist before someone uses serviceImport as part of the backend. for example if the user has mentioned serviceImport as backendRef and the CRD does not exist, we can reject the config with status backendRef kind does not exist

great idea, +1 to this, but from an implementation perspective, won't the provider layer error out if it tries to Watch the ServiceImport resource w/o have a CRD installed ?

[arkodg]

LGTM, thanks for adding this !

[tanujd11]

great idea, +1 to this, but from an implementation perspective, won't the provider layer error out if it tries to Watch the ServiceImport resource w/o have a CRD installed ?

You are right, the current one is throwing error. Should we implement a way to check the presence of CRD for watching also. Or is there any other way in your mind to tackle this

[arkodg]

@tanujd11 can you share the error you are seeing ?

one way to solve the opt in problem is add a knob in EnvoyGateway.ExtensionApis

[Xunzhuo]

LGTM after conflicts are resolved and CI passed.

[tanujd11]

@arkodg , can you take a relook at this work? I have added the knob at the place suggested by you. I will work on the doc in a subsequent PR

[arkodg]

had a chat with @tanujd11 who is trying to implement the functionality w/o having to add an explicit API knob

[Xunzhuo]

Looks like this PR has been rebased a couple of times, maybe we should get this in higher priority, in case more rebases. Thanks @tanujd11, mind having another rebase to fix conflicts?

[arkodg]

hey @tanujd11 just took a peek at your PR, the approach of using the o/p. of discoveryClient.ServerResources() to check if the service import API exists in the API server is a great way to decide on whether to watch the resource or not.

[arkodg]

seeing some test yaml fail, recommend running make testdata to fix it

[arkodg]

LGTM, added some minor nits around error string
thanks for adding this !

[zirain]

can you add a user face doc in following PR.