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

feat(cert): add cryostat CA cert to target namespaces #661

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

mwangggg
Copy link
Member

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits: git commit -S -m "YOUR_COMMIT_MESSAGE"

Fixes: #595

@mwangggg mwangggg added the feat New feature or request label Oct 24, 2023
@mergify mergify bot added the safe-to-test label Oct 24, 2023
@mwangggg
Copy link
Member Author

/build_test

@mwangggg
Copy link
Member Author

@mwangggg mwangggg marked this pull request as ready for review October 24, 2023 18:13
@mwangggg mwangggg requested review from ebaron, andrewazores and a team October 24, 2023 18:13
@mwangggg mwangggg marked this pull request as draft November 1, 2023 20:49
@mwangggg mwangggg force-pushed the 595-mirror-secrets branch 2 times, most recently from 8e46946 to 863717a Compare November 2, 2023 20:25
@mwangggg mwangggg marked this pull request as ready for review November 2, 2023 20:45
@ebaron
Copy link
Member

ebaron commented Nov 3, 2023

Sorry to complicate things further, but since these secrets will be in different namespaces from the CR, they cannot be owned by the CR. This means we can't leverage Kubernetes garbage collection to clean them up when the CR is deleted. The get around this we can use the existing finalizer to manually delete the secrets when the user attempts to delete the CR.

Here's where we use finalizers in the operator currently:

// Check if this Cryostat is being deleted
if cr.Object.GetDeletionTimestamp() != nil {
if controllerutil.ContainsFinalizer(cr.Object, cryostatFinalizer) {
// Perform finalizer logic related to RBAC objects
err := r.finalizeRBAC(ctx, cr)
if err != nil {
return reconcile.Result{}, err
}
// OpenShift-specific
err = r.finalizeOpenShift(ctx, cr)
if err != nil {
return reconcile.Result{}, err
}
err = common.RemoveFinalizer(ctx, r.Client, cr.Object, cryostatFinalizer)
if err != nil {
return reconcile.Result{}, err
}
}
// Ready for deletion
return reconcile.Result{}, nil
}

We'll want want to add something here like finalizeTLS that runs the deletion logic you've added already, but in this case it would be for all target namespaces.

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hi Ming! Just a few more comments. Are you planning to add some tests for these changes? I can help you get started with them

internal/controllers/certmanager.go Outdated Show resolved Hide resolved
internal/controllers/certmanager.go Outdated Show resolved Hide resolved
internal/controllers/certmanager.go Outdated Show resolved Hide resolved
@mwangggg
Copy link
Member Author

mwangggg commented Nov 9, 2023

/build_test

@mwangggg
Copy link
Member Author

mwangggg commented Nov 9, 2023

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hi @mwangggg, sorry it took me a while to get back to this PR. I have some more suggestions below. If you have any questions about them, please let me know.

internal/controllers/reconciler_test.go Outdated Show resolved Hide resolved
internal/controllers/certmanager.go Outdated Show resolved Hide resolved
@mwangggg mwangggg force-pushed the 595-mirror-secrets branch 3 times, most recently from 7f7c1db to b834223 Compare November 17, 2023 17:06
@mwangggg
Copy link
Member Author

/build_test

@mwangggg
Copy link
Member Author

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Oh whoops, I noticed while testing this in OpenShift that we need to be more selective with what we copy from the Secret. cert-manager includes the private key in addition to the certificate in the CA secret. We definitely don't want to copy that all over the cluster.

I've made some suggestions below to remedy this.

internal/controllers/certmanager.go Outdated Show resolved Hide resolved
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hi @mwangggg, sorry for the delay on this. This one thing jumped out at me.

internal/controllers/certmanager.go Outdated Show resolved Hide resolved
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hi Ming, a few more comments below.

internal/controllers/certmanager.go Outdated Show resolved Hide resolved
internal/controllers/reconciler_test.go Outdated Show resolved Hide resolved
internal/controllers/reconciler_test.go Outdated Show resolved Hide resolved
@mwangggg
Copy link
Member Author

/build_test

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the great work!

@ebaron ebaron merged commit c42d197 into cryostatio:main Nov 29, 2023
7 checks passed
@mwangggg mwangggg deleted the 595-mirror-secrets branch March 12, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Story] Mirror CA secret in target namespaces
3 participants