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

🌱Add generation of empty-ironic-cacert secret for handling TLS disabled scenario #237

Merged
merged 1 commit into from
Jul 8, 2021
Merged

🌱Add generation of empty-ironic-cacert secret for handling TLS disabled scenario #237

merged 1 commit into from
Jul 8, 2021

Conversation

smoshiur1237
Copy link
Member

@smoshiur1237 smoshiur1237 commented Jul 1, 2021 •
edited
Loading

In non-TLS scenario BMO is waiting for an empty secret which was removed earlier with cert manager setup in ironic. Because of that the non-tls run was broken. This PR will add the empty secret creation and during TLS-disabled, we will substitute the secret name as empty-ironic-cacert before kustomization. The change of secret name will be done in metal3-dev-env
The changes fix the issue with non-TLS.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2021
@smoshiur1237 smoshiur1237 changed the base branch from master to release-0.4 July 1, 2021 11:02
@smoshiur1237 smoshiur1237 changed the base branch from release-0.4 to master July 1, 2021 11:04
@smoshiur1237
Copy link
Member Author

/retest

@smoshiur1237
Copy link
Member Author

/test-integration

3 similar comments
@smoshiur1237
Copy link
Member Author

/test-integration

@furkatgofurov7
Copy link
Member

/test-integration

@furkatgofurov7
Copy link
Member

/test-integration

@smoshiur1237
Copy link
Member Author

/test-integration

@smoshiur1237 smoshiur1237 mentioned this pull request Jul 6, 2021
Merged
@smoshiur1237
Copy link
Member Author

/assign @furkatgofurov7
/cc @Xenwar
/cc @jan-est

@smoshiur1237
Copy link
Member Author

Test is going on in Metal3-dev-env for TLS disabled.
/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021
lentzi90
lentzi90 reviewed Jul 7, 2021
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Would it be possible to make the volume optional instead of creating this empty secret? I'm thinking that we could add optional: true here:

cluster-api-provider-metal3/config/bmo/secret_mount_patch.yaml

Lines 22 to 24 in 07cb2d8

- name: cacert
secret:
secretName: ironic-cacert

But I'm not sure if BMO can handle a missing file or if it has to be an empty file?

@smoshiur1237
Copy link
Member Author

Would it be possible to make the volume optional instead of creating this empty secret? I'm thinking that we could add optional: true here:

cluster-api-provider-metal3/config/bmo/secret_mount_patch.yaml

Lines 22 to 24 in 07cb2d8

- name: cacert
secret:
secretName: ironic-cacert

But I'm not sure if BMO can handle a missing file or if it has to be an empty file?

At this current scenario when BMO is running as part of CAPM3, BMO will wait for the secret in both TLS enabled or disabled and the CI was failing without adding the empty secret. As most of our CI master job are running with TLS enabled, secret is necessary with the settings in BMO(part of CAPM3). So the empty secret will be used only to support non-TLS. I think when we are taking BMO outside of CAPM3, don't need to create an empty secret as the setting has separate TLS and non-TLS configuration.

lentzi90
lentzi90 approved these changes Jul 7, 2021
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation! I think this is fine then 🙂

namnx228
namnx228 approved these changes Jul 7, 2021
View reviewed changes
Copy link
Member

@namnx228 namnx228 left a comment

Choose a reason for hiding this comment

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

LGTM

@smoshiur1237
Copy link
Member Author

/unhold

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2021
@Xenwar
Copy link
Member

Xenwar commented Jul 8, 2021

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@smoshiur1237 smoshiur1237 changed the title 🌱Add generation of empty-ironic-cacert secret for handling TLS disabled scenario. 🌱Add/revert generation of empty-ironic-cacert secret for handling TLS disabled scenario. Jul 8, 2021
@furkatgofurov7
Copy link
Member

/retitle 🌱Add generation of empty-ironic-cacert secret for handling TLS disabled scenario

@metal3-io-bot metal3-io-bot changed the title 🌱Add/revert generation of empty-ironic-cacert secret for handling TLS disabled scenario. 🌱Add generation of empty-ironic-cacert secret for handling TLS disabled scenario Jul 8, 2021
@smoshiur1237 smoshiur1237 mentioned this pull request Jul 8, 2021
Merged
furkatgofurov7
furkatgofurov7 reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7, lentzi90, 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 Jul 8, 2021
@metal3-io-bot metal3-io-bot merged commit 03fa3c2 into metal3-io:master Jul 8, 2021
@smoshiur1237 smoshiur1237 mentioned this pull request Jul 8, 2021
Merged
@smoshiur1237 smoshiur1237 deleted the tls-disabled-moshiur branch June 20, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@namnx228 namnx228 namnx228 approved these changes

@lentzi90 lentzi90 lentzi90 approved these changes

@jan-est jan-est Awaiting requested review from jan-est

@Xenwar Xenwar Awaiting requested review from Xenwar

Assignees

@furkatgofurov7 furkatgofurov7

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@smoshiur1237 @furkatgofurov7 @Xenwar @metal3-io-bot @namnx228 @lentzi90