Association controller bug fixes #2679
Conversation
There was a problem hiding this comment.
Good catch!
I have some comments because parts of the code are not exactly clear to me. Most of those are not related to this PR though, but I guess to existing code that was moved around by this PR. Since everything's still fresh in your mind I thought this PR may be worth adding some clarification and/or refactoring?
...controller/apmserverelasticsearchassociation/apmserverelasticsearchassociation_controller.go
Show resolved
Hide resolved
pkg/controller/kibanaassociation/association_controller_test.go
Outdated
Show resolved
Hide resolved
|
This PR adds a consistent way to detect
This simplifies the code, we don't need to rely on ownership for some of them or labels for some other ones. It's also fixes some issues found while reviewing the code. I'll update the title and the description before merging. |
...controller/apmserverelasticsearchassociation/apmserverelasticsearchassociation_controller.go
Outdated
Show resolved
Hide resolved
...controller/apmserverelasticsearchassociation/apmserverelasticsearchassociation_controller.go
Outdated
Show resolved
Hide resolved
| @@ -68,10 +69,12 @@ func ReconcileCASecret( | |||
| Expected: &expectedSecret, | |||
| Reconciled: &reconciledSecret, | |||
| NeedsUpdate: func() bool { | |||
| return !reflect.DeepEqual(expectedSecret.Data, reconciledSecret.Data) | |||
| return !reflect.DeepEqual(expectedSecret.Data, reconciledSecret.Data) || | |||
| !maps.IsSubset(expectedSecret.Labels, reconciledSecret.Labels) | |||
There was a problem hiding this comment.
#2707 should help.
I'll do a second pass in a few days for in-flight PRs that did not benefit from it so we can keep this PR as is.
| @@ -0,0 +1,74 @@ | |||
| package association | |||
There was a problem hiding this comment.
This whole file is much easier to digest since you changed the labelling logic 🙏
| @@ -290,6 +251,69 @@ func Test_deleteOrphanedResources(t *testing.T) { | |||
| }, | |||
| wantErr: false, | |||
| }, | |||
| { | |||
| name: "No more es ref in Kibana, orphan user for previous es ref in a different namespace still exist", | |||
barkbay
changed the title
This PR fixes #2675 #2674 #2694 #2695 #2692
About the unit test: I only added a unit test for #2674. My feeling is that either we are missing some tooling to properly create unit tests against controllers or adding a unit test to cover
reconcileInternalwould be out the scope of this PR. I can, however, create an issue to track this.