-
Notifications
You must be signed in to change notification settings - Fork 340
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
chore: support to deploy ingress-apisix through helm charts #153
Conversation
Codecov Report
@@ Coverage Diff @@
## master #153 +/- ##
===========================================
+ Coverage 36.27% 50.35% +14.07%
===========================================
Files 28 30 +2
Lines 1257 1414 +157
===========================================
+ Hits 456 712 +256
+ Misses 771 618 -153
- Partials 30 84 +54
Continue to review full report at Codecov.
|
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.
LGTM
charts/base/templates/rbac.yaml
Outdated
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: {{ .Values.rbac.serviceAccount }} |
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 name could be fixed format, such as {{ .Release.Name }}
or {{ .Release.Name }}-svcacc
, it will make values.yaml more simple.
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.
OK, will change it.
rules: | ||
- apiGroups: | ||
- "" | ||
resources: |
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.
Are we really need to watch so many resource?
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.
Here are the resources we may deal with at the moment.
pods/endpoints/services/replicationcontrollers/replicationcontrollers/scale
is needed by now.
configmaps/persistentvolumeclaims
can be used to do some enhancements in future.
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.
Although I can't imagine what a controller watch so many resources can do, but if you are sure they are needed, so be it
charts/base/templates/rbac.yaml
Outdated
metadata: | ||
name: {{ .Values.rbac.serviceAccount }} | ||
labels: | ||
{{- include "base.labels" . | nindent 4 }} |
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.
Do we really need user to control the labels and annotation? This will complicate values.yaml
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.
labels are not necessary here, we can remove it.
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.
Removed.
volumeMounts: | ||
- mountPath: /ingress-apisix/conf | ||
name: configuration | ||
{{- with .Values.ingressController.nodeSelector }} |
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.
nodeSelector
and affinity
is similar, just one of them is enough
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.
Changed.
imagePullPolicy: {{ .Values.ingressController.image.pullPolicy }} | ||
ports: | ||
- name: http | ||
containerPort: {{ (.Values.ingressController.config.httpListen | split ":")._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.
.Values.ingressController.config.httpListen
is :8080
, but liveness work at 80(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.
Where shows the 80
?
The containerPort is refined from the httpListen
.
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.
You use a IANA_SVC_NAME, http, to define livenessProbe's port, according here, the port is 80.
charts/ingress-apisix/values.yaml
Outdated
serviceAccount: "apisix-view-serviceaccount" | ||
|
||
service: | ||
type: ClusterIP |
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 option can not work well, if we selected NodePort
, we maybe need to specified node port, but here is no such parameter.
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.
OK, for now, the service of ingress controller doesn't have too much sense (only /healthz and Prometheus metrics), will hardcoded the service type to ClusterIP
type: ClusterIP | ||
ports: | ||
- port: {{ .Values.ingressController.service.port }} | ||
targetPort: 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.
Here also use the container's port name
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.
Fixed.
Closing #130.