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

[FAB-15648] hints to document: Non-TLS orderer with etcdraft usage #1678

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

davidkhala
Copy link
Contributor

@davidkhala davidkhala commented Aug 4, 2020

Type of change

  • Documentation update

Description

  • Notes about configure TLSCARoot
  • Notes about configure non-general-TLS orderer with etcdraft mode

Additional details

Signed-off-by: davidliu [email protected]

Related issues

FAB-15648

@davidkhala davidkhala requested review from a team as code owners August 4, 2020 05:28
@davidkhala davidkhala changed the title [FAB-15648] Add some hints to document [FAB-15648] hints to document: Non-TLS orderer with etcdraft usage Aug 4, 2020
pamandrejko
pamandrejko previously approved these changes Aug 5, 2020

Note: `ListenPort`, `ListenAddress`, `ServerCertificate`, `ServerPrivateKey` must
be either set together or unset together.
If they are unset, they are inherited from the general TLS section,
in example `general.tls.{privateKey, certificate}`.
In the use case when general TLS is disabled, these parameters must be set while:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In the use case when general TLS is disabled, these parameters must be set while:
In the use case when general TLS is disabled, these parameters must be set:

small typo I think

* `ListenPort`: The port the cluster listens on. If blank, the port is the same
port as the orderer general port (`general.listenPort`)
* `ListenPort`: The port the cluster listens on.
It must be same as `consenters[i].Port` in Channel configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the best place to mention this. It feels like this belongs in the section describing the Consenters but that would require reordering some sections of this doc. The natural flow to me would be to describe the configuration of a single raft node and then how it fits into the channel configuration, not vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I see this difficulty in how to express how they are bound. If another maintainer also agree on we are better to reorganize sections then I am glad to change accordingly.


Note: `ListenPort`, `ListenAddress`, `ServerCertificate`, `ServerPrivateKey` must
be either set together or unset together.
If they are unset, they are inherited from the general TLS section,
in example `general.tls.{privateKey, certificate}`.
In the use case when general TLS is disabled, these parameters must be set:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In the use case when general TLS is disabled, these parameters must be set:
When general TLS is disabled:


Note: `ListenPort`, `ListenAddress`, `ServerCertificate`, `ServerPrivateKey` must
be either set together or unset together.
If they are unset, they are inherited from the general TLS section,
in example `general.tls.{privateKey, certificate}`.
In the use case when general TLS is disabled, these parameters must be set:
- use different `ListenPort` than the orderer general port
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- use different `ListenPort` than the orderer general port
- use a different `ListenPort` than the orderer general port


Note: `ListenPort`, `ListenAddress`, `ServerCertificate`, `ServerPrivateKey` must
be either set together or unset together.
If they are unset, they are inherited from the general TLS section,
in example `general.tls.{privateKey, certificate}`.
In the use case when general TLS is disabled, these parameters must be set:
- use different `ListenPort` than the orderer general port
- Properly configure TLS root CAs. Search scope includes
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we need to configure the TLS root CAs when TLS is disabled? I've never tried to use the orderer without TLS enabled but this seems odd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine this is if you aren't defining the TLS CAs in the channel config? Although seems better to define them in the channel config even if TLS is disabled generally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see consenters mutual connection errors when TLS root CAs could not be found in anywhere. I am not 100% sure of the search scope, but indeed we should have at least once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about re-statement as Properly configure TLS root CAs. For instance, define them for each orderer organization in channel config @jyellick

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for losing track of this.

Yes, that sentence is better. Though I think it would be superior to say "Properly configure TLS root CAs in the channel configuration." I do not believe that recent versions of Fabric will even allow you to define Raft endpoints if their TLS certificate is not valid according to some TLS CA in the channel config.

@caod123
Copy link

caod123 commented Jan 4, 2021

It looks like this PR hasn't been touched in some time. @jyellick can you review @davidkhala's last question?

- Notes about configure TLSCARoot
- Notes about configure non-general-TLS orderer with etcdraft mode

Signed-off-by: davidliu <[email protected]>
@davidkhala
Copy link
Contributor Author

@jyellick update and look for review again

@jyellick jyellick merged commit 0744363 into hyperledger:master Jan 22, 2021
@pamandrejko
Copy link
Contributor

@Mergifyio backport release-2.3

@pamandrejko
Copy link
Contributor

@Mergifyio backport release-2.2

@mergify
Copy link

mergify bot commented Jan 22, 2021

Command backport release-2.3: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 22, 2021
- Notes about configure TLSCARoot
- Notes about configure non-general-TLS orderer with etcdraft mode

Signed-off-by: davidliu <[email protected]>
(cherry picked from commit 0744363)
@mergify
Copy link

mergify bot commented Jan 22, 2021

Command backport release-2.2: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 22, 2021
- Notes about configure TLSCARoot
- Notes about configure non-general-TLS orderer with etcdraft mode

Signed-off-by: davidliu <[email protected]>
(cherry picked from commit 0744363)
mergify bot pushed a commit that referenced this pull request Jan 22, 2021
- Notes about configure TLSCARoot
- Notes about configure non-general-TLS orderer with etcdraft mode

Signed-off-by: davidliu <[email protected]>
(cherry picked from commit 0744363)
mergify bot pushed a commit that referenced this pull request Jan 22, 2021
- Notes about configure TLSCARoot
- Notes about configure non-general-TLS orderer with etcdraft mode

Signed-off-by: davidliu <[email protected]>
(cherry picked from commit 0744363)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants