From 658fe2edbd50d7aa318f56c7e6fb4f2cb16dfa95 Mon Sep 17 00:00:00 2001 From: Damian Czaja Date: Wed, 30 Oct 2024 11:58:46 +0100 Subject: [PATCH] do not print warning, when ignoring certs --- internal/actions/csr/csr.go | 66 +++++++---------- internal/actions/csr/csr_test.go | 122 ++++++++++++++++++++++++++----- 2 files changed, 130 insertions(+), 58 deletions(-) diff --git a/internal/actions/csr/csr.go b/internal/actions/csr/csr.go index 70c14a8..3faaff9 100644 --- a/internal/actions/csr/csr.go +++ b/internal/actions/csr/csr.go @@ -68,6 +68,32 @@ func (c *Certificate) Approved() bool { return false } +func (c *Certificate) Outdated() bool { + if c.v1Beta1 != nil { + return c.v1Beta1.CreationTimestamp.Add(csrTTL).Before(time.Now()) + } + return c.v1.CreationTimestamp.Add(csrTTL).Before(time.Now()) +} + +func (c *Certificate) ForCASTAINode() bool { + if c.Name == "" { + return false + } + + if strings.HasPrefix(c.Name, "system:node") && strings.Contains(c.Name, "cast-pool") { + return true + } + + return false +} + +func (c *Certificate) NodeBootstrap() bool { + // Since we only have one handler per CSR/certificate name, + // which is the node name, we can process the controller's certificates and kubelet-bootstrap`s. + // This covers the case when the controller restarts but the bootstrap certificate was deleted without our own certificate being approved. + return c.RequestingUser == "kubelet-bootstrap" || c.RequestingUser == "system:serviceaccount:castai-agent:castai-cluster-controller" +} + func isAlreadyApproved(err error) bool { if err == nil { return false @@ -338,12 +364,7 @@ func WatchCastAINodeCSRs(ctx context.Context, log logrus.FieldLogger, client kub return nil } -var ( - errUnexpectedObjectType = errors.New("unexpected object type") - errCSRTooOld = errors.New("csr is too old") - errOwner = errors.New("owner is not bootstrap") - errNonCastAINode = errors.New("not a castai node") -) +var errUnexpectedObjectType = errors.New("unexpected object type") func processCSREvent(ctx context.Context, c chan<- *Certificate, csrObj interface{}) error { cert, err := toCertificate(csrObj) @@ -355,7 +376,7 @@ func processCSREvent(ctx context.Context, c chan<- *Certificate, csrObj interfac return nil } - if cert.Approved() { + if cert.Approved() || !cert.ForCASTAINode() || !cert.NodeBootstrap() || cert.Outdated() { return nil } @@ -367,58 +388,29 @@ func toCertificate(obj interface{}) (cert *Certificate, err error) { var name string var request []byte - isOutdated := false switch e := obj.(type) { case *certv1.CertificateSigningRequest: name = e.Name request = e.Spec.Request cert = &Certificate{Name: name, v1: e, RequestingUser: e.Spec.Username} - isOutdated = e.CreationTimestamp.Add(csrTTL).Before(time.Now()) case *certv1beta1.CertificateSigningRequest: name = e.Name request = e.Spec.Request cert = &Certificate{Name: name, v1Beta1: e, RequestingUser: e.Spec.Username} - isOutdated = e.CreationTimestamp.Add(csrTTL).Before(time.Now()) default: return nil, errUnexpectedObjectType } - if isOutdated { - return nil, fmt.Errorf("csr with certificate Name: %v RequestingUser: %v %w", cert.Name, cert.RequestingUser, errCSRTooOld) - } - - // Since we only have one handler per CSR/certificate name, - // which is the node name, we can process the controller's certificates and kubelet-bootstrap`s. - // This covers the case when the controller restarts but the bootstrap certificate was deleted without our own certificate being approved. - if cert.RequestingUser != "kubelet-bootstrap" && cert.RequestingUser != "system:serviceaccount:castai-agent:castai-cluster-controller" { - return nil, fmt.Errorf("csr with certificate Name: %v RequestingUser: %v %w", cert.Name, cert.RequestingUser, errOwner) - } - cn, err := getSubjectCommonName(name, request) if err != nil { return nil, fmt.Errorf("getSubjectCommonName: Name: %v RequestingUser: %v request: %v %w", cert.Name, cert.RequestingUser, string(request), err) } - if !isCastAINodeCsr(cn) { - return nil, fmt.Errorf("csr with certificate Name: %v RequestingUser: %v cn: %v %w", cert.Name, cert.RequestingUser, cn, errNonCastAINode) - } cert.Name = cn return cert, nil } -func isCastAINodeCsr(subjectCommonName string) bool { - if subjectCommonName == "" { - return false - } - - if strings.HasPrefix(subjectCommonName, "system:node") && strings.Contains(subjectCommonName, "cast-pool") { - return true - } - - return false -} - func sendCertificate(ctx context.Context, c chan<- *Certificate, cert *Certificate) { select { case c <- cert: @@ -450,8 +442,6 @@ func parseCSR(pemData []byte) (*x509.CertificateRequest, error) { //nolint:unparam func getOptions(signer string) metav1.ListOptions { - fields.SelectorFromSet(fields.Set{}) - return metav1.ListOptions{ FieldSelector: fields.SelectorFromSet(fields.Set{ "spec.signerName": signer, diff --git a/internal/actions/csr/csr_test.go b/internal/actions/csr/csr_test.go index 20ff7b9..b60a571 100644 --- a/internal/actions/csr/csr_test.go +++ b/internal/actions/csr/csr_test.go @@ -3,12 +3,12 @@ package csr import ( "context" "path/filepath" - "reflect" "testing" "time" "github.com/stretchr/testify/require" certv1 "k8s.io/api/certificates/v1" + certv1beta1 "k8s.io/api/certificates/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" @@ -93,8 +93,82 @@ func Test_isCastAINodeCsr(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := isCastAINodeCsr(tt.args.subjectCommonName) - require.Equal(t, tt.want, got) + cert := &Certificate{ + Name: tt.args.subjectCommonName, + } + + require.Equal(t, tt.want, cert.ForCASTAINode()) + }) + } +} + +func Test_outdatedCertificate(t *testing.T) { + tt := map[string]struct { + createTimestamp time.Time + want bool + }{ + "Outdated": { + createTimestamp: time.Now().Add(-csrTTL).Add(-time.Second), + want: true, + }, + "Not outdated": { + createTimestamp: time.Now(), + want: false, + }, + "Outdated, right before": { + createTimestamp: time.Now().Add(-csrTTL).Add(2 * time.Second), + want: false, + }, + } + + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + cert := &Certificate{ + v1: &certv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.NewTime(tc.createTimestamp), + }, + }, + } + require.Equal(t, tc.want, cert.Outdated()) + + certBeta := &Certificate{ + v1Beta1: &certv1beta1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + CreationTimestamp: metav1.NewTime(tc.createTimestamp), + }, + }, + } + require.Equal(t, tc.want, certBeta.Outdated()) + }) + } +} + +func Test_nodeBootstrap(t *testing.T) { + tt := map[string]struct { + reqUser string + want bool + }{ + "other one": { + reqUser: "dummy-user", + want: false, + }, + "kubelet-bootstrap": { + reqUser: "kubelet-bootstrap", + want: true, + }, + "castai-cluster-controller": { + reqUser: "system:serviceaccount:castai-agent:castai-cluster-controller", + want: true, + }, + } + + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + cert := &Certificate{ + RequestingUser: tc.reqUser, + } + require.Equal(t, tc.want, cert.NodeBootstrap()) }) } } @@ -106,10 +180,10 @@ func Test_toCertificate(t *testing.T) { obj interface{} } tests := []struct { - name string - args args - wantCert *Certificate - wantErr bool + name string + args args + checkFunc func(t *testing.T, cert *Certificate) + wantErr bool }{ { name: "empty event", @@ -125,9 +199,13 @@ func Test_toCertificate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ CreationTimestamp: metav1.Time{Time: time.Now().Add(-csrTTL)}, }, + Spec: testCSRv1.Spec, }, }, - wantErr: true, + checkFunc: func(t *testing.T, cert *Certificate) { + require.True(t, cert.Outdated()) + }, + wantErr: false, }, { name: "bad owner", @@ -141,19 +219,22 @@ func Test_toCertificate(t *testing.T) { }, }, }, - wantErr: true, + checkFunc: func(t *testing.T, cert *Certificate) { + require.False(t, cert.NodeBootstrap()) + }, + wantErr: false, }, { name: "ok v1", args: args{ obj: testCSRv1, }, - wantErr: false, - wantCert: &Certificate{ - Name: "system:node:gke-dev-master-cast-pool-cb53177b", - RequestingUser: "kubelet-bootstrap", - v1: testCSRv1, + checkFunc: func(t *testing.T, cert *Certificate) { + require.Equal(t, "system:node:gke-dev-master-cast-pool-cb53177b", cert.Name) + require.Equal(t, "kubelet-bootstrap", cert.RequestingUser) + require.Equal(t, testCSRv1, cert.v1) }, + wantErr: false, }, { name: "ok v1beta1", @@ -161,10 +242,10 @@ func Test_toCertificate(t *testing.T) { obj: testCSRv1beta1, }, wantErr: false, - wantCert: &Certificate{ - Name: "system:node:gke-dev-master-cast-pool-cb53177b", - RequestingUser: "kubelet-bootstrap", - v1Beta1: testCSRv1beta1, + checkFunc: func(t *testing.T, cert *Certificate) { + require.Equal(t, "system:node:gke-dev-master-cast-pool-cb53177b", cert.Name) + require.Equal(t, "kubelet-bootstrap", cert.RequestingUser) + require.Equal(t, testCSRv1beta1, cert.v1Beta1) }, }, } @@ -175,8 +256,9 @@ func Test_toCertificate(t *testing.T) { t.Errorf("toCertificate() error = %v, wantErr %v", err, tt.wantErr) return } - if !reflect.DeepEqual(gotCert, tt.wantCert) { - t.Errorf("toCertificate() gotCert = %v, want %v", gotCert, tt.wantCert) + + if tt.checkFunc != nil { + tt.checkFunc(t, gotCert) } }) }