Feat:Add Gateway Plugin Support #686

Gidi233 · 2024-09-02T13:36:16Z

What type of PR is this?
/kind api-change
/kind feature

What this PR does / why we need it:
Users can choose kuma and nginx in the configuration of rollout and fleet flagger.
Specifically, you can set the chart and value of the helm deployment gateway in the fleet configuration.
You can specify the host of Ingress (required) and the protocol used by Kuma (default is http) in the rollout configuration.
Ingress can automatically complete the deployment of canary through application.
Kuma needs to manually install Prometheus and mesh in the target cluster.

Which issue(s) this PR fixes:
Fixes #651

Does this PR introduce a user-facing change?:
For application:

    ......
      rollout:
        trafficRoutingProvider: nginx
        rolloutPolicy:
          trafficRouting:
            ......
            host: "app.example.com"
---
    ......
      rollout:
        trafficRoutingProvider: kuma
        rolloutPolicy:
          trafficRouting:
            ......
            protocol: http

For Fleet

  ......
  plugin:
    flagger:
      trafficRoutingProvider: nginx
      config:
        chart: 
        extraArgs:

netlify · 2024-09-02T13:36:30Z

✅ Deploy Preview for kurator-dev canceled.

Name	Link
🔨 Latest commit	`7a5c768`
🔍 Latest deploy log	https://app.netlify.com/sites/kurator-dev/deploys/66e39441602b280008b783d2

LiZhenCheng9527

Your PR is too large.
In fact, it can be split into multiple PRs such as API changes, fleet plugin installation implementation, feature implementation, etc.
It is convenient for development and review.

LiZhenCheng9527 · 2024-09-03T01:29:27Z

examples/rollout/ab-testingNginx.yaml

+            checkIntervalSeconds: 90
+            checkFailedTimes: 2
+            metrics:
+              - name: my-metric


The name my-metric is not accurate enough, so we suggest changing it to nginx-request-success-rate.

LiZhenCheng9527 · 2024-09-03T01:30:48Z

examples/rollout/blue_greenNginx.yaml

+            checkIntervalSeconds: 90
+            checkFailedTimes: 2
+            metrics:
+              - name: my-metric


LiZhenCheng9527 · 2024-09-03T01:31:02Z

examples/rollout/canaryKuma.yaml

+             checkIntervalSeconds: 90
+             checkFailedTimes: 2
+             metrics:
+             - name: my-metric


LiZhenCheng9527 · 2024-09-03T01:33:19Z

pkg/apis/apps/v1alpha1/types.go

+	//                   number: application.syncPolicies.rollout.port
+	// ```
+	// +optional
+	Host string `json:"host,omitempty"`


Your comments are confusing, what should I put in this field? Is it ingress.name?

I mean, you only need to fill in the host field as the host of the ingress. The other default items will be hard-coded in the code as configured above.

So the contents of this field are the contents of spec.rules.host in the above?

LiZhenCheng9527 · 2024-09-03T01:34:57Z

pkg/apis/apps/v1alpha1/types.go

+	// for kuma
+	// Defaults to http
+	// +optional
+	Protocol string `json:"protocol,omitempty"`


Is this field only used by kuma?

LiZhenCheng9527 · 2024-09-03T01:38:47Z

pkg/apis/fleet/v1alpha1/types.go

 	// PublicTestloader defines whether to install the publictestloader or not.
 	// In addition to the public testloader you can configure here,
 	// you can also specify a private testloader in the Application.Spec.SyncPolicies.Rollout.TestLoader
 	PublicTestloader bool `json:"publicTestloader,omitempty"`
+	// ProviderConfig defines the configuration for the TrafficRoutingProvider.
+	// +optional
+	ProviderConfig *Config `json:"config,omitempty"`


What can this field be configured for? Is it used to install TrafficRoutingProvider?
Please clarify in the comments

LiZhenCheng9527 · 2024-09-03T01:41:24Z

pkg/fleet-manager/application/rollout_helper.go

 			if err != nil {
-				return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s istio-injection enable", rolloutPolicy.Workload.Namespace)
+				return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s %s's Inject enable", rolloutPolicy.Workload.Namespace, provider)


Suggested change

return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s %s's Inject enable", rolloutPolicy.Workload.Namespace, provider)

return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s %s's sidecar inject enable", rolloutPolicy.Workload.Namespace, provider)

LiZhenCheng9527 · 2024-09-03T01:42:23Z

pkg/fleet-manager/application/rollout_helper.go

+		default:
+			return ctrl.Result{}, errors.Errorf("unknown provider type %s", provider)
+		}
+		log.Info("pre-operation of operating canary success")


LiZhenCheng9527 · 2024-09-03T01:45:49Z

pkg/fleet-manager/application/rollout_helper.go

@@ -270,6 +307,14 @@ func (a *ApplicationManager) deleteResourcesInMemberClusters(ctx context.Context
 		}
 		for _, cluster := range destinationClusters {
 			newClient := cluster.Client.CtrlRuntimeClient()
+
+			ns := &corev1.Namespace{}
+			if err := deleteResourceCreatedByKurator(ctx, namespacedName, newClient, ns); err != nil {


why delete ns? Are you sure all the resources in this ns were created by Kurator?

LiZhenCheng9527 · 2024-09-03T01:57:33Z

pkg/fleet-manager/application/rollout_helper.go

@@ -569,6 +710,23 @@ func addLabels(obj client.Object, key, value string) client.Object {
 	return obj
 }

+func addAnnotations(obj client.Object, keysAndValues ...string) client.Object {


Why not use map to store keysAndValues?

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

LiZhenCheng9527 · 2024-09-03T06:32:29Z

pkg/apis/apps/v1alpha1/types.go

+	//                   number: application.syncPolicies.rollout.port
+	// ```
+	// +optional
+	Host string `json:"host,omitempty"`


So the contents of this field are the contents of spec.rules.host in the above?

LiZhenCheng9527 · 2024-09-10T01:24:09Z

pkg/apis/apps/v1alpha1/types.go

@@ -239,6 +239,42 @@ type TrafficRoutingConfig struct {
 	// +optional
 	CorsPolicy *istiov1alpha3.CorsPolicy `json:"corsPolicy,omitempty"`

+	// Host defines the domain name you specify.


Is `Host: spec.rules.host' ?

LiZhenCheng9527 · 2024-09-10T01:34:58Z

pkg/fleet-manager/application/rollout_helper.go

+			labels := ingress.GetLabels()
+			if set := sets.New(strings.Split(labels[ingressLabelKey], ",")...); set.Contains(rollout.ServiceName) {
+				if set.Len() == 1 {
+					if deleteErr := kubeClient.Delete(ctx, ingress); deleteErr != nil && !apierrors.IsNotFound(deleteErr) {


Do you need to judge a NotFound Error here? There a branch ingress already in the cluster.

Okay, I'll delete it.

LiZhenCheng9527 · 2024-09-10T01:38:42Z

pkg/fleet-manager/application/rollout_helper.go

+			if set := sets.New(strings.Split(labels[ingressLabelKey], ",")...); set.Contains(rollout.ServiceName) {
+				if set.Len() == 1 {
+					if deleteErr := kubeClient.Delete(ctx, ingress); deleteErr != nil && !apierrors.IsNotFound(deleteErr) {
+						return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", namespaceName.Name, namespaceName.Namespace)


nit:

Suggested change

return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", namespaceName.Name, namespaceName.Namespace)

return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", rollout.Workload.Namespace, ingress.Name)

rollout.Workload.Namespace is the same as namespaceName.Name and I thought when an error occurs the ingress object will be null?
So I don't think it needs to be changed

LiZhenCheng9527 · 2024-09-10T01:38:56Z

pkg/fleet-manager/application/rollout_helper.go

+					labels[ingressLabelKey] = strings.Join(set.Delete(rollout.ServiceName).UnsortedList(), ",")
+					ingress.SetLabels(labels)
+					if err := kubeClient.Update(ctx, ingress); err != nil {
+						return errors.Wrapf(err, "failed to Update ingress %s in %s", namespaceName.Name, namespaceName.Namespace)


LiZhenCheng9527 · 2024-09-10T01:41:07Z

pkg/fleet-manager/application/rollout_helper.go

+					ingress.Spec.Rules = newRules
+					labels[ingressLabelKey] = strings.Join(set.Delete(rollout.ServiceName).UnsortedList(), ",")
+					ingress.SetLabels(labels)
+					if err := kubeClient.Update(ctx, ingress); err != nil {


Why put update and delete in one function?
Separate these two functions

LiZhenCheng9527 · 2024-09-10T01:44:03Z

pkg/fleet-manager/application/rollout_helper.go

+}
+
+// create/update ingress configuration
+func renderIngress(ingress *ingressv1.Ingress, rollout *applicationapi.RolloutConfig) {


Is this only used by nginx?
Maybe the function name can be replaced to renderNginxIngress

LiZhenCheng9527 · 2024-09-10T01:48:48Z

pkg/fleet-manager/application/rollout_helper.go

+		obj.SetAnnotations(ann)
+	} else {
+		for k, v := range ann {
+			annotations[k] = v


This may change the original annotations, which need to be evaluated to see if there is any .

LiZhenCheng9527 · 2024-09-10T01:50:39Z

almost LGTM

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

LiZhenCheng9527 · 2024-09-11T07:58:03Z

/lgtm

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

LiZhenCheng9527 · 2024-09-13T06:31:43Z

Please fix lint error

LiZhenCheng9527 · 2024-09-18T03:24:49Z

/cc @hzxuzhonghu This pr is ready to merge

kurator-bot · 2024-09-18T03:24:55Z

@LiZhenCheng9527: GitHub didn't allow me to request PR reviews from the following users: is, ready, to, merge, This, pr.

Note that only kurator-dev members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @hzxuzhonghu This pr is ready to merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hzxuzhonghu

Thanks

kurator-bot · 2024-09-18T09:17:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hzxuzhonghu · 2024-09-18T09:17:42Z

/lgtm

kurator-bot added the kind/api-change label Sep 2, 2024

kurator-bot requested a review from hzxuzhonghu September 2, 2024 13:36

kurator-bot added the kind/feature label Sep 2, 2024

kurator-bot requested a review from zirain September 2, 2024 13:36

kurator-bot added the size/XXL label Sep 2, 2024

Gidi233 force-pushed the dev1 branch 2 times, most recently from 662b895 to f561601 Compare September 2, 2024 14:21

LiZhenCheng9527 reviewed Sep 3, 2024

View reviewed changes

Gidi233 requested a review from LiZhenCheng9527 September 3, 2024 04:14

Gidi233 force-pushed the dev1 branch from aa8f80e to d9f17c5 Compare September 3, 2024 04:29

Gidi233 added 8 commits September 9, 2024 15:17

Install the Gateway plugin in fleet

68536a4

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Support the Gateway plugin in application

9365d8d

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Add example

a5cd58a

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Add gen

54f70cc

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

fix warning and add log

2897a44

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Change comments and redundant logic

f59ff6d

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Change example

efbb89f

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

add gen

0a31983

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Gidi233 force-pushed the dev1 branch from c52caad to 0a31983 Compare September 9, 2024 07:37

LiZhenCheng9527 reviewed Sep 10, 2024

View reviewed changes

Gidi233 force-pushed the dev1 branch 2 times, most recently from f8dfd2b to 0cfb528 Compare September 11, 2024 02:27

fix

a7a9cf9

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Gidi233 force-pushed the dev1 branch from 0cfb528 to a7a9cf9 Compare September 11, 2024 02:59

kurator-bot assigned LiZhenCheng9527 Sep 11, 2024

kurator-bot added lgtm and removed lgtm labels Sep 11, 2024

fix example

7a5c768

Signed-off-by: Gidi233 <qpbtyfh@gmail.com>

Gidi233 force-pushed the dev1 branch from c8efc8e to 7a5c768 Compare September 13, 2024 01:24

hzxuzhonghu approved these changes Sep 18, 2024

View reviewed changes

kurator-bot added the approved label Sep 18, 2024

kurator-bot assigned hzxuzhonghu Sep 18, 2024

kurator-bot added the lgtm label Sep 18, 2024

kurator-bot merged commit edd2ba6 into kurator-dev:main Sep 18, 2024
14 checks passed

	return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s %s's Inject enable", rolloutPolicy.Workload.Namespace, provider)
	return ctrl.Result{}, errors.Wrapf(err, "failed to set namespace %s %s's sidecar inject enable", rolloutPolicy.Workload.Namespace, provider)

	return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", namespaceName.Name, namespaceName.Namespace)
	return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", rollout.Workload.Namespace, ingress.Name)

Feat:Add Gateway Plugin Support #686

Feat:Add Gateway Plugin Support #686

Conversation

Gidi233 commented Sep 2, 2024

netlify bot commented Sep 2, 2024 • edited Loading

✅ Deploy Preview for kurator-dev canceled.

LiZhenCheng9527 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

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

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

LiZhenCheng9527 commented Sep 10, 2024

LiZhenCheng9527 commented Sep 11, 2024

LiZhenCheng9527 commented Sep 13, 2024

LiZhenCheng9527 commented Sep 18, 2024

kurator-bot commented Sep 18, 2024

hzxuzhonghu left a comment

Choose a reason for hiding this comment

kurator-bot commented Sep 18, 2024

hzxuzhonghu commented Sep 18, 2024

netlify bot commented Sep 2, 2024 •

edited

Loading