optional https port for s3 #4482

kmlebedev · 2023-05-18T12:43:39Z

What problem are we solving?

In kubernetes, traffic can be delivered through ingress, which itself allows you to do TLS and through the internal k8s service directly to the s3 api application and I would like to have additional https with TLS

It will also help to encrypt traffic if it not needs to be sent past the ingress #4479

How are we solving the problem?

optional https port for s3 api

How is the PR tested?

local with param -s3.port=8333 -s3.port.https=8433 -s3.key.file=/usr/local/share/ca-certificates/tls.key -s3.cert.file=/usr/local/share/ca-certificates/tls.crt

seaweedfs-s3-1      | I0518 12:26:07.332791 s3.go:263 Start Seaweed S3 API Server 30GB 3.50  at https port 8433
seaweedfs-s3-1      | I0518 12:26:07.333679 s3.go:281 Start Seaweed S3 API Server 30GB 3.50  at http port 8333

local with param -s3.port=8333 -s3.key.file=/usr/local/share/ca-certificates/tls.key -s3.cert.file=/usr/local/share/ca-certificates/tls.crt

seaweedfs-s3-1      | I0518 12:26:07.333679 s3.go:281 Start Seaweed S3 API Server 30GB 3.50  at https port 8333

local with param -s3.port=8333

seaweedfs-s3-1      | I0518 12:26:07.333679 s3.go:281 Start Seaweed S3 API Server 30GB 3.50  at http port 8333

Checks

I have added unit tests if possible.
I will add related wiki document changes and link to this PR after merging.

chrislusf · 2023-05-18T13:49:36Z

weed/command/s3.go

-		glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)
-		if s3ApiLocalListener != nil {
+		if *s3opt.portHttps == 0 {
+			glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)


the message should be "http port"?

No. Should be http because this message in blok with tls key exist.

Suggested change

glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)

glog.V(0).Infof("Start Seaweed S3 API Server %s at http port %d", util.Version(), *s3opt.port)

@chrislusf I didn’t successfully name the variable portHttps, but if it differs from 0 to zero, it means you need to run TLS on a separate port and in this code block only TLS is always started, so the message will always be https

logging on start

I0525 10:51:35.310073 s3.go:263 Start Seaweed S3 API Server 30GB 3.51 8e59d8fec at https port 443 I0525 10:51:35.310119 s3.go:281 Start Seaweed S3 API Server 30GB 3.51 8e59d8fec at http port 8080

kmlebedev · 2023-05-22T05:06:48Z

@chrislusf Hey, is this getting better?

chrislusf · 2023-05-22T05:24:17Z

weed/command/s3.go

-		}
-	} else {
+	}
+	if *s3opt.tlsPrivateKey == "" || *s3opt.portHttps > 0 {


not sure what is the intent here

not sure what is the intent here

@chrislusf The general idea is to run the s3 http server simultaneously on http and https different ports, while remaining backwards compatible when using a common port for http + TLS

Accordingly, this line allows you to start an http server without TLS on the "main" port, when TLS is already running on another https port

Did you see this comment ?

kmlebedev · 2023-05-25T06:07:19Z

@chrislusf ping

chrislusf · 2023-05-25T06:09:17Z

@chrislusf Hey, is this getting better?

comments were not addressed yet.

kmlebedev · 2023-05-25T06:20:26Z

comments were not addressed yet.

Ok. translated into resolved.

chrislusf · 2023-05-25T06:30:30Z

comments were not addressed yet.

Ok. translated into resolved.

They are marked as resolved. but no actual changes made.

seaweedfs#4482

# Conflicts: # weed/command/s3.go

kmlebedev · 2023-06-13T11:14:31Z

@chrislusf How about a separate TLS port ?

chrislusf · 2023-06-13T20:14:16Z

I suspect you did not see my comments which were marked as resolved.

kmlebedev

try finish

kmlebedev · 2023-05-18T19:14:12Z

weed/command/s3.go

-		glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)
-		if s3ApiLocalListener != nil {
+		if *s3opt.portHttps == 0 {
+			glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)


No. Should be http because this message in blok with tls key exist.

kmlebedev · 2023-05-22T06:00:51Z

weed/command/s3.go

-		}
-	} else {
+	}
+	if *s3opt.tlsPrivateKey == "" || *s3opt.portHttps > 0 {


not sure what is the intent here

@chrislusf The general idea is to run the s3 http server simultaneously on http and https different ports, while remaining backwards compatible when using a common port for http + TLS

Accordingly, this line allows you to start an http server without TLS on the "main" port, when TLS is already running on another https port

kmlebedev · 2023-05-25T07:21:40Z

weed/command/s3.go

-		glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)
-		if s3ApiLocalListener != nil {
+		if *s3opt.portHttps == 0 {
+			glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)


@chrislusf I didn’t successfully name the variable portHttps, but if it differs from 0 to zero, it means you need to run TLS on a separate port and in this code block only TLS is always started, so the message will always be https

logging on start

I0525 10:51:35.310073 s3.go:263 Start Seaweed S3 API Server 30GB 3.51 8e59d8fec at https port 443 I0525 10:51:35.310119 s3.go:281 Start Seaweed S3 API Server 30GB 3.51 8e59d8fec at http port 8080

kmlebedev · 2023-06-14T06:49:48Z

weed/command/s3.go

-		}
-	} else {
+	}
+	if *s3opt.tlsPrivateKey == "" || *s3opt.portHttps > 0 {


Did you see this comment ?

kmlebedev · 2023-06-14T06:52:08Z

I suspect you did not see my comments which were marked as resolved.

It looks like I created a review and until I finished it all the messages were in the pending status

#4482 Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

seaweedfs#4482 Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

optional https port for s3

11a3ee2

chrislusf reviewed May 18, 2023

View reviewed changes

chrislusf reviewed May 22, 2023

View reviewed changes

Merge branch 'master' into s3portHttps

8e59d8f

kmlebedev pushed a commit to kmlebedev/seaweedfs that referenced this pull request May 26, 2023

chart with optional https port for s3

408e131

seaweedfs#4482

kmlebedev mentioned this pull request May 26, 2023

chart with optional https port for s3 #4507

Merged

2 tasks

Merge branch 'master' into s3portHttps

9779221

# Conflicts: # weed/command/s3.go

kmlebedev requested a review from chrislusf June 14, 2023 06:46

kmlebedev commented Jun 14, 2023

View reviewed changes

chrislusf merged commit 4dd890d into seaweedfs:master Jun 14, 2023
5 checks passed

chrislusf pushed a commit that referenced this pull request Jun 20, 2023

chart with optional https port for s3 (#4507)

0515014

#4482 Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

hencoappel mentioned this pull request Jul 3, 2023

weed server -s3 segfault #4627

Closed

kmlebedev added a commit to kmlebedev/seaweedfs that referenced this pull request Dec 22, 2023

optional https port for s3 (seaweedfs#4482)

c959a6e

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

kmlebedev added a commit to kmlebedev/seaweedfs that referenced this pull request Dec 22, 2023

chart with optional https port for s3 (seaweedfs#4507)

b6bfbb3

seaweedfs#4482 Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optional https port for s3 #4482

optional https port for s3 #4482

kmlebedev commented May 18, 2023 •

edited

chrislusf May 18, 2023

kmlebedev May 18, 2023

chrislusf May 22, 2023

kmlebedev May 25, 2023

kmlebedev commented May 22, 2023

chrislusf May 22, 2023

kmlebedev May 22, 2023

kmlebedev Jun 14, 2023

kmlebedev commented May 25, 2023

chrislusf commented May 25, 2023

kmlebedev commented May 25, 2023

chrislusf commented May 25, 2023

kmlebedev commented Jun 13, 2023

chrislusf commented Jun 13, 2023

kmlebedev left a comment

kmlebedev May 18, 2023

kmlebedev May 22, 2023

kmlebedev May 25, 2023

kmlebedev Jun 14, 2023

kmlebedev commented Jun 14, 2023

	glog.V(0).Infof("Start Seaweed S3 API Server %s at https port %d", util.Version(), *s3opt.port)
	glog.V(0).Infof("Start Seaweed S3 API Server %s at http port %d", util.Version(), *s3opt.port)

optional https port for s3 #4482

optional https port for s3 #4482

Conversation

kmlebedev commented May 18, 2023 • edited

What problem are we solving?

How are we solving the problem?

How is the PR tested?

Checks

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

kmlebedev commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmlebedev commented May 25, 2023

chrislusf commented May 25, 2023

kmlebedev commented May 25, 2023

chrislusf commented May 25, 2023

kmlebedev commented Jun 13, 2023

chrislusf commented Jun 13, 2023

kmlebedev 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

kmlebedev commented Jun 14, 2023

kmlebedev commented May 18, 2023 •

edited