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

Allow secrets to be owned by external tools #1428

Merged
merged 2 commits into from
Apr 26, 2022
Merged

Allow secrets to be owned by external tools #1428

merged 2 commits into from
Apr 26, 2022

Conversation

farodin91
Copy link
Contributor

@farodin91 farodin91 commented Feb 10, 2022 •
edited

This change allows that secrets can be owned by external tools such as sealed secret.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1431

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Allow secrets to be owned by external tools

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @farodin91. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2022
@srm09
Copy link
Contributor

srm09 commented Feb 10, 2022

@farodin91 Can you create a GH issue to describe the enhancement that you have PR'd in? Would be great to have that context before looking at the implementation itself.

@farodin91
Copy link
Contributor Author

@srm09 I added an issue.

@MaxRink
Copy link
Contributor

MaxRink commented Feb 11, 2022

#1431 for easier reference :)

@MaxRink
Copy link
Contributor

MaxRink commented Feb 11, 2022 •
edited

/ok-to-test
i can definitely understand the need

srm09
srm09 requested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@srm09 srm09 left a comment

Choose a reason for hiding this comment

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

This PR needs to handle the deletion flow for the identity/secret. To be more explicit, checking for ownerRefs and either removing the ownerRef and/or deleting the secret depending on the owners.

pkg/identity/identity.go
@@ -120,6 +121,20 @@ func IsSecretIdentity(cluster *infrav1.VSphereCluster) bool {
return cluster.Spec.IdentityRef.Kind == infrav1.SecretKind
}

func IsOwnedByIdentityOrCluster(ownerReferences []metav1.OwnerReference) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add unit tests for this function, we are gradually trying to increase the overall test coverage for the project.

  • When no onwerRefs are present
  • When ownerRefs are present:
    • test assumption on line 127 that ownerRef could be set by CAPV objects of different versions
    • when OwnerRef.Kind is VSphereCluster
    • when OwnerRef.Kind is VSphereClusterIdentity
    • specifically test for the APIVersion of the owner ref having type vmware.infrastructure.cluster.x-k8s.io/v1beta1 (this might fail.)

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 added a table test

@srm09
Copy link
Contributor

srm09 commented Feb 15, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 15, 2022
@farodin91
Copy link
Contributor Author

@srm09 Shouldn't Kubernetes handle ownerRefs correctly? As, it is currently done for just one ownerRef.

@farodin91 farodin91 force-pushed the allow-secrets-to-owned-by-external-tools branch from d7cacaa to 2848e1d Compare February 22, 2022 08:55
@farodin91
Copy link
Contributor Author

@srm09 Would you like to review it again?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@farodin91
Allow secrets to be owned by external tools
cb073ce 
Signed-off-by: Jan Jansen <jan.jansen@gdata.de>
@farodin91
add test
68c43d4 
Signed-off-by: Jan Jansen <jan.jansen@gdata.de>
@farodin91 farodin91 force-pushed the allow-secrets-to-owned-by-external-tools branch from 2848e1d to 68c43d4 Compare April 4, 2022 12:58
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022
@srm09
Copy link
Contributor

srm09 commented Apr 25, 2022

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2022
@srm09 srm09 added this to the v1.2.0 milestone Apr 25, 2022
@srm09
Copy link
Contributor

srm09 commented Apr 26, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: srm09

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2022
@srm09
Copy link
Contributor

srm09 commented Apr 26, 2022

/retest

@k8s-ci-robot k8s-ci-robot merged commit 3e5df18 into kubernetes-sigs:main Apr 26, 2022
@farodin91 farodin91 deleted the allow-secrets-to-owned-by-external-tools branch April 26, 2022 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srm09 srm09 srm09 requested changes

@MaxRink MaxRink Awaiting requested review from MaxRink

@vrabbi vrabbi Awaiting requested review from vrabbi

Assignees

@srm09 srm09

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Secrets can be only owned by the cluster or the identity itself not by any external tooling.
4 participants
@farodin91 @k8s-ci-robot @srm09 @MaxRink