feat(ctrl): implements MeshFederation reconciliation #170

bartoszmajsak · 2025-01-27T06:33:43Z

This commit introduces the initial implementation of the MeshFederation
controller. The controller is responsible for:

Managing the MeshFederation server lifecycle (Start serving FDS #152)
Configuring MeshFederation resources, including:
- Ingress Gateway
- PeerAuthentication
- EnvoyFilter (for OpenShift Router)
- Routes (for OpenShift Router)
Watching Kubernetes Services to:
- Push SotW updates to all connected peers (Broadcast exported services #153)
- Update MeshFederation cluster configuration

Support both label selectors and expressions (#52 #143) as export rules.

Basic EnvTest tests are included to verify the setup (happy paths for both
istio and openshift-router ingress types).

Introducing MeshFederation.Status.ExportedServices to keep track of
exported services in each reconcile.

Current limitations / random thoughts

There are loose ends I identified while implementing it. Most are in TODOs,
but I thought keeping them in the PR description helps with getting initial context.

not tested for multiple MeshFederation instances
- we currently create one FDS server per federation, but we have a single port for
  all which is hardcoded (:15080), some ideas:
  - share server across federations
    - ❓ is that a bottleneck?
    - we are lacking a way to identify to which federation a remote belongs to,
      which makes it impossible to e.g. push SotW to only relevant peers on a given
      MeshFederation reconcile. Perhaps expanding FDS protocol can solve it.
  - alternatively we can make the port customizable, but that would mean configuring
    all remotes in a single federation instead of relying on the defaults
- defaulting root ns (istio-system) can cause issues when we create mesh-wide resources)
not ensuring one-per-ns MeshFederation
- should be done through admission webhook if that becomes a requirement

Fixes #152
Fixes #52
Fixes #143
Fixes #153

bartoszmajsak · 2025-01-27T06:35:19Z

This PR contains two other, previously stacked PR:

feat(ctrl): predicate on finalizer change #165
feat(testing): introduces k8s envtest skeleton #167 (hence the size - see comments in this PR)

This commit introduces the initial implementation of the MeshFederation controller. The controller is responsible for: - Managing the MeshFederation server lifecycle (openshift-service-mesh#152) - removes a need for a channel to trigger push - pushing directly instead - Configuring MeshFederation resources, including: - IngressGateway - PeerAuthentication - EnvoyFilter (for OpenShift Router) - Routes (for OpenShift Router) - Watching Kubernetes services to: - Push SotW updates to all connected peers (openshift-service-mesh#153) - Update MeshFederation cluster configuration - Support both label selectors and expressions (openshift-service-mesh#52 openshift-service-mesh#143) Basic EnvTest tests are included to verify the setup. Fixes openshift-service-mesh#152 openshift-service-mesh#52 openshift-service-mesh#143 openshift-service-mesh#153

eoinfennessy

Initial pass with a few comments/suggestions. I'll continue the review later.

internal/controller/meshfederation/meshfederation_controller.go

internal/pkg/discovery/grpc_server.go

internal/controller/meshfederation/meshfederation_controller.go

There is no point in stopping reconcile - we want to continue with bringing the cluster to the desired state. We should: - report on status - raise event

adds retry logic with delay

bartoszmajsak · 2025-01-27T21:14:22Z

Lets not ok-to-merge yet, ideally we should combine it with #168, as there were API changes among other things.

bartoszmajsak · 2025-01-28T06:43:06Z

internal/controller/meshfederation/meshfederation_controller.go

+		Owns(&routev1.Route{}).
+		Watches(
+			// TODO(design): initial reconcile will trigger a lot of requests - one for each service. This can become expensive.
+			&corev1.Service{},


FOLLOW-UP

We don't need to watch the corev1.Service object, we can limit it only to what we are interested in at this point - labels. For this purpose watch on PartialObjectMetadata is sufficient. This will also reduce the payload send from k8s api-server.

@eoinfennessy I think you can also think about it for the zones as you rely on namespace only IIRC. Thing I don't know yet is if that's the same cache as regular objects.

Thank you! I actually wrote a TODO for this very thing! :) https://github.com/openshift-service-mesh/istio-zones/blob/main/internal/controller/zone_controller.go#L256-L257

Yeah I saw that the other day and woke up with PartialObjectMetadata thing in my head so I just dumped it here before first coffee :)

eoinfennessy

Looks great! I just added a few small nits/suggestions.

eoinfennessy · 2025-01-28T11:01:47Z

internal/controller/meshfederation/meshfederation_controller.go

+	return accResult, errors.Join(errs...)
+}
+
+func (r *Reconciler) handleServiceToExport(ctx context.Context, object client.Object) []reconcile.Request {


Nit: I think the name could be a little clearer. Something like this:

Suggested change

func (r *Reconciler) handleServiceToExport(ctx context.Context, object client.Object) []reconcile.Request {

func (r *Reconciler) mapServiceToReconcileRequests(ctx context.Context, object client.Object) []reconcile.Request {

eoinfennessy · 2025-01-28T11:04:04Z

internal/controller/meshfederation/meshfederation_controller.go

+	meshFederations := &v1alpha1.MeshFederationList{}
+	// TODO paginate? options?
+	if errList := r.Client.List(ctx, meshFederations); errList != nil {
+		logger.Error(errList, "failed mapping Service to MeshFederations", "service", object.GetName()+"/"+object.GetNamespace())


Suggested change

logger.Error(errList, "failed mapping Service to MeshFederations", "service", object.GetName()+"/"+object.GetNamespace())

logger.Error(errList, "failed to list MeshFederations when mapping Service to reconcile requests", "service", object.GetName()+"/"+object.GetNamespace())

eoinfennessy · 2025-01-28T11:26:22Z

internal/controller/meshfederation/reconcilers.go

+	createGateway := func() objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway] {
+		result := &v1alpha3.Gateway{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "federation-ingress-gateway",
+				Namespace: meshFederation.Spec.ControlPlaneNamespace,
+				Labels:    map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},
+			},
+		}
+
+		result.Spec = desiredSpec()
+
+		return objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway]{
+			Obj: result,
+			DesiredSpec: func() istionetv1alpha3.Gateway {
+				return desiredSpec()
+			},
+		}
+	}


Nit

Suggested change

createGateway := func() objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway] {

result := &v1alpha3.Gateway{

ObjectMeta: metav1.ObjectMeta{

Name: "federation-ingress-gateway",

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},

},

}

result.Spec = desiredSpec()

return objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway]{

Obj: result,

DesiredSpec: func() istionetv1alpha3.Gateway {

return desiredSpec()

},

}

}

createGateway := func() objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway] {

result := &v1alpha3.Gateway{

ObjectMeta: metav1.ObjectMeta{

Name: "federation-ingress-gateway",

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},

},

Spec: desiredSpec(),

}

return objReconciler[*v1alpha3.Gateway, istionetv1alpha3.Gateway]{

Obj: result,

DesiredSpec: func() istionetv1alpha3.Gateway {

return desiredSpec()

},

}

}

eoinfennessy · 2025-01-28T11:43:40Z

internal/controller/meshfederation/reconcilers.go

+	envoyFilter := func(svcName, svcNamespace string, port int32) objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter] {
+		result := &v1alpha3.EnvoyFilter{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("sni-%s-%s-%d", svcName, svcNamespace, port),
+				Namespace: meshFederation.Spec.ControlPlaneNamespace,
+				Labels: map[string]string{
+					"federation.openshift-service-mesh.io/peer": "todo",
+				},
+			},
+		}
+
+		result.Spec = desiredSpec(svcName, svcNamespace, port)
+
+		return objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter]{
+			Obj: result,
+			DesiredSpec: func() istionetv1alpha3.EnvoyFilter {
+				return desiredSpec(svcName, svcNamespace, port)
+			},
+		}
+	}


Nit

Suggested change

envoyFilter := func(svcName, svcNamespace string, port int32) objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter] {

result := &v1alpha3.EnvoyFilter{

ObjectMeta: metav1.ObjectMeta{

Name: fmt.Sprintf("sni-%s-%s-%d", svcName, svcNamespace, port),

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{

"federation.openshift-service-mesh.io/peer": "todo",

},

},

}

result.Spec = desiredSpec(svcName, svcNamespace, port)

return objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter]{

Obj: result,

DesiredSpec: func() istionetv1alpha3.EnvoyFilter {

return desiredSpec(svcName, svcNamespace, port)

},

}

}

envoyFilter := func(svcName, svcNamespace string, port int32) objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter] {

result := &v1alpha3.EnvoyFilter{

ObjectMeta: metav1.ObjectMeta{

Name: fmt.Sprintf("sni-%s-%s-%d", svcName, svcNamespace, port),

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{

"federation.openshift-service-mesh.io/peer": "todo",

},

},

Spec: desiredSpec(svcName, svcNamespace, port),

}

return objReconciler[*v1alpha3.EnvoyFilter, istionetv1alpha3.EnvoyFilter]{

Obj: result,

DesiredSpec: func() istionetv1alpha3.EnvoyFilter {

return desiredSpec(svcName, svcNamespace, port)

},

}

}

eoinfennessy · 2025-01-28T11:45:30Z

internal/controller/meshfederation/reconcilers.go

+	createRoute := func(svcName, svcNamespace string, port int32) objReconciler[*routev1.Route, routev1.RouteSpec] {
+		result := &routev1.Route{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("%s-%s-%d-to-federation-ingress-gateway", svcName, svcNamespace, port),
+				Namespace: meshFederation.Spec.ControlPlaneNamespace,
+				Labels:    map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},
+			},
+		}
+
+		result.Spec = desiredSpec(svcName, svcNamespace, port)
+
+		return objReconciler[*routev1.Route, routev1.RouteSpec]{
+			Obj: result,
+			DesiredSpec: func() routev1.RouteSpec {
+				return desiredSpec(svcName, svcNamespace, port)
+			},
+		}
+	}


Nit

Suggested change

createRoute := func(svcName, svcNamespace string, port int32) objReconciler[*routev1.Route, routev1.RouteSpec] {

result := &routev1.Route{

ObjectMeta: metav1.ObjectMeta{

Name: fmt.Sprintf("%s-%s-%d-to-federation-ingress-gateway", svcName, svcNamespace, port),

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},

},

}

result.Spec = desiredSpec(svcName, svcNamespace, port)

return objReconciler[*routev1.Route, routev1.RouteSpec]{

Obj: result,

DesiredSpec: func() routev1.RouteSpec {

return desiredSpec(svcName, svcNamespace, port)

},

}

}

createRoute := func(svcName, svcNamespace string, port int32) objReconciler[*routev1.Route, routev1.RouteSpec] {

result := &routev1.Route{

ObjectMeta: metav1.ObjectMeta{

Name: fmt.Sprintf("%s-%s-%d-to-federation-ingress-gateway", svcName, svcNamespace, port),

Namespace: meshFederation.Spec.ControlPlaneNamespace,

Labels: map[string]string{"federation.openshift-service-mesh.io/peer": "todo"},

},

Spec: desiredSpec(svcName, svcNamespace, port),

}

return objReconciler[*routev1.Route, routev1.RouteSpec]{

Obj: result,

DesiredSpec: func() routev1.RouteSpec {

return desiredSpec(svcName, svcNamespace, port)

},

}

}

eoinfennessy · 2025-01-28T11:54:27Z

internal/pkg/discovery/adss_handler.go

+	resources := pushRequest.Resources
+	if resources == nil {
+		var err error
+		resources, err = adss.generateResources(pushRequest.TypeUrl)
+		if err != nil {
+			return err
+		}
+	}


Readability suggestion avoiding variable reassignment:

Suggested change

resources := pushRequest.Resources

if resources == nil {

var err error

resources, err = adss.generateResources(pushRequest.TypeUrl)

if err != nil {

return err

}

}

var resources []*anypb.Any

if pushRequest.Resources == nil {

var err error

resources, err = adss.generateResources(pushRequest.TypeUrl)

if err != nil {

return err

}

} else {

resources = pushRequest.Resources

}

eoinfennessy · 2025-01-28T14:27:27Z

internal/controller/meshfederation/meshfederation_controller.go

+			// TODO(design): do we want to keep all the services? seems convenient, but shouldn't we be concerned about the size?
+			saved.Status.ExportedServices = meshFederation.Status.ExportedServices


FOLLOW-UP

@jewertow @bartoszmajsak

It may be possible to minimise the number of full SotW pushes by comparing the old list of services to the new list of services and only pushing if the list has changed. This could be achieved by storing a hash of the services in the MeshFederation's status (status.lastPushHash: 1234abcd) and only pushing if this hash changes.

bartoszmajsak requested review from eoinfennessy and jewertow January 27, 2025 06:33

openshift-ci bot added the size/XXL label Jan 27, 2025

bartoszmajsak force-pushed the mesh-federation-controller branch from e0d1ba7 to 326005f Compare January 27, 2025 06:35

bartoszmajsak changed the title ~~feat(ctrl): implement MeshFederation reconciliation~~ feat(ctrl): implements MeshFederation reconciliation Jan 27, 2025

bartoszmajsak force-pushed the mesh-federation-controller branch 7 times, most recently from dd49df9 to 204092f Compare January 27, 2025 08:20

jewertow approved these changes Jan 27, 2025

View reviewed changes

bartoszmajsak force-pushed the mesh-federation-controller branch from 204092f to 3fba4e1 Compare January 27, 2025 14:08

eoinfennessy reviewed Jan 27, 2025

View reviewed changes

bartoszmajsak added 11 commits January 27, 2025 15:29

chore: makes finalizerHandler part of reconcile struct

47d00a5

chore: moves slice assignment outside of conditions loop

78723aa

chore: logs error on selector error, does not reconcile

4cb86dc

chore: corrects comment

027ff1a

chore: removes reduntant impl assertion

129283d

fix: weird auto-merge

38d5af6

fix: do not requeue on push-related errors

46eb4e6

There is no point in stopping reconcile - we want to continue with bringing the cluster to the desired state. We should: - report on status - raise event

chore: minor todos

d9e5048

chore: rename

6cb2f96

chore: narrows service watch with generation or label change predicates

e635d1f

chore: renames to server.StartOnce

8655a60

adds retry logic with delay

bartoszmajsak added the do-not-merge/work-in-progress label Jan 27, 2025

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 27, 2025

bartoszmajsak commented Jan 28, 2025

View reviewed changes

eoinfennessy approved these changes Jan 28, 2025

View reviewed changes

eoinfennessy reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ctrl): implements MeshFederation reconciliation #170

feat(ctrl): implements MeshFederation reconciliation #170

bartoszmajsak commented Jan 27, 2025 •

edited

Loading

bartoszmajsak commented Jan 27, 2025 •

edited

Loading

eoinfennessy left a comment

bartoszmajsak commented Jan 27, 2025 •

edited

Loading

bartoszmajsak Jan 28, 2025

eoinfennessy Jan 28, 2025

bartoszmajsak Jan 28, 2025

eoinfennessy left a comment

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

eoinfennessy Jan 28, 2025

	func (r *Reconciler) handleServiceToExport(ctx context.Context, object client.Object) []reconcile.Request {
	func (r *Reconciler) mapServiceToReconcileRequests(ctx context.Context, object client.Object) []reconcile.Request {

	logger.Error(errList, "failed mapping Service to MeshFederations", "service", object.GetName()+"/"+object.GetNamespace())
	logger.Error(errList, "failed to list MeshFederations when mapping Service to reconcile requests", "service", object.GetName()+"/"+object.GetNamespace())

		// TODO(design): do we want to keep all the services? seems convenient, but shouldn't we be concerned about the size?
		saved.Status.ExportedServices = meshFederation.Status.ExportedServices

feat(ctrl): implements MeshFederation reconciliation #170

Are you sure you want to change the base?

feat(ctrl): implements MeshFederation reconciliation #170

Conversation

bartoszmajsak commented Jan 27, 2025 • edited Loading

Current limitations / random thoughts

bartoszmajsak commented Jan 27, 2025 • edited Loading

eoinfennessy left a comment

Choose a reason for hiding this comment

bartoszmajsak commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

FOLLOW-UP

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoinfennessy 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

FOLLOW-UP

bartoszmajsak commented Jan 27, 2025 •

edited

Loading

bartoszmajsak commented Jan 27, 2025 •

edited

Loading

bartoszmajsak commented Jan 27, 2025 •

edited

Loading