Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add websocket for celestia-node #14

Closed
wants to merge 15 commits into from
Closed

Conversation

aWN4Y25pa2EK
Copy link
Contributor

@aWN4Y25pa2EK aWN4Y25pa2EK commented Jul 11, 2024

No description provided.

@aWN4Y25pa2EK aWN4Y25pa2EK changed the title feat: add p2p webtransport for celestia-node feat: add websocket for celestia-node Jul 11, 2024
Copy link
Member

@smuu smuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the effort!

@@ -326,6 +338,14 @@ spec:
path: OAZHALLLMV4Q
- key: my_celes_key_info
path: my_celes_key.info
- name: tls-certs
secret:
secretName: tls-da-wss
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the secret name configurable via the values.
It should be optional to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, document what the secret should contain.

@@ -272,6 +276,14 @@ spec:
- name: home-dir # This is needed so that the process has permissions to create files in the home directory
mountPath: {{ .Values.node.settings.home }}
readOnly: false
- name: tls-certs
mountPath: /home/celestia/tls/cert.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use {{ .Values.node.settings.home }} instead of /home/celestia

subPath: fullchain.pem
readOnly: true
- name: tls-certs
mountPath: /home/celestia/tls/key.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use {{ .Values.node.settings.home }} instead of /home/celestia

secret:
secretName: tls-da-wss
items:
- key: fullchain.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename file to cert.pem

items:
- key: fullchain.pem
path: fullchain.pem
- key: privkey.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename file to key.pem

@smuu
Copy link
Member

smuu commented Jul 22, 2024

Can we close this, as websocket is not yet supported by libp2p?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this file?

Comment on lines 162 to 184
ListenAddresses:
# IPv4
- /ip4/0.0.0.0/udp/2121/quic-v1/webtransport
- /ip6/::/udp/2121/quic-v1/webtransport
- /ip4/0.0.0.0/udp/2121/quic-v1
- /ip6/::/udp/2121/quic-v1
#- /ip4/0.0.0.0/tcp/2122/wss
- /ip4/0.0.0.0/tcp/2121
# IPv6
- /ip6/::/udp/2121/quic-v1/webtransport
- /ip6/::/udp/2121/quic-v1
#- /ip6/::/tcp/2122/wss
- /ip6/::/tcp/2121
AnnounceAddresses: []
NoAnnounceAddresses:
# IPv4
- /ip4/127.0.0.1/udp/2121/quic-v1/webtransport
- /ip4/0.0.0.0/udp/2121/quic-v1/webtransport
- /ip6/::/udp/2121/quic-v1/webtransport
- /ip4/0.0.0.0/udp/2121/quic-v1
- /ip4/127.0.0.1/udp/2121/quic-v1
- /ip6/::/udp/2121/quic-v1
- /ip4/0.0.0.0/tcp/2121
- /ip4/127.0.0.1/tcp/2122/wss
- /ip4/127.0.0.1/tcp/2121
- /ip6/::/tcp/2121
# IPv6
- /ip6/::1/udp/2121/quic-v1/webtransport
- /ip6/::1/udp/2121/quic-v1
- /ip6/::1/tcp/2122/wss
- /ip6/::1/tcp/2121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging, we need to check this.

Comment on lines +41 to +52
livenessProbe:
enabled: true
periodSeconds: 10
failureThreshold: 3
readinessProbe:
enabled: true
periodSeconds: 10
failureThreshold: 30
startupProbe:
enabled: true
periodSeconds: 10
failureThreshold: 60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these values different from the default?, if not, we could get rid of them to make it even easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, not really

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a env variable CELESTIA_TLS_PATH : /home/celestia/tls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a scenario where we want to deploy a helm chart without an tls certificate.

I propose to add a boolean to the setting, which will mount the secret and set the env var if it is set to true.

node:
  settings
    tls:
      enabled: <true/false>
      secretName: <secretName>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smuu smuu deleted the task/add-web-socket branch September 26, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants