✨ Add changes to clusteradm accept to disable csr update based on annotation on ManagedCluster #468
Conversation
jeffw17
force-pushed
the
update-clusteradm-accept-aws-irsa
branch
3 times, most recently
from
59cddbf to
dfbcabc
Compare
There was a problem hiding this comment.
I've added some nit comments, but my major concern is that it looks like anyone can just stamp the annotation on their managedcluster and bypass the CSR check. WDYT?
@jaswalkiranavtar Did we discuss this already and this is not a concern?
pkg/cmd/accept/exec.go
Outdated
| if err != nil { | ||
| return approved, fmt.Errorf("fail to approve the csr for cluster %s: %v", clusterName, err) | ||
| return false, fmt.Errorf("fail to get managed cluster") |
There was a problem hiding this comment.
| return false, fmt.Errorf("fail to get managed cluster") | |
| return false, fmt.Errorf("fail to get managedcluster %s: %v", clusterName, err") |
more consistent with the previous error message.
There was a problem hiding this comment.
Yes, this is part of our design. This will work for only EKS based registration. If somebody is using default csr authentication, and they add these annotations, their registration won't complete. If they want to complete the registration with csr based authentication, they have to remove this annotation.
pkg/cmd/accept/exec.go
Outdated
| return approved, fmt.Errorf("fail to approve the csr for cluster %s: %v", clusterName, err) | ||
| } | ||
| } else { | ||
| approved = hasEksArn |
There was a problem hiding this comment.
| approved = hasEksArn | |
| approved = true |
minor nit for readability.
jaswalkiranavtar
force-pushed
the
update-clusteradm-accept-aws-irsa
branch
from
66d424b to
7d04f3e
Compare
…ation on ManagedCluster Signed-off-by: “Jeffrey <[email protected]> Signed-off-by: Gaurav Jaswal <[email protected]>
jaswalkiranavtar
force-pushed
the
update-clusteradm-accept-aws-irsa
branch
from
c69cd4f to
374da7c
Compare
| return approved, fmt.Errorf("fail to approve the csr for cluster %s: %v", clusterName, err) | ||
| return false, fmt.Errorf("fail to get managedcluster %s: %v", clusterName, err) | ||
| } | ||
| _, hasEksArn := managedCluster.Annotations["agent.open-cluster-management.io/managed-cluster-arn"] |
There was a problem hiding this comment.
Done. Does it look ok? Will sign the commit tomorrow.
|
/lgtm |
Signed-off-by: Gaurav Jaswal <[email protected]>
jaswalkiranavtar
force-pushed
the
update-clusteradm-accept-aws-irsa
branch
from
a79e25c to
3bd4b84
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeffw17, mikeshng, qiujian16 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 |
openshift-merge-bot
bot
merged commit 32f8ec8
into
open-cluster-management-io:main
Summary
For AWSIRSA, we want to disable CSR update when running clusteradm accept command and we verify by checking the annotation on the managed cluster to see if it contains the ARN.
Related issue(s)
Fixes # 514