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

Create singleton recipe for Cert Manager #2328

Open
wants to merge 10 commits into
base: scripts-dev
Choose a base branch
from

Conversation

YCShen1010
Copy link
Contributor

@YCShen1010 YCShen1010 commented Dec 6, 2024

What this PR does:

Create a new template for Cert Manager and resources cert manager configs including set of Application, Assignment, PolicyAssignment and new streamlined Recipe

Issue: https://github.ibm.com/IBMPrivateCloud/roadmap/issues/64751

Condition check

  1. Before backup: if there is no IBM cert manager subscription, then it failed (checking by selectorlabel).
  2. Before restore: if there is already a cert manager webhook exists on the cluster, then failed and error out.

Test steps:

  1. Install IBM Cert Manager
  2. Create new cert manager templates
  3. Run backup and restore on fusion

Test result

Positive case:
Screenshot 2024-12-06 at 16 47 32

Negative case (if there is a cert manager present in the restore cluster, then error out):
Screenshot 2024-12-07 at 23 33 16

@ibm-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YCShen1010

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

Copy link
Member

@bluzarraga bluzarraga left a comment

Choose a reason for hiding this comment

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

Let's talk about how we want cert manager to work in these recipes during our sync up tomorrow. It sits in a weird spot since it is mandatory compared to the licensing singletons and I think that requires a little more discussion

@ibm-ci-bot ibm-ci-bot added size/XL and removed size/L labels Dec 11, 2024
@YCShen1010 YCShen1010 force-pushed the cm-recipe branch 2 times, most recently from ce5d3d5 to b5e7bf6 Compare December 12, 2024 03:48
@YCShen1010 YCShen1010 force-pushed the cm-recipe branch 2 times, most recently from cf6bdf0 to 3e964d6 Compare December 12, 2024 04:11
recipe:
apiVersion: spp-data-protection.isf.ibm.com/v1alpha1
name: cert-manager-recipe
namespace: ibm-spectrum-fusion-ns
Copy link
Member

Choose a reason for hiding this comment

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

I like the backup-restore-workflow.yaml idea, good job

@ibm-ci-bot ibm-ci-bot removed the size/L label Dec 12, 2024
@@ -32,7 +32,7 @@ spec:
includeClusterResources: true
includedResourceTypes:
- customresourcedefinitions.apiextensions.k8s.io
name: cert-manager-crd
name: cert-manager-config-crd
Copy link
Member

Choose a reason for hiding this comment

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

we can probably generalize this and make sure to BR all cert manager crds

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 have changed the this name to cert-manager-crds, which will include all CRDs of certificate, issuer, and cert-manager configurations with the label foundationservices.cloudpak.ibm.com=cert-manager.

I am not using the cert-manager-operator label for these CRDs and resources because this aligns with the label used in label-cert-manager.sh, which is still in use.

@@ -323,7 +326,7 @@ function label_subscription() {

${OC} label subscriptions.operators.coreos.com $cs_pm foundationservices.cloudpak.ibm.com=subscription -n $OPERATOR_NS --overwrite=true 2>/dev/null
if [[ $ENABLE_CERT_MANAGER -eq 1 ]]; then
${OC} label subscriptions.operators.coreos.com $cm_pm foundationservices.cloudpak.ibm.com=singleton-subscription -n $CERT_MANAGER_NAMESPACE --overwrite=true 2>/dev/null
${OC} label subscriptions.operators.coreos.com $cm_pm foundationservices.cloudpak.ibm.com=cert-manager -n $CERT_MANAGER_NAMESPACE --overwrite=true 2>/dev/null
Copy link
Member

Choose a reason for hiding this comment

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

I have a couple thoughts on changing the label name:

  1. we will need to update the instructions for OADP/Velero to make sure these are properly reflected
  2. we need to include the cert manager crds (like certificate and issuer) because they are currently not included so when we restore cert-manager, we will fail to restore certs and issuers labeled cert-manager from the label-cert-manager.sh script

Its possible to lump all cert manager resources (including the subscription) together (I think) but we need to make sure CRDs are there and I don't seem them labeled in any of our existing scripts.

Copy link
Member

Choose a reason for hiding this comment

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

These thoughts mostly apply to how the script changes affect OADP/Velero. In Fusion, we can always create the subscription first and then the resources after and its not a big deal but if we are running the restore-cert-manager.yaml file for OADP/Velero, it may not be able to create the certificates and issuers because the crds are not present yet

Copy link
Member

Choose a reason for hiding this comment

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

I think to address the above comments, we either label the cert manager crds or we continue with the singleton-subscription label. I am leaning towards labeling cert manager CRDs (certs, issuers, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the labelling subscription of cert manager, I have updated the label in both label-singleton-cert-manager.sh and label-common-services.sh scripts to foundationservices.cloudpak.ibm.com=cert-manager-operator as we discussed this could be diff from the one in label-cert-manager.sh

- hook: cert-manager-webhook-exists-check/webhookExists
- group: cert-manager-config-crd
- group: cert-manager-workload-resources
- group: cert-manager-subscription
Copy link
Member

Choose a reason for hiding this comment

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

Does the subscription need to be restored first? At least before the webhook check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, the recipe includes a group named cert-manager-subscription, which works specifically for the cert-manager subscription.
However, I have kept the singleton-subscriptions group in this PR because I have not yet opened a PR for the license operator. This group could still be used for the license subscription BR.

When I handle the license BR, I will use the specific license label for its subscription at that time

- hook: cert-manager-webhook-exists-check/webhookExists
- group: cert-manager-config-crd
- group: cert-manager-workload-resources
- group: cert-manager-subscription
Copy link
Member

Choose a reason for hiding this comment

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

same here, subscription restored first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a possibility that, after the operator is installed successfully, it could bring up its own resources, causing conflicts and blocking the restoration of resources later?
If it is safe, I could move the subscription before the CRD

- hook: cert-manager-webhook-exists-check/webhookExists
- group: cert-manager-config-crd
- group: cert-manager-config-cr
- group: cert-manager-subscription
Copy link
Member

Choose a reason for hiding this comment

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

sub first?

timeout: 600
type: check
- chks:
- condition: '{$.metadata.name} != null'
Copy link
Member

@bluzarraga bluzarraga Jan 23, 2025

Choose a reason for hiding this comment

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

I'm getting a syntax error for this condition and I am not sure "!= null" is a supported comparison. So I have two questions:

  1. What is this check trying to accomplish?
  2. Can we accomplish this goal with a different check?

To me, it seems like a very similar check as the cert-manager-webhook-check/podReady. Can we reuse this check instead? Or, could we change the comparison to equal the string we expect it to be (since it should be static if using IBM cert manager)? If the name value is ever null, then the resource would not exist so would we really be able to check that it is null?

@@ -237,6 +242,18 @@ spec:
selectResource: deployment
timeout: 600
type: check
- chks:
- condition: '{$.metadata.name} != null'
Copy link
Member

Choose a reason for hiding this comment

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

same as in individual recipe

timeout: 600
type: check
- chks:
- condition: '{$.metadata.name} != null'
Copy link
Member

Choose a reason for hiding this comment

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

same as in individual

timeout: 600
type: check
- chks:
- condition: '{$.metadata.name} != null'
Copy link
Member

Choose a reason for hiding this comment

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

same as in individual recipe

timeout: 600
type: check
- chks:
- condition: '{$.metadata.name} != null'
Copy link
Member

Choose a reason for hiding this comment

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

same as individual recipe

@YCShen1010 YCShen1010 force-pushed the cm-recipe branch 3 times, most recently from 77e38eb to 9a05c4f Compare January 24, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants