docs: add design for adding new arguments for TLS #173

bipuladh · 2024-11-15T04:28:05Z

Describe what this PR does

This PR adds a design doc, proposing the introduction of Services in front of pods that have addons sidecar container running in them.

Is there anything that requires special attention

Do you have any questions?

Is the change backward compatible?
Yes

Are there concerns around backward compatibility?
No

Provide any external context for the change, if any.

The addition of Service is required to allow secure communication between the add-ons manager and the sidecar. Related PR on the addons repo: csi-addons/kubernetes-csi-addons#692

Related issues

csi-addons/kubernetes-csi-addons#692
#172

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Madhu-1

Creating a unique service per pod in Kubernetes when using a Deployment/Damoneset (which typically manages multiple replicas) is not a common pattern. However, it is possible to do so and it will be very tricky, lets see if we can get to the state.

Madhu-1 · 2024-11-19T08:40:54Z

docs/design/podService.md

+
+To address the above challenges, this proposal suggests creating a dedicated Kubernetes Service for each add-ons sidecar Pod. This Service will:
+
+- Provide a consistent DNS name, eliminating the dependency on IP addresses.


Does this works when both deployment and daemonset runs on the host networking? You need to use service with nodePort and the accessibility will be with hostIP+port.

Madhu-1 · 2024-11-19T08:47:00Z

docs/design/podService.md

+- Enable TLS certificates to be issued with a stable DNS name.
+- Allow secure connectivity between the manager and the sidecar by abstracting away the networking specifics, especially when host networking is enabled.
+
+The Service could be of type `ClusterIP`, as the primary objective is intra-cluster communication between the manager and sidecar.


If we are trying to stick to a single IP for accessing the pod, using clusterIP can be a problem for edge case where the service is deleted and recreated.

Madhu-1 · 2024-11-19T08:48:02Z

docs/design/podService.md

+
+- The sidecar Pod will have an annotation, e.g., `csi.addons.service/name`, that stores the corresponding Service name.
+- The CSI Addons Manager will parse this annotation and resolve the Service DNS name.
+- The TLS certificate will then be generated using this DNS name, enabling secure and verifiable communication.


Who is going to generate it? Is it user or the csi operator?

Madhu-1 · 2024-11-19T08:52:51Z

docs/design/podService.md

+
+## Implementation Details
+
+1. **Service Creation**: Modify the ceph-csi-operator configuration to create a Service for each sidecar Pod, with the appropriate pod-selectors.


If the service need to be created before hand , this could be very tricky especially for Daemonset with taints and toleration. where the user can just go and a taint to a node to schedule a new csi plugin pod on the node.

Madhu-1 · 2024-11-19T08:53:38Z

docs/design/podService.md

+#### Pod annotations
+
+```yaml
+apiVersion: v1
+kind: Pod
+metadata:
+  name: addon-sidecar
+  namespace: <namespace>
+  annotations:
+    csi.addons.service/name: addon-sidecar-service
+spec:
+  containers:
+    - name: addon-sidecar
+      image: <sidecar-image>
+```


Please add an example how this looks with deployment and daemonset.

bipuladh · 2024-11-19T09:52:47Z

Creating a unique service per pod in Kubernetes when using a Deployment/Damoneset (which typically manages multiple replicas) is not a common pattern. However, it is possible to do so and it will be very tricky, lets see if we can get to the state.

Do we need a unique service per pod or could the pod selector route work as well?

bipuladh · 2024-11-19T09:59:48Z

@Madhu-1 if these replicas of these Deployments need to be identified uniquely and connected could we perhaps use StatefulSets?

Madhu-1 · 2024-11-19T11:45:36Z

@Madhu-1 if these replicas of these Deployments need to be identified uniquely and connected could we perhaps use StatefulSets?

As per kubernetes-csi community we are using deployment, we cannot switch to statefulset

Madhu-1 · 2024-11-19T11:46:12Z

Creating a unique service per pod in Kubernetes when using a Deployment/Damoneset (which typically manages multiple replicas) is not a common pattern. However, it is possible to do so and it will be very tricky, lets see if we can get to the state.

Do we need a unique service per pod or could the pod selector route work as well?

Anything works We need a way to connect to the sidecar container.

nb-ohad · 2024-11-21T08:44:34Z

@Madhu-1 @bipuladh
We need to be very careful in deciding what additional responsibilities Ceph CSI Operator is taking upon itself. Not everything can or needs to be done at this layer, especially if the changes are tied to ODF-only features or to OCP-only features.

This design needs to be discussed and drilled down in one of our upcoming ceph csi operator meeting before we commit to it

Madhu-1 · 2024-11-21T08:52:39Z

@Madhu-1 @bipuladh We need to be very careful in deciding what additional responsibilities Ceph CSI Operator is taking upon itself. Not everything can or needs to be done at this layer, especially if the changes are tied to ODF-only features or to OCP-only features.

This design needs to be discussed and drilled down in one of our upcoming ceph csi operator meeting before we commit to it

@nb-ohad nothing is committed, the above was just a review, @bipuladh is planning to have a meeting to talk more about it.

Signed-off-by: Bipul Adhikari <[email protected]>

nb-ohad · 2024-12-05T09:27:06Z

docs/design/podService.md

+
+### Operator changes
+
+The Ceph CSI Operator is only responsible for taking in the information mounted to it and project those as volumes in the CSI Addons sidecar. The deployer of CSI Addons should create these certificates and mount it at `/etc/tls/tls.crt` and `/etc/tls/tls.key`. We keep this hardcoded to reduce the number of new arguments introduced. The logger should provide enough information if the user is not mounting this correctly for easy debugging.


@bipuladh I am not sure I understand

The deployer of CSI Addons should create these certificates

Can you please elaborate or rephrase?

This is not possible. I have updated the doc to reflect that client operator would pass the certificate using a secret. The locator for the secret will be added in a new field that I propose in the latest commit.

nb-ohad · 2024-12-05T09:30:11Z

docs/design/podService.md

+# Add support for TLS certificates for CSI Addons communication
+
+The Kubernetes CSI Addons project currently connects the CSI Addons Manager to the add-on sidecar without TLS in place.
+We propose a introduction of an arugment which enabled should mount the TLS certificates into the sidecar container.


Please provide a code snippet that describes the desired outcome of this new field within the API you would like to augment.

The code snippet should be descriptive enough so the following will be clear:

The name of the new field

The type of the new field

The API is being introduced into

The location (path) in the API where the new field will be added

An explanation of what the field is describing

And validation rules needed on the field (can be provided as standalone text or as a comment within the snippet)

Signed-off-by: Bipul Adhikari <[email protected]>

nb-ohad · 2024-12-05T10:04:10Z

docs/design/podService.md

+    certificateSecretname string
+    cretificateSecretNamespace string


This is just a Namespaced Name. But I would prefer to use a LocalObjectReference instead

nb-ohad · 2024-12-05T10:04:38Z

docs/design/podService.md

+type TLSConfiguration struct {
+    certificateSecretname string
+    cretificateSecretNamespace string
+    enableTLS bool


Why do we need an enable flag? The existance of the section is a good enough indication that we should use TLD

nb-ohad · 2024-12-05T10:05:29Z

docs/design/podService.md

+	// Allow overwrite of hardcoded defaults for any driver managed by this operator
+	//+kubebuilder:validation:Optional
+	DriverSpecDefaults *DriverSpec `json:"driverSpecDefaults,omitempty"`
+    TLSConfiguration *TLSConfiguration // Conigure TLS certificates for CSI addons


I think this needs to have a Driver scope, not an operator scope. The decision to use or not use TLS should be determined on a driver basis

bipuladh · 2024-12-05T11:15:54Z

/hold

Madhu-1 reviewed Nov 19, 2024

View reviewed changes

bipuladh mentioned this pull request Nov 19, 2024

Add support for TLS csi-addons/kubernetes-csi-addons#692

Open

bipuladh force-pushed the design-doc branch from b74e34f to c8acd5d Compare December 4, 2024 07:23

docs: add design for using services for addons pods

ab3dfbf

Signed-off-by: Bipul Adhikari <[email protected]>

bipuladh force-pushed the design-doc branch from c8acd5d to ab3dfbf Compare December 4, 2024 07:28

bipuladh changed the title ~~docs: add design for using services for addons pods~~ docs: add design for adding new arguments for TLS Dec 4, 2024

bipuladh requested a review from Madhu-1 December 4, 2024 07:36

nb-ohad requested changes Dec 5, 2024

View reviewed changes

Addressing more comments; Makes changes to the proposal

54f3cab

Signed-off-by: Bipul Adhikari <[email protected]>

nb-ohad requested changes Dec 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add design for adding new arguments for TLS #173

docs: add design for adding new arguments for TLS #173

bipuladh commented Nov 15, 2024

Madhu-1 left a comment

Madhu-1 Nov 19, 2024

Madhu-1 Nov 19, 2024

Madhu-1 Nov 19, 2024

Madhu-1 Nov 19, 2024

Madhu-1 Nov 19, 2024

bipuladh commented Nov 19, 2024 •

edited

Loading

bipuladh commented Nov 19, 2024

Madhu-1 commented Nov 19, 2024

Madhu-1 commented Nov 19, 2024

nb-ohad commented Nov 21, 2024

Madhu-1 commented Nov 21, 2024

nb-ohad Dec 5, 2024

bipuladh Dec 5, 2024

nb-ohad Dec 5, 2024

nb-ohad Dec 5, 2024

nb-ohad Dec 5, 2024 •

edited

Loading

nb-ohad Dec 5, 2024

bipuladh commented Dec 5, 2024


		To address the above challenges, this proposal suggests creating a dedicated Kubernetes Service for each add-ons sidecar Pod. This Service will:

		- Provide a consistent DNS name, eliminating the dependency on IP addresses.


		## Implementation Details

		1. Service Creation: Modify the ceph-csi-operator configuration to create a Service for each sidecar Pod, with the appropriate pod-selectors.


		### Operator changes

		The Ceph CSI Operator is only responsible for taking in the information mounted to it and project those as volumes in the CSI Addons sidecar. The deployer of CSI Addons should create these certificates and mount it at `/etc/tls/tls.crt` and `/etc/tls/tls.key`. We keep this hardcoded to reduce the number of new arguments introduced. The logger should provide enough information if the user is not mounting this correctly for easy debugging.

		certificateSecretname string
		cretificateSecretNamespace string

docs: add design for adding new arguments for TLS #173

Are you sure you want to change the base?

docs: add design for adding new arguments for TLS #173

Conversation

bipuladh commented Nov 15, 2024

Describe what this PR does

Is there anything that requires special attention

Related issues

Madhu-1 left a 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

bipuladh commented Nov 19, 2024 • edited Loading

bipuladh commented Nov 19, 2024

Madhu-1 commented Nov 19, 2024

Madhu-1 commented Nov 19, 2024

nb-ohad commented Nov 21, 2024

Madhu-1 commented Nov 21, 2024

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

nb-ohad Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh commented Dec 5, 2024

bipuladh commented Nov 19, 2024 •

edited

Loading

nb-ohad Dec 5, 2024 •

edited

Loading