Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PWX-38064 Update ca.crt in the PX ServiceAccount Secret if updated #1606

Merged
merged 3 commits into from
Jul 15, 2024
Merged

PWX-38064 Update ca.crt in the PX ServiceAccount Secret if updated #1606

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7efa13a
update ca.crt in the secret if updated
Jul 11, 2024
5c7d333
address comments
Jul 12, 2024
b23b926
fix test, add setter for ca cert
Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 42 additions & 30 deletions drivers/storage/portworx/component/portworx_basic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package component

import (
"bytes"
"context"
"fmt"
"os"
Expand All @@ -19,7 +20,6 @@
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/tools/record"
"k8s.io/kubernetes/pkg/apis/core"
"sigs.k8s.io/controller-runtime/pkg/client"

pxutil "github.com/libopenstorage/operator/drivers/storage/portworx/util"
Expand Down Expand Up @@ -557,23 +557,28 @@
return err
}
}
needRefresh, err := isTokenRefreshRequired(secret)
caCrtUpdated, err := updateCaCrtIfNeeded(secret)
if err != nil {
return err
}
if needRefresh {
if err := c.refreshTokenSecret(secret, cluster, ownerRef); err != nil {
needRefreshToken, err := isTokenRefreshRequired(secret)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a updateTokenIfNeeded() wrapper to be consistent with updateCaCrtIfNeeded?

if err != nil {
return err

Check warning on line 566 in drivers/storage/portworx/component/portworx_basic.go

View check run for this annotation

Codecov / codecov/patch

drivers/storage/portworx/component/portworx_basic.go#L566 

Added line #L566 was not covered by tests
}
if needRefreshToken {
if err := c.refreshToken(secret, cluster); err != nil {
return fmt.Errorf("failed to refresh the token secret for px container: %w", err)
}
}
if caCrtUpdated || needRefreshToken {
if err := k8sutil.CreateOrUpdateSecret(c.k8sClient, secret, ownerRef); err != nil {
return err

Check warning on line 575 in drivers/storage/portworx/component/portworx_basic.go

View check run for this annotation

Codecov / codecov/patch

drivers/storage/portworx/component/portworx_basic.go#L575 

Added line #L575 was not covered by tests
}
}
return nil
}

func (c *portworxBasic) createTokenSecret(cluster *corev1.StorageCluster, ownerRef *metav1.OwnerReference) (*v1.Secret, error) {
rootCaCrt, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt")
if err != nil && !os.IsNotExist(err) {
return nil, fmt.Errorf("error reading k8s cluster certificate located inside the pod at /var/run/secrets/kubernetes.io/serviceaccount/ca.crt: %w", err)
}
secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: pxutil.PortworxServiceAccountTokenSecretName,
Expand All @@ -582,8 +587,7 @@
},
Type: v1.SecretTypeOpaque,
Data: map[string][]byte{
core.ServiceAccountRootCAKey: rootCaCrt,
core.ServiceAccountNamespaceKey: []byte(cluster.Namespace),
v1.ServiceAccountNamespaceKey: []byte(cluster.Namespace),
},
}
if err := k8sutil.CreateOrUpdateSecret(c.k8sClient, secret, ownerRef); err != nil {
Expand All @@ -592,7 +596,33 @@
return secret, nil
}

func (c *portworxBasic) refreshTokenSecret(secret *v1.Secret, cluster *corev1.StorageCluster, ownerRef *metav1.OwnerReference) error {
func updateCaCrtIfNeeded(secret *v1.Secret) (bool, error) {
rootCaCrt, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt")
if err != nil && !os.IsNotExist(err) {
return false, fmt.Errorf("error reading k8s cluster certificate located inside the pod at /var/run/secrets/kubernetes.io/serviceaccount/ca.crt: %w", err)

Check warning on line 602 in drivers/storage/portworx/component/portworx_basic.go

View check run for this annotation

Codecov / codecov/patch

drivers/storage/portworx/component/portworx_basic.go#L602 

Added line #L602 was not covered by tests
}
if len(secret.Data) == 0 || len(secret.Data[v1.ServiceAccountRootCAKey]) == 0 || !bytes.Equal(secret.Data[v1.ServiceAccountRootCAKey], rootCaCrt) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: are the len() checks really needed? bytes.Equal() should probably handle it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the equal should be able to

secret.Data[v1.ServiceAccountRootCAKey] = rootCaCrt
return true, nil
}
return false, nil
}

func isTokenRefreshRequired(secret *v1.Secret) (bool, error) {
if len(secret.Data) == 0 || len(secret.Data[v1.ServiceAccountTokenKey]) == 0 {
return true, nil
}
expirationTime, err := time.Parse(time.RFC3339, string(secret.Data[PxSaTokenRefreshTimeKey]))
if err != nil {
return false, fmt.Errorf("error parsing expiration time: %w", err)

Check warning on line 617 in drivers/storage/portworx/component/portworx_basic.go

View check run for this annotation

Codecov / codecov/patch

drivers/storage/portworx/component/portworx_basic.go#L617 

Added line #L617 was not covered by tests
}
if time.Now().UTC().After(expirationTime) {
return true, nil
}
return false, nil
}

func (c *portworxBasic) refreshToken(secret *v1.Secret, cluster *corev1.StorageCluster) error {
expirationSeconds, err := getPxSaTokenExpirationSeconds(cluster)
if err != nil {
return err
Expand All @@ -601,15 +631,11 @@
if err != nil {
return err
}
secret.Data[core.ServiceAccountTokenKey] = []byte(newToken.Status.Token)
secret.Data[v1.ServiceAccountTokenKey] = []byte(newToken.Status.Token)
// ServiceAccount token expiration time we defined might get overwritten by the maxExpirationSeconds defined by the k8s token RESTful server,
// so our token refresh machanism has to honor this server limit.
// https://github.com/kubernetes/kubernetes/blob/79fee524e65ddc7c1448d5d2554c6f91233cf98d/pkg/registry/core/serviceaccount/storage/token.go#L208
secret.Data[PxSaTokenRefreshTimeKey] = []byte(time.Now().UTC().Add(time.Duration(*newToken.Spec.ExpirationSeconds/2) * time.Second).Format(time.RFC3339))
err = k8sutil.CreateOrUpdateSecret(c.k8sClient, secret, ownerRef)
if err != nil {
return err
}
return nil
}

Expand All @@ -626,20 +652,6 @@
return tokenResp, nil
}

func isTokenRefreshRequired(secret *v1.Secret) (bool, error) {
if len(secret.Data) == 0 || len(secret.Data[v1.ServiceAccountTokenKey]) == 0 {
return true, nil
}
expirationTime, err := time.Parse(time.RFC3339, string(secret.Data[PxSaTokenRefreshTimeKey]))
if err != nil {
return false, fmt.Errorf("error parsing expiration time: %w", err)
}
if time.Now().UTC().After(expirationTime) {
return true, nil
}
return false, nil
}

func (c *portworxBasic) createPortworxKVDBService(
cluster *corev1.StorageCluster,
ownerRef *metav1.OwnerReference,
Expand Down
51 changes: 45 additions & 6 deletions drivers/storage/portworx/components_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import (
"context"
"encoding/json"
"fmt"
"io/fs"
"math/rand"
"os"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -1248,12 +1250,6 @@ func TestServiceAccountTokenRefreshOnExpire(t *testing.T) {
Name: "px-cluster",
Namespace: "kube-test",
},
Spec: corev1.StorageClusterSpec{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary

Stork: &corev1.StorkSpec{
Enabled: true,
},
Image: "portworx/image:2.10.1",
},
}
err = driver.PreInstall(cluster)
require.NoError(t, err)
Expand All @@ -1277,6 +1273,49 @@ func TestServiceAccountTokenRefreshOnExpire(t *testing.T) {
assert.True(t, updatedExpirationTime.Sub(fakeExpirationTime) > 6*time.Hour)
}

func TestUpdateServiceAccountTokenSecretCaCrt(t *testing.T) {
caCrtDir := "/var/run/secrets/kubernetes.io/serviceaccount"
caCrtPath := "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
err := os.MkdirAll(caCrtDir, fs.ModePerm)
require.NoError(t, err)
file, err := os.Create(caCrtPath)
require.NoError(t, err)
file.Close()

mockCtrl := gomock.NewController(t)
setUpMockCoreOps(mockCtrl, fakek8sclient.NewSimpleClientset())
reregisterComponents()
k8sClient := testutil.FakeK8sClient()
driver := portworx{}
err = driver.Init(k8sClient, runtime.NewScheme(), record.NewFakeRecorder(0))
require.NoError(t, err)

cluster := &corev1.StorageCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "px-cluster",
Namespace: "kube-test",
},
}
err = driver.PreInstall(cluster)
require.NoError(t, err)

saTokenSecret := &v1.Secret{}
err = testutil.Get(k8sClient, saTokenSecret, pxutil.PortworxServiceAccountTokenSecretName, cluster.Namespace)
require.NoError(t, err)
oldCaCrt := saTokenSecret.Data[v1.ServiceAccountRootCAKey]

err = os.WriteFile(caCrtPath, []byte("test"), 0644)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test is ran in a k8s pod, this is effectively removing the root certificate, which I think is very dangerous.
Shall we just skip the test if this file exists?
Another solution would be introduce the monkey patch module into operator repo and patch the os.ReadFile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making caCrtPath a variable and allow the test to set a non-default value for this unit test?

require.NoError(t, err)

err = driver.PreInstall(cluster)
require.NoError(t, err)
err = testutil.Get(k8sClient, saTokenSecret, pxutil.PortworxServiceAccountTokenSecretName, cluster.Namespace)
require.NoError(t, err)
newCaCrt := saTokenSecret.Data[v1.ServiceAccountRootCAKey]
require.NotEqual(t, oldCaCrt, newCaCrt)
require.Equal(t, string(newCaCrt), "test")
}

func TestDefaultStorageClassesWithStork(t *testing.T) {
mockCtrl := gomock.NewController(t)
versionClient := fakek8sclient.NewSimpleClientset()
Expand Down
Loading