-
Notifications
You must be signed in to change notification settings - Fork 454
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: support nacos storage #796
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
## Higress ApiServer Storage Settings | ||
## Todo simplify this | ||
apiserver: | ||
addr: 127.0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we allow user to use another address here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If run api server in other pod, we can modify this. If not, we can remove it. It can be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
按照目前的安装方式来看,是否需要移除这个配置,把apiserver固定为higress-controller的其中一个container
helm/core/values.yaml
Outdated
imagePullPolicy: IfNotPresent | ||
securePort: 8443 | ||
storage: nacos | ||
serverAddr: http://127.0.0.1:8848 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that we have a Nacos running in http://127.0.0.1:8848
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter is used to enable users to specify a nacos address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个默认值如果用户不修改的话,安装肯定会失败吧?具体表现是什么?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
新增helm参数来控制是否替用户安装nacos,这个地址就修改为替用户安装nacos的时候的nacos地址,这样可以吗
tools/hack/gen-keys.sh
Outdated
fi | ||
} | ||
|
||
initializeApiServer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When user installs Higress with API Server enabled in his own K8s cluster, how could API server get these keys and certificates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to incorporate this script into the helm installation process. Not successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about making a new keys folder under helm/core dir and copying the certificates and keys generated by gen-keys.sh to the dir, and using .Files.Get from keys dir? and so there is no need to generate certificates and keys during installation
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #796 +/- ##
==========================================
- Coverage 38.14% 38.11% -0.03%
==========================================
Files 61 61
Lines 10442 10442
==========================================
- Hits 3983 3980 -3
- Misses 6160 6162 +2
- Partials 299 300 +1 |
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
helm/core/templates/configmap.yaml
Outdated
@@ -110,7 +110,7 @@ data: | |||
apiVersion: v1 | |||
kind: Config | |||
clusters: | |||
- name: {{ .Release.Namespace }} | |||
- name: {{ .Values.clusterName | default "higress" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这句的目的是啥,为什么不能用 Namespace 呢?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里指的是k8s cluster的name,在make create-cluster中会创建名为higress的k8s集群,这里用higress-system会无法访问apiserver
test/e2e/conformance/base/nacos.yaml
Outdated
@@ -31,9 +31,10 @@ metadata: | |||
spec: | |||
containers: | |||
- name: nacos-standlone-rc3 | |||
image: registry.cn-hangzhou.aliyuncs.com/hinsteny/nacos-standlone-rc3:1.0.0-RC3 | |||
image: swsk33/nacos-standalone:2.2.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里可以考虑用官方镜像吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
官方镜像依赖于数据源,registry.cn-hangzhou.aliyuncs.com是否有2.x版本的nacos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我的意思是,是否可以可以用 nacos 发布的官方镜像
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
- configMap: | ||
name: higress-config | ||
- configMap: | ||
name: higress-apiserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
鉴于 gen-secret-configmap.sh
里是允许自定义这个 ConfigMap 的名字的,那这里挂载用的名字需要支持自定义吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gen-secret-configmap.sh
那边会改成固定的,同higress-config
一样
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
看起来是base目录下的service部分也需要通过nacos发布一下 |
Signed-off-by: sjcsjc123 <1401189096@qq.com>
- --{{ .Values.apiserver.storage }}-ns-id | ||
- {{ .Values.apiserver.namespaceID }} | ||
- --{{ .Values.apiserver.storage }}-encryption-key-file | ||
- /etc/api/nacos.key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storage 如果不是 nacos 的话,是没有以上这些参数的。这里是否要考虑开闭原则?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不太清楚开闭原则在这里的用法,有什么改进的建议吗
name: cert-config | ||
readOnly: true | ||
- mountPath: /tmp/nacos | ||
name: nacos-data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下面叫{{ .Values.apiserver.storage }}-data
,这里是nacos-data
。同样建议考虑一下开闭原则。
test/e2e/conformance/base/nacos.yaml
Outdated
@@ -31,9 +31,10 @@ metadata: | |||
spec: | |||
containers: | |||
- name: nacos-standlone-rc3 | |||
image: registry.cn-hangzhou.aliyuncs.com/hinsteny/nacos-standlone-rc3:1.0.0-RC3 | |||
image: swsk33/nacos-standalone:2.2.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我的意思是,是否可以可以用 nacos 发布的官方镜像
helm/core/values.yaml
Outdated
imagePullPolicy: IfNotPresent | ||
securePort: 8443 | ||
storage: nacos | ||
serverAddr: http://127.0.0.1:8848 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个默认值如果用户不修改的话,安装肯定会失败吧?具体表现是什么?
test/e2e/conformance/base/nacos.yaml
Outdated
@@ -47,3 +48,5 @@ spec: | |||
ports: | |||
- name: foo # name is not required for single-port Services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
现在已经不是单端口服务了,name 还是好好取一个吧
tools/hack/conf/nacos.yaml
Outdated
name: nacos | ||
clusterIP: None | ||
ports: | ||
- name: foo # name is not required for single-port Services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同上
@sjcsjc123 docker.io/swsk33/nacos-standalone:2.2.3 这个镜像变成私有的了,看是否换成nacos官方镜像,参考下:https://github.com/nacos-group/nacos-k8s/tree/master/helm 这个默认也是standalone模式 |
Signed-off-by: sjcsjc123 <1401189096@qq.com>
Signed-off-by: sjcsjc123 <1401189096@qq.com>
目前github ci上的磁盘空间好像不够用了,我把e2e测试用的base/nacos.yaml也给换成了官方镜像,以节省磁盘开销 |
higress/.github/workflows/build-and-test-plugin.yaml Lines 36 to 44 in 9c112a0
@sjcsjc123 可以配置这个action释放一些磁盘空间 |
protocol: TCP | ||
{{- if eq .Values.nacos.service.type "NodePort" }} | ||
nodePort: {{ .Values.nacos.service.nodePort }} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里格式处理一下吧。另外,什么情况下需要用到 NodePort 呢,还只有 7848?
service: | ||
port: 8848 | ||
type: NodePort | ||
nodePort: 30000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
默认需要开NodePort吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不需要的,也可以改成clusterip,我给改一下
imagePullPolicy: IfNotPresent | ||
securePort: 8443 | ||
storage: nacos | ||
serverAddr: http://higress-nacos-service.higress-system.svc.cluster.local:8848 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serverAddr 建议改名为 storageAddr,否则容易误解。
此外能否不要让用户配置 http://
这个协议头呢,在helpers里根据storage和storageAddr来生成最终的配置参数
@CH3CHO 一起看看
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我觉得可以的。之前也考虑过能不能让 nacos 直接支持这种配置方式。短时间可以在生成配置的时候处理一下。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
只让用户配置higress-nacos-service.higress-system.svc.cluster.local:8848
这个storageAddr参数吗,通过helper.tpl给处理一下http://
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
或者直接让用户配置 nacos://higress-nacos-service.higress-system.svc.cluster.local:8848
?这样 storage 也不需要单独配置了。
@sjcsjc123 这个PR还有空继续完善下吗 |
Ⅰ. Describe what this PR did
support nacos storage
Ⅱ. Does this pull request fix one issue?
fixes: #642
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews