-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add the standalone hub templates design #134
Add the standalone hub templates design #134
Conversation
ccb9a3f
to
deabb15
Compare
a4d588b
to
706b160
Compare
## Summary | ||
|
||
To support Open Cluster Management (OCM) hub templates without the policy framework, this feature provides an additional | ||
`governance-standalone-hub-templating` addon which generates a user per cluster on the hub to be leveraged by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean cluster -> managedcluster on hub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the user is on the hub but unique per managed cluster.
1. Policies can still be evaluated if the managed cluster is disconnected from the hub. | ||
1. Permissions to the hub are per managed cluster. | ||
1. Provide a hub only `ConfigurationPolicy` to provide permissions to cluster sets. | ||
1. It must work without the rest of the policy framework on the managed cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why goals are all number 1. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a Markdown shortcut to let Markdown do the ordering when rendered.
1. Updating a `ConfigurationPolicy` definition with hub templates while disconnected from the hub is not explicitly | ||
supported. This will almost always work based on the proposed design as long as there aren't new hub template API | ||
queries, but if the update happened while the hub and Configuration Policy controller were offline, it will not work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m confused. The goal says, 'Policies can still be evaluated if the managed cluster is disconnected from the hub,' but it also states, 'If the hub and Configuration Policy controller are offline, it won’t work.' I don’t understand the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nuance is that as long as you don't update the ConfigurationPolicy while disconnected from the hub, it will always work. If the ConfigurationPolicy is updated while disconnected from the hub, the hub templates may not be able to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me "Policies can still be evaluated if the managed cluster is disconnected from the hub." doesn't make sense to me. Can we add more to this sentence? line 31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more details. Does this make sense now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect THanks!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have small comments, this looks good to me.
`governance-standalone-hub-templating`. Enabling this addon will cause the following: | ||
|
||
- The addon framework creates a user/subject represented by a certificate on the hub for each managed cluster. There | ||
will be no permissions assigned to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be no permissions assigned to it. | |
will be no permissions assigned to it initially. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider starting it with some common permissions, if those aren't controversial? Like cluster claims and managed cluster labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. The corresponding ManagedCluster name and labels makes sense to keep the experience consistent. The ClusterClaims are deployed on the managed cluster, so those are accessible already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll add that.
|
||
- The addon framework creates a user/subject represented by a certificate on the hub for each managed cluster. There | ||
will be no permissions assigned to it. | ||
- The addon framework will also create a `Secret` in the `open-cluster-management-agent-addon` namespace with the hub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The addon framework will also create a `Secret` in the `open-cluster-management-agent-addon` namespace with the hub | |
- The addon framework will create a `Secret` in the addon namespace on the managed cluster with the hub |
controller restarts and thus the cache is cleared, the policy could no longer be evaluated due to hub template | ||
resolution failing. | ||
|
||
The proposed solution is for the Configuration Policy controller to create a `Secret` named `<policy UID>-last-resolved` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this - using the UID avoids a potential issue where the name could otherwise be too long.
/hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold for pending reviews
Nice work, @mprahl! I think the design makes sense and seeks to limit the attack surface on the Hub cluster.
706b160
to
51880b1
Compare
/unhold Thanks for the reviews! I addressed the comments. |
51880b1
to
2d3114d
Compare
Relates: https://issues.redhat.com/browse/ACM-15053 Signed-off-by: mprahl <[email protected]>
2d3114d
to
727fb01
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, JustinKuli, mprahl, yiraeChristineKim 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 |
da11ba7
into
open-cluster-management-io:main
Relates:
https://issues.redhat.com/browse/ACM-15053