Initial structure. Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section. #1

carlbraganza · 2023-06-25T07:16:30Z

No description provided.

Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section.

Clarified the proposal.

carlbraganza · 2023-06-27T17:07:45Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+  - [User Stories)](#user-stories)
+    - [Full snapshot backup](#full-snapshot-backup)
+    - [Incremental snapshot backup](#incremental-snapshot-backup)
+  - [Notes/Constraints/Caveats](#notesconstraintscaveats)
  - [Risks and Mitigations](#risks-and-mitigations)
 - [Design Details](#design-details)
+  - [The SnapshotMetadata Service API](#the-snapshotmetadata-service-api)
+  - [Kubernetes Components](#kubernetes-components)
+  - [Custom Resources](#custom-resources)
+    - [CSISnapshotSessionAccess](#csisnapshotsessionaccess)
+    - [CSISnapshotSessionService](#csisnapshotsessionservice)
+    - [CSISnapshotSessionData](#csisnapshotsessiondata)
+  - [The Snapshot Session Manager](#the-snapshot-session-manager)
+  - [The External Snapshot Session Sidecar](#the-external-snapshot-session-sidecar)
+  - [The SP Snapshot Session Service](#the-sp-snapshot-session-service)


I put some sections in so I could create cross references at the start, and also updated the TOC here.

carlbraganza · 2023-06-27T17:11:23Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+  > The volume could be attached to a pod with either `Block` or `Filesystem`
+    [volume modes](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#volume-mode).
+* Provide an API to retrieve the data blocks of a snapshot.
+  > The **snapshot access session** mechanism proposed here could conceivably


I am using the phrase snapshot access session consistently to describe the mechanism, and not "session" or "snapshot session".

However I have used just "Snapshot Session Manager" and "snapshot session sidecar".
This is a bit inconsistent. Should I just be using "snapshot session"?

"snapshot session" sounds good to me

carlbraganza · 2023-06-27T17:12:19Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+@TODO Prasad

-### Notes/Constraints/Caveats (Optional)
+### Notes/Constraints/Caveats


I could think of the caveats below. Please add/suggest additional.

carlbraganza · 2023-06-27T17:13:14Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md


 Consider including folks who also work outside the SIG or subproject.
 -->
+The main vulnerabilities of this proposal are:


I've come to realize that this is a very important section and the security model is explained here as the mitigation mechanism.

PrasadG193 · 2023-06-28T06:32:42Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

-    - [Story 1](#story-1)
-    - [Story 2](#story-2)
-  - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
+  - [User Stories)](#user-stories)


Suggested change

- [User Stories)](#user-stories)

- [User Stories](#user-stories)

[nit] typo

Use the hack/update-toc.sh script to update the TOC, per instructions within the KEP:

Ensure the TOC is wrapped with
<!-- toc --&rt;<!-- /toc --&rt;
tags, and then generate with hack/update-toc.sh.

PrasadG193 · 2023-06-28T06:35:42Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+  > The volume could be attached to a pod with either `Block` or `Filesystem`
+    [volume modes](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#volume-mode).
+* Provide an API to retrieve the data blocks of a snapshot.
+  > The **snapshot access session** mechanism proposed here could conceivably


"snapshot session" sounds good to me

PrasadG193 · 2023-06-28T06:40:08Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+or the sizes of the volumes and snapshots involved.
+
+A Kubernetes backup application establishes a snapshot access session by
+creating an instance of a [CSISnapshotSessionAccess](#csisnapshotsessionaccess)


Suggested change

creating an instance of a [CSISnapshotSessionAccess](#csisnapshotsessionaccess)

creating an instance of a [SnapshotSessionAccess](#snapshotsessionaccess)

We debated whether to include CSI* prefix to the CRs. To keep is consistent with VolumeSnapshots resources, I think we should not include the prefix in the CRs.

As previously, I recommend naming these CRDs following existing patterns that the community is already familiar with, to help clarify the intention. E.g.,

CsiSnapshotSessionAccess -> SnapshotSessionRequest - both K8s and CSI has similar resources named TokenRequest
CsiSnapshotSessionService -> SnapshotServiceConfiguration - there was a comment on the previous PR to use the Configuration suffix similar to MutatingWebhookConfiguration, ValidatingWebhookConfiguration etc.

Any reasons why we want to deviate from these? So far, I haven't heard any agreement or disagreement on this idea.

Sounds good!

PrasadG193 · 2023-06-28T06:57:58Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+  ensure that the invoker has access rights to the VolumeSnapshot
+  objects specified in the payload.


Suggested change

ensure that the invoker has access rights to the VolumeSnapshot

objects specified in the payload.

ensure that the invoker has access rights to the VolumeSnapshot

resource.

We cannot check the access permission on specific objects. It can be done at the resource level only.

Changing to "access rights to the VolumeSnapshot objects in the Namespace".

PrasadG193 · 2023-06-28T07:04:51Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+- The **CSISnapshotSessionServiceClusterRole** should be used in a
+  RoleBinding to grant the ServiceAccount used by the
+  [external-snapshot-session-sidecar](#the-external-snapshot-session-sidecar)
+  of the CSI driver all access in the CSI driver namespace only.


Should we also mention about access to which resources? Also, since we decided to make SnapshotSessionData CR is cluster scoped resource, we need to create ClusterRoleBinding instead of RoleBinding

The SnapshotSessionData CR has to remain namespaced - the SnapshotSessionService CR can become global.

Would we need a new role for the SnapshotSessionService? Maybe we can leave it namespaced to avoid adding another role?

ihcsim · 2023-06-28T16:02:24Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+The RPC calls each return a gRPC stream through which the metadata can be recovered.
+
+The CSI driver is not involved in the setup or management of the snapshot access session.
+The TCP endpoint returned is actually directed to a community provided


community provided

To me, "community" means both the maintainers and users. If this is part of CSI, I'd rephrase it as "directed to the CSI external-snapshot-session sidecar".

Changed to "directed to the external-snapshot-session sidecar". Not sure if "CSI" is applicable here as it is a K8s implementation artifact.

It's not a K8s "core" artifact. AIUI, all existing external-* sidecars are part of CSI because they all lived in the kubernetes-csi GH org.

ihcsim · 2023-06-28T16:04:01Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+The CSI driver provided service only focuses on the generation of the metadata
+requested.
+
+The [CSISnapshotSessionAccess](#csisnapshotsessionaccess) CR is animated


What does "animated" mean in this context?

Think puppet to puppet-master! :-)

ihcsim · 2023-06-28T16:11:44Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+service permits metadata to be returned in either an ***extent-based***
+format or a ***block*** based format, at the discretion of the CSI driver.


at the discretion of the CSI driver.

Does this imply built-in intelligence on the CSI driver to determine which format is most appropriate? Is this format also pre-specified by the user? Feels like a vendor-specific implementation.

@PrasadG193 what do you think of updating the user stories section with the "extent-based" vs. "block-based" scenario i.e. how do they benefit the users?

No intelligence implied. I think I've made it clear that it is vendor-specific: at the discretion of the CSI driver.

This came from existing implementations: VMware uses extents, AWS uses blocks. Previous iterations of the KEP had the same approach.

ihcsim · 2023-06-28T16:20:54Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+- The CSI driver's [Snapshot Session Service](#the-sp-snapshot-session-service)
+must be capable of serving metadata on a VolumeSnapshot
+concurrently with the backup application's use of a PersistentVolume
+created on that same VolumeSnapshot.
+This is because the backup application has to mount the PersistentVolume
+in Block mode in a Pod in order to read and archive the raw snapshot data blocks,
+and this read/archive loop will be driven by the stream of snapshot block metadata.


Not sure about this one. I thought this design shouldn't be influenced by how and when the backup application's data mover pod mounts, reads and access the PV?

I softened the language slightly: "... the backup application would likely mount ...".

This point is necessary, IMO, because we need to ensure that such concurrency is possible. I agree we don't need to mandate it.

ihcsim · 2023-06-28T16:25:22Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+to implement the necessary security as illustrated in the following figure:
+
+<!-- Need to update the figure to show non-namespaced CSISnapshotSessionService CR -->
+![CSI Snapshot Session Roles](./cbt-kep-multi-csi-roles.drawio.svg)


Consider using a white background. The transparent background doesn't work well with GH dark theme.

Okay - I'll add that to the comment.

ihcsim · 2023-06-28T16:30:15Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+of VolumeSnapshots. It will then
+search for a [SnapshotMetadata](#the-snapshotmetadata-service-api) service
+in the CSI driver for these VolumeSnapshots.
+On success, the TCP endpoint and CA certificate of the


Can the CA cert be stored in a cluster-scoped resource that both the backup application and service have access to? This ensures that during cert rotation, there is only one source of truth for the CA cert, instead of multiple ones embedded in CRs floating around in the cluster.

As long as the CA cert is rotated before the session expiry time then there is no problem. The manager does not/should not cache data.

ihcsim · 2023-06-28T16:40:32Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+Mutual authentication, however, is not performed, as to do so would require the
+server to trust the certificate authorities used by its clients and no
+obvious mechanism exists for this purpose.


The client can present a TLS cert signed with the server's CA trust found in the CR. The server will only trust clients that can present client-side certs signed with its CA trust. However, this can get complicated because the client will need access to an cert signer/issuer somewhere.

Instead of saying "no obvious mechanism exists", consider re-phrasing it to something like "MA can be done by requiring the client to produce a cert signed by the server's CA. But a bit complicated due to dependency on an cert signer/issuer - to be re-evaluated for future versions".

On the first para: nothing gained by the suggestion as encryption is already present and the sever knows its own certificate.

On the second para, I agree it needs some rewording. I want to convey that there is no "blessed"/"prescribed"/"standardized" K8s way of getting a client certificate that services can trust. Maybe the SIG security team will provide feedback on this.

I will change to use "no standardized mechanism" for now.

The idea behind the 1st paragraph is that the server will not accept any request from client that either can't present a client-side TLS cert, or that its TLS cert is signed by an unrecognized CA trust. It's used more for identity verification. My point is that MA is possible, but not easy and will make our design more complicated.

I will change to use "no standardized mechanism" for now.

That works.

ihcsim · 2023-06-28T16:47:15Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+  the permissions it needs to access all the
+  [custom resources defined by this proposal](#custom-resources).
+
+It is recommended that the security design be reviewed by SIG Security.


Yeah, this will need to happen.

ihcsim · 2023-06-28T16:48:24Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

+### Kubernetes Components
+The following Kubernetes components are involved at runtime:
+
+- A community provided


As above, to me "community" means both maintainers and users. If it's maintained by CSI, then it should be just "The CSI Snapshot Session Manager".

It is not maintained by "CSI": it is part of the K8s implementation of CSI.

I think your interpretation of community is spot-on: all K8s stuff is community provided at this point! This phrase was used by Ben and there were no objections to it in the WG or CSI community meeting.

I see what you mean - thanks. I just assumed that the k8s implementation of CSI is also part of CSI since they all lived in the kubernetes-csi GH repo.

ihcsim · 2023-06-28T16:52:27Z

keps/sig-storage/3314-csi-changed-block-tracking/README.md

-    - [Story 1](#story-1)
-    - [Story 2](#story-2)
-  - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
+  - [User Stories)](#user-stories)


Use the hack/update-toc.sh script to update the TOC, per instructions within the KEP:

Ensure the TOC is wrapped with
<!-- toc --&rt;<!-- /toc --&rt;
tags, and then generate with hack/update-toc.sh.

carlbraganza · 2023-06-28T19:20:15Z

Use the hack/update-toc.sh script to update the TOC, per instructions within the KEP:

Wow! I hadn't seen that. Cool! We can delay the generation TOC for when we're done.

ihcsim

LGTM - thanks for working on this 👍.

Initial structure. Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section.

Initial structure.

ba1e4c3

Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section.

carlbraganza requested review from PrasadG193 and ihcsim June 25, 2023 07:16

carlbraganza added 3 commits June 26, 2023 18:00

Removed distracting links to common K8s definitions.

fdb94b1

Clarified the proposal.

More caveats. Better grammar.

7b5403e

Use "snapshot access session".

4dfcd49

carlbraganza commented Jun 27, 2023

View reviewed changes

PrasadG193 reviewed Jun 28, 2023

View reviewed changes

ihcsim reviewed Jun 28, 2023

View reviewed changes

carlbraganza added 3 commits June 28, 2023 12:35

addressed most of the feedback in the PR.

5e50923

Updated role figure.

93f8f61

More refinements.

66eb80c

carlbraganza requested review from PrasadG193 and ihcsim June 29, 2023 02:37

ihcsim approved these changes Jun 29, 2023

View reviewed changes

carlbraganza added 2 commits June 29, 2023 09:45

Session figure. Renamed figure files.

3efbfbc

Fix background of session figure.

3df9a5b

carlbraganza merged commit ffe8562 into csi-cbt-kep Jun 29, 2023

carlbraganza added a commit that referenced this pull request Jul 3, 2023

Merge pull request #1 from ihcsim/carl-proposal-caveats-risks

a42d41e

Initial structure. Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section.

	- [User Stories)](#user-stories)
	- [User Stories](#user-stories)

	creating an instance of a [CSISnapshotSessionAccess](#csisnapshotsessionaccess)
	creating an instance of a [SnapshotSessionAccess](#snapshotsessionaccess)

		ensure that the invoker has access rights to the VolumeSnapshot
		objects specified in the payload.

		service permits metadata to be returned in either an *extent-based*
		format or a *block* based format, at the discretion of the CSI driver.

Initial structure. Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section. #1

Initial structure. Filled in the Proposal, Caveats and Risks. Put in the CSI spec in the Details section. #1

Uh oh!

Conversation

carlbraganza commented Jun 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlbraganza Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlbraganza Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlbraganza Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlbraganza Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlbraganza Jun 28, 2023 •

edited

Loading

carlbraganza Jun 28, 2023 •

edited

Loading

carlbraganza Jun 28, 2023 •

edited

Loading

carlbraganza Jun 28, 2023 •

edited

Loading

ihcsim Jun 28, 2023 •

edited

Loading

ihcsim Jun 28, 2023 •

edited

Loading