Support helm charts for network resource injector #14
Conversation
VivekThrivikraman-est
force-pushed
the
nri
branch
from
307fab6 to
c094794
Compare
| @@ -0,0 +1,163 @@ | |||
|
|
|||
There was a problem hiding this comment.
nit: please remove empty line here
| command: | ||
| - webhook | ||
| args: | ||
| - -bind-address=0.0.0.0 |
There was a problem hiding this comment.
IMO, it's not secure to bind to all IP addresses. Can we bind to localhost be default?. We need to make it configurable
| - name: tls | ||
| emptyDir: {} | ||
|
|
||
| # For third-party certificate, use secret resource |
There was a problem hiding this comment.
We should make it configurable via values or add TODO note here to make it configurable in the future. There is no option to change this template if chart will be used from the repository
There was a problem hiding this comment.
Used condition for taking appropriate config based on type of certificate.
| @@ -0,0 +1,26 @@ | |||
|
|
|||
There was a problem hiding this comment.
nit: please remove empty line here
| - webhook | ||
| args: | ||
| - -bind-address=0.0.0.0 | ||
| - -port=8443 |
There was a problem hiding this comment.
should we use port number from the values?
| port: 443 | ||
| targetPort: 8443 | ||
|
|
||
| webhookconf: |
There was a problem hiding this comment.
nit: webhookconf => webhook?
There was a problem hiding this comment.
There is webhook as key in child hierarchy also, so this was to distinguish the webhook config(kind) to the actual webhook(one in webhooks in the webhookconfig). I can change it if you feel this is better.
| apiVersion: policy/v1 | ||
| kind: PodDisruptionBudget | ||
| metadata: | ||
| name: {{ .Values.pdb.name }} |
There was a problem hiding this comment.
Do we really need this configurable?
There was a problem hiding this comment.
The other names are using templates(even in other non nri templates), should we only omit for this? I feel its ok to hardcode, but I was following the other templates already available. Or should we have it as " {{.Chart.Name}}+ "-pdb" " ?
VivekThrivikraman-est
force-pushed
the
nri
branch
from
c094794 to
1e5dfba
Compare
|
@e0ne can this review be merged? |
|
@rthakur-est merged |
Currently network resource injector does not support helm charts. This commit implements helm charts for nri.
Fixes: k8snetworkplumbingwg/network-resources-injector#118