Add internal routes for ssh hook comands #1471

lunny · 2017-04-08T13:53:27Z

This is a first part for convert database operation to internal http operation for SSH hook commands. This will reduce the potential concurrent problem when use sqlite or tidb.

lunny · 2017-04-08T13:58:48Z

related to #1023

strk · 2017-04-08T15:08:56Z

This sounds overcomplex to me. Why don't both SSH and WEB access the database via an internal API instead ?

lunny · 2017-04-08T15:23:41Z

for sqlite and tidb, web command will open database and every push or pull via SSH will open database. It will result in concurrent problem and will spent more time when push or pull via SSH than open only one.

strk · 2017-04-08T15:40:43Z

Yes I understand the goal of the code, but I'm not sure about the implementation. Using the web requires authentication to guarantee the call comes from the inside, is this what the JWT is used for ?

Also, is it currently guarantee that there's a single database connection always ? No pooling ?

typeless · 2017-04-08T15:45:42Z

As far as I know, SQLite3 can handle concurrent operations in its own right (if it's not buggy). And for http requests, they can be handled concurrently as well. So I am afraid centralizing the responsibilities to the http router couldn't help too much.

I think the most effectives, and also the most challenging at the same time, is to incrementally eliminate the global variables/singletons.

Btw, the Go compiler developers are doing the similar things (removing global variables) for the compiler recently for concurrent compiling. I think we can not avoid it either. The problem will continue to haunt us if the code still sits on top of the globals.

bkcsoft

JWT-tokens with expiration makes no sense here. 5 minutes after gitea is started all SSH-access will fail (or at least error) since the token isn't being updated. IMO we should just set one token and keep stick with that (like security.SECRET_KEY). Further security improvements involves having /internal only accessable to localhost (since gitea doesn't support multi-host setups 😉

bkcsoft · 2017-04-09T01:08:52Z

modules/private/internal.go

@@ -0,0 +1,51 @@
+package private


Why is it called private when then endpoint is /internal ? Pick one and stay with it 😉

because Go don't allow a package named internal

ooh right, internal is "special" 😒

How about using /api/private then ? It's good to be consistent :)

And private.go, for that same reason...

@strk /api/private is inaccurate than /api/internal I think.

bkcsoft · 2017-04-09T01:11:20Z

modules/setting/setting.go

+
+		now := time.Now()
+		InternalToken, err = jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+			"exp": now.Add(5 * time.Minute).Unix(),


Either use a token with expiration, OR save it in the ini-file, not both. It makes no sense to persist ephemeral values.

Yes, I have saved it to ini file.

in that case, skip the expiration

bkcsoft · 2017-04-09T01:11:40Z

modules/setting/setting.go

+		if err := os.MkdirAll(filepath.Dir(CustomConf), os.ModePerm); err != nil {
+			log.Fatal(4, "Failed to create '%s': %v", CustomConf, err)
+		}
+		if err := cfgSave.SaveTo(CustomConf); err != nil {


Either use a token with expiration, OR save it in the ini-file, not both. It makes no sense to persist ephemeral values.

I don't see a reason to keep that token in the Ini file either, just have it randomly generated upon start and let it die/regenerate on restart

because we have to share the token between gitea web and gitea serv and gitea hook. And gitea web should be run firstly, then it will save token to ini if there is no token. After that, all other commands could read it.

@strk serv still has to get it from somewhere. This still makes sense :)

got it, thanks

bkcsoft · 2017-04-09T01:12:50Z

routers/private/internal.go

+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package private


same as above, internal or private doesn't matter to me, but the package name should reflect the endpoint name 🙂

So I will change all internal to private.

NVM actually, /private makes less sense than /internal. Just add a comment above package private explaining why it's called private instead of internal 🙂

Maybe you can just call it /ssh rather than /internal/ssh if there is no conflict.

Just realized there already is modules/ssh. Does it make any sense to combine the two?

@typeless it's not only for ssh, it's also for hooks commands. The hooks commands will be fired by both SSH and HTTPS. So I think /ssh is not suitable. modules/ssh is for builtin SSH server, it's not related with this PR.

@bkcsoft done.

bkcsoft · 2017-04-09T01:13:41Z

routers/private/internal.go

+
+// RegisterRoutes registers all internal APIs routes to web application.
+// These APIs will be invoked by internal commands for example `gitea serv` and etc.
+func RegisterRoutes(m *macaron.Macaron) {


We really need to start doing this in more places ❤️

lunny · 2017-04-09T07:58:40Z

@typeless Maybe sqlite has share mode, but tidb don't support. So currently tidb in fact cannot work via SSH.
Another obviously benefit of this PR is it will reduce SSH command execute time. When a push or pull to gitea, serv will be invoked and all gitea hooks will be invoked. Every command will read the ini file and open sqlite or tidb or create connection pool on mysql/postgres/mssql. So totally there are FOUR times duplicated operations. After we move all this to an internal web service. It will no need. And the web service should have started when you run gitea web.

tboerger · 2017-04-09T13:25:00Z

And the global variables are a totally different topic that got nothing to do with that...

typeless · 2017-04-10T06:19:57Z

@lunny Eliminating duplicate operations is all good, I agree. But I still cannot see how routing requests via a single entry can prevent data races. Aren't the requests still handled concurrently without explicit serialization (i.e. being protected by mutexes)? To put it differently, is a program automatically race-free without adding any mutex when all the concurrent routines have a common entry? It's not.

Maybe you have a bigger picture in mind beyond the data race issue. I am not against what the PR does, just questioning about it being a fix for data race. The purpose of reducing duplicates can justify it already.

typeless · 2017-04-11T03:57:30Z

modules/private/internal.go

+	}
+
+	resp.Body.Close()
+	if resp.StatusCode/100 != 2 {


Valid if the StatusCode is in 200...299 ?

Just copy the code from https://github.com/go-gitea/gitea/blob/master/cmd/hook.go#L224

Ok, got it.

@lunny Please add a comment above as it's not clear why this check is here :)

…ame is internal

…nal but the route name is internal

strk · 2017-04-11T16:15:51Z

On Mon, Apr 10, 2017 at 09:06:38PM -0700, Mura Li wrote: Maybe you can just call it `/ssh` rather than `/internal/ssh` if there is no conflict.

Remember that any /string needs to become a reserved word for organizations, so the less we use the better. If we have /api already, we should use that, like `/api/internal` for example

lunny · 2017-04-12T00:47:02Z

@strk good idea, but it's difficult to control the permission if we put it in /api/internal because /api has different permission requirement from /internal.

typeless · 2017-04-12T01:48:12Z

@strk I see. I used to imagine the internal routes would not be visible from the external. All urls passed from the external client were translated rather than mixing the two namespaces. I don't know whether doing so would be too heavy-weight or not. Every added abstraction solving a problem also comes with its cost. I would avoid too-clever techniques given my knowledge in this area (webdev) is not comprehensive to say the least.

bkcsoft · 2017-04-12T17:10:05Z

because /api has different permission requirement from /internal.

@lunny This is IMO a bug that should be fixed, not something to work around. I agree with @strk that it should be /api/internal. Putting it in /api makes for less conflicts so that makes more sense. (As is now, you can no longer create internal organization/user which is a breaking change 😒 )

bkcsoft · 2017-04-18T06:00:55Z

cmd/web.go

@@ -659,6 +660,11 @@ func runWeb(ctx *cli.Context) error {
 		apiv1.RegisterRoutes(m)
 	}, ignSignIn)

+	m.Group("/internal", func() {


This one should go in /api/internal

bkcsoft · 2017-04-18T07:10:55Z

I'm fine with this workaround for the time being. LGTM

appleboy · 2017-04-19T02:29:05Z

modules/private/internal.go

+		return err
+	}
+
+	resp.Body.Close()


using defer?

lunny · 2017-04-19T02:46:38Z

will fix #1479

appleboy · 2017-04-19T02:58:27Z

LGTM

add internal routes for ssh hook comands

c015e57

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 8, 2017

lunny added this to the 1.2.0 milestone Apr 8, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 8, 2017

fix lint

af91984

bkcsoft suggested changes Apr 9, 2017

View reviewed changes

lunny mentioned this pull request Apr 10, 2017

UpdatePublicKey: near "WHERE": syntax error #1479

Closed

7 tasks

typeless reviewed Apr 11, 2017

View reviewed changes

lunny added 2 commits April 11, 2017 19:48

add comment on why package named private not internal but the route n…

d4570aa

…ame is internal

add comment above package private why package named private not inter…

dc5980e

…nal but the route name is internal

remove exp time on internal access

c76983c

bkcsoft approved these changes Apr 18, 2017

View reviewed changes

bkcsoft suggested changes Apr 18, 2017

View reviewed changes

move routes from /internal to /api/internal

4f2c3a7

bkcsoft approved these changes Apr 18, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 18, 2017

lunny added the type/bug label Apr 19, 2017

appleboy reviewed Apr 19, 2017

View reviewed changes

add comment and defer on UpdatePublicKeyUpdated

def1241

appleboy approved these changes Apr 19, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 19, 2017

lunny merged commit 2eeae84 into go-gitea:master Apr 19, 2017

lunny deleted the lunny/internal_routes branch April 19, 2017 03:45

lunny mentioned this pull request Apr 19, 2017

ssh operations refrain to work since d409d3 #1501

Closed

5 tasks

twang2218 mentioned this pull request Jul 22, 2017

Cannot boot docker image when mount a custom app.ini configuration #2197

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Add internal routes for ssh hook comands #1471

Add internal routes for ssh hook comands #1471

Conversation

lunny commented Apr 8, 2017

lunny commented Apr 8, 2017

strk commented Apr 8, 2017

lunny commented Apr 8, 2017

strk commented Apr 8, 2017

typeless commented Apr 8, 2017

bkcsoft 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

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

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

lunny commented Apr 9, 2017 • edited Loading

tboerger commented Apr 9, 2017

typeless commented Apr 10, 2017

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

strk commented Apr 11, 2017 via email

lunny commented Apr 12, 2017

typeless commented Apr 12, 2017

bkcsoft commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Apr 19, 2017

appleboy commented Apr 19, 2017

lunny commented Apr 9, 2017 •

edited

Loading