From 2e79f83844b0a0335c3aa29f675ec01315a63c4a Mon Sep 17 00:00:00 2001 From: Luca Corrieri Date: Thu, 9 Nov 2023 15:48:13 +0100 Subject: [PATCH] fix(pr): multiple bugs in the PR controller (#187) * fix(push): only inspect layers on affected repos * fix(pr): add logs when creating PR * fix(logging): add resource reconciliated * fix(pr): change name of temporary layers * fix(pr): delete temporary layers before discovery * feat(pr): add Planning state * fix(rbac): add deletecollection verb on layers * fix(pr-comment): add whitespace for correct headings --- deploy/charts/burrito/templates/rbac.yaml | 1 + .../controllers/terraformlayer/controller.go | 4 +-- .../comment/templates/comment.md | 6 +++-- .../terraformpullrequest/conditions.go | 20 +------------- .../terraformpullrequest/controller.go | 5 ++-- .../controllers/terraformpullrequest/layer.go | 27 ++++++++++++++++++- .../terraformpullrequest/states.go | 25 ++++++++++------- .../controllers/terraformrun/controller.go | 4 +-- internal/webhook/event/pullrequest.go | 3 ++- internal/webhook/event/push.go | 5 +++- 10 files changed, 59 insertions(+), 41 deletions(-) diff --git a/deploy/charts/burrito/templates/rbac.yaml b/deploy/charts/burrito/templates/rbac.yaml index 823bc8f1..2481d70c 100644 --- a/deploy/charts/burrito/templates/rbac.yaml +++ b/deploy/charts/burrito/templates/rbac.yaml @@ -162,6 +162,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/internal/controllers/terraformlayer/controller.go b/internal/controllers/terraformlayer/controller.go index 0a3c119e..03b3140c 100644 --- a/internal/controllers/terraformlayer/controller.go +++ b/internal/controllers/terraformlayer/controller.go @@ -70,7 +70,7 @@ type Reconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := log.WithContext(ctx) - log.Infof("starting reconciliation...") + log.Infof("starting reconciliation for layer %s/%s ...", req.Namespace, req.Name) layer := &configv1alpha1.TerraformLayer{} err := r.Client.Get(ctx, req.NamespacedName, layer) if errors.IsNotFound(err) { @@ -123,7 +123,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err != nil { log.Errorf("could not update layer %s status: %s", layer.Name, err) } - log.Infof("finished reconciliation cycle for layer %s", layer.Name) + log.Infof("finished reconciliation cycle for layer %s/%s", layer.Namespace, layer.Name) return result, nil } diff --git a/internal/controllers/terraformpullrequest/comment/templates/comment.md b/internal/controllers/terraformpullrequest/comment/templates/comment.md index f2032438..2de2429d 100644 --- a/internal/controllers/terraformpullrequest/comment/templates/comment.md +++ b/internal/controllers/terraformpullrequest/comment/templates/comment.md @@ -2,7 +2,8 @@ {{ len .Layers }} layer(s) affected with {{ .Commit }} commit. -{{- range .Layers }} +{{ range .Layers }} + ### Layer {{ .Path }} `{{ .ShortDiff }}` @@ -14,4 +15,5 @@ {{ .PrettyPlan }} ``` -{{- end }} + +{{ end }} diff --git a/internal/controllers/terraformpullrequest/conditions.go b/internal/controllers/terraformpullrequest/conditions.go index 094944a7..c5234eeb 100644 --- a/internal/controllers/terraformpullrequest/conditions.go +++ b/internal/controllers/terraformpullrequest/conditions.go @@ -1,15 +1,11 @@ package terraformpullrequest import ( - "context" "time" configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1" "github.com/padok-team/burrito/internal/annotations" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/selection" - "sigs.k8s.io/controller-runtime/pkg/client" ) func (r *Reconciler) IsLastCommitDiscovered(pr *configv1alpha1.TerraformPullRequest) (metav1.Condition, bool) { @@ -29,7 +25,7 @@ func (r *Reconciler) IsLastCommitDiscovered(pr *configv1alpha1.TerraformPullRequ lastBranchCommit, ok := pr.Annotations[annotations.LastBranchCommit] if !ok { condition.Reason = "UnknownLastBranchCommit" - condition.Message = "This should have happened" + condition.Message = "This should not have happened" condition.Status = metav1.ConditionFalse return condition, false } @@ -143,17 +139,3 @@ func (r *Reconciler) IsCommentUpToDate(pr *configv1alpha1.TerraformPullRequest) condition.Status = metav1.ConditionTrue return condition, true } - -func GetLinkedLayers(cl client.Client, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) { - layers := configv1alpha1.TerraformLayerList{} - requirement, err := labels.NewRequirement("burrito/managed-by", selection.Equals, []string{pr.Name}) - if err != nil { - return nil, err - } - selector := labels.NewSelector().Add(*requirement) - err = cl.List(context.TODO(), &layers, client.MatchingLabelsSelector{Selector: selector}) - if err != nil { - return nil, err - } - return layers.Items, nil -} diff --git a/internal/controllers/terraformpullrequest/controller.go b/internal/controllers/terraformpullrequest/controller.go index 44adbf3f..4e76bf5d 100644 --- a/internal/controllers/terraformpullrequest/controller.go +++ b/internal/controllers/terraformpullrequest/controller.go @@ -55,9 +55,8 @@ type Reconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := log.WithContext(ctx) - log.Infof("starting reconciliation...") + log.Infof("starting reconciliation for pull request %s/%s ...", req.Namespace, req.Name) pr := &configv1alpha1.TerraformPullRequest{} err := r.Client.Get(ctx, req.NamespacedName, pr) if errors.IsNotFound(err) { @@ -89,7 +88,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err != nil { log.Errorf("could not update pull request %s status: %s", pr.Name, err) } - log.Infof("finished reconciliation cycle for pull request %s", pr.Name) + log.Infof("finished reconciliation cycle for pull request %s/%s", pr.Namespace, pr.Name) return result, nil } diff --git a/internal/controllers/terraformpullrequest/layer.go b/internal/controllers/terraformpullrequest/layer.go index b8f1ad4d..f5507fc6 100644 --- a/internal/controllers/terraformpullrequest/layer.go +++ b/internal/controllers/terraformpullrequest/layer.go @@ -5,10 +5,14 @@ import ( "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + "sigs.k8s.io/controller-runtime/pkg/client" configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1" "github.com/padok-team/burrito/internal/annotations" controller "github.com/padok-team/burrito/internal/controllers/terraformlayer" + log "github.com/sirupsen/logrus" ) func (r *Reconciler) getAffectedLayers(repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) { @@ -63,7 +67,7 @@ func generateTempLayers(pr *configv1alpha1.TerraformPullRequest, layers []config new := configv1alpha1.TerraformLayer{ ObjectMeta: metav1.ObjectMeta{ Namespace: layer.ObjectMeta.Namespace, - GenerateName: fmt.Sprintf("%s-%s-", layer.Name, pr.Spec.ID), + GenerateName: fmt.Sprintf("%s-pr-%s-", layer.Name, pr.Spec.ID), Annotations: map[string]string{ annotations.LastBranchCommit: pr.Annotations[annotations.LastBranchCommit], annotations.LastRelevantCommit: pr.Annotations[annotations.LastBranchCommit], @@ -95,3 +99,24 @@ func generateTempLayers(pr *configv1alpha1.TerraformPullRequest, layers []config } return list } + +func GetLinkedLayers(cl client.Client, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) { + layers := configv1alpha1.TerraformLayerList{} + requirement, err := labels.NewRequirement("burrito/managed-by", selection.Equals, []string{pr.Name}) + if err != nil { + return nil, err + } + selector := labels.NewSelector().Add(*requirement) + err = cl.List(context.TODO(), &layers, client.MatchingLabelsSelector{Selector: selector}) + if err != nil { + return nil, err + } + return layers.Items, nil +} + +func (r *Reconciler) deleteTempLayers(ctx context.Context, pr *configv1alpha1.TerraformPullRequest) error { + log.Infof("deleting temporary layers for pull request %s/%s", pr.Namespace, pr.Name) + return r.Client.DeleteAllOf( + ctx, &configv1alpha1.TerraformLayer{}, client.InNamespace(pr.Namespace), client.MatchingLabels{"burrito/managed-by": pr.Name}, + ) +} diff --git a/internal/controllers/terraformpullrequest/states.go b/internal/controllers/terraformpullrequest/states.go index 7cea1bb0..bd46d70e 100644 --- a/internal/controllers/terraformpullrequest/states.go +++ b/internal/controllers/terraformpullrequest/states.go @@ -9,7 +9,6 @@ import ( "github.com/padok-team/burrito/internal/annotations" "github.com/padok-team/burrito/internal/controllers/terraformpullrequest/comment" log "github.com/sirupsen/logrus" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" ) @@ -33,7 +32,7 @@ func (r *Reconciler) GetState(ctx context.Context, pr *configv1alpha1.TerraformP return &Idle{}, conditions case isLastCommitDiscovered && areLayersStillPlanning: log.Infof("pull request %s layers are still planning, waiting", pr.Name) - return &Idle{}, conditions + return &Planning{}, conditions case isLastCommitDiscovered && !areLayersStillPlanning && !isCommentUpToDate: log.Infof("pull request %s layers have finished, posting comment", pr.Name) return &CommentNeeded{}, conditions @@ -51,10 +50,23 @@ func (s *Idle) getHandler() func(ctx context.Context, r *Reconciler, repository } } +type Planning struct{} + +func (s *Planning) getHandler() func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result { + return func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result { + return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.WaitAction} + } +} + type DiscoveryNeeded struct{} func (s *DiscoveryNeeded) getHandler() func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result { return func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result { + // Delete already existing temporary layers + err := r.deleteTempLayers(ctx, pr) + if err != nil { + log.Errorf("failed to delete temp layers for pull request %s: %s", pr.Name, err) + } layers, err := r.getAffectedLayers(repository, pr) if err != nil { log.Errorf("failed to get affected layers for pull request %s: %s", pr.Name, err) @@ -63,19 +75,12 @@ func (s *DiscoveryNeeded) getHandler() func(ctx context.Context, r *Reconciler, newLayers := generateTempLayers(pr, layers) for _, layer := range newLayers { err := r.Client.Create(ctx, &layer) - if errors.IsAlreadyExists(err) { - log.Infof("layer %s already exists, updating it", layer.Name) - err = r.Client.Update(ctx, &layer) - if err != nil { - log.Errorf("failed to update layer %s: %s", layer.Name, err) - return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError} - } - } if err != nil { log.Errorf("failed to create layer %s: %s", layer.Name, err) return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError} } } + // TODO: setting this annotation triggers another reconciliation cycle while this one is still running, which is not ideal err = annotations.Add(ctx, r.Client, pr, map[string]string{annotations.LastDiscoveredCommit: pr.Annotations[annotations.LastBranchCommit]}) if err != nil { log.Errorf("failed to add annotation %s to pull request %s: %s", annotations.LastDiscoveredCommit, pr.Name, err) diff --git a/internal/controllers/terraformrun/controller.go b/internal/controllers/terraformrun/controller.go index 78a843a7..edbeda62 100644 --- a/internal/controllers/terraformrun/controller.go +++ b/internal/controllers/terraformrun/controller.go @@ -67,7 +67,7 @@ type Reconciler struct { // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := log.WithContext(ctx) - log.Infof("starting reconciliation...") + log.Infof("starting reconciliation for run %s/%s ...", req.Namespace, req.Name) run := &configv1alpha1.TerraformRun{} err := r.Client.Get(ctx, req.NamespacedName, run) if errors.IsNotFound(err) { @@ -103,7 +103,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if err != nil { log.Errorf("could not update run %s status: %s", run.Name, err) } - log.Infof("finished reconciliation cycle for run %s", run.Name) + log.Infof("finished reconciliation cycle for run %s/%s", run.Namespace, run.Name) return result, nil } diff --git a/internal/webhook/event/pullrequest.go b/internal/webhook/event/pullrequest.go index 0b2158d2..58804c84 100644 --- a/internal/webhook/event/pullrequest.go +++ b/internal/webhook/event/pullrequest.go @@ -51,6 +51,7 @@ func (e *PullRequestEvent) Handle(c client.Client) error { func batchCreatePullRequests(ctx context.Context, c client.Client, prs []configv1alpha1.TerraformPullRequest) error { var errResult error for _, pr := range prs { + log.Infof("creating terraform pull request %s/%s", pr.Namespace, pr.Name) err := c.Create(ctx, &pr) if err != nil { errResult = multierror.Append(errResult, err) @@ -62,7 +63,7 @@ func batchCreatePullRequests(ctx context.Context, c client.Client, prs []configv func batchDeletePullRequests(ctx context.Context, c client.Client, prs []configv1alpha1.TerraformPullRequest) error { var errResult error for _, pr := range prs { - log.Info(fmt.Sprintf("deleting pull request %s", pr.Name)) + log.Infof("deleting terraform pull request %s/%s", pr.Namespace, pr.Name) err := c.Delete(ctx, &pr) if err != nil { errResult = multierror.Append(errResult, err) diff --git a/internal/webhook/event/push.go b/internal/webhook/event/push.go index 84ebe5c6..4f7e9216 100644 --- a/internal/webhook/event/push.go +++ b/internal/webhook/event/push.go @@ -47,7 +47,7 @@ func (e *PushEvent) Handle(c client.Client) error { } } - for _, layer := range e.getAffectedLayers(layers.Items, repositories.Items) { + for _, layer := range e.getAffectedLayers(layers.Items, affectedRepositories) { ann := map[string]string{} log.Printf("evaluating terraform layer %s for revision %s", layer.Name, e.Revision) if layer.Spec.Branch != e.Revision { @@ -57,6 +57,7 @@ func (e *PushEvent) Handle(c client.Client) error { ann[annotations.LastBranchCommit] = e.ChangeInfo.ShaAfter if controller.LayerFilesHaveChanged(layer, e.Changes) { + log.Infof("layer %s is affected by push event", layer.Name) ann[annotations.LastRelevantCommit] = e.ChangeInfo.ShaAfter } @@ -81,8 +82,10 @@ func (e *PushEvent) Handle(c client.Client) error { func (e *PushEvent) getAffectedRepositories(repositories []configv1alpha1.TerraformRepository) []configv1alpha1.TerraformRepository { affectedRepositories := []configv1alpha1.TerraformRepository{} + log.Infof("looking for affected repositories, event url: %s", e.URL) for _, repo := range repositories { if e.URL == NormalizeUrl(repo.Spec.Repository.Url) { + log.Infof("repository %s is affected by push event", repo.Name) affectedRepositories = append(affectedRepositories, repo) continue }