Feat/external api #74

coryschwartz · 2022-09-17T01:03:25Z

fixes: #72
fixes: #71

RobotSail · 2022-10-10T16:49:05Z

api/v1alpha1/ipfscluster_types.go

+type ExternalSettings struct {
+	// +kubebuilder:validation:Enum={ingress,loadbalancer,none}
+	// +optional
+	Strategy ExternalStrategy `json:"strategy,omitempty"`


What exactly are these fields supposed to change or control? Could you please include a one-sentence comment above the new fields and structs being added?

coryschwartz · 2022-10-18T09:28:47Z

@RobotSail can you check this out when you get the chance?

coryschwartz · 2022-10-18T09:33:25Z

This is running in the test cluster,

you can find the gateway with kubectl get services

and this can be used as a gateway just like ipfs.io. i.e. you can find content in a browser by hitting /ipfs/<CID>

RobotSail · 2022-10-18T15:08:18Z

controllers/statefulset.go

@@ -135,28 +146,28 @@ func (r *IpfsClusterReconciler) StatefulSet(m *clusterv1alpha1.IpfsCluster,
 							},
 							Ports: []corev1.ContainerPort{
 								{
-									Name:          "swarm",
-									ContainerPort: portSwarm,
+									Name:          nameIpfsSwarm,


RobotSail · 2022-10-18T15:36:12Z

controllers/service.go

+		svcName,
+		m.Name,
+		m.Namespace,
+		"LoadBalancer",


Does this always need to be a loadbalancer? Could a ClusterIP not fulfill this role?

RobotSail · 2022-10-18T15:39:36Z

controllers/ipfscluster_controller.go

@@ -161,6 +166,13 @@ func (r *IpfsClusterReconciler) createTrackedObjects(
 		&secConfig: mutSecConfig,
 		&sts:       mutSts,
 	}
+
+	if instance.Spec.Gateway.Strategy == clusterv1alpha1.ExternalStrategyLoadBalancer {


So in other words, only LoadBalancer is currently supported?

RobotSail · 2022-10-18T15:42:15Z

helm/ipfs-operator/templates/Deployment-ipfs-operator-controller-manager.yaml

@@ -43,8 +43,8 @@ spec:
        - --leader-elect
        command:
        - /manager
-        image: "{{ include "container-image" (list . .Values.image) }}"
-        imagePullPolicy: {{ .Values.image.pullPolicy }}
+        image:  quay.io/redhat-et-ipfs/ipfs-operator:v0.0.1


I'm not sure why but one of the make targets always makes this change. We want to revert this so that the template exists

RobotSail · 2022-10-18T15:44:25Z

api/v1alpha1/ipfscluster_types.go

 const (
 	// ReproviderStrategyAll Announces the CID of every stored block.
 	ReproviderStrategyAll ReproviderStrategy = "all"
 	// ReproviderStrategyPinned Only announces the pinned CIDs recursively.
 	ReproviderStrategyPinned ReproviderStrategy = "pinned"
 	// ReproviderStrategyRoots Only announces the root block of explicitly pinned CIDs.
-	ReproviderStrategyRoots ReproviderStrategy = "roots"
+	ReproviderStrategyRoots      ReproviderStrategy = "roots"
+	ExternalStrategyNone         ExternalStrategy   = "none"


The string values themselves should be in Pascal case to be consistent with the Kubernetes style.
So these would be None, LoadBalancer, Ingress

RobotSail · 2022-10-18T15:44:58Z

api/v1alpha1/ipfscluster_types.go

+	// +optional
+	Strategy ExternalStrategy `json:"strategy,omitempty"`
+	// +optional
+	Annotations map[string]string `json:"interval,omitempty"`


What is this for?

RobotSail · 2022-10-18T15:45:29Z

api/v1alpha1/ipfscluster_types.go

@@ -91,6 +105,8 @@ type IpfsClusterSpec struct {
 	// should use when reproviding content.
 	// +optional
 	Reprovider ReprovideSettings `json:"reprovider,omitempty"`
+	Gateway    ExternalSettings  `json:"gateway"`


Needs a description

RobotSail · 2022-10-18T15:51:16Z

controllers/service.go

+		m.Name,
+		m.Namespace,
+		"LoadBalancer",
+		m.Annotations,


Is there a reason we're passing the CR's annotations onto the expected service?

RobotSail · 2022-10-18T15:53:30Z

controllers/service.go

+		m.Name,
+		m.Namespace,
+		"LoadBalancer",
+		m.Annotations,


Again, this is passing the same annotations onto the expected service

RobotSail

Some changes needed but it looks good overall, though I did have some questions:

At the moment it looks like we're providing options to have LoadBalancer and Ingress, is there a reason that ClusterIP isn't supported?
I like that you provided options for what the users can select, but I saw in the code that it only does something when LoadBalancer is provided, whereas Ingress and None are both doing nothing. If ClusterIP isn't supported for whatever reason, should we just drop the other fields?
I noticed that you added the option for the user to provide their own annotations, is there a reason we can't provide these values ourselves?

patilsuraj767

Hello, I have added some observations below.

patilsuraj767 · 2023-01-10T13:37:45Z

api/v1alpha1/ipfscluster_types.go

@@ -91,6 +105,8 @@ type IpfsClusterSpec struct {
 	// should use when reproviding content.
 	// +optional
 	Reprovider ReprovideSettings `json:"reprovider,omitempty"`
+	Gateway    ExternalSettings  `json:"gateway"`
+	ClusterAPI ExternalSettings  `json:"clusterApi"`


I think we can make Gateway and ClusterAPI as optional field.

Good call. Setting these to omitempty

patilsuraj767 · 2023-01-10T13:40:57Z

controllers/ipfscluster_controller.go

+	mutgwsvc, _ := r.serviceGateway(instance, &gwsvc)
+	mutapisvc, _ := r.serviceAPI(instance, &apisvc)
+


Should we consider moving this under the below if condition on line 170?

patilsuraj767 · 2023-01-11T03:16:35Z

Along with this changes we will also need to add CLUSTER_RESTAPI_HTTPLISTENMULTIADDRESS=/ip4/0.0.0.0/tcp/9094 in configure-script - controllers/scripts/config.go and same as environment variable in statefulset - controllers/statefulset.go

coryschwartz · 2023-01-13T09:32:42Z

So as discussed before, This adds the ability to expose ports outside the cluster. At the moment, it supports only "LoadBalancer" strategy, but I expect we will want to add an Ingress strategy also in a future PR.

I think some of the old comments have been lost in resolved conversations. The reason for the "AppendAnnotations" is becasuse I am thinking of how we will configure the future Ingress strategy.

Ingress Controllers tend to accept configuration through annotations, and this will let you do such things as set Host names and URL routes.

Likewise, I the user might have external-dns automation, and this is also controlled by annotation.

The LoadBalancer, with no annotations, is probably the simplest, and crudest way to expose the service.

One of the comments is asking why we don't support ClusterIP, and we do! But those services are already created. I just didn't want to expose the administrative endpoints

If you want to try it out, there is an "gateway" service running now! There is no DNS or anything, it's reachable here on port 8080

a0371fb3c5dc7448e8aff816b8c00454-264540294.us-west-1.elb.amazonaws.com

You can try this with anything CID that works on the ipfs.io, cloudflare, or any other IPFS gateway.

Try this:
our gateway service:
a0371fb3c5dc7448e8aff816b8c00454-264540294.us-west-1.elb.amazonaws.com:8080/ipfs/QmNs9ACzPfEuosCpyykHRHhMHphoeMeH6vq826k3LYoXyx

same as other IFPS gateways
https://ipfs.filebase.io/ipfs/QmNs9ACzPfEuosCpyykHRHhMHphoeMeH6vq826k3LYoXyx
https://ipfs.io/ipfs/QmNs9ACzPfEuosCpyykHRHhMHphoeMeH6vq826k3LYoXyx

Authentication, etc, can be setup in a future PR to add Ingress Controller support

Cory Schwartz added 6 commits September 16, 2022 17:58

external apis

442d4ca

update examples

92c57b1

update helm template

1ab5584

Merge branch 'main' into feat/external-api

8446d6d

expected service function

d15b1c3

Merge branch 'main' into feat/external-api

ad08d14

coryschwartz marked this pull request as ready for review October 10, 2022 07:43

move peerid to secret

396503c

RobotSail reviewed Oct 10, 2022

View reviewed changes

Cory Schwartz added 3 commits October 18, 2022 01:34

Merge branch 'main' into feat/external-api

2aa1d1b

fix service name

5b182ef

helm chart

4057205

fix test

e89536f

RobotSail reviewed Oct 18, 2022

View reviewed changes

RobotSail requested changes Oct 18, 2022

View reviewed changes

use AppendAnnotations

03e0c8e

patilsuraj767 reviewed Jan 10, 2023

View reviewed changes

Cory Schwartz added 2 commits January 12, 2023 23:53

update helm template

57620de

address comments

7a44d58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/external api #74

Feat/external api #74

coryschwartz commented Sep 17, 2022

RobotSail Oct 10, 2022

coryschwartz commented Oct 18, 2022

coryschwartz commented Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail Oct 18, 2022

RobotSail left a comment

patilsuraj767 left a comment

patilsuraj767 Jan 10, 2023

coryschwartz Jan 13, 2023

patilsuraj767 Jan 10, 2023

patilsuraj767 commented Jan 11, 2023

coryschwartz commented Jan 13, 2023

		mutgwsvc, _ := r.serviceGateway(instance, &gwsvc)
		mutapisvc, _ := r.serviceAPI(instance, &apisvc)

Feat/external api #74

Are you sure you want to change the base?

Feat/external api #74

Conversation

coryschwartz commented Sep 17, 2022

Choose a reason for hiding this comment

coryschwartz commented Oct 18, 2022

coryschwartz commented Oct 18, 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

RobotSail left a comment

Choose a reason for hiding this comment

patilsuraj767 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

patilsuraj767 commented Jan 11, 2023

coryschwartz commented Jan 13, 2023