feat: read https config from app:config #75

waitingsong · 2018-08-29T12:35:20Z

@tofix log print http not https cause master.js@Line469 use this.options.https

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

waitingsong · 2018-08-29T12:38:02Z

问题在这儿 https://cnodejs.org/topic/5b7ac9c7c52ad1482eb940bf#5b8675012a585e4e2f26ffc0
这个 PR 存在问题是以 https 启动成功后日志里面显示的还是 http://....
问题出在 master.js
https://github.com/eggjs/egg-cluster/blob/master/lib/master.js#L468

    address.protocal = this.options.https ? 'https' : 'http';
    address.port = this.options.sticky ? this[REALPORT] : address.port;
    this[APP_ADDRESS] = getAddress(address);

这儿用的是 this.options.https 做判断而没有考虑到 app.config.cluster .
看怎么解决

codecov · 2018-08-29T13:03:13Z

Codecov Report

Merging #75 into master will increase coverage by 0.3%.
The diff coverage is 97.22%.

@@            Coverage Diff            @@
##           master      #75     +/-   ##
=========================================
+ Coverage   98.35%   98.66%   +0.3%     
=========================================
  Files           7        8      +1     
  Lines         425      448     +23     
=========================================
+ Hits          418      442     +24     
+ Misses          7        6      -1

Impacted Files	Coverage Δ
lib/utils/options.js	`100% <ø> (ø)`	⬆️
lib/app_worker.js	`100% <100%> (+3.92%)`	⬆️
lib/utils/tls_options.js	`96.87% <96.87%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0de0021...3fbca88. Read the comment docs.

atian25 · 2018-08-29T13:42:12Z

你的 master 太旧了，要 rebase

waitingsong · 2018-08-29T15:12:13Z

新pull了才开的分支啊。。。

atian25 · 2018-08-29T15:16:04Z

看下 GitHub 帮助里面，如何 sync upstream，或者删掉重新 fork （原代码要注意）

waitingsong · 2018-08-30T01:25:10Z

看了下，和 upstream 就差一个提交 b0c8d19 。倒是 upstream 的版本tag没有同步下来

waitingsong · 2018-08-30T02:21:13Z

master.js 如何获取到 app.config.cluster 配置值这个不知道如何处理

waitingsong · 2018-08-30T03:21:54Z

@atian25 咨询个 egg 在 vsc debug的配置问题
断点时间长了会遇上 cluster-client 爆超时

2018-08-30 11:10:07,836 ERROR 932 nodejs.ClusterClientNoResponseError: client no response in 75299ms exceeding maxIdleTime 60000ms, maybe the connection is close on other side.
    at Timeout.Leader._heartbeatTimer.setInterval [as _onTimeout] (E:\project\node_modules\cluster-client\lib\leader.js:75:23)

修改 vsc 配置 launch.json 为

    {
      "name": "Egg Debug",
      "type": "node",
      "request": "launch",
      "runtimeExecutable": "npm",
      "runtimeArgs": [
        "run",
        "debug",
        "--",
        "--inspect-brk"
      ],
      "args": [
        "--heartbeatInterval", "30000", // <----------这儿
      ],
      "console": "integratedTerminal",
      "restart": true,
      "protocol": "auto",
      "port": 9229,
      "autoAttachChildProcesses": true
    },

egg/lib/agent.js 初始化时此参数值为期望值，但是走到 cluster-client/lib/leader.js 初始化时参数变成默认的 20000 了。
该如何传参覆盖默认值呢？

得空还是考虑用TS重写下egg吧，调试起来效率真不高，一个参数不知道在哪儿会被修改……

atian25 · 2018-09-07T01:45:44Z

lib/app_worker.js

+  if (https) {
+    const httpsOptions = Object.assign({}, https, {
+      key: fs.readFileSync(https.key),
+      cert: fs.readFileSync(https.cert),


需要补测试

既然是 listenConfig.https 的话，可以考虑支持全部的 options，而不只是这 2 个，https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options

options.https 的优先级更高（要测试覆盖）

const httpsOptions = Object.assign({}, listenConfig.https, options.https); httpsOptions.key = fs.readFileSync(httpsOptions.key); httpsOptions.cert = fs.readFileSync(httpsOptions.cert); httpsOptions.pfx = fs.readFileSync(httpsOptions.pfx);

https://github.com/eggjs/egg-cluster/pull/75/files/c7af27258d297eb0a08094b64b3055496b02f63d#diff-ee1e398c8b3f724318c68cc38d3bde50R24
const https = options.https || listenConfig.https;
这儿是优先使用 options.https 参数的。

这个测试的判断不知道咋写。另外如果用这个PR启动了https，但是控制台输出日志里面还是 http 而不是 https。如果日志不修改，测试也不好做判断吧。

const https = options.https || listenConfig.https; 这样是 2 选 1，assign 的话，就可以外部传递来覆盖。

不过都行，一般也不需要覆盖。

要支持下其他的配置，印象中 passphrase 之前有人提过。

pfx 这个也可以顺便支持下 readFileSync

测试是可以通过 Controller 去输出 protocol 的吧，@popomore test/fixtures/server.key 这个好像开源后就没对应的测试了？

你的意思是 https 里面的值要支持单独覆盖？我之前考虑过是整体覆盖还是细节覆盖，先实现的是整体。
其他配置参数的测试用例估计比较麻烦吧……
readFile 你指的是异步读取证书？

readFile 你指的是异步读取证书

不是，pfx 是 key 和 cert 的替代品，一般也是文件内容，所以顺便也 readFileSync 下。

你的意思是 https 里面的值要支持单独覆盖？我之前考虑过是整体覆盖还是细节覆盖，先实现的是整体。

都行，我之前的想法是 assign

其他配置参数的测试用例估计比较麻烦吧……

不需要，我们只需要测试能启动 https 就 ok 了。

@atian25 处理完 listenConfig.https 的参数后是否更新到 options.https 上以便后续使用？

atian25 · 2018-09-07T01:47:21Z

master.js 如何获取到 app.config.cluster 配置值这个不知道如何处理

master 不需要获取

咨询个 egg 在 vsc debug的配置问题

非相关问题可以其他渠道沟通

得空还是考虑用TS重写下egg吧，调试起来效率真不高，一个参数不知道在哪儿会被修改……

TS 解决不了问题，也没计划考虑重写。

atian25 · 2018-09-07T14:58:48Z

protocol 那个，@popomore 有什么建议？

waitingsong · 2018-09-10T11:48:02Z

删除了 options.key|cert 参数支持。仅支持 https.key|cert 参数
egg-mock 不支持测试 https （会抛异常），故没写完整的自签发证书启动服务用例。如果在启动 https 时 egg-mock 不抛异常，我可以补上相关用例。

  describe('options with https', () => {
    let app;
    before(() => {
      app = utils.cluster('apps/options', {
        framework: path.dirname(require.resolve('egg')),
        https: {
          key: '/key.unsecure',
          cert: '/02.crt',
        },
        port: 7701,
      });
      return app.ready();
    });
    after(() => app.close());
    it('should be passed through', () => {
      return app.httpRequest()
        .get('/')
        .expect('true');
    });
  });

egg-mock 异常

POST http://127.0.0.1:7701/__egg_mock_call_function error, method: mockRestore, args: []

使用配置参数（非命令行）传入 https 参数，启动系统后日志显示为不正确的 protocol: http://

waitingsong · 2018-09-11T08:32:48Z

重构处理证书逻辑：先合并参数，最后启动app_worker 时再验证、加载证书。适用于当配置的证书（文件）无效（不存在、过期等）时可在不修改代码情况下临时通过命令行指定有效的证书文件路径启动服务的情况。

@tofix

@tofix log print http not https cause master.js@Line469 use this.options.https

validate cert file exists during start worker

test exists in tls_options.test.js

waitingsong · 2018-11-13T12:31:20Z

没人看看？

atian25 · 2018-11-14T09:12:55Z

@dead-horse @popomore 瞅瞅？

README.md

catherinessssss · 2019-01-28T07:40:21Z

所以这里是有解决从config读https配置了吗？还是说还是只能
`egg-scripts start --daemon --https.key='path' --https.cert='path'?

waitingsong · 2019-01-28T07:44:56Z

所以这里是有解决从config读https配置了吗？

这个问题应该没解决：
#75 (comment)

catherinessssss · 2019-01-28T07:46:09Z

所以这里是有解决从config读https配置了吗？

这个问题应该没解决：
#75 (comment)

那这里是解决了什么问题？

waitingsong · 2019-01-28T07:48:33Z

所以这里是有解决从config读https配置了吗？

这个问题应该没解决：
#75 (comment)

那这里是解决了什么问题？

从 package.json 读取证书配置没问题，就是日志显示 protocol 不正常

catherinessssss · 2019-01-28T07:50:34Z

所以这里是有解决从config读https配置了吗？

这个问题应该没解决：
#75 (comment)

那这里是解决了什么问题？

从 package.json 读取证书配置没问题，就是日志显示 protocol 不正常

谢谢！之后会支持通过配置去开启https吗？而不是通过package.json

waitingsong · 2019-01-28T07:57:44Z

从 package.json 读取证书配置没问题，就是日志显示 protocol 不正常

谢谢！之后会支持通过配置去开启https吗？而不是通过package.json

按道理在命令行启动时额外传入证书参数也是可行的。这个我没测试。

catherinessssss · 2019-01-28T08:00:39Z

从 package.json 读取证书配置没问题，就是日志显示 protocol 不正常

谢谢！之后会支持通过配置去开启https吗？而不是通过package.json

按道理在命令行启动时额外传入证书参数也是可行的。这个我没测试。

可以支持类似配置吗？
config.cluster = {
listen: {
port: process.env.PORT || 7001,
hostname: '0.0.0.0',
},
https: {
key: process.env.PORT || '.key',
cert: process.env.PORT || '.crt',
}
};

waitingsong · 2019-01-29T14:11:39Z

@catherinessssss
支持 process.env.PORT || '.crt'
这个文件需要在项目根目录下存在

thonatos · 2019-05-16T07:20:22Z

supertest 里面检测 protocol 用了下面的代码:

// lib/test.js
protocol = app instanceof https.Server ? 'https' : 'http';

egg-mock 里返回的 app 是这样：

// lib/cluster.js

let clusterApp = new ClusterApplication(options);
clusterApp = new Proxy(clusterApp, {
  //...
});
return clusterApp;

https 方式访问 https://127.0.0.1:8443 证书会有问题，用 urllib 会方便一点。

thonatos · 2019-05-16T13:00:53Z

这样实现貌似也可以？

// lib/app_worker.js
// ...L25
const httpsOptions = Object.assign({}, clusterConfig.https, options.https);
const protocal = (httpsOptions.key && httpsOptions.cert) ? 'https' : 'http';

process.send({
  to: 'master',
  action: 'realprotocal',
  data: protocal,
});

// lib/master.js
const REALPROTOCAL = Symbol('Master#realprotocal');

class Master extends EventEmitter {
  constructor(options) {
    this[REALPORT] = undefined;
   // ...
    this.on('realprotocal', protocal => {
      if (protocal) this[REALPROTOCAL] = protocal;
    });
  }
}

// L586
address.protocal = this[REALPROTOCAL] || (this.options.https ? 'https' : 'http');

waitingsong force-pushed the https branch from 39c316f to c7af272 Compare August 30, 2018 01:23

popomore approved these changes Aug 30, 2018

View reviewed changes

atian25 requested changes Sep 7, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

waitingsong force-pushed the https branch from c7af272 to f61b6ce Compare September 10, 2018 11:45

waitingsong force-pushed the https branch from f61b6ce to 5597759 Compare September 10, 2018 11:50

atian25 requested a review from dead-horse September 11, 2018 03:14

waitingsong force-pushed the https branch from b1127de to 3fbca88 Compare September 11, 2018 08:30

waitingsong added 10 commits September 17, 2018 15:42

feat: read https config from app:config

2993cdd

@tofix log print http not https cause master.js@Line469 use this.options.https

fix: Missing semicolon

f2f0368

feat: options.https full support type SecureContextOptions

fe79cc8

docs: Update typeof https to SecureContextOptions

f409a3a

fix(eslint)

881e79b

refactor: parameter name of mergeTLSOpts()

97e0924

feat: return undefined if result is empty object

7c49e2f

test: add test/tls_options.test.js

13f841e

test: should options.{key|cert} NOT works

34ec43e

feat(typings): add index.d.ts

5d906cb

waitingsong added 7 commits September 17, 2018 15:42

docs: fix typo

792ae9c

refactor(typings): rename interface ClusterOptions to Options

e0d9063

feat(typings): update callback signature of startCluster()

649a8bc

docs: format

a98894a

refactor: move reading cert file into parseTLSOpts()

43f2cb3

validate cert file exists during start worker

chore: prune

f6fb15c

test: remove https in options.test.js

5944a60

test exists in tls_options.test.js

waitingsong force-pushed the https branch from 453bc61 to 5944a60 Compare September 17, 2018 07:43

whxaxes reviewed Nov 14, 2018

View reviewed changes

README.md Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read https config from app:config #75

feat: read https config from app:config #75

waitingsong commented Aug 29, 2018

waitingsong commented Aug 29, 2018 •

edited

codecov bot commented Aug 29, 2018 •

edited

atian25 commented Aug 29, 2018 •

edited

waitingsong commented Aug 29, 2018

atian25 commented Aug 29, 2018 •

edited

waitingsong commented Aug 30, 2018

waitingsong commented Aug 30, 2018

waitingsong commented Aug 30, 2018 •

edited

atian25 Sep 7, 2018

waitingsong Sep 7, 2018

atian25 Sep 7, 2018

atian25 Sep 7, 2018 •

edited

atian25 Sep 7, 2018

waitingsong Sep 7, 2018

atian25 Sep 7, 2018

waitingsong Sep 10, 2018 •

edited

atian25 commented Sep 7, 2018

This comment has been minimized.

atian25 commented Sep 7, 2018

waitingsong commented Sep 10, 2018 •

edited

waitingsong commented Sep 11, 2018 •

edited

waitingsong commented Nov 13, 2018

atian25 commented Nov 14, 2018

catherinessssss commented Jan 28, 2019 •

edited

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019 •

edited

waitingsong commented Jan 29, 2019

thonatos commented May 16, 2019

thonatos commented May 16, 2019

feat: read https config from app:config #75

Are you sure you want to change the base?

feat: read https config from app:config #75

Conversation

waitingsong commented Aug 29, 2018

Checklist

Affected core subsystem(s)

Description of change

waitingsong commented Aug 29, 2018 • edited

codecov bot commented Aug 29, 2018 • edited

Codecov Report

atian25 commented Aug 29, 2018 • edited

waitingsong commented Aug 29, 2018

atian25 commented Aug 29, 2018 • edited

waitingsong commented Aug 30, 2018

waitingsong commented Aug 30, 2018

waitingsong commented Aug 30, 2018 • edited

atian25 Sep 7, 2018

Choose a reason for hiding this comment

waitingsong Sep 7, 2018

Choose a reason for hiding this comment

atian25 Sep 7, 2018

Choose a reason for hiding this comment

atian25 Sep 7, 2018 • edited

Choose a reason for hiding this comment

atian25 Sep 7, 2018

Choose a reason for hiding this comment

waitingsong Sep 7, 2018

Choose a reason for hiding this comment

atian25 Sep 7, 2018

Choose a reason for hiding this comment

waitingsong Sep 10, 2018 • edited

Choose a reason for hiding this comment

atian25 commented Sep 7, 2018

This comment has been minimized.

atian25 commented Sep 7, 2018

waitingsong commented Sep 10, 2018 • edited

waitingsong commented Sep 11, 2018 • edited

waitingsong commented Nov 13, 2018

atian25 commented Nov 14, 2018

catherinessssss commented Jan 28, 2019 • edited

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019

waitingsong commented Jan 28, 2019

catherinessssss commented Jan 28, 2019 • edited

waitingsong commented Jan 29, 2019

thonatos commented May 16, 2019

thonatos commented May 16, 2019

waitingsong commented Aug 29, 2018 •

edited

codecov bot commented Aug 29, 2018 •

edited

atian25 commented Aug 29, 2018 •

edited

atian25 commented Aug 29, 2018 •

edited

waitingsong commented Aug 30, 2018 •

edited

atian25 Sep 7, 2018 •

edited

waitingsong Sep 10, 2018 •

edited

waitingsong commented Sep 10, 2018 •

edited

waitingsong commented Sep 11, 2018 •

edited

catherinessssss commented Jan 28, 2019 •

edited

catherinessssss commented Jan 28, 2019 •

edited