fix envvars in args

[LuckySB]

fix env vars substitution error in hostnameOverride-patch.json

https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#use-environment-variables-to-define-arguments

I also want to note that in version 1.13.0, this patch is no longer necessary to apply.
https://github.com/kubernetes/kubernetes/pull/71283/files

#4036

[LuckySB]

ci check this

[LuckySB]

/assign @woopstar

[chadswen]

@LuckySB Do you have a fix for the other roles also? I haven't searched for the root cause elsewhere, but this PR looks like it's only applicable to windows nodes. We should be able to set hostnameOverride in the config used by kubeadm if that's not working properly yet.

[LuckySB]

/assign @chadswen

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LuckySB
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: chadswen

If they are not already assigned, you can assign the PR to them by writing /assign @chadswen in a comment when ready.

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[LuckySB]

@LuckySB Do you have a fix for the other roles also? I haven't searched for the root cause elsewhere, but this PR looks like it's only applicable to windows nodes. We should be able to set hostnameOverride in the config used by kubeadm if that's not working properly yet.

The win_nodes role adds restrictions to the daemonset kube_proxy, so that it runs only on linux-based nodes.
As far as I know, a kubespray does not know how to add nodes to the Windows.

I also want to note that in version 1.13.0, this patch is no longer necessary to apply.
https://github.com/kubernetes/kubernetes/pull/71283/files

[dannyk81]

@LuckySB just hit this when deployed v1.13.2 with Kubespray (from master branch).

kubeadm already sets --hostname-override=$(NODE_NAME) and the current patch add the incorrect one and causes the error/warning.

      spec:
        containers:
        - command:
          - /usr/local/bin/kube-proxy
          - --config=/var/lib/kube-proxy/config.conf
          - --hostname-override=$(NODE_NAME)
          - --hostname-override=${NODE_NAME}

@chadswen I'm a bit confused here as well, but this patch seems to be applied on linux nodes as well 🤷‍♂️ not sure why it's called win_nodes 😄

[dannyk81]

@LuckySB with this PR, I get the following result after deployment:

    Command:
      /usr/local/bin/kube-proxy
      --config=/var/lib/kube-proxy/config.conf
      --hostname-override=$(NODE_NAME)
      --hostname-override=$(NODE_NAME)

This is better, but there's still duplicate entries.

[LuckySB]

@LuckySB with this PR, I get the following result after deployment:

    Command:
      /usr/local/bin/kube-proxy
      --config=/var/lib/kube-proxy/config.conf
      --hostname-override=$(NODE_NAME)
      --hostname-override=$(NODE_NAME)

This is better, but there's still duplicate entries.

Thank you very much.
I added escaping special characters to regexp search
and now it correctly checks the arguments of kube-proxy and does not duplicate them.

[dannyk81]

@LuckySB with this PR, I get the following result after deployment:

    Command:
      /usr/local/bin/kube-proxy
      --config=/var/lib/kube-proxy/config.conf
      --hostname-override=$(NODE_NAME)
      --hostname-override=$(NODE_NAME)

This is better, but there's still duplicate entries.

Thank you very much.
I added escaping special characters to regexp search
and now it correctly checks the arguments of kube-proxy and does not duplicate them.

You're welcome @LuckySB

I was also thinking that considering that this patch is only needed for k8s version < 1.13.0, it may be easier to just apply a conditional on the whole task block, here:

kubespray/roles/win_nodes/kubernetes_patch/tasks/main.yml

Lines 34 to 35 in 0888f65

	when:
	- not kube_proxy_remove

by adding:

  - kube_version is version('v1.13.0', '<')

and remove the search conditional entirely.

[LuckySB]

I was also thinking that considering that this patch is only needed for k8s version < 1.13.0, it may be easier to just apply a conditional on the whole task block, here:

kubespray/roles/win_nodes/kubernetes_patch/tasks/main.yml

Lines 34 to 35 in 0888f65

when:

• not kube_proxy_remove
  by adding:

  - kube_version is version('v1.13.0', '<')

and remove the search conditional entirely.

Of course you are right. from version 1.13 this patch will not be needed

But if you add this condition and remove the condition with the search, then on the version less than 1.13 every time you start the kubespray, the line '--hostname-override' will be added

[dannyk81]

Ok, I see your point. Thanks @LuckySB

…

On Sat., Jan. 26, 2019, 12:47 Sergey ***@***.*** wrote: I was also thinking that considering that this patch is only needed for k8s version < 1.13.0, it may be easier to just apply a conditional on the whole task block, here: kubespray/roles/win_nodes/kubernetes_patch/tasks/main.yml <https://github.com/kubernetes-sigs/kubespray/blob/0888f65b24ec58bd84c5d6d5dd7d25aed0a7229e/roles/win_nodes/kubernetes_patch/tasks/main.yml#L34-L35> Lines 34 to 35 in 0888f65 <http:///kubernetes-sigs/kubespray/commit/0888f65b24ec58bd84c5d6d5dd7d25aed0a7229e> when: - not kube_proxy_remove by adding: - kube_version is version('v1.13.0', '<') and remove the search conditional entirely. Of course you are right. from version 1.13 this patch will not be needed But if you add this condition and remove the condition with the search, then on the version less than 1.13 every time you start the kubespray, the line '--hostname-override' will be added — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4037 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXJk7Or5w1-0V2phO79hhzHXmyR4Xhh_ks5vHJTAgaJpZM4Z855E> .

[dannyk81]

Hey @chadswen! following our slack chat about this, do you think we can get this merged?