Merge pull request #1099 from relyt0925/token-expiration

add token expiration support to prevent nodepool upgrades from killing in flight provisions.

api/v1alpha1/nodepool_types.go

@@ -27,6 +27,12 @@ const (
 	IgnitionCACertMissingReason   string = "IgnitionCACertMissing"
 )
 
+const (
+	// IgnitionServerTokenExpirationTimestampAnnotation holds the time that a ignition token expires and should be
+	// removed from the cluster.
+	IgnitionServerTokenExpirationTimestampAnnotation = "hypershift.openshift.io/ignition-token-expiration-timestamp"
+)
+
 func init() {
 	SchemeBuilder.Register(&NodePool{})
 	SchemeBuilder.Register(&NodePoolList{})

hypershift-operator/controllers/nodepool/nodepool_controller.go

@@ -439,8 +439,8 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
 			return reconcile.Result{}, fmt.Errorf("failed to get token Secret: %w", err)
 		}
 		if err == nil {
-			if err := r.Delete(ctx, tokenSecret); err != nil && !apierrors.IsNotFound(err) {
-				return reconcile.Result{}, fmt.Errorf("failed to delete token Secret: %w", err)
+			if err := setExpirationTimestampOnToken(ctx, r.Client, tokenSecret); err != nil && !apierrors.IsNotFound(err) {
+				return reconcile.Result{}, fmt.Errorf("failed to set expiration on token Secret: %w", err)
 			}
 		}
 
@@ -678,6 +678,8 @@ func reconcileTokenSecret(tokenSecret *corev1.Secret, nodePool *hyperv1.NodePool
 
 	tokenSecret.Annotations[TokenSecretAnnotation] = "true"
 	tokenSecret.Annotations[nodePoolAnnotation] = client.ObjectKeyFromObject(nodePool).String()
+	// active token should never be marked as expired.
+	delete(tokenSecret.Annotations, hyperv1.IgnitionServerTokenExpirationTimestampAnnotation)
 
 	if tokenSecret.Data == nil {
 		tokenSecret.Data = map[string][]byte{}
@@ -1452,3 +1454,13 @@ func validateInfraID(infraID string) error {
 	}
 	return nil
 }
+
+func setExpirationTimestampOnToken(ctx context.Context, c client.Client, tokenSecret *corev1.Secret) error {
+	// this should be a reasonable value to allow all in flight provisions to complete.
+	timeUntilExpiry := 2 * time.Hour
+	if tokenSecret.Annotations == nil {
+		tokenSecret.Annotations = map[string]string{}
+	}
+	tokenSecret.Annotations[hyperv1.IgnitionServerTokenExpirationTimestampAnnotation] = time.Now().Add(timeUntilExpiry).Format(time.RFC3339)
+	return c.Update(ctx, tokenSecret)
+}

hypershift-operator/controllers/nodepool/nodepool_controller_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"regexp"
 	"testing"
+	"time"
 
 	. "github.com/onsi/gomega"
 	api "github.com/openshift/hypershift/api"
@@ -1044,3 +1045,50 @@ func TestGetName(t *testing.T) {
 	name = getName(base, suffix, length+1)
 	g.Expect(alphaNumeric.MatchString(string(name[0]))).To(BeTrue())
 }
+
+func TestSetExpirationTimestampOnToken(t *testing.T) {
+	fakeName := "test-token"
+	fakeNamespace := "master-cluster1"
+	fakeCurrentTokenVal := "tokenval1"
+
+	testCases := []struct {
+		name        string
+		inputSecret *corev1.Secret
+	}{
+		{
+			name: "when set expiration timestamp on token is called on a secret then the expiration timestamp is set",
+			inputSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+				Data: map[string][]byte{
+					TokenSecretTokenKey: []byte(fakeCurrentTokenVal),
+				},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			c := fake.NewClientBuilder().WithObjects(tc.inputSecret).Build()
+			err := setExpirationTimestampOnToken(context.Background(), c, tc.inputSecret)
+			g.Expect(err).To(Not(HaveOccurred()))
+			actualSecretData := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+			}
+			err = c.Get(context.Background(), client.ObjectKeyFromObject(actualSecretData), actualSecretData)
+			g.Expect(err).To(Not(HaveOccurred()))
+			rawExpirationTimestamp, ok := actualSecretData.Annotations[hyperv1.IgnitionServerTokenExpirationTimestampAnnotation]
+			g.Expect(ok).To(BeTrue())
+			expirationTimestamp, err := time.Parse(time.RFC3339, rawExpirationTimestamp)
+			g.Expect(err).To(Not(HaveOccurred()))
+			// ensures the 2 hour expiration is active. 119 minutes is one minute less than 2 hours. gives time for
+			// test to run.
+			g.Expect(time.Now().Add(119 * time.Minute).Before(expirationTimestamp)).To(BeTrue())
+		})
+	}
+}

ignition-server/README.md

@@ -12,8 +12,14 @@ The TokenSecret controller watches token Secrets and:
 
 i.e a token is active a total of 11 hours, 5.5 main and then 5.5 in the rotated spot.
 
+### Token Deletion 
+
+Token secrets and the associated tokens are revoked/deleted immediately when the corresponding nodePool is deleted. 
+
+When a NodePool is upgraded or goes through a config change, a new token secret is created that corresponds to the updated config value and the old token secret is marked with an expiration date by the NodePool controller. Once the expiration date has passed: the old token secret is deleted and the associated tokens are revoked. This strategy is done to allow in flight provisions that occurred in proximity to the nodePool upgrade to complete.
+
 ## Ignition provider
 An interface to be implemented to produce a valid ignition payload out of a given release/config pair.
 
 ### MachineConfigServer Provider
-An Ignition provider implementation which runs pods of the Machine Config Server ephemerally in order to produce a valid ignition payload.
+An Ignition provider implementation which runs pods of the Machine Config Server ephemerally in order to produce a valid ignition payload.

ignition-server/controllers/tokensecret_controller.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/google/uuid"
+	hyperv1 "github.com/openshift/hypershift/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -107,6 +108,36 @@ func processIfMatchesAnnotation(logger logr.Logger, obj client.Object) bool {
 	return false
 }
 
+// isTokenExpired parses an expiration annotation and compares with the current time to see if a token is expired.
+// invalid formats are considered expired
+func isTokenExpired(logrInstance logr.Logger, tokenAnnotations map[string]string) bool {
+	if expirationTimestampRaw, ok := tokenAnnotations[hyperv1.IgnitionServerTokenExpirationTimestampAnnotation]; ok {
+		expirationTime, err := time.Parse(time.RFC3339, expirationTimestampRaw)
+		if err != nil {
+			logrInstance.Error(err, "Invalid format for expiration time")
+			logrInstance.Info("Due to invalid expiration format: marking token as expired")
+			return true
+		}
+		return time.Now().After(expirationTime)
+	}
+	return false
+}
+
+// processExpiredToken handles clearing the cache of the expired token(s) and ensuring the token secret is removed
+// from the management cluster
+func (r *TokenSecretReconciler) processExpiredToken(ctx context.Context, tokenSecret *corev1.Secret) error {
+	if oldToken, ok := tokenSecret.Data[TokenSecretOldTokenKey]; ok {
+		r.PayloadStore.Delete(string(oldToken))
+	}
+	if currentToken, ok := tokenSecret.Data[TokenSecretTokenKey]; ok {
+		r.PayloadStore.Delete(string(currentToken))
+	}
+	if err := r.Client.Delete(ctx, tokenSecret); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	}
+	return nil
+}
+
 func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 	log.Info("Reconciling")
@@ -145,6 +176,12 @@ func (r *TokenSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, nil
 	}
 
+	// if a token is expired remove it
+	if isTokenExpired(log, tokenSecret.Annotations) {
+		err := r.processExpiredToken(ctx, tokenSecret)
+		return ctrl.Result{}, err
+	}
+
 	// Otherwise proceed to generate the payload and cache the content.
 	// Rotate the Token if necessary.
 	now := time.Now()

ignition-server/controllers/tokensecret_controller_test.go

@@ -2,6 +2,8 @@ package controllers
 
 import (
 	"context"
+	"github.com/go-logr/logr"
+	"github.com/openshift/hypershift/api/v1alpha1"
 	"testing"
 	"time"
 
@@ -353,3 +355,160 @@ func TestRotateTokenID(t *testing.T) {
 	g.Expect(value).To(BeEquivalentTo(existingValue))
 	g.Expect(ok).To(BeTrue())
 }
+
+func TestIsTokenExpired(t *testing.T) {
+	testCases := []struct {
+		name              string
+		annotations       map[string]string
+		expectedIsExpired bool
+	}{
+		{
+			name:              "when there's no token expiration timestamp annotation it should return that it is not expired (false)",
+			annotations:       map[string]string{},
+			expectedIsExpired: false,
+		},
+		{
+			name: "when the token expiration timestamp is in the past it should return that it is expired (true)",
+			annotations: map[string]string{
+				v1alpha1.IgnitionServerTokenExpirationTimestampAnnotation: time.Now().Add(-4 * time.Hour).Format(time.RFC3339),
+			},
+			expectedIsExpired: true,
+		},
+		{
+			name: "when the token expiration timestamp is in the future it should return that it is not expired (false)",
+			annotations: map[string]string{
+				v1alpha1.IgnitionServerTokenExpirationTimestampAnnotation: time.Now().Add(4 * time.Hour).Format(time.RFC3339),
+			},
+			expectedIsExpired: false,
+		},
+		{
+			name: "when the token expiration timestamp has an invalid value it should return that it is expired (true)",
+			annotations: map[string]string{
+				v1alpha1.IgnitionServerTokenExpirationTimestampAnnotation: "badvalue",
+			},
+			expectedIsExpired: true,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			actualIsExpired := isTokenExpired(logr.Discard(), tc.annotations)
+			g.Expect(actualIsExpired).To(Equal(tc.expectedIsExpired))
+		})
+	}
+}
+
+func TestProcessedExpiredToken(t *testing.T) {
+	fakeName := "test-token"
+	fakeNamespace := "master-cluster1"
+	fakeCurrentTokenVal := "tokenval1"
+	fakeOldTokenVal := "oldtokenval2"
+	fakeIndependentTokenVal := "independenttokenval1"
+	fakeTokenContent := []byte(`blah`)
+
+	testCases := []struct {
+		name                       string
+		inputSecret                *corev1.Secret
+		inputEntries               map[string][]byte
+		expectedRemainingEntries   map[string][]byte
+		expectedEntriesToBeRemoved map[string][]byte
+	}{
+		{
+			name: "when a token secret exists and the cache is populated then the secret is deleted and the token entries removed from cache",
+			inputEntries: map[string][]byte{
+				fakeCurrentTokenVal: fakeTokenContent,
+				fakeOldTokenVal:     fakeTokenContent,
+			},
+			inputSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+				Data: map[string][]byte{
+					TokenSecretOldTokenKey: []byte(fakeOldTokenVal),
+					TokenSecretTokenKey:    []byte(fakeCurrentTokenVal),
+				},
+			},
+			expectedRemainingEntries: nil,
+			expectedEntriesToBeRemoved: map[string][]byte{
+				fakeCurrentTokenVal: fakeTokenContent,
+				fakeOldTokenVal:     fakeTokenContent,
+			},
+		},
+		{
+			name: "when a token secret exists with only one token and the cache is populated then the secret is deleted and the token entries removed from cache",
+			inputEntries: map[string][]byte{
+				fakeCurrentTokenVal: fakeTokenContent,
+			},
+			inputSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+				Data: map[string][]byte{
+					TokenSecretTokenKey: []byte(fakeCurrentTokenVal),
+				},
+			},
+			expectedRemainingEntries: nil,
+			expectedEntriesToBeRemoved: map[string][]byte{
+				fakeCurrentTokenVal: fakeTokenContent,
+			},
+		},
+		{
+			name: "when a token secret exists and an independent secrets entry is also in the cache then only the processed tokens are removed",
+			inputEntries: map[string][]byte{
+				fakeCurrentTokenVal:     fakeTokenContent,
+				fakeIndependentTokenVal: fakeTokenContent,
+			},
+			inputSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+				Data: map[string][]byte{
+					TokenSecretTokenKey: []byte(fakeCurrentTokenVal),
+				},
+			},
+			expectedRemainingEntries: map[string][]byte{
+				fakeIndependentTokenVal: fakeTokenContent,
+			},
+			expectedEntriesToBeRemoved: map[string][]byte{
+				fakeCurrentTokenVal: fakeTokenContent,
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			payloadStore := NewPayloadStore()
+			for tokenKey, tokenVal := range tc.inputEntries {
+				payloadStore.Set(tokenKey, CacheValue{
+					Payload: tokenVal,
+				})
+			}
+			r := TokenSecretReconciler{
+				Client:           fake.NewClientBuilder().WithObjects(tc.inputSecret).Build(),
+				IgnitionProvider: &fakeIgnitionProvider{},
+				PayloadStore:     payloadStore,
+			}
+			err := r.processExpiredToken(context.Background(), tc.inputSecret)
+			g.Expect(err).To(Not(HaveOccurred()))
+			for expectedTokenKey := range tc.expectedRemainingEntries {
+				_, ok := payloadStore.Get(expectedTokenKey)
+				g.Expect(ok).To(BeTrue())
+			}
+			for expectedTokenKey := range tc.expectedEntriesToBeRemoved {
+				_, ok := payloadStore.Get(expectedTokenKey)
+				g.Expect(ok).To(BeFalse())
+			}
+			secretToFetch := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fakeName,
+					Namespace: fakeNamespace,
+				},
+			}
+			err = r.Client.Get(context.Background(), client.ObjectKeyFromObject(secretToFetch), secretToFetch)
+			g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
+		})
+	}
+}