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: support nacos storage #796

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

sjcsjc123
Copy link
Collaborator

Ⅰ. 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

Signed-off-by: sjcsjc123 <[email protected]>
Makefile.core.mk Outdated Show resolved Hide resolved
docker/Dockerfile.higress Outdated Show resolved Hide resolved
helm/core/templates/controller-service.yaml Outdated Show resolved Hide resolved
## Higress ApiServer Storage Settings
## Todo simplify this
apiserver:
addr: 127.0.0.1
Copy link
Collaborator

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?

Copy link
Collaborator Author

@sjcsjc123 sjcsjc123 Jan 22, 2024

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.

Copy link
Collaborator Author

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 Show resolved Hide resolved
imagePullPolicy: IfNotPresent
securePort: 8443
storage: nacos
serverAddr: http://127.0.0.1:8848
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个默认值如果用户不修改的话,安装肯定会失败吧?具体表现是什么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

新增helm参数来控制是否替用户安装nacos,这个地址就修改为替用户安装nacos的时候的nacos地址,这样可以吗

fi
}

initializeApiServer
Copy link
Collaborator

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?

Copy link
Collaborator Author

@sjcsjc123 sjcsjc123 Jan 22, 2024

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.

Copy link
Collaborator

@2456868764 2456868764 Jan 23, 2024

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 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.11%. Comparing base (b4f72d3) to head (b306f61).

Additional details and impacted files

Impacted file tree graph

@@            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     

see 1 file with indirect coverage changes

@@ -110,7 +110,7 @@ data:
apiVersion: v1
kind: Config
clusters:
- name: {{ .Release.Namespace }}
- name: {{ .Values.clusterName | default "higress" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这句的目的是啥,为什么不能用 Namespace 呢?

Copy link
Collaborator Author

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

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

Choose a reason for hiding this comment

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

这里可以考虑用官方镜像吗?

Copy link
Collaborator Author

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

Copy link
Collaborator

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 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Makefile.core.mk Outdated Show resolved Hide resolved
tools/hack/gen-secret-configmap.sh Outdated Show resolved Hide resolved
tools/hack/gen-secret-configmap.sh Show resolved Hide resolved
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
- configMap:
name: higress-config
- configMap:
name: higress-apiserver
Copy link
Collaborator

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 的名字的,那这里挂载用的名字需要支持自定义吗?

Copy link
Collaborator Author

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 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
docker/run-higress.sh Outdated Show resolved Hide resolved
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
@sjcsjc123
Copy link
Collaborator Author

2024-01-24T09:33:53.978099Z	error	ingress	Fallback service higress-conformance-infra/infra-backend-v1 within ingress higress-conformance-infra/higress-conformance-infra-default-backend is not found
2024-01-24T09:33:53.978130Z	info	ingress	resource type networking.istio.io/v1alpha3/DestinationRule, configs number 1
2024-01-24T09:33:53.978147Z	info	ingress	controller tracing ConstructEnvoyFilters
2024-01-24T09:33:53.978155Z	info	ingress	controller gzip ConstructEnvoyFilters
2024-01-24T09:33:53.978163Z	info	ingress	controller global-option ConstructEnvoyFilters
2024-01-24T09:33:53.978537Z	info	ingress	Append 1 configmap EnvoyFilters
2024-01-24T09:33:53.978553Z	info	ingress	resource type networking.istio.io/v1alpha3/EnvoyFilter, configs number 1
2024-01-24T09:33:53.978637Z	error	ingress	Fallback service higress-conformance-infra/infra-backend-v1 within ingress higress-conformance-infra/higress-conformance-infra-default-backend is not found

看起来是base目录下的service部分也需要通过nacos发布一下

Makefile.core.mk Outdated Show resolved Hide resolved
- --{{ .Values.apiserver.storage }}-ns-id
- {{ .Values.apiserver.namespaceID }}
- --{{ .Values.apiserver.storage }}-encryption-key-file
- /etc/api/nacos.key
Copy link
Collaborator

Choose a reason for hiding this comment

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

storage 如果不是 nacos 的话,是没有以上这些参数的。这里是否要考虑开闭原则?

Copy link
Collaborator Author

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

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。同样建议考虑一下开闭原则。

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

Choose a reason for hiding this comment

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

我的意思是,是否可以可以用 nacos 发布的官方镜像

imagePullPolicy: IfNotPresent
securePort: 8443
storage: nacos
serverAddr: http://127.0.0.1:8848
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个默认值如果用户不修改的话,安装肯定会失败吧?具体表现是什么?

@@ -47,3 +48,5 @@ spec:
ports:
- name: foo # name is not required for single-port Services
Copy link
Collaborator

Choose a reason for hiding this comment

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

现在已经不是单端口服务了,name 还是好好取一个吧

test/e2e/conformance/utils/kubernetes/apply.go Outdated Show resolved Hide resolved
test/e2e/conformance/utils/suite/suite.go Outdated Show resolved Hide resolved
name: nacos
clusterIP: None
ports:
- name: foo # name is not required for single-port Services
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上

@johnlanni
Copy link
Collaborator

@sjcsjc123 docker.io/swsk33/nacos-standalone:2.2.3 这个镜像变成私有的了,看是否换成nacos官方镜像,参考下:https://github.com/nacos-group/nacos-k8s/tree/master/helm

这个默认也是standalone模式

@sjcsjc123
Copy link
Collaborator Author

@sjcsjc123 docker.io/swsk33/nacos-standalone:2.2.3 这个镜像变成私有的了,看是否换成nacos官方镜像,参考下:https://github.com/nacos-group/nacos-k8s/tree/master/helm

这个默认也是standalone模式

目前github ci上的磁盘空间好像不够用了,我把e2e测试用的base/nacos.yaml也给换成了官方镜像,以节省磁盘开销

image

@johnlanni
Copy link
Collaborator

- name: Free Up GitHub Actions Ubuntu Runner Disk Space 🔧
uses: jlumbroso/free-disk-space@main
with:
tool-cache: false
android: true
dotnet: true
haskell: true
large-packages: true
swap-storage: true

@sjcsjc123 可以配置这个action释放一些磁盘空间

Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
protocol: TCP
{{- if eq .Values.nacos.service.type "NodePort" }}
nodePort: {{ .Values.nacos.service.nodePort }}
{{- end }}
Copy link
Collaborator

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

Choose a reason for hiding this comment

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

默认需要开NodePort吗?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不需要的,也可以改成clusterip,我给改一下

test/e2e/conformance/base/nacos.yaml Show resolved Hide resolved
imagePullPolicy: IfNotPresent
securePort: 8443
storage: nacos
serverAddr: http://higress-nacos-service.higress-system.svc.cluster.local:8848
Copy link
Collaborator

@johnlanni johnlanni Mar 26, 2024

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 一起看看

Copy link
Collaborator

Choose a reason for hiding this comment

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

我觉得可以的。之前也考虑过能不能让 nacos 直接支持这种配置方式。短时间可以在生成配置的时候处理一下。

Copy link
Collaborator Author

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://

Copy link
Collaborator

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 也不需要单独配置了。

@johnlanni
Copy link
Collaborator

@sjcsjc123 这个PR还有空继续完善下吗

@johnlanni johnlanni marked this pull request as draft April 23, 2024 08:16
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.

Support the use of nacos as storage when deploying with k8s
5 participants