Istio ext authz service config #17

eguzki · 2022-05-11T09:01:51Z

what

Authorino set up in Istio as a Auth GRPC service

add Gateway API CRDs dependency
istio install makefile targets using the operator
Upgrade installed Istio to 1.13.2. Isito 1.12.X has an issue.
Implemented in the controller:
- setup logger: support for log levels (debug → info → error) and configurable log modes: production, development
- status conditions: Ready condition
- reconcile Istio mesh configuration for the kuadrant external authorizer. When the kuadrant CR is deleted, the configuration is cleaned up. Currently, the external authorizer address is hardcoded to

- envoyExtAuthzGrpc:
    port: 50051
    service: authorino-authorino-authorization.kuadrant-system.svc.cluster.local
  name: kuadrant-authorization

The name/NS of the IstioOperator CR used to configure external authorizer is read from en var

	Environemnt Variable Name	Default Value
IstioOperator name	`ISTIOOPERATOR_NAME`	`istiocontrolplane`
IstioOperator namespace	`ISTIOOPERATOR_NAMESPACE`	`istio-system`

verification steps

create kuadrant cluster locally with kind

make kind-create-kuadrant-cluster

Check istio config does not have any kuadrant's external authorizer. For that, the istiocontrolplane named IstioOperator CR is checked:

k get istiooperator istiocontrolplane -n istio-system -o jsonpath='{.spec.meshConfig.extensionProviders}'

The result should be empty.

Create empty Kuadrant CR object

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

Check istio config does have kuadrant¡s external authorizer. For that, the istiocontrolplane named IstioOperator CR is checked:

k get istiooperator istiocontrolplane -n istio-system -o jsonpath='{.spec.meshConfig.extensionProviders}'
- envoyExtAuthzGrpc:
    port: 50051
    service: authorino-authorino-authorization.kuadrant-system.svc.cluster.local
  name: kuadrant-authorization

Delete the kuadrant CR

k delete kuadrant kuadrant-sample

Check istio config does no longer have any kuadrant's external authorizer. For that, the istiocontrolplane named IstioOperator CR is checked:

k get istiooperator istiocontrolplane -n istio-system -o jsonpath='{.spec.meshConfig.extensionProviders}'
[]

The result should be empty (or empty array)

Currently Istio does not provide them

Fixes issue istio/istio#36114 (comment) preventing the operator to successfully reconcile changes in the IstioOperator custom resource

eguzki · 2022-05-18T07:18:43Z

ready for review @Kuadrant/engineering

rahulanand16nov · 2022-05-18T09:40:11Z

pkg/log/log.go

+	// Log is the base logger
+	Log logr.Logger = ctrllog.NullLogger{}
+)


I don't think we require an upgrade here, right?

we will when upgrading logr to 1.X

rahulanand16nov

LGTM!

controllers/kuadrant_controller.go

guicassolato · 2022-05-18T11:08:53Z

controllers/kuadrant_controller.go

+	//          service: AUTHORINO SERVICE
+	//        name: kuadrant-authorization
+
+	extensionProvidersObj, ok := iop.Spec.MeshConfig["extensionProviders"]


My previous comment actually applies better here.

guicassolato · 2022-05-18T11:18:06Z

controllers/kuadrant_status.go

+	logger := logr.FromContext(ctx)
+	newStatus := r.calculateStatus(kObj, specErr)
+
+	equalStatus := kObj.Status.Equals(newStatus, logger)


I suppose this is to prevent unnecessary updates on the status, where a minimum spottable diff would be the value of LastTransitionTime of a particular condition, is that right?

Is it worth the cost of checking while:

leader election is enabled and therefore only one replica at a time will reconcile the resource?

status update is an idempotent operation?

Please, no need to change anything. I'm just trying to understand the flow.

To prevent unnecessary updates on the status, specifically related to the LastTransitionTime field, conditions are first copied from the existing CR and then desired values applied. After that, they are compared. The status is only updated when there are changes.

leader election is enabled and therefore only one replica at a time will reconcile the resource?

I do not fully understand this point. The controller does not expect anyone to change the status. But even if it changes, the controller will set its desired state.

status update is an idempotent operation?

Yes it is. Identical status requests should always turn into a identical status objects in the CR.

leader election is enabled and therefore only one replica at a time will reconcile the resource?

I do not fully understand this point. The controller does not expect anyone to change the status. But even if it changes, the controller will set its desired state.

When leader election is enabled in a manager, no matter how many replicas of the operator are running, only the leader will perform the reconciliation tasks under this manager; therefore, also the status update. IOW, I think there's no concurrency to worry about. (Unless reconcilers are invoked in concurrent goroutines. Are they? 🤔)

status update is an idempotent operation?

Yes it is.

So why carrying checking for changes in the subresource? Because sorting the array and marshalling 2 JSON objects cost less than sending the update request? If the update happens in the background (because we don't have to wait until it finishes to return from reconciling, right?), wouldn't it be better to just trust idempotence and skip the check?

This procedure does not try to prevent race conditions. The race conditions and updating stale data is handled by the API server. When you update the status (same applies to the spec), the operation update makes sure the update is made from the latest available data, otherwise you get Conflict error. If there is a race condition, one of the updates would fail and the controller needs to retry reading first the latest object data.

This procedure tries to follow the controller pattern: get existing, get desired. If existing does not exit, create desired. If existing exists, compare to desired, if there are changes, update.

So why carrying checking for changes in the subresource? Because sorting the array and marshalling 2 JSON objects cost less than sending the update request? If the update happens in the background (because we don't have to wait until it finishes to return from reconciling, right?), wouldn't it be better to just trust idempotence and skip the check?

Not very concerned about the cost, TBH. IMO, one update request has much more cost (I would say several orders of magnitude) than serializing in memory struct objects which are not expected to be "big".

It is more about my instinct of doing the right thing: I do not update if there is nothing to update. That has a cost of checking, indeed. Worth doing it most of the times. This controller is now very simple, but it could be watching many other types and get events from those types, for instance the IstioOperator. So I could expect many reconciliation loops to happen. I think it is wise to save updates and do not update all the time. Open to other approaches, though ;)

In Authorino and in Authorino Operator we went with a different approach, where we trust idempotence of the status update operation and leave the rest to the API server.

But it's your call.

one update request has much more cost (I would say several orders of magnitude) than serializing in memory struct objects which are not expected to be "big"

Maybe I wasn't clear before. You don't have to wait for the state update to finish. You can fork a goroutine to handle the update and return immediately. So, from the perspective of freeing the reconciler/closing the connection, performing the update without the check is theoretically faster.

On the other hand, I totally understand if you don't want to handle this in the background (it could fail!)

And, as you said, sorting two arrays of 5-6 elements each + serializing two small structs as JSON, we're talking nanoseconds might not worth saving.

Anyway, the PR LGTM. As I said before, I was just trying to understand the flow. Not suggesting changing anything.

controllers/kuadrant_status.go

guicassolato · 2022-05-18T11:26:53Z

main.go

+	setupLog.Info(fmt.Sprintf("go version: %s", runtime.Version()))
+	setupLog.Info(fmt.Sprintf("go os/arch: %s/%s", runtime.GOOS, runtime.GOARCH))


Nice touch! I might steel this idea for kuadrant/authorino now that we're building for multi-arch. 🙂

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

* Fix API type to include security requirements * Generate manifests after the API type update

eguzki added 10 commits May 9, 2022 15:55

add Gateway API CRDs dependency

3391e14

Currently Istio does not provide them

fix istioctl target

ded3e28

istio install using the istio operator

39de362

upgrade to istio 1.13.2

c35aaf2

Fixes issue istio/istio#36114 (comment) preventing the operator to successfully reconcile changes in the IstioOperator custom resource

kuadrant CRD emtpy spec

9a3d9d5

update bundles

94dce02

setup kuadrant logger

d28d2cf

controller: ready status condition

c33c6a7

controller: reconcile external authz provider

8e49cce

fix istiooperator rbac access permissions

a602ab4

eguzki marked this pull request as ready for review May 11, 2022 09:48

fix integration tests

0bf8c01

eguzki requested review from mikenairn and a team May 11, 2022 09:50

rahulanand16nov reviewed May 18, 2022

View reviewed changes

rahulanand16nov approved these changes May 18, 2022

View reviewed changes

guicassolato reviewed May 18, 2022

View reviewed changes

controllers/kuadrant_controller.go Show resolved Hide resolved

guicassolato reviewed May 18, 2022

View reviewed changes

controllers/kuadrant_status.go Outdated Show resolved Hide resolved

guicassolato reviewed May 18, 2022

View reviewed changes

controllers/kuadrant_status.go Show resolved Hide resolved

guicassolato reviewed May 18, 2022

View reviewed changes

eguzki and others added 2 commits May 18, 2022 15:58

Update controllers/kuadrant_status.go

6ea6a97

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

protect when istiooperator spec nil

65ff8d8

guicassolato approved these changes May 18, 2022

View reviewed changes

eguzki merged commit 4cb9b00 into main May 19, 2022

eguzki deleted the istio-ext-authz-service-config branch May 19, 2022 07:52

didierofrivia pushed a commit that referenced this pull request Sep 26, 2022

Fix api spec (#17)

9ec47cc

* Fix API type to include security requirements * Generate manifests after the API type update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Istio ext authz service config #17

Istio ext authz service config #17

eguzki commented May 11, 2022 •

edited

Loading

eguzki commented May 18, 2022

rahulanand16nov May 18, 2022

eguzki May 18, 2022

rahulanand16nov left a comment

guicassolato May 18, 2022

eguzki May 18, 2022

guicassolato May 18, 2022

eguzki May 18, 2022

guicassolato May 18, 2022

eguzki May 18, 2022

guicassolato May 18, 2022

guicassolato May 18, 2022

eguzki May 18, 2022

		setupLog.Info(fmt.Sprintf("go version: %s", runtime.Version()))
		setupLog.Info(fmt.Sprintf("go os/arch: %s/%s", runtime.GOOS, runtime.GOARCH))

Istio ext authz service config #17

Istio ext authz service config #17

Conversation

eguzki commented May 11, 2022 • edited Loading

what

verification steps

eguzki commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulanand16nov left a 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

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

eguzki commented May 11, 2022 •

edited

Loading