Expose new Admin endpoint on orderer for the channel participation API #1939

wlahti · 2020-09-28T20:41:05Z

Type of change

New feature

Description

First commit

split out common system logic for http endpoints from existing operations system code

Second commit

expose new Admin endpoint on the orderer and move the channel participation API to this endpoint

Related issues

FAB-18246

yacovm · 2020-09-28T21:22:17Z

sampleconfig/orderer.yaml

+#   - This configures the admin server endpoint for the orderer
+#
+################################################################################
+Operations:


I'm confused, why do you have another operations endpoint, exactly like the existing one?

Aren't both endpoints supposed to be used by users possessing local admin credentials? Why do you need a separate port, isn't it possible to use the existing one?

My understanding is that the role using this admin endpoint to join/leave channels and the role using the operations endpoint to access metrics, change log levels. etc. are distinct and should be configurable separately.

Yes, the key here is that the identity performing operations is not a Fabric admin, it's a system admin. We would not want the identity used to perform health check, pull metrics, or modify log level to be the same identity responsible for joining/leaving channels. This is something that @sykesm brought up during the RFC review process and a new port is the simplest option going forward. In the future, the plan is to bring some non-MTLS authentication mechanism in, but that it outside the scope of the channel participation effort.

Yes, the key here is that the identity performing operations is not a Fabric admin, it's a system admin. We would not want the identity used to perform health check, pull metrics, or modify log level to be the same identity responsible for joining/leaving channels.

You can still accomplish this with a single port. You can allow both types of connections and then classify the client by its certificate.

a new port is the simplest option going forward.

I agree. It is much simpler this way.

@yacovm

I'm confused,

rightly so, it's a typo, should be Admin:

Ah, thanks for clarifying, Yoav. Definitely overlooked this. Fixed.

wlahti · 2020-09-29T15:17:28Z

Opened a new flake, FAB-18251, for the earlier flake (and pushed a likely fix: #1941). Current flake is FAB-16233. Grabbed the logs for that one and I'll take a look to see if anything jumps out.

jyellick · 2020-10-02T12:10:54Z

common/system/system.go

+	TLS           TLS
+}
+
+type System struct {


This name stutters, and I'm generally a little confused by the choice of package name. As a reader I would struggle to know what a system.System logically represents. I'd invite feedback for a better name... the more natural one would be http.Server or similar, but there is the obvious mental conflict with the stdlib. Maybe something like fabhttp.Server or mtlshttp.Server or... hopefully someone else has a better thought, but I really don't love system.System.

Agreed. Of those options, fabhttp.Server seems more obvious to me in terms of describing what it is.

Renamed to fabhttp.Server for now.

jyellick · 2020-10-02T12:11:47Z

common/system/system.go

+func NewSystem(o Options) *System {
+	logger := o.Logger
+	if logger == nil {
+		logger = flogging.MustGetLogger("system.runner")


Why is this system.runner instead of simply the package name? What value does runner add?

I was following what's there in the operations system code. I'm not opposed to renaming this.

jyellick · 2020-10-02T12:25:38Z

orderer/common/admin/system.go

+	Version       string
+}
+
+type System struct {


Wasn't the whole point of factoring the operations.System out into system.System so that you could re-use it? This seems like entirely duplicated code?

Hmmm, I'm honestly not sure what happened with the code here. It looks like I messed things up when I split things into two commits. I'll get this back to what I intended and push an update...

Oh right, this was my first pass making sure I understood the common code that was needed before splitting it out into a separate package. I'll remove this dead code.

jyellick · 2020-10-02T12:29:32Z

sampleconfig/orderer.yaml

+#   - This configures the admin server endpoint for the orderer
+#
+################################################################################
+Operations:


Yes, the key here is that the identity performing operations is not a Fabric admin, it's a system admin. We would not want the identity used to perform health check, pull metrics, or modify log level to be the same identity responsible for joining/leaving channels. This is something that @sykesm brought up during the RFC review process and a new port is the simplest option going forward. In the future, the plan is to bring some non-MTLS authentication mechanism in, but that it outside the scope of the channel participation effort.

tock-ibm · 2020-10-11T10:06:24Z

orderer/common/server/main.go

+func newAdminServer(ops localconfig.Admin) *fabhttp.Server {
+	return fabhttp.NewServer(fabhttp.Options{
+		Logger:        flogging.MustGetLogger("orderer.admin"),
+		ListenAddress: ops.ListenAddress,
+		TLS: fabhttp.TLS{
+			Enabled:            ops.TLS.Enabled,
+			CertFile:           ops.TLS.Certificate,
+			KeyFile:            ops.TLS.PrivateKey,
+			ClientCertRequired: ops.TLS.ClientAuthRequired,
+			ClientCACertFiles:  ops.TLS.ClientRootCAs,


ops is not the right name for this argument... copy & paste error I assume.

tock-ibm · 2020-10-11T10:08:40Z

sampleconfig/orderer.yaml

+#   - This configures the admin server endpoint for the orderer
+#
+################################################################################
+Operations:


a new port is the simplest option going forward.

I agree. It is much simpler this way.

tock-ibm · 2020-10-11T10:48:56Z

sampleconfig/orderer.yaml

+################################################################################
+#
+#   Admin Configuration
+#
+#   - This configures the admin server endpoint for the orderer
+#
+################################################################################
+Operations:


This field should be called Admin:, we already have Operations: above.
This probably means we're missing a unit tests that loads an updated orderer.yaml.

tock-ibm · 2020-10-11T10:54:51Z

sampleconfig/orderer.yaml

+#   - This configures the admin server endpoint for the orderer
+#
+################################################################################
+Operations:


@yacovm

I'm confused,

rightly so, it's a typo, should be Admin:

tock-ibm · 2020-10-11T11:01:27Z

integration/nwo/orderer_template.go

+  TLS:
+    Enabled: {{ .TLSEnabled }}
+    PrivateKey: {{ $w.OrdererLocalTLSDir Orderer }}/server.key
+    Certificate: {{ $w.OrdererLocalTLSDir Orderer }}/server.crt


Does this (using OrdererLocalTLSDir) mean that in all the tests we share the crypto material of the Operations and Admin endpoints?
Is this what we want?
Is there a testing benefit for separating OrdererLocalTLSDir from say, OrdererAdminTLSDir?

tock-ibm · 2020-10-11T11:07:32Z

Update commit message to reflect package names after last commit.

wlahti · 2020-10-12T12:49:58Z

Update commit message to reflect package names after last commit.

Thanks for catching this. Noticed it last week but forgot to get back to updating it.

tock-ibm · 2020-10-13T05:47:47Z

Looks good to me.

Move the common logic for http server/handler and TLS authentication used by the operations.System into a separate fabhttp.Server and refactor the operations.System to embed the server. This is being done to allow reuse of the common code for the orderer Admin endpoint, which will host the Channel Participation API. FAB-18246 Signed-off-by: Will Lahti <wtlahti@us.ibm.com>

Move the channel participation API to use this endpoint. FAB-18246 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com>

wlahti · 2020-10-26T11:59:38Z

Just a rebase on master since this has been sitting around a while.

C0rWin · 2020-10-26T12:09:54Z

common/fabhttp/server.go

+	return server
+}
+
+func (s *Server) Run(signals <-chan os.Signal, ready chan<- struct{}) error {


I might be missed that, but where do you use this method? It's seems unused.

This allows us to run as an ifrit.Runner. It's currently used in tests via ifrit.Invoke. I preserved this functionality as it was added by Matt when he implemented the operations service.

Ok, thanks for the clarification.

wlahti requested a review from a team as a code owner September 28, 2020 20:41

wlahti changed the title ~~Expose new Admin endpoint on orderer~~ Expose new Admin endpoint on orderer for the channel participation API Sep 28, 2020

yacovm reviewed Sep 28, 2020

View reviewed changes

jyellick reviewed Oct 2, 2020

View reviewed changes

wlahti force-pushed the fab-18246 branch from 9d8771f to 9e58e21 Compare October 2, 2020 16:08

tock-ibm reviewed Oct 11, 2020

View reviewed changes

wlahti force-pushed the fab-18246 branch from 9e58e21 to 3daa197 Compare October 12, 2020 12:48

tock-ibm previously approved these changes Oct 13, 2020

View reviewed changes

C0rWin added the channel-participation-api label Oct 15, 2020

wlahti dismissed tock-ibm’s stale review via fedb578 October 20, 2020 13:30

wlahti force-pushed the fab-18246 branch 2 times, most recently from fedb578 to 73b136f Compare October 20, 2020 16:46

wlahti added 2 commits October 26, 2020 07:58

Expose new Admin endpoint on orderer

6808c8e

Move the channel participation API to use this endpoint. FAB-18246 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com>

wlahti force-pushed the fab-18246 branch from 73b136f to 6808c8e Compare October 26, 2020 11:59

C0rWin reviewed Oct 26, 2020

View reviewed changes

C0rWin approved these changes Oct 26, 2020

View reviewed changes

C0rWin merged commit d59af2c into hyperledger:master Oct 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose new Admin endpoint on orderer for the channel participation API #1939

Expose new Admin endpoint on orderer for the channel participation API #1939

wlahti commented Sep 28, 2020

yacovm Sep 28, 2020

wlahti Sep 29, 2020

jyellick Oct 2, 2020

yacovm Oct 3, 2020

tock-ibm Oct 11, 2020

tock-ibm Oct 11, 2020

wlahti Oct 13, 2020

wlahti commented Sep 29, 2020

jyellick Oct 2, 2020

wlahti Oct 2, 2020

wlahti Oct 2, 2020

jyellick Oct 2, 2020

wlahti Oct 2, 2020

wlahti Oct 13, 2020

jyellick Oct 2, 2020

wlahti Oct 2, 2020

wlahti Oct 2, 2020

wlahti Oct 2, 2020

jyellick Oct 2, 2020

tock-ibm Oct 11, 2020

wlahti Oct 13, 2020

tock-ibm Oct 11, 2020

tock-ibm Oct 11, 2020

tock-ibm Oct 11, 2020

tock-ibm Oct 11, 2020

tock-ibm commented Oct 11, 2020

wlahti commented Oct 12, 2020

tock-ibm commented Oct 13, 2020

wlahti commented Oct 26, 2020

C0rWin Oct 26, 2020

wlahti Oct 26, 2020 •

edited

Loading

C0rWin Oct 26, 2020

Expose new Admin endpoint on orderer for the channel participation API #1939

Expose new Admin endpoint on orderer for the channel participation API #1939

Conversation

wlahti commented Sep 28, 2020

Type of change

Description

Related issues

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

wlahti commented Sep 29, 2020

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

tock-ibm commented Oct 11, 2020

wlahti commented Oct 12, 2020

tock-ibm commented Oct 13, 2020

wlahti commented Oct 26, 2020

Choose a reason for hiding this comment

wlahti Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlahti Oct 26, 2020 •

edited

Loading