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.

Sign up for GitHub

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

Jump to bottom

store openstack external cloud controller ca.cert in a k8s secret instead of the host filesystem #7603

Merged
merged 1 commit into from
May 26, 2021
Merged

store openstack external cloud controller ca.cert in a k8s secret instead of the host filesystem #7603

merged 1 commit into from
May 26, 2021

Conversation

cristicalin
Copy link
Contributor

@cristicalin cristicalin commented May 11, 2021 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR changes the way we store openstack CA certificate file and the external-openstack-cloud-config from distributing them on the host filesystem to storing them in the same secret we use to distribute the cloud.conf.

Which issue(s) this PR fixes:

Fixes #7553

Special notes for your reviewer:
This is an alternative solution to the one proposed in #7568

Does this PR introduce a user-facing change?:

Openstack cloud config: store cloud.conf and API CA cert in k8s secret and avoid writing them to disk

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. 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 May 11, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @cristicalin. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 11, 2021
@floryut floryut added this to the 2.16 milestone May 11, 2021
@cristicalin cristicalin mentioned this pull request May 12, 2021
Closed
@floryut
Copy link
Member

floryut commented May 21, 2021

/cc @bl0m1

@k8s-ci-robot
Copy link
Contributor

@floryut: GitHub didn't allow me to request PR reviews from the following users: bl0m1.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @bl0m1

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.

oomichi
oomichi reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@oomichi oomichi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @oomichi

roles/kubernetes-apps/external_cloud_controller/openstack/tasks/main.yml
- external_openstack_cacert is defined
- external_openstack_cacert | length > 0
tags: external-openstack

- name: External OpenStack Cloud Controller | Write External OpenStack cloud-config
template:
src: "external-openstack-cloud-config.j2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we remove this template file external-openstack-cloud-config.j2 with this pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this actually needs a bit of rethought now that you pointed it out. The external-openstack-cloud-config is rendered onto the first tube controller then it is slurp'ed up and put into the secret, so the answer to your question is no, we cannot remove that file, but I should have not deleted this task in the first place. I pushed a fix for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oomichi I modified the logic to avoid writing the external-openstack-cloud-config to any node and write it directly in the Kubernetes secret.

@k8s-ci-robot k8s-ci-robot requested a review from oomichi May 24, 2021 22:28
@cristicalin cristicalin force-pushed the store_openstack_cacert_in_secret branch from b80d5e3 to 7e7c301 Compare May 25, 2021 04:37
@cristicalin cristicalin force-pushed the store_openstack_cacert_in_secret branch from 7e7c301 to 0933868 Compare May 25, 2021 07:45
@cristicalin
Copy link
Contributor Author

I might be wrong but the molecule test failure seems somewhat unrelated.

@k8s-ci-robot
Copy link
Contributor

@cristicalin: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@alijahnas
Copy link
Contributor

/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 May 25, 2021
@alijahnas
Copy link
Contributor

/retest

@oomichi
Copy link
Contributor

oomichi commented May 25, 2021

Thanks for updating.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021
@floryut
Copy link
Member

floryut commented May 26, 2021 •
edited
Loading

Good for you @alijahnas ?

@alijahnas
Copy link
Contributor

@floryut yeah it /lgtm, I would /approve :)

floryut
floryut approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, floryut

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 May 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit c5ccedb into kubernetes-sigs:master May 26, 2021
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 17, 2021
@cristicalin @LuckySB
store openstack external cloud controller ca.cert in a k8s secret ins…
0ed84e3 
…tead of the host filesystem (kubernetes-sigs#7603)
@Id2ndR Id2ndR mentioned this pull request Jun 18, 2021
Merged
@floryut floryut mentioned this pull request Sep 8, 2021
Closed
@cristicalin cristicalin mentioned this pull request Feb 17, 2022
Closed
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
@cristicalin @sakuraiyuta
store openstack external cloud controller ca.cert in a k8s secret ins…
8971ddb 
…tead of the host filesystem (kubernetes-sigs#7603)
@VannTen VannTen mentioned this pull request Oct 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floryut floryut floryut approved these changes

@alijahnas alijahnas Awaiting requested review from alijahnas

@LuckySB LuckySB Awaiting requested review from LuckySB

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@oomichi oomichi

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. kind/bug Categorizes issue or PR as related to a bug. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
2.16
Development

Successfully merging this pull request may close these issues.

Could not init cloud provider "openstack"
5 participants
@cristicalin @k8s-ci-robot @floryut @alijahnas @oomichi