Skip to content

Commit

Permalink
Update unit/e2e test, sort map keys and minor fixes
Browse files Browse the repository at this point in the history 
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
  • Loading branch information
@Rizwana777 @jgwest
Rizwana777 authored and jgwest committed Oct 16, 2024
1 parent 223293d commit 6bfa67b
Show file tree
Hide file tree
Showing 7 changed files with 223 additions and 173 deletions.
4 changes: 3 additions & 1 deletion api/v1alpha1/argorollouts_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,10 @@ type Plugin struct {
}

type Plugins struct {
// TrafficManagement holds a list of traffic management plugins used to control traffic routing during rollouts.
TrafficManagement []Plugin `json:"trafficManagement,omitempty"`
Metric []Plugin `json:"metric,omitempty"`
// Metric holds a list of metric plugins used to gather and report metrics during rollouts.
Metric []Plugin `json:"metric,omitempty"`
}

// ArgoRolloutsNodePlacementSpec is used to specify NodeSelector and Tolerations for Rollouts workloads
Expand Down
6 changes: 6 additions & 0 deletions config/crd/bases/argoproj.io_rolloutmanagers.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,12 @@ spec:
type: array
type: object
plugins:
description: Plugins specify the traffic and metric plugins in Argo
Rollout
properties:
metric:
description: Metric holds a list of metric plugins used to gather
and report metrics during rollouts.
items:
properties:
location:
Expand All @@ -303,6 +307,8 @@ spec:
type: object
type: array
trafficManagement:
description: TrafficManagement holds a list of traffic management
plugins used to control traffic routing during rollouts.
items:
properties:
location:
Expand Down
46 changes: 34 additions & 12 deletions controllers/configmap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package rollouts

import (
"context"
"sort"

"fmt"
"reflect"
Expand All @@ -12,6 +13,7 @@ import (
corev1 "k8s.io/api/core/v1"

"k8s.io/apimachinery/pkg/api/errors"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -62,10 +64,17 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro
}
}

// Convert traffic plugins map to slice
// Sort trafficRouterPluginsMap keys for deterministic ordering
trafficRouterPluginKeys := make([]string, 0, len(trafficRouterPluginsMap))
for key := range trafficRouterPluginsMap {
trafficRouterPluginKeys = append(trafficRouterPluginKeys, key)
}
sort.Strings(trafficRouterPluginKeys)

// Convert trafficRouterPluginsMap to sorted slice
trafficRouterPlugins := make([]pluginItem, 0, len(trafficRouterPluginsMap))
for _, plugin := range trafficRouterPluginsMap {
trafficRouterPlugins = append(trafficRouterPlugins, plugin)
for _, key := range trafficRouterPluginKeys {
trafficRouterPlugins = append(trafficRouterPlugins, trafficRouterPluginsMap[key])
}

// Append metric plugins specified in RolloutManager CR
Expand All @@ -81,10 +90,17 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro
}
}

// Convert metric plugins map to slice
// Sort metricPluginsMap keys for deterministic ordering
metricPluginKeys := make([]string, 0, len(metricPluginsMap))
for key := range metricPluginsMap {
metricPluginKeys = append(metricPluginKeys, key)
}
sort.Strings(metricPluginKeys)

// Convert metricPluginsMap to sorted slice
metricPlugins := make([]pluginItem, 0, len(metricPluginsMap))
for _, plugin := range metricPluginsMap {
metricPlugins = append(metricPlugins, plugin)
for _, key := range metricPluginKeys {
metricPlugins = append(metricPlugins, metricPluginsMap[key])
}

desiredTrafficRouterPluginString, err := yaml.Marshal(trafficRouterPlugins)
Expand Down Expand Up @@ -149,6 +165,10 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro
func (r *RolloutManagerReconciler) restartRolloutsPod(ctx context.Context, namespace string) error {
deployment := &appsv1.Deployment{}
if err := r.Client.Get(ctx, types.NamespacedName{Name: DefaultArgoRolloutsResourceName, Namespace: namespace}, deployment); err != nil {
if errors.IsNotFound(err) {
// If Deployment isn't found, return nil as there is no child pod to restart
return nil
}
return fmt.Errorf("failed to get deployment: %w", err)
}

Expand All @@ -164,14 +184,16 @@ func (r *RolloutManagerReconciler) restartRolloutsPod(ctx context.Context, names
for i := range podList.Items {
pod := podList.Items[i]
log.Info("Deleting Rollouts Pod", "podName", pod.Name)
if err := r.Client.Delete(ctx, &pod); err != nil {
if errors.IsNotFound(err) {
log.Info(fmt.Sprintf("Pod %s already deleted", pod.Name))
continue
if pod.ObjectMeta.DeletionTimestamp == nil {
if err := r.Client.Delete(ctx, &pod); err != nil {
if errors.IsNotFound(err) {
log.Info(fmt.Sprintf("Pod %s already deleted", pod.Name))
continue
}
return fmt.Errorf("failed to delete Rollouts Pod %s: %w", pod.Name, err)
}
return fmt.Errorf("failed to delete Rollouts Pod %s: %w", pod.Name, err)
log.Info("Rollouts Pod deleted successfully", "podName", pod.Name)
}
log.Info("Rollouts Pod deleted successfully", "podName", pod.Name)
}

return nil
Expand Down
144 changes: 66 additions & 78 deletions controllers/configmap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ import (
"github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"sigs.k8s.io/controller-runtime/pkg/client"

appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
Expand All @@ -15,6 +19,7 @@ var _ = Describe("ConfigMap Test", func() {
var a v1alpha1.RolloutManager
var r *RolloutManagerReconciler
var sa *corev1.ServiceAccount
var existingDeployment *v1.Deployment
const trafficrouterPluginLocation = "https://custom-traffic-plugin-location"
const metricPluginLocation = "https://custom-metric-plugin-location"

Expand All @@ -33,7 +38,7 @@ var _ = Describe("ConfigMap Test", func() {
}
Expect(r.Client.Create(ctx, sa)).To(Succeed())

existingDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a)
existingDeployment = deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin-test", "tmp-test"}, "linux-test", sa.Name, a)
Expect(r.Client.Create(ctx, existingDeployment)).To(Succeed())

})
Expand Down Expand Up @@ -67,83 +72,10 @@ var _ = Describe("ConfigMap Test", func() {

})

// Commented out because we are overwriting user-defined plugin values in the ConfigMap
// with the plugins defined in the CR. This will be removed once the PR has been reviewed.

/*It("verifies that the config map reconciler will not overwrite a custom plugin that is added to the ConfigMap by the user", func() {
// By("creating a ConfigMap containing default Openshift")
expectedConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: DefaultRolloutsConfigMapName,
Namespace: a.Namespace,
Labels: map[string]string{
"app.kubernetes.io/name": DefaultRolloutsConfigMapName,
},
},
}
By("calling reconcileConfigMap, which will add the default plugin to the ConfigMap")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())
By("fetching the ConfigMap")
fetchedConfigMap := &corev1.ConfigMap{}
Expect(fetchObject(ctx, r.Client, a.Namespace, expectedConfigMap.Name, fetchedConfigMap)).To(Succeed())
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(OpenShiftRolloutPluginName))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).ToNot(ContainSubstring("test/plugin"))
By("adding a new trafficRouter plugin to test the update plugin logic")
trafficRouterPlugins := []pluginItem{
{
Name: "test/plugin",
Location: "https://test-path",
},
}
newConfigMap := fetchedConfigMap.DeepCopy()
{
pluginString, err := yaml.Marshal(trafficRouterPlugins)
Expect(err).ToNot(HaveOccurred())
newConfigMap.Data = map[string]string{
TrafficRouterPluginConfigMapKey: string(pluginString),
}
}
By("updating the ConfigMap to contain only a user provided plugin")
Expect(r.Client.Update(ctx, newConfigMap)).To(Succeed())
By("calling reconcileConfigMap")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())
By("verifying that when ConfigMap is reconciled, it contains both plugins")
Expect(fetchObject(ctx, r.Client, a.Namespace, expectedConfigMap.Name, fetchedConfigMap)).To(Succeed())
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring("test/plugin"))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(OpenShiftRolloutPluginName))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(r.OpenShiftRoutePluginLocation))
By("calling reconcileConfigMap again, to verify nothing changes when reconcile is called again")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())
Expect(fetchObject(ctx, r.Client, a.Namespace, expectedConfigMap.Name, fetchedConfigMap)).To(Succeed())
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring("test/plugin"))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(OpenShiftRolloutPluginName))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(r.OpenShiftRoutePluginLocation))
// overriding this value with new test url to verify whether it updated the existing configMap with the new url
r.OpenShiftRoutePluginLocation = "test-updated-url"
By("calling reconcileConfigMap")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())
Expect(fetchObject(ctx, r.Client, a.Namespace, expectedConfigMap.Name, fetchedConfigMap)).To(Succeed())
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring("test/plugin"))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(OpenShiftRolloutPluginName))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring("test-updated-url"))
}) */

It("verifies traffic and metric plugin creation/modification and ensures OpenShiftRolloutPlugin existence", func() {
By("Add a pod that matches the deployment's selector")
addTestPodToFakeClient(r, a.Namespace, existingDeployment)

expectedConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: DefaultRolloutsConfigMapName,
Expand All @@ -158,6 +90,8 @@ var _ = Describe("ConfigMap Test", func() {
{Name: "custom-metric-plugin", Location: metricPluginLocation, SHA256: "sha256-test"},
}

Expect(r.Client.Update(ctx, &a)).To(Succeed())

By("Call reconcileConfigMap")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())

Expand Down Expand Up @@ -187,6 +121,8 @@ var _ = Describe("ConfigMap Test", func() {
{Name: "custom-metric-plugin", Location: updatedPluginLocation, SHA256: "sha256-test"},
}

Expect(r.Client.Update(ctx, &a)).To(Succeed())

By("Call reconcileConfigMap again after update")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())

Expand Down Expand Up @@ -215,7 +151,59 @@ var _ = Describe("ConfigMap Test", func() {
},
}

By("Call reconcileConfigMap again after update")
Expect(r.Client.Update(ctx, &a)).To(Succeed())

By("Calling reconcileConfigMap again after the attempt to update OpenShiftRolloutPlugin")
Expect(r.reconcileConfigMap(ctx, a)).ToNot(Succeed(), "the plugin %s cannot be modified or added through the RolloutManager CR", OpenShiftRolloutPluginName)

By("Remove plugins from RolloutManager spec should remove plugins from ConfigMap")
a.Spec.Plugins.TrafficManagement = nil
a.Spec.Plugins.Metric = nil

Expect(r.Client.Update(ctx, &a)).To(Succeed())

By("Call reconcileConfigMap after plugins are removed")
Expect(r.reconcileConfigMap(ctx, a)).To(Succeed())

By("Fetched ConfigMap after removing plugins")
Expect(fetchObject(ctx, r.Client, a.Namespace, expectedConfigMap.Name, fetchedConfigMap)).To(Succeed())

By("Verify that the fetched ConfigMap contains OpenShiftRolloutPlugin after removing plugins from CR")
Expect(fetchedConfigMap.Name).To(Equal(expectedConfigMap.Name))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(OpenShiftRolloutPluginName))
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).To(ContainSubstring(r.OpenShiftRoutePluginLocation))

By("Verify that the ConfigMap no longer contains removed plugins")
Expect(fetchedConfigMap.Data[TrafficRouterPluginConfigMapKey]).NotTo(ContainSubstring("custom-traffic-plugin"))
Expect(fetchedConfigMap.Data[MetricPluginConfigMapKey]).NotTo(ContainSubstring("custom-metric-plugin"))

By("Verify that the pod has been deleted after the above update.")
rolloutsPodList := &corev1.PodList{}
err := r.Client.List(ctx, rolloutsPodList, client.InNamespace(a.Namespace), client.MatchingLabels(existingDeployment.Spec.Selector.MatchLabels))
Expect(err).NotTo(HaveOccurred())
Expect(len(rolloutsPodList.Items)).To(BeNumerically("==", 0))
})
})

func addTestPodToFakeClient(r *RolloutManagerReconciler, namespace string, deployment *appsv1.Deployment) {
// Create a test pod with labels that match the deployment's selector
testPod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-rollouts-pod",
Namespace: namespace,
Labels: deployment.Spec.Selector.MatchLabels,
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: DefaultArgoRolloutsResourceName,
Image: "argoproj/argo-rollouts:latest",
},
},
},
}

// Add the pod to the fake client
err := r.Client.Create(context.TODO(), testPod)
Expect(err).ToNot(HaveOccurred())
}
20 changes: 20 additions & 0 deletions docs/crd_reference.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,23 @@ spec:
skipNotificationSecretDeployment: true
```
### RolloutManager example with metric and trafficManagement Plugins
``` yaml
apiVersion: argoproj.io/v1alpha1
kind: RolloutManager
metadata:
name: argo-rollout
labels:
example: with-plugins
spec:
plugins:
trafficManagement:
- name: argoproj-labs/gatewayAPI
location: https://github.com/argoproj-labs/rollouts-plugin-trafficrouter-gatewayapi/releases/download/v0.4.0/gatewayapi-plugin-linux-amd64
metric:
- name: "argoproj-labs/sample-prometheus"
location: https://github.com/argoproj-labs/sample-rollouts-metric-plugin/releases/download/v0.0.3/metric-plugin-linux-amd64
sha256: a597a017a9a1394a31b3cbc33e08a071c88f0bd8
```
Loading

0 comments on commit 6bfa67b

Please sign in to comment.