WIP: Graduate ServerSideApply feature gates to Beta#7908
WIP: Graduate ServerSideApply feature gates to Beta#7908erikgb wants to merge 4 commits intocert-manager:masterfrom
Conversation
004b40a to
acd1d33
Compare
b0b1b64 to
db48006
Compare
db48006 to
1a378e4
Compare
|
Priority assessment: Enabling would be the starting point. If we don't do this, we can't work on apply configurations, and can't start using the modern way of doing it, the way controller-runtime does. |
|
Let's enable SSA! And disable SSA for our tests and figure it out later. |
IMO we cannot enable SSA with broken tests, nor run tests with the feature gate disabled. We need to focus on fixing the broken test setup first. |
|
I've been doing some investigating on how the tests break, and I'm going to put what I've learned here so it's accessible to others. You all might know this already 😁 I've definitely not gone through every failure... Unit testsI've been testing a local checkout of this PR. I checked some unit tests with With a patch[2] leveraging cmp I can get rid of the time marshalling differences for these tests, which makes sense to me. For the other differences, there's no easy way to fix it; the patch is fundamentally a different thing to the object we're comparing it against. In [1] the object has a Request, but the patch (correctly) doesn't. There's no saving the fact that a lot of busywork needs to be done there, if we want the tests to look anything like how they look now. Actual Behaviour DifferencesI can see a fundamentally different behaviour in cert-manager thanks to the integration tests when run against this branch.
This doesn't seem tremendously important - it's probably fine for it not to be removed with SSA, I think? - but it's a fundamental difference and it's good the tests have caught it! EDIT: seems this has been a known thing for a while: cert-manager/pkg/controller/certificates/issuing/issuing_controller.go Lines 417 to 420 in d00260d Extras[1]: [2]: diff --git i/pkg/controller/test/actions.go w/pkg/controller/test/actions.go
index 6f1925b9e..d12f1c828 100644
--- i/pkg/controller/test/actions.go
+++ w/pkg/controller/test/actions.go
@@ -19,8 +19,10 @@ package test
import (
"fmt"
"reflect"
+ "time"
"github.com/google/go-cmp/cmp"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/json"
coretesting "k8s.io/client-go/testing"
@@ -117,6 +119,13 @@ func (a *action) Matches(act coretesting.Action) error {
}
return p.String() == "ObjectMeta.ManagedFields"
}, cmp.Ignore()),
+ cmp.Transformer("metav1.Time", func(in *metav1.Time) string {
+ if in == nil {
+ return "<nil>"
+ }
+
+ return in.Time.Format(time.RFC3339)
+ }),
)
if diff != "" {
return fmt.Errorf("unexpected difference between SSA patch and expected object (-want +got):\n%s", diff) |
|
I've been digging into this a bit more, and it seems part of the change for the certificate request unit tests I've been digging into is here, called eventually via Ready. I tried with this patch against master: diff --git i/pkg/api/util/conditions.go w/pkg/api/util/conditions.go
index df247f29c..97dd16688 100644
--- i/pkg/api/util/conditions.go
+++ w/pkg/api/util/conditions.go
@@ -17,6 +17,8 @@ limitations under the License.
package util
import (
+ "time"
+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
"k8s.io/utils/clock"
@@ -251,7 +253,7 @@ func SetCertificateRequestCondition(cr *cmapi.CertificateRequest, conditionType
Message: message,
}
- nowTime := metav1.NewTime(Clock.Now())
+ nowTime := metav1.NewTime(Clock.Now().Truncate(time.Second))
newCondition.LastTransitionTime = &nowTime
// Search through existing conditionsAnd unit tests fail as expected, with the output much more closely matching what we'd observe in an actual cluster. This process of updating I think the time comparisons we're making in our unit tests aren't important, since we're using fake clocks anyway. When SSA is on by default, the if statements here are going down different branches, which causes the unit test failures. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
|
@erikgb: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Pull Request Motivation
Kind
/kind feature
Release Note
CyberArk tracker: VC-45578