use k8s atomic writer for storage of secrets / configmaps #93

gabemontero · 2022-04-14T16:22:05Z

Taking a glance at the latest shared resource epics, I cannot find a story related to this (@coreydaley @adambkaplan do let me know if that is just an oversight on my part), but an item we have discussed for completing productization of shared resource is switching actual storage of the secret/configmap keys to using k8s atomic writer to provide "atomic" access to data via some creative linux symlinking so that if the contents change, users can use inotify or fanotify to coordinate access.

This is how a Pod's serviceaccount secrets have been stored since the early days of k8s, and this approach was created after some early bugs where intermittent errors would occur when read and writes of that secret data were done at the same time.

The upstream Secrets Store CSI Driver also adopted this approach somewhat recently.

As I mentioned last time we discussed this in team meetings, they actually create a copy of atomic_writer.go in their tree (even though they still already vendor in k8s/k8s). In looking at the history of https://github.com/kubernetes-sigs/secrets-store-csi-driver/commits/main/pkg/util/fileutil/atomic_writer.go their updates from their initial copy are only superficial.

So I eschewed their copy approach and simply vendor in k8s/k8s like we do for openshift/builder and use the k8s/k8s atomic_writier.

Lastly, I moved our simple config map example to a locally created config map vs. one in the ocp config managed namespace. It made it eaiser for manual testing around removing / adding keys and confirming that atomic_writer "handles it".

/assign @coreydaley
/assign @adambkaplan

openshift-ci · 2022-04-14T16:22:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coreydaley · 2022-04-14T16:26:16Z

pkg/csidriver/driver.go


 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
+	atomic "k8s.io/kubernetes/pkg/volume/util"


I think that this should be something like "volumeutil", "kvolumeutil" or just "kutil" to be more descriptive to the package it is pulling in, not what feature of that package you are using.

coreydaley · 2022-04-14T16:27:04Z

pkg/csidriver/driver.go

-	// container image must be empty, or the directory does not exist, and is created for the Pod's container as
-	// part of provisioning the container.
-	if err := commonOSRemove(podPath, fmt.Sprintf("commonUpsertRanger key %s volid %s share id %s pod name %s", key, dv.GetVolID(), dv.GetSharedDataId(), dv.GetPodName())); err != nil {
+	// NOTE: atomic_writer handles any pruning of secret/configmap keys where were present before, but are no longer


s/where/that/

coreydaley · 2022-04-14T16:32:52Z

pkg/csidriver/driver.go

+		err = aw.Write(podFile)
+		if err != nil {
+			return err
+		}


if err = aw.Write(podFile); err != nil { return err }

coreydaley · 2022-04-14T16:34:59Z

pkg/csidriver/driver.go

-		// has been mounted as a separate dir in our share, so skip
-		if info.IsDir() {
-			return nil
+	dirFile, err := os.Open(dir)


Should there be a deferred close directory after this?

coreydaley · 2022-04-14T16:36:20Z

pkg/csidriver/driver.go

+			err = os.RemoveAll(fileName)
+			if err != nil {
+				klog.V(0).Infof("commonOSRemove: %s", err.Error())
+				return err
+			}


if err = os.RemoveAll(fileName); err != nil { klog.V(0).Infof("commonOSRemove: %s", err.Error()) return err }

If this is going to be loglevel 0, it should probably be more human-friendly text, or maybe you meant it to be loglevel 4 like the others?

coreydaley · 2022-04-14T16:39:19Z

pkg/csidriver/driver_test.go

+			linkName, err := os.Readlink(info.Name())
+			if err == nil {
+				searchName = linkName
+			}


if linkName, err := os.Readlink(info.Name()); err == nil { searchName = linkName }

coreydaley · 2022-04-14T16:41:45Z

pkg/csidriver/driver.go

-	if err := os.MkdirAll(podPath, os.ModePerm); err != nil {
+	podFile := map[string]atomic.FileProjection{}
+	aw, err := atomic.NewAtomicWriter(podPath, "shared-resource-csi-driver")
+	if err != nil {
 		return err
 	}
 	if payload.ByteData != nil {


Can a payload have both ByteData and StringData populated at the same time? Can payload have both be nil also?

yes and yes

see the ConfigMap godoc wrt both populated

both nil means an empty secret/configmap

coreydaley · 2022-04-14T16:45:57Z

pkg/csidriver/driver.go

+			err = syscall.Unlink(filepath.Join(dir, dirEntry.Name()))
+			if err != nil {
+				klog.V(0).Infof("commonOSRemove: %s", err.Error())
+				return err
+			}


if err = syscall.Unlink(filepath.Join(dir, dirEntry.Name())); err != nil { klog.V(0).Infof("commonOSRemove: %s", err.Error()) return err }

This should probably be loglevel 4 or more human-friendly text.

coreydaley · 2022-04-14T16:47:08Z

pkg/csidriver/driver.go

+			err = syscall.Unlink(filepath.Join(dir, dirEntry.Name()))
+			if err != nil {
+				klog.V(0).Infof("commonOSRemove: %s", err.Error())
+				return err
+			}


if err = syscall.Unlink(filepath.Join(dir, dirEntry.Name())); err != nil { klog.V(0).Infof("commonOSRemove: %s", err.Error()) return err }

Loglevel 4 or more human-friendly text

gabemontero · 2022-04-14T17:46:18Z

Compile errors I am not seeing locally:

Running e2e suite normal
# sigs.k8s.io/json/internal/golang/encoding/json
vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.go:1249:12: sf.IsExported undefined (type reflect.StructField has no field or method IsExported)
vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.go:1255:18: sf.IsExported undefined (type reflect.StructField has no field or method IsExported)
FAIL	github.com/openshift/csi-driver-shared-resource/test/e2e [build failed]
FAIL

and

 INFO[2022-04-14T16:24:29Z] env GOOS=linux GOARCH=amd64 go test -mod=vendor -count 1 ./cmd/... ./pkg/...
# sigs.k8s.io/json/internal/golang/encoding/json
vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.go:1249:12: sf.IsExported undefined (type reflect.StructField has no field or method IsExported)
vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.go:1255:18: sf.IsExported undefined (type reflect.StructField has no field or method IsExported)
FAIL

probably need to bump the golang version in the Dockerfile and/or openshift/release and/or what I am using locally

gabemontero · 2022-04-14T18:24:58Z

hmm .... it might even depend on which version of go 1.17 you use ... I'm getting some hits on internet searches. This repo is already using 1.17 ... my laptop is at 1.17.6

I saw were Luke emailed aos-devel about 1.18.1

looking some more ...

gabemontero · 2022-04-14T18:31:31Z

weirdly the unit test at least is using 1.16 which definitely has the issue:

�[36mINFO�[0m[2022-04-14T16:22:34Z] Tagging openshift/release:golang-1.16 into pipeline:root.

e2e's are 1.17 ... though I cannot tell with dot rel off of 1.17

�[36mINFO�[0m[2022-04-14T16:22:35Z] Tagging ocp/builder:rhel-8-golang-1.17-openshift-4.11 into pipeline:ocp_builder_rhel-8-golang-1.17-openshift-4.11.

gabemontero · 2022-04-14T21:40:34Z

always forget the ci-operator yam file ... that at least fixed the unit test

gabemontero · 2022-04-15T02:12:54Z

removed permissions needs a little work ; my simple test did not catch what the more complicated e2e caught

will pick back up after the holiday

golang version at least sorted out

gabemontero · 2022-04-18T14:26:40Z

hmm ... slow passes for me locally

/test e2e-aws-csi-driver-slow

gabemontero · 2022-04-18T14:33:45Z

hmm ... slow passes for me locally

/test e2e-aws-csi-driver-slow

reviewed ... the test waits 10 min and the relist is 10 min ... could have hit edge case. May push tweak to wait time based on next set of results.

gabemontero · 2022-04-18T15:49:44Z

/test e2e-aws-csi-driver-disruptive

gabemontero · 2022-04-18T16:44:16Z

ok e2e's all green

doing one more run with the slow e2e interval tweaked

@coreydaley have the updates from your comments in a separate commit

PTAL / thanks

coreydaley · 2022-04-18T16:46:43Z

test/e2e/slow_test.go

 	testArgs.SearchString = "invoker"
 	framework.ExecPod(testArgs)

 	framework.CreateShareRelatedRBAC(testArgs)
-	t.Logf("%s: wait up to 10 minutes for examining pod %s since the controller does not currently watch all clusterroles and clusterrolebindings and reverse engineer which ones satisfied the SAR calls, so we wait for relist on shares", time.Now().String(), testArgs.Name)
+	t.Logf("%s: wait up to 11 minutes for examining pod %s since the controller does not currently watch all clusterroles and clusterrolebindings and reverse engineer which ones satisfied the SAR calls, so we wait for relist on shares", time.Now().String(), testArgs.Name)


Does this need the testArgs.TestDuration or will it roll over from the previous one?

rolls over from previous

Ok, thanks.

coreydaley · 2022-04-18T17:12:47Z

/lgtm

gabemontero · 2022-04-18T17:13:47Z

/lgtm

thanks @coreydaley

that said,

/hold

have to squash your review commit

…e doc

gabemontero · 2022-04-18T17:15:10Z

/hold cancel

ok @coreydaley can you re-lgtm? thanks

coreydaley · 2022-04-18T17:15:48Z

/lgtm

openshift-ci · 2022-04-18T18:25:46Z

@gabemontero: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gabemontero · 2022-04-18T18:29:46Z

/label docs-approved

no docs impact - internal implementation detail - also see relative lack on docs in upstream k8s around using inotify, etc. ... considered a "linux" thing

gabemontero · 2022-04-18T18:30:07Z

/label px-approved

internal implementation detail

gabemontero · 2022-04-18T18:31:24Z

launching clusterbot cluster with this PR for final verification of entitled build usage

gabemontero · 2022-04-18T19:17:54Z

forgot, cannot do entitled builds with clusterbot clusters

had to verify manually via local build ... will do so again after this merges and we have a level with it

/label qe-approved

gabemontero · 2022-04-19T12:44:30Z

verified openshift builds with entitlements at level 4.11.0-0.ci-2022-04-18-211945

bump(*): k8s 1.23; k8s/k8s for atomic_writer

40624a9

openshift-ci bot assigned adambkaplan and coreydaley Apr 14, 2022

openshift-ci bot requested review from coreydaley and otaviof April 14, 2022 16:22

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2022

coreydaley requested changes Apr 14, 2022

View reviewed changes

gabemontero force-pushed the atomic-writer branch from 8503d02 to ec47d2e Compare April 15, 2022 15:49

coreydaley reviewed Apr 18, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2022

gabemontero added 4 commits April 18, 2022 13:14

switch from vanilla write file to atomic writer

f2a430d

move simple example to local config map; fix and update simple exampl…

7bddf44

…e doc

ci-operator yaml to go 1.17

b1b41e1

tweak slow test window to fully allow for 10 minute relist

73f2ef6

gabemontero force-pushed the atomic-writer branch from b717afa to 73f2ef6 Compare April 18, 2022 17:14

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Apr 18, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Apr 18, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 18, 2022

openshift-merge-robot merged commit 6e3b1e0 into openshift:master Apr 18, 2022

gabemontero deleted the atomic-writer branch April 18, 2022 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use k8s atomic writer for storage of secrets / configmaps #93

use k8s atomic writer for storage of secrets / configmaps #93

gabemontero commented Apr 14, 2022

openshift-ci bot commented Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

gabemontero Apr 18, 2022

coreydaley Apr 14, 2022

coreydaley Apr 14, 2022

gabemontero commented Apr 14, 2022 •

edited

gabemontero commented Apr 14, 2022

gabemontero commented Apr 14, 2022

gabemontero commented Apr 14, 2022

gabemontero commented Apr 15, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

coreydaley Apr 18, 2022

gabemontero Apr 18, 2022

coreydaley Apr 18, 2022

coreydaley commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

coreydaley commented Apr 18, 2022

openshift-ci bot commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 19, 2022

use k8s atomic writer for storage of secrets / configmaps #93

use k8s atomic writer for storage of secrets / configmaps #93

Conversation

gabemontero commented Apr 14, 2022

openshift-ci bot commented Apr 14, 2022

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

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

gabemontero commented Apr 14, 2022 • edited

gabemontero commented Apr 14, 2022

gabemontero commented Apr 14, 2022

gabemontero commented Apr 14, 2022

gabemontero commented Apr 15, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

coreydaley commented Apr 18, 2022

openshift-ci bot commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 18, 2022

gabemontero commented Apr 19, 2022

gabemontero commented Apr 14, 2022 •

edited