Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Remove finalizers from secret when reference is removed #54

Merged
merged 1 commit into from
May 19, 2020

Conversation

timebertt
Copy link
Contributor

@timebertt timebertt commented May 18, 2020

What this PR does / why we need it:
This PR adds a new controller for secrets, that is only responsible for adding/removing grm's finalizer to/from secrets referenced by ManagedResources.
It watches ManagedResources and enqueues all secrets on a generation change, that are/were referenced by the ManagedResource. It additionally enqueues secrets with its finalizer on create and update to remove its finalizer if it missed an important update event during a downtime.

So now, grm properly removes its finalizer from secrets that are not referenced by a ManagedResource anymore.

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

Special notes for your reviewer:
Tests will fail because of changes introduced by #46, but should run green after rebasing on #53
✅ test-wise depends on #53

Release note:

`gardener-resource-manager` now properly removes its finalizer from secrets, that are not referenced by a `ManagedResource` anymore.
Please ensure, that `gardener-resource-manager` has the required permissions to also update secrets now.

@timebertt timebertt requested a review from a team as a code owner May 18, 2020 11:50
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 18, 2020
@timebertt
Copy link
Contributor Author

/kind bug
/needs review

@ghost ghost added kind/bug Bug needs/review Needs review labels May 18, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 18, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 18, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 18, 2020
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good! Just nits :)

pkg/controller/managedresources/secret_controller.go Outdated Show resolved Hide resolved
pkg/mapper/to_secret.go Outdated Show resolved Hide resolved
pkg/predicate/has_finalizer.go Outdated Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 19, 2020
@timebertt timebertt requested a review from rfranzke May 19, 2020 05:20
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 19, 2020
}
}

func metaHasFinalizer(meta metav1.Object, finalizer string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm okay, I was more thinking about also including the

	if e.MetaNew == nil {
		log.Error(nil, "Update event has no new object meta", "event", e)
		return false
	}

part in this function. But anyways, it's fine and doesn't matter too much.

/lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, then I misunderstood.
Anyways, let's keep it this way, so we can have more meaningful logs in case of errors (like "Create/Update event" and also the event attribute) without passing basically everything to the helper function :)

@ghost ghost added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels May 19, 2020
@gardener-attic gardener-attic deleted a comment May 19, 2020
@gardener-attic gardener-attic deleted a comment May 19, 2020
@gardener-attic gardener-attic deleted a comment from timebertt May 19, 2020
@rfranzke rfranzke merged commit aa9a7a7 into gardener-attic:master May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalizer is not removed from secret
6 participants