OCPBUGS-31384: use update only secret-apply for cert rotation logic #1705

tkashem · 2024-03-29T20:50:33Z

No description provided.

tkashem · 2024-03-29T22:17:32Z

/test unit

tkashem · 2024-03-29T23:31:55Z

/test unit

benluddy · 2024-04-01T15:14:22Z

pkg/operator/resource/resourceapply/core.go

+// NOTE: DO NOT USE, this is meant to be a short term hack for a very specific use case,
+// and will not work for your application


I don't trust that this won't be used anyway, can we do more?

well, another alternative is for the certrotation package to have its own copy of the function, but then we lose out on any improvements made on the library function.

pkg/operator/certrotation/signer_test.go

benluddy · 2024-04-01T15:50:19Z

pkg/operator/certrotation/signer_test.go

@@ -19,6 +26,170 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 )

+func TestRotatedSigningCASecretWithMultipleControllers(t *testing.T) {
+	ns, name := "ns", "test-signer"


Nit: const

benluddy · 2024-04-01T16:02:36Z

pkg/operator/certrotation/signer_test.go

+		t.Fatal(err)
+	}
+
+	// give it a second so we have a unique signer name,


Can we change the way signer names are chosen so that they are practically guaranteed to be unique? We were looking at kube-apiserver logs last week and observed that the issuer name was the same for the external and internal serving certs. Given the issue context, it's not unrealistic to imagine two successive rotations producing signers with the same name.

It uses time.Now().Unix(), we could use UnixMicro or UnixNano to significantly reduce the chances of issuer name being same.
FWIW, i think it is outside the scope of this PR, i don't think that changing it will have a bad ripple effect on any cluster, not sure any tool relies on the current format

pkg/operator/certrotation/signer_test.go

benluddy · 2024-04-01T16:17:46Z

pkg/operator/certrotation/signer_test.go

+	wg.Wait()
+	// controllers are done, we don't expect the signer to change
+	secretGot, err := client.Get(context.TODO(), name, metav1.GetOptions{})


Is this test deterministic? I would rather see one or more tests like #1699 that always repro some of the known bad sequences, unless we can reproducibly fuzz different action sequences.

benluddy · 2024-04-01T16:47:48Z

pkg/operator/certrotation/signer_test.go

+			indexer.Delete(existing)
+			if err := indexer.Add(action.GetObject()); err != nil {


Is this working around a problem with calling Update directly?

yes, it ensures that the list cache always has the updated object

benluddy · 2024-04-01T16:48:25Z

pkg/operator/certrotation/signer_test.go

+				Validity:              24 * time.Hour,
+				Refresh:               12 * time.Hour,
+				Client:                getter,
+				Lister:                corev1listers.NewSecretLister(indexer),


Would it be simpler to provide a test implementation of SecretLister using the fake client's object tracker instead of using client reactor functions to simulate a reflector?

FWIW the client reactor lets us have more control between a) object changes and b) the cache is updated, not that we are exercising it in this test, but yeah i think we could use a fake SecretLister implementation.
Also I have designed the test to not rely on the reactor in order to synchronize events between controller A and B, so we could definitely use a SecretLister to simplify things

tkashem · 2024-04-01T17:24:57Z

/test unit

tkashem · 2024-04-01T21:07:04Z

The test is deterministic, it reproduces a certain race condition that results in new generation of signer

go test -race -v -count=1 ./pkg/operator/certrotation/ -run=TestRotatedSigningCASecretShouldNotUseDelete
=== RUN   TestRotatedSigningCASecretShouldNotUseDelete
    signer_test.go:240: [controller-A] op=Get, secret=test-signer, err: <nil>
    signer_test.go:227: [controller-A] op=Delete, secret=test-signer
I0401 16:33:48.353793 3261385 event.go:364] Event(v1.ObjectReference{Kind:"", Namespace:"ns", Name:"controller-B", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'SignerUpdateRequired' "test-signer" in "ns" requires a new signing cert/key pair: missing notAfter
    signer_test.go:219: [controller-A] op=Create, secret=ns/test-signer
I0401 16:33:48.459482 3261385 event.go:364] Event(v1.ObjectReference{Kind:"", Namespace:"ns", Name:"controller-A", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'SecretDeleted' Deleted Secret/test-signer -n ns
I0401 16:33:48.459632 3261385 event.go:364] Event(v1.ObjectReference{Kind:"", Namespace:"ns", Name:"controller-A", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'SecretCreated' Created Secret/test-signer -n ns because it was missing
    signer_test.go:240: [controller-B] op=Get, secret=test-signer, err: <nil>
    signer_test.go:223: [controller-B] op=Update, secret=ns/test-signer
I0401 16:33:48.465283 3261385 event.go:364] Event(v1.ObjectReference{Kind:"", Namespace:"ns", Name:"controller-B", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}): type: 'Normal' reason: 'SecretUpdated' Updated Secret/test-signer -n ns because it changed
    signer_test.go:191: the signer cert has mutated unexpectedly
    signer_test.go:194: the signer key has mutated unexpectedly
    signer_test.go:199: diff:   &v1.Secret{
          	TypeMeta: {},
          	ObjectMeta: v1.ObjectMeta{
          		... // 3 identical fields
          		SelfLink:                   "",
          		UID:                        "",
        - 		ResourceVersion:            "10",
        + 		ResourceVersion:            "",
          		Generation:                 0,
          		CreationTimestamp:          {},
          		DeletionTimestamp:          nil,
          		DeletionGracePeriodSeconds: nil,
        - 		Labels:                     nil,
        + 		Labels:                     map[string]string{"auth.openshift.io/managed-certificate-type": "signer"},
          		Annotations: map[string]string{
        - 			"auth.openshift.io/certificate-issuer":     "ns_test-signer@1712003625",
        + 			"auth.openshift.io/certificate-issuer":     "ns_test-signer@1712003628",
        - 			"auth.openshift.io/certificate-not-after":  "2024-04-02T20:33:46Z",
        + 			"auth.openshift.io/certificate-not-after":  "2024-04-02T20:33:48Z",
        - 			"auth.openshift.io/certificate-not-before": "2024-04-01T20:33:45Z",
        + 			"auth.openshift.io/certificate-not-before": "2024-04-01T20:33:47Z",
        + 			"openshift.io/owning-component":            "test",
          		},
        - 		OwnerReferences: nil,
        + 		OwnerReferences: []v1.OwnerReference{{Name: "operator"}},
          		Finalizers:      nil,
          		ManagedFields:   nil,
          	},
          	Immutable: nil,
          	Data: map[string][]uint8{
          		"tls.crt": []uint8(
          			"""
          			-----BEGIN CERTIFICATE-----
        - 			MIIDLTCCAhWgAwIBAgIIQgm7uDYzbzUwDQYJKoZIhvcNAQELBQAwJDEiMCAGA1UE
        - 			AwwZbnNfdGVzdC1zaWduZXJAMTcxMjAwMzYyNTAeFw0yNDA0MDEyMDMzNDVaFw0y
        - 			NDA0MDIyMDMzNDZaMCQxIjAgBgNVBAMMGW5zX3Rlc3Qtc2lnbmVyQDE3MTIwMDM2
        - 			MjUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDoOMcF2OSLeGm8PndD
        - 			R4RBLTfme7/hQpfs8Pszxv7e80UeZ6YrFr7yeWwiCGXSMM0wLOnloCXaamxX82VS
        - 			lauof8wCMtoS2zuyobhHcDXyI+4tPkh3AuuXY16biU/c3UZVAl78JqsDWN4Kg4PZ
        - 			0JTIJ7AGXvE6pcj6OQdj1RtDXoT4nXVlE8GZ23KVcL3GEnMwDBC9Ux7TGcbEtYJe
        - 			t5aBodll4vBXr3s02KeQE+N094mbskbOZc82QMX0wNvYS766FeEKtZaCF95msLpC
        - 			u9oMb9ifC9OpKt1f1RfYeWdagH4+3+l1v3jA5gxMFBIWDsc9amvVv52VcB907/QV
        - 			RQgTAgMBAAGjYzBhMA4GA1UdDwEB/wQEAwICpDAPBgNVHRMBAf8EBTADAQH/MB0G
        - 			A1UdDgQWBBQw3MjuSLWOen9oz626A2VZEXY0VDAfBgNVHSMEGDAWgBQw3MjuSLWO
        - 			en9oz626A2VZEXY0VDANBgkqhkiG9w0BAQsFAAOCAQEAeKPY27TxGHJRJlPf+awe
        - 			00DQIV3iQktS5i3fCcknpMfcBEYZef0AQuUAMUXAHpmw1NN0glyF+xur+XlFUV0S
        - 			cpWcYi5rEUGYySmEBV81W2jvx/f410G5Ai+VJMzsCaODKIWAZaKzp2nPPYulPAlS
        - 			WoeowvCFUpSVer8thvP4mxN4npc35/dQSLqLJsziC//oKLkgjg7522ekI5io4Rot
        - 			YSebWqYIMfGhGP06QbkrgV6mqivRZNm2gPhPGR8U9l1OPMa1Erb+vJQThLU+KULp
        - 			8VOrvQOBA9raHQnbeccZE3bp7HvyRKQtj21ki8ngVggubMdLWd7YMy4rcBThUdDe
        - 			IQ==
        + 			MIIDLTCCAhWgAwIBAgIISyfpWR4KhRQwDQYJKoZIhvcNAQELBQAwJDEiMCAGA1UE
        + 			AwwZbnNfdGVzdC1zaWduZXJAMTcxMjAwMzYyODAeFw0yNDA0MDEyMDMzNDdaFw0y
        + 			NDA0MDIyMDMzNDhaMCQxIjAgBgNVBAMMGW5zX3Rlc3Qtc2lnbmVyQDE3MTIwMDM2
        + 			MjgwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDHlfsn6jWuk92paAGL
        + 			dvzKNDd+DGYBph6tF/utR6YFOEIUvzOfn/VFJM4ziHLgGWEtK+cFJZ8H8NqRblS1
        + 			0sZgJWxvjpPmcIiM6XLHAxTfPIGp1xB0gmpyp8NzLx4Wf75a8Po1dh48mCeyCCxB
        + 			wlV5/UzWAJbjA5P3AjVKLvZhQxbmzYct3pmMcUmCuuv4PwLYCOpLD4wGWbLmgQbI
        + 			yVdoiUdkJxLX047rKiuWgKnw8Dj/Lfgly2JqFA68H3kwV0vzdbbEoxWm0/q1I/9K
        + 			gSUSbTc6bbmEj6RKjkCNSHvVzs+gWNQP+XE6bo2U4yS2FIF0xCxHG8fdNCpIgfFD
        + 			B2KHAgMBAAGjYzBhMA4GA1UdDwEB/wQEAwICpDAPBgNVHRMBAf8EBTADAQH/MB0G
        + 			A1UdDgQWBBTMdScB2mSW7has+f8PQhZI7FmsaDAfBgNVHSMEGDAWgBTMdScB2mSW
        + 			7has+f8PQhZI7FmsaDANBgkqhkiG9w0BAQsFAAOCAQEAFZ0z6x6Cb8bKlvlnXjKf
        + 			h6tgDhwbWNFWyUXZ2941Iq64pcnF06mzvBElY9qpUDaRLxMKYNhdmExTh4DMlBJP
        + 			Ys6wGFExFE4o7Mfl+6ZpguKyAGWK6VR4KgQv9MYpT3zbzvo2ZYNgLC4v9m7nXrTb
        + 			Xyy/5jHafM6k4/eC46fn4XgNZ+F2Gx7nJpkdVuKjWqwnRcxtnT1a7a5y0mcX3E72
        + 			YoN5RgTtsxvq51w7Flugb41W7xb2I7gKzT61vfJvFwlrIlc3HkfUJHZstm1htQxB
        + 			5bTjrgrISAfFY+W08VrLm5PA7Qaoj+g8nVQ3hZHcBPQDdVdQ7829zCOQz1hWQCDH
        + 			xg==
          			... // 2 identical lines
          			"""
          		),
          		"tls.key": []uint8(
          			"""
          			-----BEGIN RSA PRIVATE KEY-----
        - 			MIIEpQIBAAKCAQEA6DjHBdjki3hpvD53Q0eEQS035nu/4UKX7PD7M8b+3vNFHmem
        - 			Kxa+8nlsIghl0jDNMCzp5aAl2mpsV/NlUpWrqH/MAjLaEts7sqG4R3A18iPuLT5I
        - 			dwLrl2Nem4lP3N1GVQJe/CarA1jeCoOD2dCUyCewBl7xOqXI+jkHY9UbQ16E+J11
        - 			ZRPBmdtylXC9xhJzMAwQvVMe0xnGxLWCXreWgaHZZeLwV697NNinkBPjdPeJm7JG
        - 			zmXPNkDF9MDb2Eu+uhXhCrWWghfeZrC6QrvaDG/YnwvTqSrdX9UX2HlnWoB+Pt/p
        - 			db94wOYMTBQSFg7HPWpr1b+dlXAfdO/0FUUIEwIDAQABAoIBAHqqEycZjI/HiUKw
        - 			VFsrmca71f1ffNnGTW4RVP/iq5qlMet/oJy+JRr73IyVlwNSV/CMqPhsgdI/yP8k
        - 			SG85NDWLW/4FUoAHYh9XoXnK4hQHurYXYjvLRRrwmHbcL11hXNdmqznSSx83gAJZ
        - 			ufoCXbTkkeyrlgz+qYzTNv4bAY2xjCOediCzxEmElB18+s3XUGUqgW5i4I6GTJUL
        - 			YbZM5v0rOikkJ9qtaP9ZWgU+74Msxay5vyaqPndcL00Z5dtqRxDRtlu6WWEIZcj5
        - 			FW+Dev1NzW/haYEycEPggWIksARBQydDICpX5qQTLtwJ78TjUc24kAdehNAIbA73
        - 			ArnlZfECgYEA7JfewnEbkrGT5HMhYp0PWRe/R3m7AsTiNmtmbZEmY0V0MQY/7+Tq
        - 			TKNt+BXxcFip63baIR0Imrn6oqk2Rk1x7KZeFxpoVHQr8RSeBYaXUSsFOdZEj4wh
        - 			y98WBsbCyhlcnZwz5+QDcwSMZqxjnJjroetZDluNWO4A3tuJtgQ59XcCgYEA+0Uc
        - 			3mmFZ9MUPJTdfFuQPbhweFOcBZ+gPT+tmThcdtq5uVH36yUCiIAYVuU4yq5pU/I8
        - 			vCIzkBUynDVkULrIw6JF3hoBHGdUKS4r3FpYMUJBaOvl+YIYiu0qhX3A84hFQ0us
        - 			0uVfEyaDmWm6wRMKZaJy4J7mrNhwgkGsCekM2UUCgYEApP6kkuVeXUcJ5F+GAMeX
        - 			VIKDGs2B5cR4HYt4uyBmrQRaq3W4UQxLeXbf0gs7fSXYGiWgqUceIQliCN4Iw65u
        - 			rKK5K1N3Pq5llpLSQPhDvo9J7bPHzHPlfc/uBHvIjDhzplawvB7/aM7bOx8tuJ/M
        - 			c/c0/BZM1J/ma4Se3RjFcrECgYEA6OS+RUf45qq2bxBr2f/kSdDSLjUQwAWcOCj1
        - 			drbOvTsuOZ7H86NyHaLc7G25neLarXww3w+0Sy/aoP675De8LdgSejIQJYuAbkrl
        - 			THBQnheTGPTNS95RcTMx3lkYpqD/0cgqf68p2E4yW9eryZkkYd+YKqodlMdoQNMt
        - 			6mGgDyECgYEA2jSWCtktj0p95FMfv7EC9tMLqw8XJx1vMfnFw75iakpi9OXjkR/T
        - 			tV11U4MAesFc6bJybg5Y1nkjuyuYEvmlYK7Bbsmu7I6+fYoABnrCxrYChRxFdSdW
        - 			bRMg2YFY236wBoIsNrtWaxLVBeEOaoBvgWa2/7W/2n9WfhxPEYZE9mg=
        + 			MIIEpQIBAAKCAQEAx5X7J+o1rpPdqWgBi3b8yjQ3fgxmAaYerRf7rUemBThCFL8z
        + 			n5/1RSTOM4hy4BlhLSvnBSWfB/DakW5UtdLGYCVsb46T5nCIjOlyxwMU3zyBqdcQ
        + 			dIJqcqfDcy8eFn++WvD6NXYePJgnsggsQcJVef1M1gCW4wOT9wI1Si72YUMW5s2H
        + 			Ld6ZjHFJgrrr+D8C2AjqSw+MBlmy5oEGyMlXaIlHZCcS19OO6yorloCp8PA4/y34
        + 			JctiahQOvB95MFdL83W2xKMVptP6tSP/SoElEm03Om25hI+kSo5AjUh71c7PoFjU
        + 			D/lxOm6NlOMkthSBdMQsRxvH3TQqSIHxQwdihwIDAQABAoIBAQCpx9YdQElmNvcL
        + 			EyStRQ0J3Z2PJnDn2i6iRJKd9yMtsYvVJkl98o6swQCAKgS+yhg2WvBtGnHMSYFE
        + 			0bxR5/lE9NDnnTwHfZdLd5Nh5CcvN8N9fSvMUNzIqBnFtEE+FnER34iZTd+u4Ch0
        + 			dCthzTT1Txq7uUih2PtX1pMKhiSk2vLSb+/Qqx06xeiZW6DezubF2QEyda6Tzuo3
        + 			xQXX2OD55RRQRZnQv5rjSDbMA78JCP0DwKGQoZ09HYg1Q7H8tZANnkTjVigc3vTc
        + 			pJeGIlYcUVzHp4hbQynbLTZYrXCmAAZ1NwhWPl5bUczsQmZCUOiYBLlnITglQv2F
        + 			hqboz4zxAoGBAN5jpQEFSZOtewo++U64KKS8gTwZTtyCDXf29TJcXjq7xRQkjrW1
        + 			3BFC8gda5oKLAg7+iHKr55gNZrB1YcdkgRqppv3tWbDx6jBwl0JbfvSmaI1BhRtY
        + 			U+/XBhaOjmkd8c81R9zqi4pZRduGPITGNY5SIuT05XFJt0IZaxkv5J+lAoGBAOXA
        + 			D+kAaFzAMNV1+PuRCPFW9bLAgv4tycX+mh8jrc+CMJZMnkceDEQy5lAhb1MjJSzW
        + 			QWlLVNgnH2OSXtjac0UL6oA5a6uJfK/nG0VNJyTUM8pb7XUsImhMFRL7FPNSOPLD
        + 			7CPkrgxg7ujbZh1uXS18OL3NgK9RSRQwy/BCIKG7AoGBAIWkrAwo+UZeAortvTSp
        + 			RwN4pNFRBCtPnHhzWHajO51gKdcpAPoCB/X3nSr+XXglwV7xZ15CIDMoGoYAhHom
        + 			088KFOiUMko7ltj1UHD4OxsaxcndjfgY5JhFR8tWcA6LiD1Vb5I7ARBrBagey0+f
        + 			LaARjBa7dQbXneGDFPFV7rZhAoGARPW0ENS9fnF1duzVEfVDgOUAFGoyJ0bpFFPK
        + 			QOR9rBZArSxMKb58IhBBDvYqKwMWinG46njg+4wqoMFzVJWlGals9pXFmpRG56lv
        + 			hwqUYDqNXQTgrlXT8gg8Hxlb/XjFfSCPhWqDT1Xc/+myqczRjPCHO3kuUfENBVFs
        + 			NS40CzcCgYEA1GmN3bEptgfQGP+mbtR9YAJPk0HvecAxcGnRGeVbsImlestnbSF6
        + 			bFGmxcNzS9I1d97uT0zgsnzifDfgToDgayOgo8flm65pJcbj7dmPz9P4MuD4YtzO
        + 			mh68CnJNqfjQmest4Ap505+1pWrvLgokWlkI4w0/V47BiXWb0snRfJ4=
          			... // 2 identical lines
          			"""
          		),
          	},
          	StringData: nil,
        - 	Type:       "SecretTypeTLS",
        + 	Type:       "kubernetes.io/tls",
          }
--- FAIL: TestRotatedSigningCASecretShouldNotUseDelete (3.44s)
FAIL
FAIL	github.com/openshift/library-go/pkg/operator/certrotation	3.561s
FAIL

p0lyn0mial · 2024-04-02T07:50:53Z

pkg/operator/certrotation/signer_test.go

+	clientset.PrependReactor("update", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		switch action := action.(type) {
+		case clienttesting.UpdateActionImpl:
+			indexer.Delete(existing)


indexer.Update doesn't work ?

p0lyn0mial · 2024-04-02T07:51:36Z

pkg/operator/certrotation/signer_test.go

+
+	// the list cache is synced as soon as we have a delete, create, or update
+	clientset.PrependReactor("delete", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		if err = indexer.Delete(existing); err != nil {


shouldn't we fail the test as soon as a call to delete is made ?

p0lyn0mial · 2024-04-02T07:53:20Z

pkg/operator/certrotation/signer_test.go

+
+	// give it a second so we have a unique signer name,
+	// and also unique not-after, and not-before values
+	<-time.After(2 * time.Second)


Is this necessary ? The new certificate will have different keys generated so we will be able to detect the change, no?

pkg/operator/certrotation/signer_test.go

p0lyn0mial · 2024-04-02T07:55:08Z

pkg/operator/certrotation/signer_test.go

+		t.Errorf("expected the secret type to be: %q, but got: %q", corev1.SecretTypeTLS, secretGot.Type)
+	}
+
+	t.Logf("diff: %s", cmp.Diff(secretWant, secretGot))


should we fail if the secrets are different ?

the owner annotation might get added, so the goal of the test is to verify that the crt/key do not change

p0lyn0mial · 2024-04-02T07:55:34Z

pkg/operator/certrotation/signer_test.go

+	}
+
+	t.Logf("diff: %s", cmp.Diff(secretWant, secretGot))
+}


shouldn't we validate clientset.Actions at the end of the test ?

p0lyn0mial · 2024-04-02T07:57:45Z

pkg/operator/certrotation/signer_test.go

+	clientset.PrependReactor("create", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+		switch action := action.(type) {
+		case clienttesting.CreateActionImpl:
+			indexer.Delete(existing)


why do we have to perform deletion here ?

p0lyn0mial · 2024-04-02T07:59:02Z

pkg/operator/certrotation/signer_test.go

+		}
+		return false, nil, nil
+	})
+	clientset.PrependReactor("create", "secrets", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {


why do we need a custom reactor for create call ?

p0lyn0mial · 2024-04-02T16:02:53Z

pkg/operator/certrotation/signer.go

@@ -75,7 +75,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
 	// apply necessary metadata (possibly via delete+recreate) if secret exists
 	// this is done before content update to prevent unexpected rollouts
 	if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(signingCertKeyPairSecret) {
-		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
+		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecretDoNotUse(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)


we should expose an option on EnsureSigningCertKeyPair that will force the usage of ApplySecretDoNotUse. Otherwise components that pull the new version will be broken.

yes, that makes sense, this way we have the client opt-in for the new behavior we are adding, so just revendoring library-go does not automatically changes the client to use the new behavior

p0lyn0mial · 2024-04-02T17:03:52Z

pkg/operator/certrotation/signer_test.go

+	client := clientset.CoreV1().Secrets(ns)
+	newControllerFn := func(ctrlName string, wrapped *wrapped) *RotatedSigningCASecret {
+		recorder := events.NewKubeRecorderWithOptions(clientset.CoreV1().Events(ns), options, "operator", &corev1.ObjectReference{Name: ctrlName, Namespace: ns})
+		return &RotatedSigningCASecret{


I think that we should also create a test for the other controller (RotatedSelfSignedCertKeySecret) , wdyt ?

openshift-ci · 2024-04-02T17:46:06Z

@tkashem: all tests passed!

Full PR test history. Your PR dashboard.

Instructions 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/test-infra repository. I understand the commands that are listed here.

p0lyn0mial · 2024-04-02T17:55:27Z

/lgtm

p0lyn0mial · 2024-04-02T17:56:20Z

/approve

mfojtik · 2024-04-02T17:58:02Z

/approve

openshift-ci · 2024-04-02T17:58:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, p0lyn0mial, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/operator/OWNERS~~ [mfojtik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-04-02T23:46:16Z

@tkashem: Jira Issue OCPBUGS-31384: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-31384 has been moved to the MODIFIED state.

In response to this:

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

p0lyn0mial · 2024-04-05T08:28:58Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-04-05T08:29:46Z

@p0lyn0mial: #1705 failed to apply on top of branch "release-4.15":

Applying: use update only secret-apply for cert rotation logic
Using index info to reconstruct a base tree...
M	pkg/operator/certrotation/signer.go
M	pkg/operator/certrotation/signer_test.go
M	pkg/operator/certrotation/target.go
M	pkg/operator/certrotation/target_test.go
M	pkg/operator/resource/resourceapply/core.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/resource/resourceapply/core.go
Auto-merging pkg/operator/certrotation/target_test.go
CONFLICT (content): Merge conflict in pkg/operator/certrotation/target_test.go
Auto-merging pkg/operator/certrotation/target.go
Auto-merging pkg/operator/certrotation/signer_test.go
CONFLICT (content): Merge conflict in pkg/operator/certrotation/signer_test.go
Auto-merging pkg/operator/certrotation/signer.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 use update only secret-apply for cert rotation logic
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.15

Instructions 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/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 29, 2024

openshift-ci bot requested review from soltysh and stlaz March 29, 2024 20:51

tkashem force-pushed the secret-hack-4.16 branch 3 times, most recently from dd8c828 to 808b66e Compare March 29, 2024 22:07

tkashem force-pushed the secret-hack-4.16 branch from 808b66e to 7964d6d Compare March 29, 2024 22:20

tkashem force-pushed the secret-hack-4.16 branch 2 times, most recently from 52e5349 to 5930e37 Compare April 1, 2024 14:23

benluddy reviewed Apr 1, 2024

View reviewed changes

tkashem force-pushed the secret-hack-4.16 branch from 5930e37 to faf58ee Compare April 1, 2024 21:05

tkashem changed the title ~~[WIP] use update only secret-apply for cert rotation logic~~ use update only secret-apply for cert rotation logic Apr 1, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2024

tkashem force-pushed the secret-hack-4.16 branch from faf58ee to c6b5cf6 Compare April 1, 2024 21:28

p0lyn0mial reviewed Apr 2, 2024

View reviewed changes

pkg/operator/certrotation/signer_test.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Apr 2, 2024

View reviewed changes

tkashem force-pushed the secret-hack-4.16 branch from c6b5cf6 to f568ed3 Compare April 2, 2024 14:06

p0lyn0mial reviewed Apr 2, 2024

View reviewed changes

tkashem force-pushed the secret-hack-4.16 branch from f568ed3 to 374ea07 Compare April 2, 2024 16:39

p0lyn0mial reviewed Apr 2, 2024

View reviewed changes

use update only secret-apply for cert rotation logic

96e8597

tkashem force-pushed the secret-hack-4.16 branch from 374ea07 to 96e8597 Compare April 2, 2024 17:38

openshift-ci bot assigned p0lyn0mial Apr 2, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2024

openshift-merge-bot bot merged commit f5bf387 into openshift:master Apr 2, 2024
4 checks passed

This was referenced Apr 2, 2024

certrotation: a unit test for the cert issue #1699

Closed

OCPBUGS-31384: use RotatedSigningCASecret and RotatedSelfSignedCertKeySecret only in update mode openshift/cluster-kube-controller-manager-operator#800

Merged

tkashem mentioned this pull request Apr 2, 2024

OCPBUGS-31384: use RotatedSigningCASecret controller in update only mode openshift/cluster-kube-apiserver-operator#1659

Merged

tkashem changed the title ~~use update only secret-apply for cert rotation logic~~ OCPBUGS-31384: use update only secret-apply for cert rotation logic Apr 2, 2024

p0lyn0mial mentioned this pull request Apr 3, 2024

use RotatedSigningCASecret and RotatedSelfSignedCertKeySecret only in update mode openshift/cluster-etcd-operator#1234

Closed

p0lyn0mial mentioned this pull request Apr 5, 2024

OCPBUGS-31807: [release-4.15] use update only secret-apply for cert rotation logic #1710

Merged

p0lyn0mial mentioned this pull request Aug 12, 2024

OCPBUGS-38335: certrotation: avoid using applySecret to catch racy cert updates #1772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-31384: use update only secret-apply for cert rotation logic #1705

OCPBUGS-31384: use update only secret-apply for cert rotation logic #1705

tkashem commented Mar 29, 2024

tkashem commented Mar 29, 2024

tkashem commented Mar 29, 2024

benluddy Apr 1, 2024

tkashem Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

tkashem Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

tkashem Apr 1, 2024

benluddy Apr 1, 2024

tkashem Apr 1, 2024 •

edited

Loading

tkashem commented Apr 1, 2024

tkashem commented Apr 1, 2024 •

edited

Loading

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

tkashem Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

tkashem Apr 2, 2024 •

edited

Loading

p0lyn0mial Apr 2, 2024

openshift-ci bot commented Apr 2, 2024

p0lyn0mial commented Apr 2, 2024

p0lyn0mial commented Apr 2, 2024

mfojtik commented Apr 2, 2024

openshift-ci bot commented Apr 2, 2024

openshift-ci-robot commented Apr 2, 2024

p0lyn0mial commented Apr 5, 2024

openshift-cherrypick-robot commented Apr 5, 2024

		// NOTE: DO NOT USE, this is meant to be a short term hack for a very specific use case,
		// and will not work for your application

		indexer.Delete(existing)
		if err := indexer.Add(action.GetObject()); err != nil {

OCPBUGS-31384: use update only secret-apply for cert rotation logic #1705

OCPBUGS-31384: use update only secret-apply for cert rotation logic #1705

Conversation

tkashem commented Mar 29, 2024

tkashem commented Mar 29, 2024

tkashem commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

tkashem commented Apr 1, 2024

tkashem commented Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 2, 2024

p0lyn0mial commented Apr 2, 2024

p0lyn0mial commented Apr 2, 2024

mfojtik commented Apr 2, 2024

openshift-ci bot commented Apr 2, 2024

openshift-ci-robot commented Apr 2, 2024

p0lyn0mial commented Apr 5, 2024

openshift-cherrypick-robot commented Apr 5, 2024

tkashem Apr 1, 2024 •

edited

Loading

tkashem commented Apr 1, 2024 •

edited

Loading

tkashem Apr 2, 2024 •

edited

Loading