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

api: Enables the binding of APIs in protected groups #1104

Merged
merged 5 commits into from
May 23, 2022

Conversation

astefanutti
Copy link
Member

@astefanutti astefanutti commented May 19, 2022

Summary

This PR enables the binding of APIs whose groups are protected, as defined in kubernetes/enhancements#1111.

The proposed solution delegates to the user the responsibility of adding the api-approved.kubernetes.io annotation to the APIResourceSchema resource, as for standard CRDs, so it makes it explicit, and prevents from accidentally declaring APIs in a protected group. With this PR, the APIBinding controller will simply propagate the annotation to the projected CRDs.

In the API import case, where CRDs are generated following the creation of APIResourceImport resources, the NegociatedAPIResource controller already adds the approval annotation, assuming the API have already been explicitly declared as approved in the downstream workload cluster.

TODO:

  • Propagate the approval annotation from the APIResourceSchema resources to the generated CRDs
  • Add admission to reject APIResouceSchema resources in protected groups without the approval annotation
  • Improve APIBinding condition message when the generated CRD is invalid
  • Add e2e test

Related issue(s)

Fixes #1083

@astefanutti astefanutti marked this pull request as ready for review May 20, 2022 09:08
@sttts
Copy link
Member

sttts commented May 20, 2022

One comment. Otherwise, looks very good.

@sttts
Copy link
Member

sttts commented May 21, 2022

Compare 47d0d37 in #1084. Aren't those changes missing?

@astefanutti
Copy link
Member Author

astefanutti commented May 23, 2022

Compare 47d0d37 in #1084. Aren't those changes missing?

Unless I'm mistaken, the changes from 1ed6920 aren't strictly required for the protected API use case. My understanding is that the approved annotation is set at the end of the import execution path, when the NegociatedAPIResource is projected into the published CRD:

if crdhelpers.IsProtectedCommunityGroup(gvr.Group) {
if cr.ObjectMeta.Annotations == nil {
cr.ObjectMeta.Annotations = map[string]string{}
}
cr.ObjectMeta.Annotations["api-approved.kubernetes.io"] = "https://github.com/kcp-dev/kubernetes/pull/4"
}

That being said, it would be required if we were to propagated the set of annotations arbitrarily, then the set would have to be transported along the resources chain (APIResourceImport, NegotiatedAPIResource, ...).

@sttts
Copy link
Member

sttts commented May 23, 2022

That being said, it would be required if we were to propagated the set of annotations arbitrarily, then the set would have to be transported along the resources chain

But it does not hurt either. Bonus: we don't get this fake pull request number 4, but the real one in case of CRDs on the pcluster side.

@astefanutti
Copy link
Member Author

That being said, it would be required if we were to propagated the set of annotations arbitrarily, then the set would have to be transported along the resources chain

But it does not hurt either. Bonus: we don't get this fake pull request number 4, but the real one in case of CRDs on the pcluster side.

Agreed, that's a good point about that fake approval PR 👍🏼.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot export or bind to resources in protected groups
2 participants