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

Bind and unbind managed resources conservatively #175

Merged
merged 2 commits into from
May 9, 2020

Conversation

negz
Copy link
Member

@negz negz commented May 6, 2020

Description of your changes

Fixes #177

The two commits in this PR gate against a handful of possible issues:

  1. Composite resource C composes managed resource M. Claim A explicitly sets its resource reference to managed resource M, thereby making M doubly owned by a claim and a composition.
  2. Claim A dynamically provisions managed resource M. Claim B runs and wins a race against Claim A to bind to the newly provisioned and bindable M.
  3. Claim A dynamically provisions managed resource M. Claim B explicitly sets its resource reference to M, and is then deleted, thereby deleting M which it was never bound to and did not dynamically provision.

We noticed both of these issues during review of #174 (which did not introduce them). I'd like to wait until that PR is merged before I manually test this.

Note (cc @prasek) that one implication of this change is that a resource claim may not claim a managed resource that was statically provisioned by an infrastructure stack. i.e. You could not author an infra stack that created a CloudSQLInstance, then later author a MySQLInstance that claimed that CloudSQLInstance. To my knowledge we don't do this today - infra stacks only provision "unclaimable" supporting infrastructure like VPC networks.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the RBAC permissions in clusterrole.yaml to include any new types.

@negz negz requested review from kasey, muvaf and hasheddan May 6, 2020 04:20
@hasheddan hasheddan added the release/behaviour-change This PR will change controller behaviour when released. label May 6, 2020
… them

This gates against two possible issues:

1. Claim A dynamically provisions managed resource M. Claim B runs and wins a
   race against Claim A to bind to the newly provisioned and bindable M.
2. Claim A dynamically provisions managed resource M. Claim B explicitly sets
   its resource reference to M, and is then deleted, thereby deleting M which it
   was never bound to and did not dynamically provision.

Signed-off-by: Nic Cope <negz@rk0n.org>
@negz negz changed the title Ensure resources reference the claim that is trying to bind or unbind them Bind and unbind managed resources conservatively May 7, 2020
@negz
Copy link
Member Author

negz commented May 7, 2020

@muvaf @hasheddan Can you sanity check the commit I just added that guards against binding to managed resources with controller refs. Do we have any cases where a resource (e.g. a stack) creates and is the controller reference of a managed resource that is intended to be claimable?

@negz
Copy link
Member Author

negz commented May 8, 2020

I've rebased this on #174 (which is now merged) and tested that:

  • Dynamic provisioning still works as expected.
  • Static provisioning still works as expected.
  • Deleting a claim that is trying but failing to dynamically provision a managed resource deletes said managed resource.
  • I get an error when I try to nefariously 'unbind' a managed resource that is already bound to a different claim.
  • I get an error when I nefariously try to race the claim that dynamically provisioned a managed resource to bind said managed resource.
  • I get an error when I try to claim a managed resource that is part of a composition.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Do we have any cases where a resource (e.g. a stack) creates and is the controller reference of a managed resource that is intended to be claimable?

We do not currently provision claimable resources in stacks (i.e. all managed resources are network related, and the only other resources are providers or classes). However, we don't explicitly guard against doing so, so a stack could provision a managed resource and I believe it would be unable to be claimed in this situation. I think that is ok as I see stacks likely getting replaced by composition.

pkg/reconciler/claimbinding/api.go Outdated Show resolved Hide resolved
This will ensure resource claims don't bind to managed resources that are part
of a composite resource while these two features live side by side. I suspect it
will also nesure resource claims don't bind to a managed resource produced by a
stack. As far as I know we don't have any use cases in which a stack creates a
managed resource that is expected to be claimable.

Signed-off-by: Nic Cope <negz@rk0n.org>
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

LGTM! I needed to refresh my memory about why we can't add controller ref of claim to the managed resource and the reason was namespaced cannot own cluster-scoped.

@negz negz merged commit 926e188 into crossplane:master May 9, 2020
@negz negz deleted the somewhatbounded branch May 9, 2020 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/behaviour-change This PR will change controller behaviour when released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composed resources are still claimable
4 participants