diff --git a/README.md b/README.md index 67345614..8c53848a 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Key features: - It will default to `5m` if not set - `EXPIRY_THRESHOLD` - i.e. to reconcile a new access token if there is less than 10 mins left from expiry, set the value to `10m` - It will default to `15m` if not set -- Optionally, you can enable restart of pods in the same namespace as the `GithubApp` that match any of the labels you define in `spec.restartPods.labels` +- Optionally, you can enable rolling upgrade to deployments in the same namespace as the `GithubApp` that match any of the labels you define in `spec.rolloutDeployment.labels` - This is useful where pods need to be recreated to pickup the new secret data. ## Example creating a secret to hold a GitHub App private key @@ -66,8 +66,8 @@ spec: EOF ``` -## Example GithubApp object with pod restart on token renew -- Below example will restart the pods in the `team-1` namespace when the github token is modified, matching any of labels: +## Example GithubApp object with pod restart (deployment rolling upgrade) on token renew +- Below example will upgrade deployments in the `team-1` namespace when the github token is modified, matching any of labels: - foo: bar - foo2: bar2 ```sh @@ -81,7 +81,7 @@ spec: appId: 123123 installId: 12312312 privateKeySecret: github-app-secret - restartPods: + rolloutDeployment: labels: foo: bar foo2: bar2 @@ -143,7 +143,7 @@ Current integration tests cover the scenarios: - Reconcile of access token is valid. - Reconcile error is recorded in a `GithubApp` object's `status.error` field - The `status.error` field is cleared on succesful reconcile for a `GithubApp` object. -- Pods are deleted matching a label if defined in `spec.restartPods.labels` for a `GithubApp`. +- Pods are deleted matching a label if defined in `spec.rolloutDeployment.labels` for a `GithubApp`. **Run the controller in the foreground for testing:** ```sh diff --git a/api/v1/githubapp_types.go b/api/v1/githubapp_types.go index 76601375..3a1e3e79 100644 --- a/api/v1/githubapp_types.go +++ b/api/v1/githubapp_types.go @@ -22,10 +22,10 @@ import ( // GithubAppSpec defines the desired state of GithubApp type GithubAppSpec struct { - AppId int `json:"appId"` - InstallId int `json:"installId"` - PrivateKeySecret string `json:"privateKeySecret"` - RestartPods *RestartPodsSpec `json:"restartPods,omitempty"` + AppId int `json:"appId"` + InstallId int `json:"installId"` + PrivateKeySecret string `json:"privateKeySecret"` + RolloutDeployment *RolloutDeploymentSpec `json:"rolloutDeployment,omitempty"` } // GithubAppStatus defines the observed state of GithubApp @@ -48,8 +48,8 @@ type GithubApp struct { Status GithubAppStatus `json:"status,omitempty"` } -// RestartPodsSpec defines the specification for restarting pods -type RestartPodsSpec struct { +// RolloutDeploymentSpec defines the specification for restarting pods +type RolloutDeploymentSpec struct { Labels map[string]string `json:"labels,omitempty"` } diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 61381b0b..606f854c 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -86,9 +86,9 @@ func (in *GithubAppList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GithubAppSpec) DeepCopyInto(out *GithubAppSpec) { *out = *in - if in.RestartPods != nil { - in, out := &in.RestartPods, &out.RestartPods - *out = new(RestartPodsSpec) + if in.RolloutDeployment != nil { + in, out := &in.RolloutDeployment, &out.RolloutDeployment + *out = new(RolloutDeploymentSpec) (*in).DeepCopyInto(*out) } } @@ -120,7 +120,7 @@ func (in *GithubAppStatus) DeepCopy() *GithubAppStatus { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *RestartPodsSpec) DeepCopyInto(out *RestartPodsSpec) { +func (in *RolloutDeploymentSpec) DeepCopyInto(out *RolloutDeploymentSpec) { *out = *in if in.Labels != nil { in, out := &in.Labels, &out.Labels @@ -131,12 +131,12 @@ func (in *RestartPodsSpec) DeepCopyInto(out *RestartPodsSpec) { } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RestartPodsSpec. -func (in *RestartPodsSpec) DeepCopy() *RestartPodsSpec { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RolloutDeploymentSpec. +func (in *RolloutDeploymentSpec) DeepCopy() *RolloutDeploymentSpec { if in == nil { return nil } - out := new(RestartPodsSpec) + out := new(RolloutDeploymentSpec) in.DeepCopyInto(out) return out } diff --git a/config/crd/bases/githubapp.samir.io_githubapps.yaml b/config/crd/bases/githubapp.samir.io_githubapps.yaml index 720846d0..ee0808f8 100644 --- a/config/crd/bases/githubapp.samir.io_githubapps.yaml +++ b/config/crd/bases/githubapp.samir.io_githubapps.yaml @@ -45,8 +45,8 @@ spec: type: integer privateKeySecret: type: string - restartPods: - description: RestartPodsSpec defines the specification for restarting + rolloutDeployment: + description: RolloutDeploymentSpec defines the specification for restarting pods properties: labels: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 97a1153a..d67e023d 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -7,10 +7,8 @@ rules: - apiGroups: - "" resources: - - pods + - deployments verbs: - - create - - delete - get - list - patch diff --git a/internal/controller/githubapp_controller.go b/internal/controller/githubapp_controller.go index 59549929..5dbe160b 100644 --- a/internal/controller/githubapp_controller.go +++ b/internal/controller/githubapp_controller.go @@ -27,6 +27,7 @@ import ( "time" githubappv1 "github-app-operator/api/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -63,7 +64,7 @@ const ( //+kubebuilder:rbac:groups=githubapp.samir.io,resources=githubapps/status,verbs=get;update;patch //+kubebuilder:rbac:groups=githubapp.samir.io,resources=githubapps/finalizers,verbs=update //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;update;create;delete;watch;patch -//+kubebuilder:rbac:groups="",resources=pods,verbs=get;list;update;create;delete;watch;patch +//+kubebuilder:rbac:groups="",resources=deployments,verbs=get;list;update;watch;patch // Reconcile function func (r *GithubAppReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -393,8 +394,8 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g return fmt.Errorf("failed after creating secret: %v", err) } // Restart the pods is required - if err := r.restartPods(ctx, githubApp); err != nil { - return fmt.Errorf("failed to restart pods after after creating secret: %v", err) + if err := r.rolloutDeployment(ctx, githubApp); err != nil { + return fmt.Errorf("failed to rollout deployment after after creating secret: %v", err) } return nil } @@ -431,8 +432,8 @@ func (r *GithubAppReconciler) generateOrUpdateAccessToken(ctx context.Context, g return fmt.Errorf("failed after updating secret: %v", err) } // Restart the pods is required - if err := r.restartPods(ctx, githubApp); err != nil { - return fmt.Errorf("failed to restart pods after updating secret: %v", err) + if err := r.rolloutDeployment(ctx, githubApp); err != nil { + return fmt.Errorf("failed to rollout deployment after updating secret: %v", err) } l.Info("Access token updated in the existing Secret successfully") @@ -533,40 +534,53 @@ func generateAccessToken(ctx context.Context, appID int, installationID int, pri return accessToken, metav1.NewTime(expiresAt), nil } -// Function to bounce pods as per `spec.restartPods.labels` in GithubApp (in the same namespace) -func (r *GithubAppReconciler) restartPods(ctx context.Context, githubApp *githubappv1.GithubApp) error { +// Function to bounce pods as per `spec.rolloutDeployment.labels` in GithubApp (in the same namespace) +func (r *GithubAppReconciler) rolloutDeployment(ctx context.Context, githubApp *githubappv1.GithubApp) error { l := log.FromContext(ctx) - // Check if restartPods field is defined - if githubApp.Spec.RestartPods == nil || len(githubApp.Spec.RestartPods.Labels) == 0 { - // No action needed if restartPods is not defined or no labels are specified + // Check if rolloutDeployment field is defined + if githubApp.Spec.RolloutDeployment == nil || len(githubApp.Spec.RolloutDeployment.Labels) == 0 { + // No action needed if rolloutDeployment is not defined or no labels are specified return nil } - // Loop through each label specified in restartPods.labels and restart pods matching each label - for key, value := range githubApp.Spec.RestartPods.Labels { + // Loop through each label specified in rolloutDeployment.labels and restart pods matching each label + for key, value := range githubApp.Spec.RolloutDeployment.Labels { // Create a list options with label selector listOptions := &client.ListOptions{ Namespace: githubApp.Namespace, LabelSelector: labels.SelectorFromSet(map[string]string{key: value}), } - // List pods with the label selector - podList := &corev1.PodList{} - if err := r.List(ctx, podList, listOptions); err != nil { - return fmt.Errorf("failed to list pods with label %s=%s: %v", key, value, err) + // List Deployments with the label selector + deploymentList := &appsv1.DeploymentList{} + if err := r.List(ctx, deploymentList, listOptions); err != nil { + return fmt.Errorf("failed to list Deployments with label %s=%s: %v", key, value, err) } - // Restart each pod by deleting it - for _, pod := range podList.Items { - // Set deletion timestamp on the pod - if err := r.Delete(ctx, &pod); err != nil { - return fmt.Errorf("failed to delete pod %s/%s: %v", pod.Namespace, pod.Name, err) + // Trigger rolling upgrade for matching deployments + for _, deployment := range deploymentList.Items { + + // Add a timestamp label to trigger a rolling upgrade + deployment.Spec.Template.ObjectMeta.Labels["ghApplastUpdateTime"] = time.Now().Format("20060102150405") + + // Patch the Deployment + if err := r.Update(ctx, &deployment); err != nil { + return fmt.Errorf( + "failed to upgrade deployment %s/%s: %v", + deployment.Namespace, + deployment.Name, + err, + ) } - // Log pod deletion + + // Log deployment upgrade l.Info( - "Pod marked for deletion to refresh secret", - "Pod Name", pod.Name, + "Deployment rolling upgrade triggered", + "Name", + deployment.Name, + "Namespace", + deployment.Namespace, ) } } diff --git a/internal/controller/githubapp_controller_test.go b/internal/controller/githubapp_controller_test.go index a3fe6017..ef2a45db 100644 --- a/internal/controller/githubapp_controller_test.go +++ b/internal/controller/githubapp_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "fmt" + "os" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -155,8 +156,12 @@ var _ = Describe("GithubApp controller", func() { }) }) - Context("When reconciling a GithubApp with spec.restartPods.labels.foo as bar", func() { - It("Should eventually delete the pod with the matching label foo: bar", func() { + Context("When reconciling a GithubApp with spec.rolloutDeployment.labels.foo as bar", func() { + It("Should eventually upgrade the deployment matching label foo: bar", func() { + if os.Getenv("USE_EXISTING_CLUSTER") == "" { + fmt.Println("Skipping deployment rollout test case as not a real cluster...") + return // Skip the test case in envtest since required deployment controller + } ctx := context.Background() By("Creating a new namespace") @@ -165,20 +170,20 @@ var _ = Describe("GithubApp controller", func() { By("Creating the privateKeySecret in namespace2") test_helpers.CreatePrivateKeySecret(ctx, k8sClient, namespace2, "privateKey") - By("Creating a pod with the label foo: bar") - pod1 := test_helpers.CreatePodWithLabel(ctx, k8sClient, "foo", namespace2, "foo", "bar") + By("Creating a deployment with the label foo: bar") + deploy1, pod1 := test_helpers.CreateDeploymentWithLabel(ctx, k8sClient, "foo", namespace2, "foo", "bar") - By("Creating a pod with the label foo: bar2") - pod2 := test_helpers.CreatePodWithLabel(ctx, k8sClient, "foo", namespace2, "foo", "bar2") + By("Creating a deployment with the label foo: bar2") + deploy2, pod2 := test_helpers.CreateDeploymentWithLabel(ctx, k8sClient, "foo2", namespace2, "foo", "bar2") - By("Creating a GithubApp with the spec.restartPods.labels foo: bar") - restartPodsSpec := &githubappv1.RestartPodsSpec{ + By("Creating a GithubApp with the spec.rolloutDeployment.labels foo: bar") + rolloutDeploymentSpec := &githubappv1.RolloutDeploymentSpec{ Labels: map[string]string{ "foo": "bar", }, } - // Create a GithubApp instance with the RestartPods field initialized - test_helpers.CreateGitHubAppAndWait(ctx, k8sClient, namespace2, githubAppName2, restartPodsSpec) + // Create a GithubApp instance with the RolloutDeployment field initialized + test_helpers.CreateGitHubAppAndWait(ctx, k8sClient, namespace2, githubAppName2, rolloutDeploymentSpec) By("Waiting for pod1 with the label 'foo: bar' to be deleted") // Wait for the pod to be deleted by the reconcile loop @@ -198,9 +203,13 @@ var _ = Describe("GithubApp controller", func() { return pod2.DeletionTimestamp == nil }, "10s", "2s").Should(BeTrue(), "Pod2 is marked for deletion") - // Delete pod2 - err := k8sClient.Delete(ctx, pod2) - Expect(err).ToNot(HaveOccurred(), "Failed to delete pod2: %v", err) + // Delete deploy1 + err := k8sClient.Delete(ctx, deploy1) + Expect(err).ToNot(HaveOccurred(), "Failed to delete deploy1: %v", err) + + // Delete deploy2 + err = k8sClient.Delete(ctx, deploy2) + Expect(err).ToNot(HaveOccurred(), "Failed to delete deploy2: %v", err) // Delete the GitHubApp after reconciliation test_helpers.DeleteGitHubAppAndWait(ctx, k8sClient, namespace2, githubAppName2) diff --git a/internal/controller/test_helpers/test_helpers.go b/internal/controller/test_helpers/test_helpers.go index 202294a7..c64d978a 100644 --- a/internal/controller/test_helpers/test_helpers.go +++ b/internal/controller/test_helpers/test_helpers.go @@ -12,9 +12,11 @@ import ( gomega "github.com/onsi/gomega" githubappv1 "github-app-operator/api/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -76,7 +78,7 @@ func CreateGitHubAppAndWait( k8sClient client.Client, namespace, name string, - restartPodsSpec *githubappv1.RestartPodsSpec, + rolloutDeploymentSpec *githubappv1.RolloutDeploymentSpec, ) { // create the GitHubApp githubApp := githubappv1.GithubApp{ @@ -85,10 +87,10 @@ func CreateGitHubAppAndWait( Namespace: namespace, }, Spec: githubappv1.GithubAppSpec{ - AppId: appId, - InstallId: installId, - PrivateKeySecret: privateKeySecret, - RestartPods: restartPodsSpec, // Optionally pass restartPods + AppId: appId, + InstallId: installId, + PrivateKeySecret: privateKeySecret, + RolloutDeployment: rolloutDeploymentSpec, // Optionally pass rolloutDeployment }, } gomega.Expect(k8sClient.Create(ctx, &githubApp)).Should(gomega.Succeed()) @@ -186,33 +188,77 @@ func CheckGithubAppStatusError( }, "30s", "5s").Should(gomega.BeTrue(), "Failed to set status.Error field within timeout") } -// Funtion to create a busybox pod with a label -func CreatePodWithLabel( +/* +Function to create a Deployment with a pod template and label +This will only work on a real cluster and NOT envtest since +it requires the Deployment controller +*/ +func CreateDeploymentWithLabel( ctx context.Context, k8sClient client.Client, - podName string, + deploymentName string, namespace string, - labeKey string, + labelKey string, labelValue string, -) *corev1.Pod { - pod := &corev1.Pod{ +) (*appsv1.Deployment, *corev1.Pod) { + + // just create 1 replica + replicas := int32(1) + + // Pod template + podTemplate := corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: podName, - Namespace: namespace, Labels: map[string]string{ - labeKey: labelValue, + "app": deploymentName, }, }, Spec: corev1.PodSpec{ Containers: []corev1.Container{ { - Name: podName, + Name: deploymentName, Image: "busybox", }, }, }, } - gomega.Expect(k8sClient.Create(ctx, pod)).Should(gomega.Succeed()) - return pod + // Deployment spec + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: deploymentName, + Namespace: namespace, + Labels: map[string]string{ + labelKey: labelValue, + }, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": deploymentName, + }, + }, + Template: podTemplate, + }, + } + + // Create the Deployment + gomega.Expect(k8sClient.Create(ctx, deployment)).Should(gomega.Succeed()) + + // Create a list options with label selector + listOptions := &client.ListOptions{ + Namespace: namespace, + LabelSelector: labels.SelectorFromSet(map[string]string{"app": deploymentName}), + } + podList := &corev1.PodList{} + // Wait for the pod list to be populated + gomega.Eventually(func() []corev1.Pod { + gomega.Expect(k8sClient.List(ctx, podList, listOptions)).Should(gomega.Succeed()) + return podList.Items + }, "30s", "5s").ShouldNot(gomega.BeEmpty()) + + pod := &podList.Items[0] + + // Return the pod name + return deployment, pod }