diff --git a/pkg/cmd/render/certs.go b/pkg/cmd/render/certs.go index f4787b3ec..1f11b077f 100644 --- a/pkg/cmd/render/certs.go +++ b/pkg/cmd/render/certs.go @@ -36,30 +36,26 @@ func createCertSecrets(nodes []*corev1.Node) ([]corev1.Secret, []corev1.ConfigMa ManagementState: operatorv1.Managed, }, }, &operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 1, - OperatorStatus: operatorv1.OperatorStatus{Conditions: []operatorv1.OperatorCondition{}}, - NodeStatuses: []operatorv1.NodeStatus{{CurrentRevision: 1, TargetRevision: 1}}, + OperatorStatus: operatorv1.OperatorStatus{Conditions: []operatorv1.OperatorCondition{}}, + NodeStatuses: []operatorv1.NodeStatus{}, }, nil, nil) kubeInformers := v1helpers.NewKubeInformersForNamespaces(fakeKubeClient, "", "kube-system", operatorclient.TargetNamespace, operatorclient.OperatorNamespace, operatorclient.GlobalUserSpecifiedConfigNamespace) - secretInformer := kubeInformers.InformersFor(operatorclient.GlobalUserSpecifiedConfigNamespace).Core().V1().Secrets() - secretClient := fakeKubeClient.CoreV1() - secretLister := &tlshelpers.SecretClientLister{ - SecretClient: secretClient.Secrets(operatorclient.GlobalUserSpecifiedConfigNamespace), - Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace, - } + secretInformer := kubeInformers.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets() + secretLister := secretInformer.Lister() + secretClient := v1helpers.CachedSecretGetter(fakeKubeClient.CoreV1(), kubeInformers) recorder := events.NewInMemoryRecorder("etcd") // create openshift-config signers first, they will remain in openshift-config and are needed for the controller sync loop to function // TODO(thomas): once the rotation process is in place, we can remove that special case etcdSignerCert := tlshelpers.CreateBootstrapSignerCert(secretInformer, secretLister, secretClient, recorder) - _, _, err := etcdSignerCert.EnsureSigningCertKeyPair(context.Background()) + _, err := etcdSignerCert.EnsureSigningCertKeyPair(context.Background()) if err != nil { return nil, nil, fmt.Errorf("could not create etcd signer certificate: %w", err) } metricsSignerCert := tlshelpers.CreateBootstrapMetricsSignerCert(secretInformer, secretLister, secretClient, recorder) - _, _, err = metricsSignerCert.EnsureSigningCertKeyPair(context.Background()) + _, err = metricsSignerCert.EnsureSigningCertKeyPair(context.Background()) if err != nil { return nil, nil, fmt.Errorf("could not create etcd metrics signer certificate: %w", err) } diff --git a/pkg/cmd/render/certs_test.go b/pkg/cmd/render/certs_test.go index 2267881d5..039f377c4 100644 --- a/pkg/cmd/render/certs_test.go +++ b/pkg/cmd/render/certs_test.go @@ -133,7 +133,6 @@ func assertBundleCorrectness(t *testing.T, secrets []corev1.Secret, bundles []co return bytes.Compare(bundleCerts[i].Raw, bundleCerts[j].Raw) < 0 }) - require.Equal(t, len(signers), len(bundleCerts)) require.Equal(t, signers, bundleCerts) } } diff --git a/pkg/operator/ceohelpers/common.go b/pkg/operator/ceohelpers/common.go index eaec56684..0f8b0bcd2 100644 --- a/pkg/operator/ceohelpers/common.go +++ b/pkg/operator/ceohelpers/common.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/cluster-etcd-operator/pkg/dnshelpers" "github.com/openshift/cluster-etcd-operator/pkg/etcdcli" "net" @@ -30,8 +29,6 @@ const MachineDeletionHookName = "EtcdQuorumOperator" // MachineDeletionHookOwner holds an owner of the Machine Deletion Hook const MachineDeletionHookOwner = "clusteroperator/etcd" -var RevisionRolloutInProgressErr = fmt.Errorf("revision rollout in progress, can't establish current revision") - // ReadDesiredControlPlaneReplicasCount reads the current Control Plane replica count func ReadDesiredControlPlaneReplicasCount(operatorClient v1helpers.StaticPodOperatorClient) (int, error) { operatorSpec, _, _, err := operatorClient.GetStaticPodOperatorState() @@ -200,38 +197,3 @@ func VotingMemberIPListSet(ctx context.Context, cli etcdcli.EtcdClient) (sets.St return currentVotingMemberIPListSet, nil } - -// RevisionRolloutInProgress will return true if any node status reports its target revision is different from the current revision and the latest known revision. -func RevisionRolloutInProgress(status operatorv1.StaticPodOperatorStatus) bool { - latestRevision := status.LatestAvailableRevision - for _, nodeStatus := range status.NodeStatuses { - if (nodeStatus.TargetRevision > 0 && nodeStatus.CurrentRevision != nodeStatus.TargetRevision) || - nodeStatus.CurrentRevision != latestRevision { - return true - } - } - - return false -} - -// CurrentRevision will only return the current revision if no revision rollout is in progress and all revisions across nodes -// are the exact same. Otherwise, an error will be returned. -func CurrentRevision(status operatorv1.StaticPodOperatorStatus) (int32, error) { - if RevisionRolloutInProgress(status) { - return 0, RevisionRolloutInProgressErr - } - - if len(status.NodeStatuses) == 0 { - return 0, fmt.Errorf("no node status") - } - - latestRevision := status.LatestAvailableRevision - for _, nodeStatus := range status.NodeStatuses { - if latestRevision != nodeStatus.CurrentRevision { - return 0, fmt.Errorf("node [%s] is not on latest revision yet: %d vs latest revision %d", - nodeStatus.NodeName, nodeStatus.CurrentRevision, latestRevision) - } - } - - return latestRevision, nil -} diff --git a/pkg/operator/ceohelpers/common_test.go b/pkg/operator/ceohelpers/common_test.go index bfeb170b0..d8bdccc61 100644 --- a/pkg/operator/ceohelpers/common_test.go +++ b/pkg/operator/ceohelpers/common_test.go @@ -1,8 +1,6 @@ package ceohelpers import ( - "errors" - "github.com/stretchr/testify/require" "testing" "k8s.io/apimachinery/pkg/runtime" @@ -77,152 +75,3 @@ func TestReadDesiredControlPlaneReplicaCount(t *testing.T) { }) } } - -func TestRevisionRolloutInProgress(t *testing.T) { - scenarios := []struct { - name string - status operatorv1.StaticPodOperatorStatus - expected bool - }{ - { - name: "revs equal single node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 1, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 1, TargetRevision: 1}, - }, - }, - expected: false, - }, - { - name: "revs not equal single node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 1, TargetRevision: 3}, - }, - }, - expected: true, - }, - { - name: "revs not equal multi node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 3}, - {NodeName: "node-2", CurrentRevision: 1, TargetRevision: 3}, - }, - }, - expected: true, - }, - { - name: "revs equal multi node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 3}, - {NodeName: "node-2", CurrentRevision: 3, TargetRevision: 3}, - }, - }, - expected: false, - }, - } - - for _, scenario := range scenarios { - t.Run(scenario.name, func(t *testing.T) { - require.Equal(t, scenario.expected, RevisionRolloutInProgress(scenario.status)) - }) - } -} - -func TestCurrentRevision(t *testing.T) { - scenarios := []struct { - name string - status operatorv1.StaticPodOperatorStatus - expected int32 - expectedErr error - }{ - { - name: "revs equal single node", - status: operatorv1.StaticPodOperatorStatus{ - NodeStatuses: []operatorv1.NodeStatus{}, - }, - expectedErr: errors.New("no node status"), - }, - { - name: "revs equal single node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 22, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 22, TargetRevision: 22}, - }, - }, - expected: 22, - }, - { - name: "revs not equal single node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 1, TargetRevision: 3}, - }, - }, - expected: 0, - expectedErr: RevisionRolloutInProgressErr, - }, - { - name: "target revs not equal multi node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 3}, - {NodeName: "node-2", CurrentRevision: 1, TargetRevision: 3}, - }, - }, - expected: 0, - expectedErr: RevisionRolloutInProgressErr, - }, - { - name: "revs equal multi node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 0}, - {NodeName: "node-2", CurrentRevision: 3, TargetRevision: 0}, - }, - }, - expected: 3, - }, - { - name: "revs differ multi node", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 3, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 3}, - {NodeName: "node-2", CurrentRevision: 2, TargetRevision: 2}, - }, - }, - expected: 0, - expectedErr: errors.New("revision rollout in progress, can't establish current revision"), - }, - { - name: "latest rev far ahead", - status: operatorv1.StaticPodOperatorStatus{ - LatestAvailableRevision: 25, - NodeStatuses: []operatorv1.NodeStatus{ - {NodeName: "node-1", CurrentRevision: 3, TargetRevision: 3}, - }, - }, - expected: 0, - expectedErr: errors.New("revision rollout in progress, can't establish current revision"), - }, - } - - for _, scenario := range scenarios { - t.Run(scenario.name, func(t *testing.T) { - revision, err := CurrentRevision(scenario.status) - require.Equal(t, scenario.expectedErr, err) - require.Equal(t, scenario.expected, revision) - }) - } -} diff --git a/pkg/operator/etcdcertsigner/etcdcertsignercontroller.go b/pkg/operator/etcdcertsigner/etcdcertsignercontroller.go index af6d35006..1fd686b2b 100644 --- a/pkg/operator/etcdcertsigner/etcdcertsignercontroller.go +++ b/pkg/operator/etcdcertsigner/etcdcertsignercontroller.go @@ -2,18 +2,14 @@ package etcdcertsigner import ( "context" - "crypto/x509" "fmt" - "strconv" "strings" "time" - "github.com/openshift/library-go/pkg/crypto" corev1informers "k8s.io/client-go/informers/core/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" - "k8s.io/klog/v2" apiannotations "github.com/openshift/api/annotations" operatorv1 "github.com/openshift/api/operator/v1" @@ -91,11 +87,8 @@ func NewEtcdCertSignerController( ) factory.Controller { eventRecorder = eventRecorder.WithComponentSuffix("etcd-cert-signer-controller") cmInformer := kubeInformers.InformersFor(operatorclient.TargetNamespace).Core().V1().ConfigMaps() - cmGetter := kubeClient.CoreV1() - cmLister := &tlshelpers.ConfigMapClientLister{ - ConfigMapClient: cmGetter.ConfigMaps(operatorclient.TargetNamespace), - Namespace: operatorclient.TargetNamespace, - } + cmLister := cmInformer.Lister() + cmGetter := v1helpers.CachedConfigMapGetter(kubeClient.CoreV1(), kubeInformers) signerCaBundle := tlshelpers.CreateSignerCertRotationBundleConfigMap( cmInformer, cmLister, @@ -111,11 +104,8 @@ func NewEtcdCertSignerController( ) secretInformer := kubeInformers.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets() - secretClient := kubeClient.CoreV1() - secretLister := &tlshelpers.SecretClientLister{ - SecretClient: secretClient.Secrets(operatorclient.TargetNamespace), - Namespace: operatorclient.TargetNamespace, - } + secretLister := secretInformer.Lister() + secretClient := v1helpers.CachedSecretGetter(kubeClient.CoreV1(), kubeInformers) signerCert := tlshelpers.CreateSignerCert(secretInformer, secretLister, secretClient, eventRecorder) etcdClientCert := tlshelpers.CreateEtcdClientCert(secretInformer, secretLister, secretClient, eventRecorder) @@ -168,22 +158,7 @@ func (c *EtcdCertSignerController) sync(ctx context.Context, syncCtx factory.Syn return fmt.Errorf("skipping EtcdCertSignerController reconciliation due to insufficient quorum") } - _, currentStatus, _, err := c.operatorClient.GetStaticPodOperatorState() - if currentStatus == nil || err != nil { - return fmt.Errorf("skipping EtcdCertSignerController can't get current static pod status: %w", err) - } - - if ceohelpers.RevisionRolloutInProgress(*currentStatus) { - klog.V(4).Infof("skipping EtcdCertSignerController revision rollout in progress") - return nil - } - - currentRevision, err := ceohelpers.CurrentRevision(*currentStatus) - if err != nil { - return fmt.Errorf("skipping EtcdCertSignerController can't get current revision: %w", err) - } - - if err := c.syncAllMasterCertificates(ctx, syncCtx.Recorder(), currentStatus, currentRevision); err != nil { + if err := c.syncAllMasterCertificates(ctx, syncCtx.Recorder()); err != nil { _, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ Type: "EtcdCertSignerControllerDegraded", Status: operatorv1.ConditionTrue, @@ -205,7 +180,7 @@ func (c *EtcdCertSignerController) sync(ctx context.Context, syncCtx factory.Syn return updateErr } -func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context, recorder events.Recorder, status *operatorv1.StaticPodOperatorStatus, currentRevision int32) error { +func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context, recorder events.Recorder) error { // TODO(thomas): it is of utmost importance to keep the existing signer certs for now // when we just create a new signer cert, the new revision does not allow the peer to join the existing two-node // cluster based on the old CA. Any newly rotated additions will come for free through the signerCaBundle and will work out of the box. @@ -215,16 +190,26 @@ func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context return err } + // EnsureConfigMapCABundle is stateful w.r.t to the configmap it manages, so we can simply add it to the bundle before the new one + _, err = c.certConfig.signerCaBundle.EnsureConfigMapCABundle(ctx, signerCaPair) + if err != nil { + return fmt.Errorf("error on ensuring signer bundle for existing pair: %w", err) + } + // TODO(thomas): we need to transition that new signer as a replacement for the above - today we only bundle it - newSignerCaPair, signerUpdated, err := c.certConfig.signerCert.EnsureSigningCertKeyPair(ctx) + newSignerCaPair, err := c.certConfig.signerCert.EnsureSigningCertKeyPair(ctx) if err != nil { return fmt.Errorf("error on ensuring etcd-signer cert: %w", err) } - if signerUpdated { - if err := c.updateCertRevision(ctx, c.certConfig.signerCert.Name, currentRevision); err != nil { - return fmt.Errorf("error while updating status rev: %w", err) - } + signerBundle, err := c.certConfig.signerCaBundle.EnsureConfigMapCABundle(ctx, newSignerCaPair) + if err != nil { + return fmt.Errorf("error on ensuring signer bundle for new pair: %w", err) + } + + _, err = c.certConfig.etcdClientCert.EnsureTargetCertKeyPair(ctx, signerCaPair, signerBundle) + if err != nil { + return fmt.Errorf("error on ensuring etcd client cert: %w", err) } metricsSignerCaPair, err := tlshelpers.ReadConfigMetricsSignerCert(ctx, c.secretClient) @@ -232,32 +217,25 @@ func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context return err } - // TODO(thomas): we need to transition that new signer as a replacement for the above - today we only bundle it - newMetricsSignerCaPair, metricsSignerUpdated, err := c.certConfig.metricsSignerCert.EnsureSigningCertKeyPair(ctx) + _, err = c.certConfig.metricsSignerCaBundle.EnsureConfigMapCABundle(ctx, metricsSignerCaPair) if err != nil { - return fmt.Errorf("error on ensuring metrics-signer cert: %w", err) + return fmt.Errorf("error on ensuring metrics signer bundle for existing pair: %w", err) } - if metricsSignerUpdated { - if err := c.updateCertRevision(ctx, c.certConfig.metricsSignerCert.Name, currentRevision); err != nil { - return fmt.Errorf("error while updating status rev: %w", err) - } + // TODO(thomas): we need to transition that new signer as a replacement for the above - today we only bundle it + newMetricsSignerCaPair, err := c.certConfig.metricsSignerCert.EnsureSigningCertKeyPair(ctx) + if err != nil { + return fmt.Errorf("error on ensuring metrics-signer cert: %w", err) } - signerBundle, metricsSignerBundle, err := c.ensureBundles(ctx, signerCaPair, newSignerCaPair, metricsSignerCaPair, newMetricsSignerCaPair) + metricsSignerBundle, err := c.certConfig.metricsSignerCaBundle.EnsureConfigMapCABundle(ctx, newMetricsSignerCaPair) if err != nil { - return fmt.Errorf("error on ensuring bundles: %w", err) + return fmt.Errorf("error on ensuring metrics signer bundle: %w", err) } - // TODO(thomas): if either of the signers were updated, we only allow the next revision to update its leaf certs for safety - - // ----------------------------------------------------------------- - // Leaf Certificates - // ----------------------------------------------------------------- - - err = c.ensureClientCerts(ctx, metricsSignerCaPair, metricsSignerBundle, signerCaPair, signerBundle) + _, err = c.certConfig.metricsClientCert.EnsureTargetCertKeyPair(ctx, metricsSignerCaPair, metricsSignerBundle) if err != nil { - return fmt.Errorf("error on client certs: %w", err) + return fmt.Errorf("error on ensuring metrics client cert: %w", err) } nodeCfgs, err := c.createNodeCertConfigs() @@ -295,7 +273,7 @@ func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context // pod controller to watch. A single secret ensures that a cert change // (e.g. node addition or cert rotation) triggers at most one static pod // rollout. If multiple secrets were written, the static pod controller - // might initiate rollout before all secrets had been signerUpdated. + // might initiate rollout before all secrets had been updated. secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: operatorclient.TargetNamespace, @@ -312,40 +290,6 @@ func (c *EtcdCertSignerController) syncAllMasterCertificates(ctx context.Context return err } -func (c *EtcdCertSignerController) ensureBundles(ctx context.Context, signerCaPair *crypto.CA, newSignerCaPair *crypto.CA, metricsSignerCaPair *crypto.CA, newMetricsSignerCaPair *crypto.CA) (signerBundle []*x509.Certificate, metricsSignerBundle []*x509.Certificate, err error) { - // EnsureConfigMapCABundle is stateful w.r.t to the configmap it manages, so we can simply add the existing signer to the bundle before the new one - _, err = c.certConfig.signerCaBundle.EnsureConfigMapCABundle(ctx, signerCaPair) - if err != nil { - return nil, nil, fmt.Errorf("error on ensuring signer bundle for existing pair: %w", err) - } - signerBundle, err = c.certConfig.signerCaBundle.EnsureConfigMapCABundle(ctx, newSignerCaPair) - if err != nil { - return nil, nil, fmt.Errorf("error on ensuring signer bundle for new pair: %w", err) - } - _, err = c.certConfig.metricsSignerCaBundle.EnsureConfigMapCABundle(ctx, metricsSignerCaPair) - if err != nil { - return nil, nil, fmt.Errorf("error on ensuring metrics signer bundle for existing pair: %w", err) - } - metricsSignerBundle, err = c.certConfig.metricsSignerCaBundle.EnsureConfigMapCABundle(ctx, newMetricsSignerCaPair) - if err != nil { - return nil, nil, fmt.Errorf("error on ensuring metrics signer bundle: %w", err) - } - return -} - -func (c *EtcdCertSignerController) ensureClientCerts(ctx context.Context, metricsSignerCaPair *crypto.CA, metricsSignerBundle []*x509.Certificate, signerCaPair *crypto.CA, signerBundle []*x509.Certificate) error { - _, err := c.certConfig.metricsClientCert.EnsureTargetCertKeyPair(ctx, metricsSignerCaPair, metricsSignerBundle) - if err != nil { - return fmt.Errorf("error on ensuring metrics client cert: %w", err) - } - - _, err = c.certConfig.etcdClientCert.EnsureTargetCertKeyPair(ctx, signerCaPair, signerBundle) - if err != nil { - return fmt.Errorf("error on ensuring etcd client cert: %w", err) - } - return nil -} - // Nodes change internally the whole time (e.g. due to IPs changing), we thus re-create the cert configs every sync loop. // This works, because initialization is cheap and all state is kept in secrets, configmaps and their annotations. func (c *EtcdCertSignerController) createNodeCertConfigs() ([]*nodeCertConfigs, error) { @@ -394,29 +338,6 @@ func (c *EtcdCertSignerController) createNodeCertConfigs() ([]*nodeCertConfigs, return cfgs, nil } -func (c *EtcdCertSignerController) updateCertRevision(ctx context.Context, certSecretName string, revision int32) error { - _, _, err := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ - Type: fmt.Sprintf("EtcdCertSignerController-rotation-rev-%s", certSecretName), - Status: operatorv1.ConditionTrue, - Message: fmt.Sprintf("%d", revision), - })) - - return err -} - -func getCertRotationRevision(status *operatorv1.StaticPodOperatorStatus, certSecretName string) (int32, error) { - condition := v1helpers.FindOperatorCondition(status.Conditions, fmt.Sprintf("EtcdCertSignerController-rotation-rev-%s", certSecretName)) - if condition == nil { - return int32(0), nil - } - - rev, err := strconv.ParseInt(condition.Message, 10, 32) - if err != nil { - return 0, fmt.Errorf("could not parse condition message for cert secret [%s], msg=[%s]: %w", certSecretName, condition.Message, err) - } - return int32(rev), nil -} - func addCertSecretToMap(allCerts map[string][]byte, secret *corev1.Secret) map[string][]byte { for k, v := range secret.Data { // in library-go the certs are stored as tls.crt and tls.key - which we trim away to stay backward compatible diff --git a/pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go b/pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go index 5cff08234..fc9018e74 100644 --- a/pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go +++ b/pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go @@ -35,7 +35,7 @@ import ( ) func TestSyncSkipsOnInsufficientQuorum(t *testing.T) { - _, _, controller, recorder := setupController(t, []runtime.Object{}) + _, controller, recorder := setupController(t, []runtime.Object{}) err := controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder)) require.NoError(t, err) @@ -44,44 +44,18 @@ func TestSyncSkipsOnInsufficientQuorum(t *testing.T) { u.FakeEtcdMemberWithoutServer(0), u.FakeEtcdMemberWithoutServer(1), } - status := u.StaticPodOperatorStatus( - u.WithLatestRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - ) - _, _, controller, recorder = setupControllerWithEtcd(t, []runtime.Object{u.BootstrapConfigMap(u.WithBootstrapStatus("complete"))}, etcdMembers, status) + _, controller, recorder = setupControllerWithEtcd(t, []runtime.Object{ + u.BootstrapConfigMap(u.WithBootstrapStatus("complete")), + }, etcdMembers) err = controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder)) assert.Equal(t, "EtcdCertSignerController can't evaluate whether quorum is safe: etcd cluster has quorum of 2 which is not fault tolerant: [{Member:name:\"etcd-0\" peerURLs:\"https://10.0.0.1:2380\" clientURLs:\"https://10.0.0.1:2907\" Healthy:true Took: Error:} {Member:ID:1 name:\"etcd-1\" peerURLs:\"https://10.0.0.2:2380\" clientURLs:\"https://10.0.0.2:2907\" Healthy:true Took: Error:}]", err.Error()) } -func TestSyncSkipsOnRevisionRollingOut(t *testing.T) { - etcdMembers := []*etcdserverpb.Member{ - u.FakeEtcdMemberWithoutServer(0), - u.FakeEtcdMemberWithoutServer(1), - } - status := u.StaticPodOperatorStatus( - u.WithLatestRevision(5), - u.WithNodeStatusAtCurrentRevision(5), - u.WithNodeStatusAtCurrentRevision(5), - u.WithNodeStatusAtCurrentRevision(3), - ) - fakeClient, _, controller, recorder := setupControllerWithEtcd(t, []runtime.Object{u.BootstrapConfigMap(u.WithBootstrapStatus("complete"))}, etcdMembers, status) - err := controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder)) - require.Nil(t, err) - // ensure no secret has been queried to determine that we early exited the controller - for _, action := range fakeClient.Actions() { - if action.Matches("get", "secrets") { - require.Fail(t, "found action that queried a secret, assuming the operator logic ran") - } - } -} - // Validate that a successful test run will result in a secret per // cert type per node and an aggregated secret per cert type. func TestSyncAllMasters(t *testing.T) { - fakeKubeClient, fakeOperatorClient, controller, recorder := setupController(t, []runtime.Object{}) + fakeKubeClient, controller, recorder := setupController(t, []runtime.Object{}) require.NoError(t, controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder))) nodes, secretMap := allNodesAndSecrets(t, fakeKubeClient) @@ -89,11 +63,10 @@ func TestSyncAllMasters(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) } func TestNewNodeAdded(t *testing.T) { - fakeKubeClient, fakeOperatorClient, controller, recorder := setupController(t, []runtime.Object{}) + fakeKubeClient, controller, recorder := setupController(t, []runtime.Object{}) require.NoError(t, controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder))) @@ -102,7 +75,6 @@ func TestNewNodeAdded(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) _, err := fakeKubeClient.CoreV1().Nodes().Create(context.TODO(), u.FakeNode("master-3", u.WithMasterLabel(), u.WithNodeInternalIP("10.0.0.4")), metav1.CreateOptions{}) require.NoError(t, err) @@ -114,11 +86,10 @@ func TestNewNodeAdded(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) } func TestNodeChangingIPs(t *testing.T) { - fakeKubeClient, fakeOperatorClient, controller, recorder := setupController(t, []runtime.Object{}) + fakeKubeClient, controller, recorder := setupController(t, []runtime.Object{}) require.NoError(t, controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder))) @@ -127,7 +98,6 @@ func TestNodeChangingIPs(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) n, err := fakeKubeClient.CoreV1().Nodes().Get(context.TODO(), "master-1", metav1.GetOptions{}) require.NoError(t, err) @@ -143,11 +113,10 @@ func TestNodeChangingIPs(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) } func TestClientCertsRemoval(t *testing.T) { - fakeKubeClient, fakeOperatorClient, controller, recorder := setupController(t, []runtime.Object{}) + fakeKubeClient, controller, recorder := setupController(t, []runtime.Object{}) require.NoError(t, controller.Sync(context.TODO(), factory.NewSyncContext("test", recorder))) @@ -156,7 +125,6 @@ func TestClientCertsRemoval(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) oldClientCert, err := fakeKubeClient.CoreV1().Secrets(operatorclient.TargetNamespace).Get(context.TODO(), tlshelpers.EtcdClientCertSecretName, metav1.GetOptions{}) require.NoError(t, err) @@ -176,15 +144,13 @@ func TestClientCertsRemoval(t *testing.T) { assertNodeCerts(t, nodes, secretMap) assertStaticPodAllCerts(t, nodes, secretMap) assertClientCerts(t, secretMap) - assertOperatorStatus(t, fakeOperatorClient) - // test that the secrets actually differ and the cert was regenerated require.NotEqual(t, oldClientCert.Data, secretMap[tlshelpers.EtcdClientCertSecretName]) require.NotEqual(t, oldMetricClientCert.Data, secretMap[tlshelpers.EtcdMetricsClientCertSecretName]) } func TestSecretApplyFailureSyncError(t *testing.T) { - fakeKubeClient, _, controller, recorder := setupController(t, []runtime.Object{}) + fakeKubeClient, controller, recorder := setupController(t, []runtime.Object{}) fakeKubeClient.PrependReactor("create", "secrets", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, fmt.Errorf("apply failed") }) @@ -248,17 +214,6 @@ func assertClientCerts(t *testing.T, secretMap map[string]corev1.Secret) { require.Containsf(t, secretMap, tlshelpers.EtcdMetricsClientCertSecretName, "expected secret/%s to exist", tlshelpers.EtcdMetricsClientCertSecretName) } -func assertOperatorStatus(t *testing.T, client v1helpers.StaticPodOperatorClient) { - _, status, _, _ := client.GetStaticPodOperatorState() - revision, err := getCertRotationRevision(status, tlshelpers.EtcdSignerCertSecretName) - require.NoError(t, err) - require.Equal(t, int32(3), revision) - - revision, err = getCertRotationRevision(status, tlshelpers.EtcdMetricsSignerCertSecretName) - require.NoError(t, err) - require.Equal(t, int32(3), revision) -} - func checkCertNodeValidity(t *testing.T, node corev1.Node, certName, keyName string, secretData map[string][]byte) { cfg, err := crypto.GetTLSCertificateConfigFromBytes(secretData[certName], secretData[keyName]) require.NoError(t, err) @@ -286,28 +241,17 @@ func checkCertPairSecret(t *testing.T, secretName, certName, keyName string, sec } } -func setupController(t *testing.T, objects []runtime.Object) (*fake.Clientset, v1helpers.StaticPodOperatorClient, factory.Controller, events.Recorder) { +func setupController(t *testing.T, objects []runtime.Object) (*fake.Clientset, factory.Controller, events.Recorder) { etcdMembers := []*etcdserverpb.Member{ u.FakeEtcdMemberWithoutServer(0), u.FakeEtcdMemberWithoutServer(1), u.FakeEtcdMemberWithoutServer(2), } - status := u.StaticPodOperatorStatus( - u.WithLatestRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - u.WithNodeStatusAtCurrentRevision(3), - ) - - return setupControllerWithEtcd(t, objects, etcdMembers, status) + return setupControllerWithEtcd(t, objects, etcdMembers) } // setupController configures EtcdCertSignerController for testing with etcd members. -func setupControllerWithEtcd( - t *testing.T, - objects []runtime.Object, - etcdMembers []*etcdserverpb.Member, - staticPodStatus *operatorv1.StaticPodOperatorStatus) (*fake.Clientset, v1helpers.StaticPodOperatorClient, factory.Controller, events.Recorder) { +func setupControllerWithEtcd(t *testing.T, objects []runtime.Object, etcdMembers []*etcdserverpb.Member) (*fake.Clientset, factory.Controller, events.Recorder) { // Add nodes and CAs objects = append(objects, &corev1.Namespace{ @@ -346,7 +290,12 @@ func setupControllerWithEtcd( ManagementState: operatorv1.Managed, }, }, - staticPodStatus, + u.StaticPodOperatorStatus( + u.WithLatestRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + u.WithNodeStatusAtCurrentRevision(3), + ), nil, nil, ) @@ -391,7 +340,7 @@ func setupControllerWithEtcd( kubeInformerForNamespace.InformersFor(ns).WaitForCacheSync(stopChan) } - return fakeKubeClient, fakeOperatorClient, controller, recorder + return fakeKubeClient, controller, recorder } func newCASecret(t *testing.T, secretName string) *corev1.Secret { diff --git a/pkg/testutils/testutils.go b/pkg/testutils/testutils.go index 8388808e8..3ceb2ad81 100644 --- a/pkg/testutils/testutils.go +++ b/pkg/testutils/testutils.go @@ -243,7 +243,6 @@ func WithNodeStatusAtCurrentRevision(current int32) func(*operatorv1.StaticPodOp return func(status *operatorv1.StaticPodOperatorStatus) { status.NodeStatuses = append(status.NodeStatuses, operatorv1.NodeStatus{ CurrentRevision: current, - TargetRevision: current, }) } } @@ -253,7 +252,6 @@ func WithNodeStatusAtCurrentRevisionNamed(current int32, name string) func(*oper status.NodeStatuses = append(status.NodeStatuses, operatorv1.NodeStatus{ NodeName: name, CurrentRevision: current, - TargetRevision: current, }) } } diff --git a/pkg/tlshelpers/client_lister.go b/pkg/tlshelpers/client_lister.go deleted file mode 100644 index dd4706a07..000000000 --- a/pkg/tlshelpers/client_lister.go +++ /dev/null @@ -1,77 +0,0 @@ -package tlshelpers - -import ( - "context" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - v1 "k8s.io/client-go/kubernetes/typed/core/v1" - corev1listers "k8s.io/client-go/listers/core/v1" -) - -// Below lister implementations are required to make the library-go certificate logic more robust. -// Especially in render, we were relying on a race condition between the informer cache and the client -// to create the bundles correctly. This lister always queries the API (or its mock) to directly return the -// configmap/secret at all times - no caching involved. -// Be mindful that this implementation only allows for one namespace, driven by the client passed to the struct. -// Calls to ConfigMaps/Secrets will thus panic, because we can not create a new client from within. - -type ConfigMapClientLister struct { - ConfigMapClient v1.ConfigMapInterface - Namespace string -} - -func (c *ConfigMapClientLister) Get(name string) (get *corev1.ConfigMap, err error) { - get, err = c.ConfigMapClient.Get(context.TODO(), name, metav1.GetOptions{}) - return -} - -func (c *ConfigMapClientLister) List(selector labels.Selector) ([]*corev1.ConfigMap, error) { - retLst, err := c.ConfigMapClient.List(context.TODO(), metav1.ListOptions{LabelSelector: selector.String()}) - if err != nil { - return nil, err - } - var cms []*corev1.ConfigMap - for i := range retLst.Items { - cms = append(cms, &retLst.Items[i]) - } - - return cms, nil -} - -func (c *ConfigMapClientLister) ConfigMaps(ns string) corev1listers.ConfigMapNamespaceLister { - if ns != c.Namespace { - panic("unsupported operation, can't recreate the client here") - } - return c -} - -type SecretClientLister struct { - SecretClient v1.SecretInterface - Namespace string -} - -func (c *SecretClientLister) Get(name string) (get *corev1.Secret, err error) { - get, err = c.SecretClient.Get(context.TODO(), name, metav1.GetOptions{}) - return -} - -func (c *SecretClientLister) List(selector labels.Selector) ([]*corev1.Secret, error) { - retLst, err := c.SecretClient.List(context.TODO(), metav1.ListOptions{LabelSelector: selector.String()}) - if err != nil { - return nil, err - } - var cms []*corev1.Secret - for i := range retLst.Items { - cms = append(cms, &retLst.Items[i]) - } - - return cms, nil -} - -func (c *SecretClientLister) Secrets(ns string) corev1listers.SecretNamespaceLister { - if ns != c.Namespace { - panic("unsupported operation, can't recreate the client here") - } - return c -} diff --git a/pkg/tlshelpers/client_lister_test.go b/pkg/tlshelpers/client_lister_test.go deleted file mode 100644 index 5bee054b7..000000000 --- a/pkg/tlshelpers/client_lister_test.go +++ /dev/null @@ -1,83 +0,0 @@ -package tlshelpers - -import ( - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" - "testing" -) - -func TestPanicOnDifferentNamespaces(t *testing.T) { - ls := &ConfigMapClientLister{Namespace: "ns"} - require.Panics(t, func() { - ls.ConfigMaps("other") - }) - require.Equal(t, ls, ls.ConfigMaps("ns")) - - lsx := &SecretClientLister{Namespace: "ns"} - require.Panics(t, func() { - lsx.Secrets("other") - }) - require.Equal(t, lsx, lsx.Secrets("ns")) -} - -func TestConfigMaps(t *testing.T) { - fakeObjs := []runtime.Object{ - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm1"}, - }, - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm2", Labels: map[string]string{"a": "b"}}, - }, - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm3"}, - }, - } - fakeKubeClient := fake.NewSimpleClientset(fakeObjs...) - ls := &ConfigMapClientLister{ConfigMapClient: fakeKubeClient.CoreV1().ConfigMaps("ns"), Namespace: "ns"} - list, err := ls.List(labels.Everything()) - require.NoError(t, err) - require.Equal(t, 3, len(list)) - - parsedLabel, err := labels.Parse("a=b") - require.NoError(t, err) - list, err = ls.List(parsedLabel) - require.NoError(t, err) - require.Equal(t, 1, len(list)) - - get, err := ls.Get("cm1") - require.NoError(t, err) - require.Equal(t, "cm1", get.Name) -} - -func TestSecrets(t *testing.T) { - fakeObjs := []runtime.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm1"}, - }, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm2", Labels: map[string]string{"a": "b"}}, - }, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cm3"}, - }, - } - fakeKubeClient := fake.NewSimpleClientset(fakeObjs...) - ls := &SecretClientLister{SecretClient: fakeKubeClient.CoreV1().Secrets("ns"), Namespace: "ns"} - list, err := ls.List(labels.Everything()) - require.NoError(t, err) - require.Equal(t, 3, len(list)) - - parsedLabel, err := labels.Parse("a=b") - require.NoError(t, err) - list, err = ls.List(parsedLabel) - require.NoError(t, err) - require.Equal(t, 1, len(list)) - - get, err := ls.Get("cm1") - require.NoError(t, err) - require.Equal(t, "cm1", get.Name) -}