Have Istio optional #645

eguzki · 2024-05-13T15:48:23Z

What

Part of #325

This is a preparation work for on-boarding EnvoyGateway on following up PRs. It is, essentially, moving Istio from being the required GatewayAPI provider to optional gateway provider. Among other things, being optional means that when Istio is not installed in the cluster, the kuadrant operator does not die. Additionally, make room for on-boarding other GatewayAPI providers.

Golang package	Required environment	Makefile env setup target	Makefile test run target
`github.com/kuadrant/kuadrant-operator/tests/bare_k8s`	no gateway provider, no GatewayAPI CRDs. Just Kuadrant API and Kuadrant dependencies.	`make local-k8s-env-setup`	`make test-bare-k8s-integration`
`github.com/kuadrant/kuadrant-operator/tests/gatewayapi`	no gateway provider. GatewayAPI CRDs, Kuadrant API and Kuadrant dependencies.	`make local-gatewayapi-env-setup`	`make test-gatewayapi-env-integration`
`github.com/kuadrant/kuadrant-operator/controllers`	at least one gatewayapi provider. It can be any: istio, envoygateway, ...	`make local-env-setup GATEWAYAPI_PROVIDER=[ISTIO\|SAIL]` (Default Istio)	`make test-integration`
`github.com/kuadrant/kuadrant-operator/tests/istio`	GatewayAPI CRDs, Istio, Kuadrant API and Kuadrant dependencies.	`make local-env-setup GATEWAYAPI_PROVIDER=ISTIO`	`make test-istio-env-integration`

Defined behavior

GatewayAPI CRDs	Istio	Kuadrant CR status
NO	NO	Not reconciled
YES	NO	Error: GatewayAPI provider not found
YES	YES	OK

Verification Steps: run kuadrant in cluster without GatewayAPI provider

run environment

make local-gatewayapi-env-setup

Run the operator

make run

Create Kuadrant CR

kubectl -n kuadrant-system apply -f - <<EOF
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
spec: {}
EOF

Kudrant CR status reports not ready status

kubectl get kuadrant kuadrant -n kuadrant-system -o jsonpath='{.status}' | yq e -P
conditions:
  - lastTransitionTime: "2024-05-27T21:25:38Z"
    message: GatewayAPI provider not found
    reason: GatewayAPIPRoviderNotFound
    status: "False"
    type: Ready
observedGeneration: 1

codecov · 2024-05-13T15:52:00Z

Codecov Report

Attention: Patch coverage is 76.98020% with 93 lines in your changes are missing coverage. Please review.

Project coverage is 83.08%. Comparing base (ece13e8) to head (29dd203).
Report is 107 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   80.20%   83.08%   +2.87%     
==========================================
  Files          64       72       +8     
  Lines        4492     5704    +1212     
==========================================
+ Hits         3603     4739    +1136     
- Misses        600      639      +39     
- Partials      289      326      +37

Flag	Coverage Δ
bare-k8s-integration	`4.45% <49.25%> (?)`
controllers-integration	`72.57% <63.86%> (?)`
gatewayapi-integration	`10.97% <52.22%> (?)`
integration	`?`
istio-integration	`56.22% <63.86%> (?)`
unit	`32.76% <14.66%> (+2.73%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	`71.42% <ø> (ø)`
api/v1beta2 (u)	`93.58% <100.00%> (+2.16%)`	⬆️
pkg/common (u)	`88.13% <ø> (-0.70%)`	⬇️
pkg/istio (u)	`75.09% <ø> (+1.17%)`	⬆️
pkg/log (u)	`94.73% <ø> (ø)`
pkg/reconcilers (u)	`∅ <ø> (∅)`
pkg/rlptools (u)	`79.84% <ø> (+0.39%)`	⬆️
controllers (i)	`82.09% <79.66%> (+5.28%)`	⬆️

Files	Coverage Δ
...ontrollers/authpolicy_istio_authorizationpolicy.go	`87.37% <100.00%> (+0.06%)`	⬆️
controllers/test_common.go	`100.00% <100.00%> (ø)`
pkg/library/gatewayapi/utils.go	`96.47% <90.00%> (ø)`
controllers/authpolicy_controller.go	`84.05% <66.66%> (+3.35%)`	⬆️
controllers/dnspolicy_controller.go	`76.82% <66.66%> (-6.65%)`	⬇️
controllers/gateway_kuadrant_controller.go	`81.57% <66.66%> (+0.44%)`	⬆️
controllers/ratelimitpolicy_controller.go	`84.61% <66.66%> (+10.61%)`	⬆️
...lers/ratelimitpolicy_enforced_status_controller.go	`92.59% <66.66%> (ø)`
controllers/target_status_controller.go	`90.16% <66.66%> (ø)`
controllers/tlspolicy_controller.go	`70.52% <66.66%> (+3.85%)`	⬆️
... and 6 more

... and 24 files with indirect coverage changes

controllers/suite_test.go

guicassolato · 2024-05-28T13:15:32Z

pkg/istio/external_authorizer.go

+	return true, nil
+}
+
+func unregisterExternalAuthorizerOSSM(ctx context.Context, cl client.Client, kObj *kuadrantv1beta1.Kuadrant) error {


It looks like we're extracting too much. These OSSM functions do not belong to the istio package IMO.

The decision of whether to activate Istio or OSSM should continue to happen in the base controller or some abstraction from where we fork towards one path or the other.

ok, moving back the logic to register either in istio or OSSM to the controller.

KevFan

Looks good generally, left some comments regarding extra make command in the some github workflows 🧑‍💻

.github/workflows/test.yaml

tests/gatewayapi/suite_test.go

KevFan · 2024-05-30T12:24:22Z

controllers/kuadrant_controller.go

+	ok, err := kuadrantgatewayapi.IsGatewayAPIInstalled(mgr.GetRESTMapper())
+	if err != nil {
+		return err
+	}
+	if !ok {
+		r.Logger().Info("Kuadrant controller disabled. GatewayAPI was not found")
+		return nil
+	}
+


Do you think we should document or put into the log that the operator pod will need to be restarted because the disabled controllers are not reactively started when the dependencies are installed ? 🤔

This is a great point. I would try to avoid the restart if we can. If we are to keep the pod running anyway, at least it could retry the conditions in a loop, setup the reconcilers automatically in case the state changes.

Yeah, ideally the controller should keep trying until it finds it.

I thought about implementing it. But I wanted to discuss first the implementation

It can be done in another PR.

KevFan

Verified, looks good to me anyway 👍

controllers/authpolicy_authconfig.go

make/development-environments.mk

make/integration-tests.mk

eguzki · 2024-05-30T15:07:04Z

rebasing as there are conflicts with some files

The operator runs even when Istio or GatewayAPI are not present

…not installed

…wayAPI providers

Co-authored-by: Kevin Fan <[email protected]>

Co-authored-by: Guilherme Cassolato <[email protected]>

…d cluster ips' CommitID: 3cd0fa9

guicassolato · 2024-06-03T10:17:59Z

pkg/istio/mesh_config.go

+type authorizer interface {
+	GetExtensionProvider() *istiomeshv1alpha1.MeshConfig_ExtensionProvider
+}
+
+type ConfigWrapper interface {
+	GetConfigObject() client.Object
+	GetMeshConfig() (*istiomeshv1alpha1.MeshConfig, error)
+	SetMeshConfig(*istiomeshv1alpha1.MeshConfig) error
+}
+
+type KuadrantAuthorizer struct {
+	extensionProvider *istiomeshv1alpha1.MeshConfig_ExtensionProvider
+}
+
+// NewKuadrantAuthorizer Creates a new KuadrantAuthorizer
+func NewKuadrantAuthorizer(namespace string) *KuadrantAuthorizer {
+	return &KuadrantAuthorizer{
+		extensionProvider: createKuadrantAuthorizer(namespace),
+	}
+}
+
+// GetExtensionProvider Returns the Istio MeshConfig ExtensionProvider for Kuadrant
+func (k *KuadrantAuthorizer) GetExtensionProvider() *istiomeshv1alpha1.MeshConfig_ExtensionProvider {
+	return k.extensionProvider
+}


I thought we were keeping these interfaces and functions at the abstract level. Does this mean that having a Kuadrant Authorizer became a Istio-specific thing?

yes, it is an istio specific thing.

I don't follow. We won't deploy Authorino when integrating with Envoy Gateway?

Yes, we will. But this "kuadrant authorizer" is not deploying authorino. The kuadrant controller is. The "kuadrant authorizer" is to register external auth in istio control plane. AFAIK, EnvoyGateway does not require any registration step in the control plane of EG. @adam-cattermole can confirm.

I understand, @eguzki. My point is that this abstraction ("Kuadrant Authorizer config") has not been as much of an abstraction as it could be or originally intended.

At the level of the Kuadrant CR/Kuadrant deployment, configuring the Kuadrant Authorizer means ensuring that the Kuadrant deployment has all the configuration it needs for ext_authz requests to be sent to an instance of Authorino in the data plane, when handling traffic for the gateways in the scope of this Kuadrant deployment and it's applicable.

Because so far we only support Istio-based Gateway API controllers, it's easy overlook the difference, but I'm arguing for the decoupling of the abstraction that ensures the external authorizer is configured for a given Gateway API controller from the implementation details that get it done.

IWO, these interfaces belong to the Kuadrant controller and should be fixed to not know as much as they do today about Istio IMHO. In fact, they should not know anything at all about Istio. Only the Istio-specific implementations should know that the way to get the external authorizer configured (in their case) is by patching a "mesh config." Perhaps for other Gateway API controllers (e.g. EG), configuring the external authorizer at the level of the Kuadrant CR means something else; it could even be a no-op – but even that should be hidden behind the abstraction IMO.

Now that we are decoupling the controller from Istio, it looked like the right opportunity to fix this.

That abstraction is not meant to decouple from gateway API provider. It is totally coupled to Istio.

type ConfigWrapper interface { GetConfigObject() client.Object GetMeshConfig() (*istiomeshv1alpha1.MeshConfig, error) SetMeshConfig(*istiomeshv1alpha1.MeshConfig) error } type KuadrantAuthorizer struct { extensionProvider *istiomeshv1alpha1.MeshConfig_ExtensionProvider }

I think the intended design wanted to abstract from the different "flavors" of Istio. But actually, IMHO, there is no abstraction actually. It is something like: "if istioOperator -> do this; else if istio configmap -> do that; else if ossm -> do other thing; else panic".

Anyway, I get your point and I will try something I had in mind.

Prefer to stab it in separate PR to unblock this one? It's all refactoring in the end.

guicassolato

/lgtm

Nice work, @eguzki. Let's unblock this and let to tackle the issue of the loosely coupled Gateway API controllers with the future refactoring for sure to come.

eguzki changed the title ~~limitador_cluster_envoyfilter_controller optional~~ Have Istio optional May 13, 2024

eguzki added the kind/enhancement New feature or request label May 17, 2024

eguzki self-assigned this May 17, 2024

eguzki commented May 23, 2024

View reviewed changes

controllers/suite_test.go Outdated Show resolved Hide resolved

This was referenced May 23, 2024

[wip] refactor: parrallel tests #657

Closed

Envoy Gateway Support #325

Closed

eguzki force-pushed the istio-optional branch from d9a3850 to f439203 Compare May 27, 2024 20:55

eguzki marked this pull request as ready for review May 28, 2024 07:47

eguzki requested a review from a team as a code owner May 28, 2024 07:47

guicassolato reviewed May 28, 2024

View reviewed changes

KevFan requested changes May 28, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

.github/workflows/test.yaml Outdated Show resolved Hide resolved

.github/workflows/test.yaml Outdated Show resolved Hide resolved

eguzki requested review from guicassolato and KevFan May 28, 2024 16:55

guicassolato reviewed May 30, 2024

View reviewed changes

tests/gatewayapi/suite_test.go Outdated Show resolved Hide resolved

KevFan reviewed May 30, 2024

View reviewed changes

KevFan approved these changes May 30, 2024

View reviewed changes

guicassolato reviewed May 30, 2024

View reviewed changes

controllers/authpolicy_authconfig.go Outdated Show resolved Hide resolved

guicassolato reviewed May 30, 2024

View reviewed changes

make/development-environments.mk Outdated Show resolved Hide resolved

guicassolato reviewed May 30, 2024

View reviewed changes

make/integration-tests.mk Outdated Show resolved Hide resolved

eguzki added 9 commits May 30, 2024 17:08

limitador_cluster_envoyfilter_controller optional

9c7deed

rate_limiting_wasmplugin_controller optional

3ef0011

istio optional

ab5129d

The operator runs even when Istio or GatewayAPI are not present

locking istio deps on its own golang package. part 1

30093c4

locking istio deps on its own golang package. part 2

2428aac

istio tests. part 1

50105d2

istio tests. part 2

3a12362

pkg/istio/external_authorizer.go: fix kuadrant cr removal when istio …

bc0f3ac

…not installed

doc/development.md: update

e76cbfa

eguzki and others added 16 commits May 30, 2024 17:10

istio tests. part 3

8901b9b

istio tests. part 4

80a726d

doc/development.md: little change

5d127ec

kuadrant status reports on lack of at least one of the supported Gate…

123a1b4

…wayAPI providers

bring changes from main

3b9a9af

fix conflicts

7318b5e

fix unittests

e339d3a

fix controllers/kuadrant_controller_test.go name

581920a

Update .github/workflows/test.yaml

2d9c870

Co-authored-by: Kevin Fan <[email protected]>

Update .github/workflows/test.yaml

9809827

Co-authored-by: Kevin Fan <[email protected]>

Update .github/workflows/test.yaml

61cd88f

Co-authored-by: Kevin Fan <[email protected]>

moving istio registration logic back to controllers

95a6087

Update tests/gatewayapi/suite_test.go

cdc04d7

Co-authored-by: Guilherme Cassolato <[email protected]>

controllers/authpolicy_authconfig.go: revert unnecessary change

e3ccfe5

GATEWAYAPI_PROVIDER=istio in lowercase

ec7d131

bring change from 'Start subnet offset at 1 to avoid overlap with kin…

b97782d

…d cluster ips' CommitID: 3cd0fa9

eguzki force-pushed the istio-optional branch from 5812c99 to b97782d Compare May 30, 2024 15:13

eguzki mentioned this pull request May 30, 2024

istio-optional: iteration 2 #681

Merged

eguzki requested review from KevFan and guicassolato June 3, 2024 09:04

guicassolato reviewed Jun 3, 2024

View reviewed changes

integration tests: INTEGRATION_TESTS_EXTRA_ARGS makefile variable

29dd203

guicassolato approved these changes Jun 3, 2024

View reviewed changes

eguzki merged commit 833650b into main Jun 3, 2024
25 of 26 checks passed

eguzki deleted the istio-optional branch June 3, 2024 15:07

eguzki mentioned this pull request Jun 10, 2024

Kuadrant CR is not removed when unregisterExternalAuthorizer fails #697

Closed

didierofrivia mentioned this pull request Nov 18, 2024

Fix Mesh provider selection #183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have Istio optional #645

Have Istio optional #645

eguzki commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

guicassolato May 28, 2024

eguzki May 28, 2024

eguzki May 28, 2024

KevFan left a comment

KevFan May 30, 2024 •

edited

Loading

guicassolato May 30, 2024

eguzki May 30, 2024

guicassolato May 30, 2024

KevFan left a comment

eguzki commented May 30, 2024

guicassolato Jun 3, 2024

eguzki Jun 3, 2024

guicassolato Jun 3, 2024

eguzki Jun 3, 2024

guicassolato Jun 3, 2024

eguzki Jun 3, 2024

guicassolato Jun 3, 2024

guicassolato left a comment

Have Istio optional #645

Have Istio optional #645

Conversation

eguzki commented May 13, 2024 • edited Loading

What

Defined behavior

Verification Steps: run kuadrant in cluster without GatewayAPI provider

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

KevFan May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

eguzki commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guicassolato left a comment

Choose a reason for hiding this comment

eguzki commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

KevFan May 30, 2024 •

edited

Loading