[WIP] Create new Deny Assignment backend controller - #4046
Conversation
084b007 to
0aa9bbf
Compare
|
|
||
| // isClusterReadyForDenyAssignments checks if a cluster is in a state where | ||
| // deny assignments should be managed | ||
| func isClusterReadyForDenyAssignments(cluster *api.HCPOpenShiftCluster) bool { |
There was a problem hiding this comment.
Same comment as the one I left in #3970 (comment)
i.e not relying on the state to perform the deny assignment creation but the presence of the MRG in azure
| return nil | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get HCP cluster: %w", err) |
There was a problem hiding this comment.
Let's use TrackEror(...) throughout; same as #3970 (comment)
| } | ||
|
|
||
| // SyncOnce reconciles deny assignments for a single cluster | ||
| func (s *denyAssignmentSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { |
There was a problem hiding this comment.
The controller should only be run in envrionments where we can create deny assignments;
Stage + Prod. We can 't create deny assignments in lower environments.
Same as #3970 (comment)
The approach taken by cs is a binary flag / env var that drives whether deny assignment creation feature is enabled or disabled
| logger := utils.LoggerFromContext(ctx) | ||
| principalIDs := make(map[string]string) | ||
|
|
||
| msiClient, err := armmsi.NewUserAssignedIdentitiesClient(subscriptionID, c.credential, nil) |
There was a problem hiding this comment.
What credential will be used to create this client? is it the FPA or the SMI (service managed identity)?
If it is the former, I do not think if we can retrieve the client id directly via ARM - there could be permissions issues.
CS uses the FPA to retrieve control plane MIs info from MSI RP directly and the SMI credential to retrieve the principal id of the data plane clients directly from ARM.
The way I see this flow is;
(1) FPA is used to retrieve credential of the SMI
(2) From which a credential retrieve in step(1) a client is created similar to what's done here
(3) The client is used to fetch the principal id of those CP + DP.
0aa9bbf to
456c77e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rogbas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
|
@rogbas: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
| // isClusterReadyForDenyAssignments checks if a cluster is in a state where | ||
| // deny assignments should be managed | ||
| func isClusterReadyForDenyAssignments(cluster *api.HCPOpenShiftCluster) bool { | ||
| // Only process clusters that are Succeeded or Updating |
There was a problem hiding this comment.
This doesn't seem correct. We shouldn't depend on the provisioning state. We should create it as long as it's not there and the cluster hasn't been marked as deleted
|
Superseded by #6269 |
3 participants
No description provided.