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

增加未启用密钥的预校验能力,方便启用密钥前观察客户端的行为是否服务预期 #5216

Closed
larry4xie opened this issue Aug 29, 2024 · 7 comments · Fixed by #5236

Comments

@larry4xie
Copy link
Contributor

larry4xie commented Aug 29, 2024

你的特性请求和某个问题有关吗?请描述

  1. 背景:使用 apollo 前期没有开启密钥功能,现在想要开启密钥功能,目前的规则是 “一旦有启用的访问密钥,客户端将被要求配置密钥,否则无法获取配置
  2. 遇到的问题:客户端经过改造完成后,正式启用密钥前,没有简单直接的方式判断所有的客户端的行为是否符合预期
  3. 特性诉求:因此希望 apollo 提供一种对未启用密钥的预校验能力,不阻塞请求,但是对于未正确携带密钥的客户端访问进行日志记录,方便做正式启用密钥前的 check

清晰简洁地描述一下你希望的解决方案

  1. 增加开关配置标识是否开启密钥的预校验能力,默认不开启(兼容性
  2. 密钥的预校验能力大概逻辑如下
    1. 触发条件:开关开启 + 有密钥配置 + 所有密钥均未开启
    2. 触发后行为:打印日志,日志至少包含 appId, clientIp, authorization 等信息

BTW:如果认为这是一个合理的诉求,这边可以提一个 PR 进行实现

@nobodyiam
Copy link
Member

需求是合理的,不过实现方式可以讨论下,例如除了打印日志外,是否需要有其它方式感知预校验失败了

@larry4xie
Copy link
Contributor Author

larry4xie commented Aug 30, 2024

需求是合理的,不过实现方式可以讨论下,例如除了打印日志外,是否需要有其它方式感知预校验失败了

  • 触发后行为除了打印日志外,还可以增加基于现有的 Tracer#logEvent 进行感知,虽然我们现在没有开启这个功能,不过如果后续 apollo 提供 Prometheus 实现的时候可以用上
  • 我这边目前想到的另外两个可以讨论的实现细节:一个是开关的命名(例如 apollo.access-key.auth.precheck-enabled)、一个是触发条件(上面已经表达)
  • 不知道你这边还有其他需要讨论的地方不?

@nobodyiam
Copy link
Member

nobodyiam commented Aug 31, 2024

我这边目前想到的另外两个可以讨论的实现细节:一个是开关的命名(例如 apollo.access-key.auth.precheck-enabled)、一个是触发条件(上面已经表达)

我觉得不用增加开关,在密钥上增加一个状态即可,例如:观察模式,每个密钥都可以单独配置是否启用观察模式,默认值都是未启用观察模式。
当密钥校验失败时,判断是否是观察模式,如果是则打印日志并放行,如果不是,则拦截请求。

@larry4xie
Copy link
Contributor Author

我觉得不用增加开关,在密钥上增加一个状态即可,例如:观察模式,每个密钥都可以单独配置是否启用观察模式,默认值都是未启用观察模式。 当密钥校验失败时,判断是否是观察模式,如果是则打印日志并放行,如果不是,则拦截请求。

这种方式之前也简单想过,增加一个观察模式状态,在一些场景下会产生一定的歧义

  • 例如:同时存在 启用观察 两个状态,这个时候如果没有匹配启用的密钥就会走拒绝访问,从而走不到观察模式
  • 可选解决方案:可以通过密钥管理模块以及 UI 配合,不允许同时存在 启用观察 状态,不过这样复杂度会稍微提升

这两种方式我都可以接受, @nobodyiam 要不你来做个决策呗(或者有其他思路)

@nobodyiam
Copy link
Member

这确实是个问题,不过我在想实际场景中是否会存在既有启用(拦截模式)又有观察模式的场景?
之前设计多秘钥主要是针对秘钥轮转的场景,一般情况下只会存在一个有效的秘钥。
考虑到第二个方案对用户配置的约束比较多,目前更倾向于第一个方案吧。

具体实现上,可以在 UI 上做一些扩展,例如目前是启用和禁用,可以把启用再进一步细化为

  • 启用(拦截模式)
  • 启用(观察模式)

表字段中目前是 IsEnabled,可以新增一个字段例如 Mode 并设置默认值 0,那么

  • IsEnabled = 0 的时候就是禁用
  • IsEnabled = 1 and Mode = 0 的时候就是拦截模式,保持现有逻辑
  • IsEnabled = 1 and Mode = 1 的时候就是观察模式,只打日志不拦截

@larry4xie
Copy link
Contributor Author

larry4xie commented Sep 3, 2024

不过我在想实际场景中是否会存在既有启用(拦截模式)又有观察模式的场景?

实际场景中,我理解是不存在“既有启用(拦截模式)又有观察模式的”,观察模式只适用于正式启用之前的观察阶段

最后再确认一下最终实现方案:

  1. 增加 Mode 字段配合现有 IsEnabled 字段,密钥一共会有三种状态
    1. 禁用:IsEnabled = 0 + Mode=0
    2. 启用:IsEnabled = 1 + Mode=0
    3. 观察:IsEnabled = 1 + Mode=1
  2. 暂时不做用户配置和 UI 上的约束,只通过页面提示告知规则:如果当前环境存在启用状态的密钥,密钥的观察状态将不生效
  3. 观察状态只打日志不拦截,输出 LoggerTracer#logEvent 两类日志

@nobodyiam 如果没有问题,我就最近找时间实现一下?

@nobodyiam
Copy link
Member

禁用:IsEnabled = 0 + Mode=0

禁用应该不用看 Mode 字段的值,其它我觉得 OK

larry4xie added a commit to larry4xie/apollo that referenced this issue Sep 22, 2024
…polloconfig#5216)

- ALTER TABLE `AccessKey` ADD COLUMN `Mode`, 0: filter,1: observer
- portal: CRUD for observe status access-key
- configservice: pre-check and logging via ClientAuthenticationFilter
nobodyiam added a commit that referenced this issue Oct 8, 2024
…5216) (#5236)

* feat: add observe status access-key for pre-check and logging only (#5216)

- ALTER TABLE `AccessKey` ADD COLUMN `Mode`, 0: filter,1: observer
- portal: CRUD for observe status access-key
- configservice: pre-check and logging via ClientAuthenticationFilter

* changelog: add observe status access-key for pre-check and logging only (#5216)

* refactor: #5236

* refactor: #5236 test code

* refactor: #5236 Update apolloconfigdb.sql

Co-authored-by: Jason Song <nobodyiam@gmail.com>

* changelog & refactor: #5236

---------

Co-authored-by: Jason Song <nobodyiam@gmail.com>
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 a pull request may close this issue.

2 participants