Skip to content
This repository has been archived by the owner on Mar 24, 2020. It is now read-only.

✨ Modify the userdata secret creation logic #264

Merged
merged 2 commits into from Feb 26, 2020

Conversation

maelk
Copy link
Member

@maelk maelk commented Feb 21, 2020

What this PR does / why we need it:
Currently, we always create a secret with the userdata for BMH. Since CABPK v1alpha3, CABPK creates a secret containing the userData. This PR removes the duplication of the secret where possible.

Several cases:

  • no DataSecretName set, but Data is set in the Machine Bootstrap -> creates the secret
  • DataSecretName set, but Machine and BMH are in different namespaces -> creates the secret, since the secret is referenced by name only in both cases, assuming that it is in the same namespace as the machine or as the BMH
  • DataSecretName set and Machine and BMH in same namespace -> re-use CABPK secret

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelk

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2020
@maelk
Copy link
Member Author

maelk commented Feb 21, 2020

/cc @jan-est

@maelk
Copy link
Member Author

maelk commented Feb 21, 2020

/test-v1a3-integration

Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

Few small comments but otherwise looks good to me

if (m.Machine.Spec.Bootstrap.DataSecretName == nil &&
m.Machine.Spec.Bootstrap.Data != nil) ||
(m.Machine.Spec.Bootstrap.DataSecretName != nil &&
m.Machine.Namespace != host.Namespace) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems a very complex logic, can we simplify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

not really. There are two cases, the first one is that objects are not in the same namespace when DataSecretName is set, and the second one is that DataSecretName is not set but Data is set. I don't see how to make it simpler.

tmpBootstrapSecret.Finalizers = []string{}
err = m.client.Update(ctx, &tmpBootstrapSecret)
if err != nil {
m.setError("Failed to delete BareMetalMachine",
Copy link
Member

Choose a reason for hiding this comment

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

Should the message contain some info about the actual reason i.e. cannot delete Bootstrap secret, Failed to delete BMM or something like that?

// Delete the secret with use data
err = m.client.Delete(ctx, &tmpBootstrapSecret)
if err != nil {
m.setError("Failed to delete BareMetalMachine",
Copy link
Member

Choose a reason for hiding this comment

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

Should the message contain some info about the actual reason i.e. cannot delete Bootstrap secret, Failed to delete BMM or something like that?

Create the UserData secret if BMH and Machine are in different
namespaces and DataSecretName is set or when it is unset
and Data is set.
@maelk
Copy link
Member Author

maelk commented Feb 25, 2020

/test-v1a3-integration

1 similar comment
@maelk
Copy link
Member Author

maelk commented Feb 25, 2020

/test-v1a3-integration

@kashifest
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@metal3-io-bot metal3-io-bot merged commit dbc3e11 into metal3-io:master Feb 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants