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

ssz1997 · 2024-07-11T23:40:41Z

What this PR does / why we need it:
The root certificate file ca.crt is also rsync’d to px runc container. In the rare case of this certificate is modified/replaced, we need to inform the px runc container as well. Thus update the file in the secret from opertator.

ssz1997 · 2024-07-11T23:41:02Z

drivers/storage/portworx/components_test.go

@@ -1248,12 +1250,6 @@ func TestServiceAccountTokenRefreshOnExpire(t *testing.T) {
 			Name:      "px-cluster",
 			Namespace: "kube-test",
 		},
-		Spec: corev1.StorageClusterSpec{


Not necessary

ssz1997 · 2024-07-11T23:43:12Z

drivers/storage/portworx/components_test.go

+	require.NoError(t, err)
+	oldCaCrt := saTokenSecret.Data[v1.ServiceAccountRootCAKey]
+
+	err = os.WriteFile(caCrtPath, []byte("test"), 0644)


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.

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

codecov · 2024-07-12T00:23:34Z

Codecov Report

Attention: Patch coverage is 68.42105% with 12 lines in your changes missing coverage. Please review.

Project coverage is 75.75%. Comparing base (f08ac8a) to head (b23b926).

Files	Patch %	Lines
...ivers/storage/portworx/component/portworx_basic.go	68.42%	6 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   75.75%   75.75%   -0.01%     
==========================================
  Files          77       77              
  Lines       20797    20812      +15     
==========================================
+ Hits        15755    15766      +11     
- Misses       3920     3922       +2     
- Partials     1122     1124       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: shsun_pure <shsun@purestorage.com>

pureneelesh · 2024-07-12T21:14:18Z

drivers/storage/portworx/component/portworx_basic.go

 	if err != nil {
 		return err
 	}
-	if needRefresh {
-		if err := c.refreshTokenSecret(secret, cluster, ownerRef); err != nil {
+	needRefreshToken, err := isTokenRefreshRequired(secret)


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

pureneelesh · 2024-07-12T21:17:21Z

drivers/storage/portworx/component/portworx_basic.go

+	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)
+	}
+	if len(secret.Data) == 0 || len(secret.Data[v1.ServiceAccountRootCAKey]) == 0 || !bytes.Equal(secret.Data[v1.ServiceAccountRootCAKey], rootCaCrt) {


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

yeah the equal should be able to

pureneelesh · 2024-07-12T21:48:54Z

drivers/storage/portworx/components_test.go

+	require.NoError(t, err)
+	oldCaCrt := saTokenSecret.Data[v1.ServiceAccountRootCAKey]
+
+	err = os.WriteFile(caCrtPath, []byte("test"), 0644)


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

pureneelesh · 2024-07-12T22:52:11Z

drivers/storage/portworx/component/portworx_basic.go

@@ -46,6 +46,8 @@ const (

 var (
 	defaultPxSaTokenExpirationSeconds = int64(12 * 60 * 60)
+	// Accessible for testing purpose
+	RootCaCrtPath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"


Slightly cleaner way is to keep this variable private and add a function setRootCaCrtPathForTesting(path string) to set it.

…1606) * update ca.crt in the secret if updated Signed-off-by: shsun_pure <shsun@purestorage.com> * address comments * fix test, add setter for ca cert --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com>

#1617) * [PWX-37620] Create/Refresh token and secret and mount to oci-mon pod (#1582) * create and refresh token/secret Signed-off-by: shsun_pure <shsun@purestorage.com> * mount self-managed servceaccount secret to oci-mon pod Signed-off-by: shsun_pure <shsun@purestorage.com> * always use the refreshing token for px Signed-off-by: shsun_pure <shsun@purestorage.com> * fix only modify arr copy Signed-off-by: shsun_pure <shsun@purestorage.com> * move token management to basic; fix keep adding secret vol to spec * merge master * fix and add tests; fix token refresh duration unit * update secret mount path * address comments * address more comments * make token refresh interval a parameter * fix token secret expiration data; address comments * fix failed UT from master branch --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * PWX-37620 Vendor in latest sched ops (#1571) * vendor in latest sched ops Signed-off-by: shsun_pure <shsun@purestorage.com> * fix generated mock --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * fix compile due to manually resolving conflicts * PWX-37982 Use the expiration time returned by k8s api server for px sa token (#1596) * use the expiration time returned by k8s api server Signed-off-by: shsun_pure <shsun@purestorage.com> * add comment for the change --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * Fix PX ServiceAccount token audience (#1602) * fix px sa token audience Signed-off-by: shsun_pure <shsun@purestorage.com> * remove token audience Signed-off-by: shsun_pure <shsun@purestorage.com> --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * PWX-38064 Update ca.crt in the PX ServiceAccount Secret if updated (#1606) * update ca.crt in the secret if updated Signed-off-by: shsun_pure <shsun@purestorage.com> * address comments * fix test, add setter for ca cert --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * PWX-37884 Add portworx serviceaccount token refresh integration test (#1598) * addPxSaTokenIntegrationTest Signed-off-by: shsun_pure <shsun@purestorage.com> * try see if k8s enforce token minimum expiration time * add testrail id * add manually delete secaret scenario test * add test log lines * change token refresh time to 10min * fix test and address comments * use the token to make an k8s api call * address comment * skip pdb validation for certain versions of operator and px --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * PWX-37884 Refactor px serviceaccount token integration test (#1615) * refactor test Signed-off-by: shsun_pure <shsun@purestorage.com> * uninstall cluster and verify token secret deletion * address comments * fix test --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com> * fix tests --------- Signed-off-by: shsun_pure <shsun@purestorage.com> Co-authored-by: shsun_pure <shsun@purestorage.com>

github-actions bot added area/components area/portworx size/m labels Jul 11, 2024

ssz1997 commented Jul 11, 2024

View reviewed changes

update ca.crt in the secret if updated

7efa13a

Signed-off-by: shsun_pure <shsun@purestorage.com>

ssz1997 force-pushed the updateRootCertInSaSecretForPx branch from 2c0543a to 7efa13a Compare July 12, 2024 17:57

pureneelesh approved these changes Jul 12, 2024

View reviewed changes

github-actions bot added the stage/approved label Jul 12, 2024

address comments

5c7d333

pureneelesh reviewed Jul 12, 2024

View reviewed changes

pureneelesh approved these changes Jul 12, 2024

View reviewed changes

fix test, add setter for ca cert

b23b926

pureneelesh approved these changes Jul 15, 2024

View reviewed changes

ssz1997 merged commit a57ea27 into master Jul 15, 2024
7 of 9 checks passed

ssz1997 deleted the updateRootCertInSaSecretForPx branch July 15, 2024 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ssz1997 commented Jul 11, 2024

ssz1997 Jul 11, 2024

ssz1997 Jul 11, 2024

pureneelesh Jul 12, 2024

codecov bot commented Jul 12, 2024 •

edited

Loading

pureneelesh Jul 12, 2024

pureneelesh Jul 12, 2024

ssz1997 Jul 12, 2024

pureneelesh Jul 12, 2024

pureneelesh Jul 12, 2024

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

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

Conversation

ssz1997 commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2024 • edited Loading

Codecov Report

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

codecov bot commented Jul 12, 2024 •

edited

Loading