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

Restart Ironic when certificate is updated #247

Merged
merged 1 commit into from May 17, 2021
Merged

Restart Ironic when certificate is updated #247

merged 1 commit into from May 17, 2021

Conversation

namnx228
Copy link
Member

When the TLS certificate is updated, Ironic will reload (or restart) to get the new certificate.

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 25, 2021
@namnx228
Copy link
Member Author

/assign @dtantsur
/cc @elfosardo
/cc @maelk
/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228 namnx228 changed the title Certificate updated for Ironic Reload Ironic when certificate is updated Mar 25, 2021
@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

dtantsur
dtantsur requested changes Mar 26, 2021
View reviewed changes
Copy link
Member

@dtantsur dtantsur left a comment

Choose a reason for hiding this comment

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

I don't think this approach will fly, you probably need whatever party orchestrates the containers to restart them.

ironic-scripts/runironic-conductor Outdated Show resolved Hide resolved
@namnx228
Copy link
Member Author

I don't think this approach will fly, you probably need whatever party orchestrates the containers to restart them.

Yes, I agree that this approach is not an optimal way. However, at the moment, we are needing this feature soon, so a straightforward solution is good to go, IMO. But I think we will need to design a more K8s native solution in the future.

@namnx228 namnx228 force-pushed the cert-rotation-nam branch 2 times, most recently from 65f37d3 to 5762b61 Compare March 29, 2021 12:00
@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@maelk
Copy link
Member

maelk commented Mar 30, 2021

Considering that cert-manager can rotate the certificates at any time, without notifications (it just updates the secret), I don't see a better solution than Ironic being able to detect that the certificate changed and reload it, or since it does not support that for now, having a helper script that will cause a restart of Ironic.

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228 namnx228 changed the title Reload Ironic when certificate is updated Restart Ironic when certificate is updated Mar 31, 2021
@namnx228
Copy link
Member Author

namnx228 commented Apr 1, 2021

/test-integration
/test-centos-integration

@namnx228 namnx228 requested a review from dtantsur April 6, 2021 07:14
@namnx228
Copy link
Member Author

namnx228 commented Apr 6, 2021

/test-integration
/test-centos-integration

@s3rj1k
Copy link
Member

s3rj1k commented Apr 12, 2021

inotify would not work on NFS for example

dtantsur
dtantsur requested changes Apr 12, 2021
View reviewed changes
prepare-image.sh Outdated
@@ -2,7 +2,7 @@

set -euxo pipefail

dnf install -y python3 python3-requests
dnf install -y python3 python3-requests epel-release
Copy link
Member

Choose a reason for hiding this comment

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

Please make enabling epel conditional on RESTART_CONTAINER_CERTIFICATE_UPDATED, not everyone wants to have it (and it may even conflict with tripleo repos).

Copy link
Member Author

Choose a reason for hiding this comment

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

@dtantsur The environment variable RESTART_CONTAINER_CERTIFICATE_UPDATED can only be set when we run the container, but the package needs to be installed when the image is built. Do you have any ideas how should we handle it?

Copy link
Member

Choose a reason for hiding this comment

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

You can provide options in Dockerfiles as well

Copy link
Member Author

Choose a reason for hiding this comment

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

When we use the image, for example in BMO, we only pull the images from quay.io. We don't rebuild the image using the Dockerfile. In this case, we cannot pick the option that is provided in Dockerfiles.

Copy link
Member

Choose a reason for hiding this comment

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

Well, you have to rebuild it or publish an alternative image. Or you can only install inotify from EPEL without pulling in anything else.

09:45 <dtantsur> hi folks! I remember long ago it was not recommended to use EPEL with OpenStack repositories. What's the situation now?
10:26 <apevec> dtantsur, the same

ironic-scripts/configure-ironic.sh Outdated Show resolved Hide resolved
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 5, 2021
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 5, 2021
@namnx228
Copy link
Member Author

namnx228 commented May 5, 2021

/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

namnx228 commented May 6, 2021

/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

namnx228 commented May 7, 2021

/test-integration
/test-centos-integration

@namnx228 namnx228 requested a review from dtantsur May 10, 2021 11:43
@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

dtantsur
dtantsur reviewed May 10, 2021
View reviewed changes
prepare-image.sh Outdated
@@ -11,6 +11,7 @@ if [[ ! -z ${EXTRA_PKGS_LIST:-} ]]; then
xargs -rtd'\n' dnf --setopt=install_weak_deps=False install -y < /tmp/${EXTRA_PKGS_LIST}
fi
fi
dnf install -y https://download-ib01.fedoraproject.org/pub/epel/8/Everything/x86_64/Packages/i/inotify-tools-3.14-19.el8.x86_64.rpm
Copy link
Member

Choose a reason for hiding this comment

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

This will break once this version is deleted. What you need is to install epel-release but disable the added repo via dnf (dnf config-manager --set-disabled epel or something like that). Then you can use --enablerepo=epel when installing inotify-tools specifically.

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228
Copy link
Member Author

/test-integration
/test-centos-integration

@namnx228 namnx228 requested a review from dtantsur May 11, 2021 14:26
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, namnx228

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2021
maelk
maelk reviewed May 17, 2021
View reviewed changes
Copy link
Member

@maelk maelk left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2021
@metal3-io-bot metal3-io-bot merged commit 6529e63 into metal3-io:master May 17, 2021
@namnx228 namnx228 mentioned this pull request May 18, 2021
Merged
elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Dec 21, 2021
@openshift-merge-robot
Merge pull request metal3-io#247 from zaneb/openshift/coreos-ipv6
aa180b5 
Use IPv6-friendly URLs for CoreOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maelk maelk maelk left review comments

@s3rj1k s3rj1k s3rj1k left review comments

@dtantsur dtantsur dtantsur approved these changes

@elfosardo elfosardo Awaiting requested review from elfosardo

@zaneb zaneb Awaiting requested review from zaneb

Assignees

@dtantsur dtantsur

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@namnx228 @maelk @s3rj1k @metal3-io-bot @dtantsur @zaneb