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

feat: read https config from app:config #75

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

waitingsong
Copy link
Contributor

@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
Copy link
Contributor Author

waitingsong commented Aug 29, 2018

问题在这儿 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
Copy link

codecov bot commented Aug 29, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

atian25 commented Aug 29, 2018

你的 master 太旧了,要 rebase

@waitingsong
Copy link
Contributor Author

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

@atian25
Copy link
Member

atian25 commented Aug 29, 2018

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

@waitingsong
Copy link
Contributor Author

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

@waitingsong
Copy link
Contributor Author

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

@waitingsong
Copy link
Contributor Author

waitingsong commented Aug 30, 2018

@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吧,调试起来效率真不高,一个参数不知道在哪儿会被修改……

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

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

@atian25 atian25 Sep 7, 2018

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

readFile 你指的是异步读取证书

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

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

都行,我之前的想法是 assign

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

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

Copy link
Contributor Author

@waitingsong waitingsong Sep 10, 2018

Choose a reason for hiding this comment

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

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

@atian25
Copy link
Member

atian25 commented Sep 7, 2018

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

master 不需要获取

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

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

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

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

@atian25

This comment has been minimized.

@atian25
Copy link
Member

atian25 commented Sep 7, 2018

protocol 那个,@popomore 有什么建议?

@waitingsong
Copy link
Contributor Author

waitingsong commented Sep 10, 2018

  1. 删除了 options.key|cert 参数支持。仅支持 https.key|cert 参数
  2. 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: []
  1. 使用配置参数(非命令行)传入 https 参数,启动系统后日志显示为不正确的 protocol: http://

@waitingsong
Copy link
Contributor Author

waitingsong commented Sep 11, 2018

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

@waitingsong
Copy link
Contributor Author

没人看看?

@atian25
Copy link
Member

atian25 commented Nov 14, 2018

@dead-horse @popomore 瞅瞅?

README.md Show resolved Hide resolved
@catherinessssss
Copy link

catherinessssss commented Jan 28, 2019

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

@waitingsong
Copy link
Contributor Author

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

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

@catherinessssss
Copy link

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

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

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

@waitingsong
Copy link
Contributor Author

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

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

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

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

@catherinessssss
Copy link

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

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

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

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

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

@waitingsong
Copy link
Contributor Author

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

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

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

@catherinessssss
Copy link

catherinessssss commented Jan 28, 2019

从 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
Copy link
Contributor Author

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

@thonatos
Copy link
Member

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
Copy link
Member

这样实现貌似也可以?

// 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');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants