Fix: fail to install Kruise rollout in customized namespace

StevenLeiZhang commented 1 year ago

Signed-off-by: StevenLeiZhang zhangleiic@163.com

Fix: #36

Investigation: The namespace could be specified by user, when he run helm install with --set installation.namespace=myroll

but serviceaccount and webhook's names are binded with NS. That makes pod run into error state

How to fix it? Just set there names with "rollout.name", which is defined in helm template file.

How to test it: run

 helm install  krollout ./kruise-rollout --set installation.namespace=myroll --set image.pullPolicy="IfNotPresent" --debug

to install this Chart, and all Pod can be up successfully.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

[ ] I have bumped the chart version according to versioning
[ ] I have updated the chart changelog with all the changes that come with this pull request according to changelog.
[ ] Any new values are backwards compatible and/or have sensible default.
[ ] I have signed off all my commits as required by DCO.

Changes are automatically published when merged to master. They are not published on branches.

kruise-bot commented 1 year ago

Welcome @StevenLeiZhang! It looks like this is your first PR to openkruise/charts 🎉

zmberg commented 1 year ago

@StevenLeiZhang tks, I think you can remove the values.yaml rollout.fullname parameter, and use explicit string for name. for example:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: kruise-rollout-controller-manager
  namespace: {{ .Values.installation.namespace }}

StevenLeiZhang commented 1 year ago

@StevenLeiZhang tks, I think you can remove the values.yaml rollout.fullname parameter, and use explicit string for name. for example:
apiVersion: v1
kind: ServiceAccount
metadata:
  name: kruise-rollout-controller-manager
  namespace: {{ .Values.installation.namespace }}

No，It does not work。 Please refer to

https://github.com/openkruise/rollouts/blob/08dd7878ff4786f9f500f6ba3d216fc8ff6cdbc7/pkg/webhook/util/controller/webhook_controller.go#L50

    mutatingWebhookConfigurationName   = "kruise-rollout-mutating-webhook-configuration"
    validatingWebhookConfigurationName = "kruise-rollout-validating-webhook-configuration"

but in. https://github.com/openkruise/charts/blob/201a65e191d2b50e2d281d0aadc46e73a22d87da/versions/kruise-rollout/0.3.0-rc/templates/webhookconfiguration.yaml#L6

It is named by NamaSpace. That cause mismatch and Controller pod can not be up.

zmberg commented 1 year ago

@StevenLeiZhang tks, I think you can remove the values.yaml rollout.fullname parameter, and use explicit string for name. for example:
apiVersion: v1
kind: ServiceAccount
metadata:
  name: kruise-rollout-controller-manager
  namespace: {{ .Values.installation.namespace }}
No，It does not work。 Please refer to

https://github.com/openkruise/rollouts/blob/08dd7878ff4786f9f500f6ba3d216fc8ff6cdbc7/pkg/webhook/util/controller/webhook_controller.go#L50
  mutatingWebhookConfigurationName   = "kruise-rollout-mutating-webhook-configuration"
  validatingWebhookConfigurationName = "kruise-rollout-validating-webhook-configuration"
but in.

https://github.com/openkruise/charts/blob/201a65e191d2b50e2d281d0aadc46e73a22d87da/versions/kruise-rollout/0.3.0-rc/templates/webhookconfiguration.yaml#L6

It is named by NamaSpace. That cause mismatch and Controller pod can not be up.

@StevenLeiZhang You are right, so all name with format {{ .Values.installation.namespace }}-xxxx must be convert to kruise-rollout-xxxx.

for exmaple: change {{ .Values.installation.namespace }}-mutating-webhook-configuration to kruise-rollout-mutating-webhook-configuration.

StevenLeiZhang commented 1 year ago

@StevenLeiZhang tks, I think you can remove the values.yaml rollout.fullname parameter, and use explicit string for name. for example:
apiVersion: v1
kind: ServiceAccount
metadata:
  name: kruise-rollout-controller-manager
  namespace: {{ .Values.installation.namespace }}
No，It does not work。 Please refer to https://github.com/openkruise/rollouts/blob/08dd7878ff4786f9f500f6ba3d216fc8ff6cdbc7/pkg/webhook/util/controller/webhook_controller.go#L50
    mutatingWebhookConfigurationName   = "kruise-rollout-mutating-webhook-configuration"
    validatingWebhookConfigurationName = "kruise-rollout-validating-webhook-configuration"
but in. https://github.com/openkruise/charts/blob/201a65e191d2b50e2d281d0aadc46e73a22d87da/versions/kruise-rollout/0.3.0-rc/templates/webhookconfiguration.yaml#L6

It is named by NamaSpace. That cause mismatch and Controller pod can not be up.
@StevenLeiZhang You are right, so all name with format {{ .Values.installation.namespace }}-xxxx must be convert to kruise-rollout-xxxx.

for exmaple: change {{ .Values.installation.namespace }}-mutating-webhook-configuration to kruise-rollout-mutating-webhook-configuration.

Yes, so my fix is

{{ template "rollout.name" . }}-mutating-webhook-configuration

because {{ template "rollout.name" . }} is exactly "kruise-rollout", which is defined in Chart template. In case user wants to customize it, he just need change name in Chart template.

zmberg commented 1 year ago

In case user wants to customize it, he just need change name in Chart template.

These places feel that they should not be modified, as if the user should not have a modified scene. In addition, I have verified your pr locally, but it cannot work, for example:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: {{ .Values.installation.namespace }}-manager-rolebinding
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: {{ .Values.installation.namespace }}-manager-role
subjects:
  - kind: ServiceAccount
    name: {{ .Values.installation.namespace }}-controller-manager
    namespace: {{ .Values.installation.namespace }}

StevenLeiZhang commented 1 year ago

In case user wants to customize it, he just need change name in Chart template.

These places feel that they should not be modified, as if the user should not have a modified scene. In addition, I have verified your pr locally, but it cannot work, for example:
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: {{ .Values.installation.namespace }}-manager-rolebinding
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: {{ .Values.installation.namespace }}-manager-role
subjects:
  - kind: ServiceAccount
    name: {{ .Values.installation.namespace }}-controller-manager
    namespace: {{ .Values.installation.namespace }}

updated my PR, and followed https://github.com/openkruise/rollouts/blob/master/docs/tutorials/basic_usage.md to test it.

zmberg commented 1 year ago

/lgtm

zmberg commented 1 year ago

/approve

kruise-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openkruise/charts/blob/master/OWNERS)~~ [zmberg] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

openkruise / charts

Fix: fail to install Kruise rollout in customized namespace #37