ocs-to-ocs api server design #1413

sp98 · 2021-11-30T12:27:09Z

This PR adds the design for API server that would help
Consumer clusters to communicate with OCS Provider cluster.

Signed-off-by: Santosh Pillai [email protected]

leseb · 2021-12-06T09:11:22Z

docs/design/ocs-to-ocs-api-server.md

+    - Current use case does not involve frequent requests between client and server. So performance aspect of the gRPC can be ignored.
+
+- #### Security:
+    - Both provide security with SSL/TLS support.


Suggested change

- Both provide security with SSL/TLS support.

- Both provide security with TLS support.

leseb · 2021-12-06T09:11:36Z

docs/design/ocs-to-ocs-api-server.md

+### gRPC vs REST
+
+- #### Performance:
+    - gRPC is more performant than REST due to smaller payloads (and other reasons).


Name the other reasons or remove the statement?

I would change it to: gRPC is more performant than REST due to the binary payload

I would add that the binary format (protobuf) allows for compile time validation.

Updated the verbiage and added some more reasons.

leseb · 2021-12-06T09:12:16Z

docs/design/ocs-to-ocs-api-server.md

+    - Both provide security with SSL/TLS support.
+    - gRPC endpoint can't be invoked via browser/postman.
+
+We will use gRPC as we are on k8s and gRPC has more advantages compared to REST


At this stage, the advantages are not clear... Also, I'm not sure why being on k8s justifies using gRPC.

Security is a big concern especially when discussing services.
There is no benefit to use REST

leseb · 2021-12-06T09:13:55Z

docs/design/ocs-to-ocs-api-server.md

+- Read the `status.state` of the StorageConsumer CR:
+  - if `status` is `onboarding`, then update the `spec.Capacity` in the CR. Return `UNAVAILABLE`.
+  - if `status` is `Provisioning`, return `UNAVAILABLE`.
+  - if `status` is `Ready`, fetch the `rbdPoolName` and the`cephUser` details from the CR status. Generate and return json connection details.


Replace rbdPoolName with blockPoolName?

docs/design/ocs-to-ocs-api-server.md

oritwas · 2021-12-06T11:29:04Z

docs/design/ocs-to-ocs-api-server.md

+This document defines the API server used for communication between OCS provider and OCS consumer clusters.
+
+
+## Tool


We already decided on gRPC.
You can mention that REST can be supported easily.
I don't see the need to mention OpenAPI3

removed OpenAPI3

oritwas · 2021-12-06T12:52:24Z

docs/design/ocs-to-ocs-api-server.md

+message OnBoardConsumerRequest{
+    // token provided by the provider cluster admin to authenticate the consumer
+    string token =1;
+    // capacity is a valid k8s resource quantity


Is there a protobuf defined for capacity in k8s already?
Please document that this is the desired capacity and that the provider may grant less

Yes, there is protobuf for Resource Capacity in K8s already. Using that now. Also updated the doc.

But it seems that K8s protobuf is using an older version. And we won't be able to use it in our project. Getting following error

protoc-gen-go: invalid Go import path "resource" for "k8s.io/apimachinery/pkg/api/resource/generated.proto" The import path must contain at least one forward slash ('/') character. See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information. --go_out: protoc-gen-go: Plugin failed with status code 1.

Existing issue on github

docs/design/ocs-to-ocs-api-server.md

oritwas · 2021-12-06T12:53:52Z

docs/design/ocs-to-ocs-api-server.md

+// StorageConfigResponse holds the response for the GetStorageConfig API request
+message StorageConfigResponse{
+    // data contains the json blob to be used by the consumer cluster to connect with the provider cluster
+    google.protobuf.Struct data = 1;


How about calling it blob?

The type should be bytes[] (byte array)

replaced google.protobuf.Struct with bytes. I don't think blob might be good idea. It would be equally confusing as data, if not less.

docs/design/ocs-to-ocs-api-server.md

jarrpa · 2021-12-07T15:04:52Z

docs/design/ocs-to-ocs-api-server.md

+// UpdateCapacityResponse holds the response for UpdateCapacity API request
+message UpdateCapacityResponse{
+
+}


Similarly here, provide the granted capacity, just in case.

jarrpa · 2021-12-07T15:06:23Z

docs/design/ocs-to-ocs-api-server.md

+| Invalid ID | 3 INVALID_ARGUMENT | Request failed to invalid Consumer ID | Contact the Provider Cluster Admin to verify the StorageConsumer ID
+| Invalid Capacity | 3 INVALID_ARGUMENT | Request failed due to invalid capacity | Contact the Provider Cluster Admin to get the valid capacity value


How do you distinguish between these two? Might it be more accurate to say:

Invalid | 3 INAVLID_ARGUMENT | Request failed due to invalid Consumer ID or Capacity | ....

jarrpa · 2021-12-07T15:07:06Z

docs/design/ocs-to-ocs-api-server.md

+
+## PROJECT STRUCTURE
+- The API server would be part of the OCS Operator.
+- It would be deployed as separate deployment on the OCS cluster only after user has enabled it in the StorageCluster CR via `spec.allowRemoteStorageConsumers`


Suggested change

- It would be deployed as separate deployment on the OCS cluster only after user has enabled it in the StorageCluster CR via `spec.allowRemoteStorageConsumers`

- It would be deployed as separate Deployment only after user has enabled it in the StorageCluster CR via `spec.allowRemoteStorageConsumers`

docs/design/ocs-to-ocs-api-server.md

jarrpa · 2021-12-07T15:07:53Z

docs/design/ocs-to-ocs-api-server.md

+This document defines the API server used for communication between OCS provider and OCS consumer clusters.
+
+
+## Tool


Move all this to the bottom as an ## Alternatives section.

moved this section at the end.

openshift-ci · 2021-12-07T15:26:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sp98
To complete the pull request process, please ask for approval from jarrpa after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

This PR adds the design for API server that would help Consumer clusters to communicate with OCS Provider cluster. Signed-off-by: Santosh Pillai <[email protected]>

sp98 · 2021-12-08T00:55:31Z

/retest

malayparida2000 · 2022-10-14T07:24:43Z

@sp98 is this pr relevant now?

openshift-ci · 2023-07-17T08:40:27Z

@sp98: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws	`a69c7a9`	link	false	`/test red-hat-storage-ocs-ci-e2e-aws`
ci/prow/ocs-operator-bundle-e2e-aws	`a69c7a9`	link	true	`/test ocs-operator-bundle-e2e-aws`

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.

sp98 force-pushed the api-server-design branch 2 times, most recently from bdc271a to 32bb718 Compare December 6, 2021 01:43

leseb reviewed Dec 6, 2021

View reviewed changes

sp98 force-pushed the api-server-design branch from 32bb718 to 1bc3026 Compare December 6, 2021 09:53

sp98 mentioned this pull request Dec 6, 2021

docs: Add a design doc for the ocs to ocs StorageConsumer CR and its controller #1422

Open

sp98 force-pushed the api-server-design branch from 1bc3026 to adf9058 Compare December 6, 2021 10:07

oritwas reviewed Dec 6, 2021

View reviewed changes

sp98 force-pushed the api-server-design branch 2 times, most recently from fa1e77f to 076d5f8 Compare December 6, 2021 11:52

oritwas reviewed Dec 6, 2021

View reviewed changes

docs/design/ocs-to-ocs-api-server.md Show resolved Hide resolved

oritwas reviewed Dec 6, 2021

View reviewed changes

sp98 force-pushed the api-server-design branch from 076d5f8 to ff7d173 Compare December 7, 2021 14:18

sp98 requested a review from leseb December 7, 2021 14:24

sp98 force-pushed the api-server-design branch from ff7d173 to a5457a1 Compare December 7, 2021 14:26

jarrpa requested changes Dec 7, 2021

View reviewed changes

openshift-ci bot assigned jarrpa Dec 7, 2021

sp98 force-pushed the api-server-design branch from a5457a1 to eb2cfdc Compare December 7, 2021 15:26

sp98 force-pushed the api-server-design branch from eb2cfdc to b9be71e Compare December 7, 2021 15:28

ocs-to-ocs api server design

a69c7a9

This PR adds the design for API server that would help Consumer clusters to communicate with OCS Provider cluster. Signed-off-by: Santosh Pillai <[email protected]>

sp98 force-pushed the api-server-design branch from b9be71e to a69c7a9 Compare December 7, 2021 15:29

sp98 requested a review from jarrpa December 7, 2021 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocs-to-ocs api server design #1413

ocs-to-ocs api server design #1413

sp98 commented Nov 30, 2021

leseb Dec 6, 2021

sp98 Dec 6, 2021

leseb Dec 6, 2021

oritwas Dec 6, 2021

oritwas Dec 6, 2021

sp98 Dec 6, 2021 •

edited

Loading

leseb Dec 6, 2021

oritwas Dec 6, 2021

leseb Dec 6, 2021

sp98 Dec 6, 2021

oritwas Dec 6, 2021

sp98 Dec 7, 2021

oritwas Dec 6, 2021

sp98 Dec 7, 2021

sp98 Dec 8, 2021 •

edited

Loading

oritwas Dec 6, 2021

oritwas Dec 6, 2021 •

edited

Loading

sp98 Dec 7, 2021 •

edited

Loading

jarrpa Dec 7, 2021

jarrpa Dec 7, 2021

sp98 Dec 7, 2021

jarrpa Dec 7, 2021

jarrpa Dec 7, 2021

sp98 Dec 7, 2021

openshift-ci bot commented Dec 7, 2021

sp98 commented Dec 8, 2021

malayparida2000 commented Oct 14, 2022

openshift-ci bot commented Jul 17, 2023

	- Both provide security with SSL/TLS support.
	- Both provide security with TLS support.

		This document defines the API server used for communication between OCS provider and OCS consumer clusters.


		## Tool

		\| Invalid ID \| 3 INVALID_ARGUMENT \| Request failed to invalid Consumer ID \| Contact the Provider Cluster Admin to verify the StorageConsumer ID
		\| Invalid Capacity \| 3 INVALID_ARGUMENT \| Request failed due to invalid capacity \| Contact the Provider Cluster Admin to get the valid capacity value

	- It would be deployed as separate deployment on the OCS cluster only after user has enabled it in the StorageCluster CR via `spec.allowRemoteStorageConsumers`
	- It would be deployed as separate Deployment only after user has enabled it in the StorageCluster CR via `spec.allowRemoteStorageConsumers`

ocs-to-ocs api server design #1413

Are you sure you want to change the base?

ocs-to-ocs api server design #1413

Conversation

sp98 commented Nov 30, 2021

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

sp98 Dec 6, 2021 • edited Loading

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

sp98 Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oritwas Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

sp98 Dec 7, 2021 • edited Loading

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

openshift-ci bot commented Dec 7, 2021

sp98 commented Dec 8, 2021

malayparida2000 commented Oct 14, 2022

openshift-ci bot commented Jul 17, 2023

sp98 Dec 6, 2021 •

edited

Loading

sp98 Dec 8, 2021 •

edited

Loading

oritwas Dec 6, 2021 •

edited

Loading

sp98 Dec 7, 2021 •

edited

Loading