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

properly generate extravolumes in kubeadmconfig for centos #6708

Conversation

lukasz-bielinski
Copy link
Contributor

kind bug

What this PR does / why we need it:
This is fix, for proper extravolumes generation for centos/rh in kubeadm config file.
Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:
NONE

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 17, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @lukasz-bielinski. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 17, 2020
@floryut
Copy link
Member

floryut commented Sep 17, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 17, 2020
@lukasz-bielinski
Copy link
Contributor Author

/retest

@EppO
Copy link
Contributor

EppO commented Sep 18, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Sep 18, 2020

AFAIK the >- to |- changes from strip to keep newlines in the multi line block. How is the newline situation important in JSON?

I've used: https://ansible.sivel.net/test/ to test it, i get ['/etc/pki/tls', '/etc/pki/ca-trust'] for ansible_os_family: RedHat and ['/usr/share/ca-certificates'] for ansible_os_family: Debian.

What is the problem?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2020
@lukasz-bielinski
Copy link
Contributor Author

@Miouge1 without change introduced in this pr loop in line 250

{% for dir in ssl_ca_dirs %}
  - name: {{ dir | regex_replace('^/(.*)$', '\\1' ) | regex_replace('/', '-') }}
    hostPath: {{ dir }}
    mountPath: {{ dir }}
    readOnly: true

in file

roles/kubernetes/master/templates/kubeadm-config.v1beta2.yaml.j2

gives invalid output. when centos is used var ssl_ca_dirs is treated as one line. which gives us
in /etc/kubernetes/kubeadm-config.yaml

  - name: ('-etc-pki-tls', '-etc-pki-ca-trust')
    hostPath: ('/etc/pki/tls', '/etc/pki/ca-trust')
    mountPath: ('/etc/pki/tls', '/etc/pki/ca-trust')

and in /etc/kubernetes/manifests/kube-apiserver.yaml

  volumes:
  - hostPath:
      path: ('/etc/pki/tls', '/etc/pki/ca-trust')
      type: ""
    name: ('-etc-pki-tls', '-etc-pki-ca-trust')

entries mentioned above are not valid.

introduced change fixes it. and we have proper files generated. proper entries below:
/etc/kubernetes/kubeadm-config.yaml

  - name: etc-pki-tls
    hostPath: /etc/pki/tls
    mountPath: /etc/pki/tls
    readOnly: true
  - name: etc-pki-ca-trust
    hostPath: /etc/pki/ca-trust
    mountPath: /etc/pki/ca-trust
    readOnly: true

/etc/kubernetes/manifests/kube-apiserver.yaml

  - hostPath:
      path: /etc/pki/ca-trust
      type: ""
    name: etc-pki-ca-trust
  - hostPath:
      path: /etc/pki/tls
      type: ""
    name: etc-pki-tls

@EppO
Copy link
Contributor

EppO commented Sep 18, 2020

Don't merge your branch with master, rebase it on top of it:

# run the following commands in your branch
git reset --hard origin/master
git cherry-pick d8179a2521733ace4020ba99fd376ec3fbffc9d4
git push --force

@lukasz-bielinski lukasz-bielinski force-pushed the properly-generate-extravolumes-in-kubeadmconfig-for-centos-and-rh branch from ad342df to e8ccf73 Compare September 18, 2020 18:06
@lukasz-bielinski
Copy link
Contributor Author

Don't merge your branch with master, rebase it on top of it:

# run the following commands in your branch
git reset --hard origin/master
git cherry-pick d8179a2521733ace4020ba99fd376ec3fbffc9d4
git push --force

@EppO done

@EppO
Copy link
Contributor

EppO commented Sep 18, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Sep 23, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lukasz-bielinski, Miouge1

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5f03433 into kubernetes-sigs:master Sep 23, 2020
erulabs added a commit to kubesail/kubespray that referenced this pull request Sep 23, 2020
* 'master' of https://github.com/kubernetes-sigs/kubespray:
  remove variable 'etcd_ionice', because ionice removed from container image etcd:v3.4.x (kubernetes-sigs#6735)
  calico: default to using kdd datastore (kubernetes-sigs#6693)
  Update docker packages to 19.03.13 + add docker f32 (kubernetes-sigs#6712)
  Fix snapshot.storage apiVersion (kubernetes-sigs#6711)
  properly generate extravolumes in kubeadmconfig for centos (kubernetes-sigs#6708)
  Fix reserved memory unit in kubelet configuration (kubernetes-sigs#6725)
  Fix unintended SIGPIPE (kubernetes-sigs#6721)
  Expose offline install overrides in inventory (kubernetes-sigs#6728)
  Added ability to set calico vxlan vni and port. defaults to calico's … (kubernetes-sigs#6678)
  Change health check from TCP to HTTPS (kubernetes-sigs#6487)
  Add multi architeture support to flannel (kubernetes-sigs#6166)
  Remove pypi repo and pip extra flags (kubernetes-sigs#6729)
  Fails if kubeadm_version do not matches kubernetes version (kubernetes-sigs#6302)
  Add external_openstack_lbaas_provider setting for occm (kubernetes-sigs#6566)
  add new variable allowing additionnal audit webhook server options (kubernetes-sigs#6726)
  Fix example value for etcd_quota_backend_bytes (kubernetes-sigs#6724)
  Added support for setting tiller_service_account and tiller_replicas (kubernetes-sigs#6696)
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants