From 591e1960f2684ba96a641d88592cd8b2f7ccd903 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Mon, 26 Feb 2024 14:26:14 -0800 Subject: [PATCH] Expose Cluster Control Plane ref through cluster scope Signed-off-by: Vince Prignano --- controllers/awsmachine_controller.go | 28 +------------------ controllers/awsmachine_controller_test.go | 2 -- .../awsmachine_controller_unit_test.go | 3 -- pkg/cloud/interfaces.go | 3 ++ pkg/cloud/scope/cluster.go | 7 +++++ pkg/cloud/scope/machine.go | 23 +++++++++------ pkg/cloud/scope/machine_test.go | 11 +++----- pkg/cloud/scope/managedcontrolplane.go | 7 +++++ pkg/cloud/scope/shared.go | 26 +++++++++++++++++ pkg/cloud/services/ec2/instances.go | 1 - pkg/cloud/services/ec2/instances_test.go | 2 -- .../services/secretsmanager/secret_test.go | 2 -- pkg/cloud/services/ssm/secret_test.go | 2 -- test/mocks/capa_clusterscoper_mock.go | 16 +++++++++++ 14 files changed, 79 insertions(+), 54 deletions(-) diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go index 7ef74fe8c5..416ba0c420 100644 --- a/controllers/awsmachine_controller.go +++ b/controllers/awsmachine_controller.go @@ -32,7 +32,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -61,7 +60,6 @@ import ( "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/userdata" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/controllers/external" capierrors "sigs.k8s.io/cluster-api/errors" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" @@ -147,6 +145,7 @@ func (r *AWSMachineReconciler) getObjectStoreService(scope scope.S3Scope) servic // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,verbs=get;list;watch;update;patch;delete // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awsmachines/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets;,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch @@ -202,16 +201,10 @@ func (r *AWSMachineReconciler) Reconcile(ctx context.Context, req ctrl.Request) infrav1.SetDefaults_AWSMachineSpec(&awsMachine.Spec) - cp, err := r.getControlPlane(ctx, log, cluster) - if err != nil { - return ctrl.Result{}, err - } - // Create the machine scope machineScope, err := scope.NewMachineScope(scope.MachineScopeParams{ Client: r.Client, Cluster: cluster, - ControlPlane: cp, Machine: machine, InfraCluster: infraCluster, AWSMachine: awsMachine, @@ -1225,22 +1218,3 @@ func (r *AWSMachineReconciler) ensureInstanceMetadataOptions(ec2svc services.EC2 return ec2svc.ModifyInstanceMetadataOptions(instance.ID, machine.Spec.InstanceMetadataOptions) } - -// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch - -func (r *AWSMachineReconciler) getControlPlane(ctx context.Context, log *logger.Logger, cluster *clusterv1.Cluster) (*unstructured.Unstructured, error) { - var ns string - - if ns = cluster.Spec.ControlPlaneRef.Namespace; ns == "" { - ns = cluster.Namespace - } - - controlPlane, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, ns) - if err != nil { - log.Error(err, "unable to get ControlPlane referenced in the given cluster", "cluster", fmt.Sprintf("%s/%s", cluster.Namespace, cluster.Name)) - - return nil, err - } - - return controlPlane, nil -} diff --git a/controllers/awsmachine_controller_test.go b/controllers/awsmachine_controller_test.go index 733d6ce9e9..01122cad0e 100644 --- a/controllers/awsmachine_controller_test.go +++ b/controllers/awsmachine_controller_test.go @@ -30,7 +30,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -418,7 +417,6 @@ func getMachineScope(cs *scope.ClusterScope, awsMachine *infrav1.AWSMachine) (*s InfrastructureReady: true, }, }, - ControlPlane: &unstructured.Unstructured{}, Machine: &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "test", diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go index dd444c5275..8d2d6fc5bd 100644 --- a/controllers/awsmachine_controller_unit_test.go +++ b/controllers/awsmachine_controller_unit_test.go @@ -33,7 +33,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" @@ -131,7 +130,6 @@ func TestAWSMachineReconciler(t *testing.T) { }, }, InfraCluster: cs, - ControlPlane: &unstructured.Unstructured{}, AWSMachine: awsMachine, }, ) @@ -160,7 +158,6 @@ func TestAWSMachineReconciler(t *testing.T) { InfrastructureReady: true, }, }, - ControlPlane: &unstructured.Unstructured{}, Machine: &clusterv1.Machine{ Spec: clusterv1.MachineSpec{ ClusterName: "capi-test", diff --git a/pkg/cloud/interfaces.go b/pkg/cloud/interfaces.go index 7d6115d429..751d9603ea 100644 --- a/pkg/cloud/interfaces.go +++ b/pkg/cloud/interfaces.go @@ -18,6 +18,7 @@ package cloud import ( awsclient "github.com/aws/aws-sdk-go/aws/client" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" @@ -67,6 +68,8 @@ type ClusterScoper interface { // Cluster returns the cluster object. ClusterObj() ClusterObject + // UnstructuredControlPlane returns the unstructured control plane object. + UnstructuredControlPlane() (*unstructured.Unstructured, error) // IdentityRef returns the AWS infrastructure cluster identityRef. IdentityRef() *infrav1.AWSIdentityReference diff --git a/pkg/cloud/scope/cluster.go b/pkg/cloud/scope/cluster.go index fd67eede86..399b07f6e0 100644 --- a/pkg/cloud/scope/cluster.go +++ b/pkg/cloud/scope/cluster.go @@ -22,6 +22,7 @@ import ( awsclient "github.com/aws/aws-sdk-go/aws/client" "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -395,3 +396,9 @@ func (s *ClusterScope) Partition() string { func (s *ClusterScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRule { return s.AWSCluster.Spec.NetworkSpec.DeepCopy().AdditionalControlPlaneIngressRules } + +// UnstructuredControlPlane returns the unstructured object for the control plane, if any. +// When the reference is not set, it returns an empty object. +func (s *ClusterScope) UnstructuredControlPlane() (*unstructured.Unstructured, error) { + return getUnstructuredControlPlane(context.TODO(), s.client, s.Cluster) +} diff --git a/pkg/cloud/scope/machine.go b/pkg/cloud/scope/machine.go index fcb735c22e..f69fa572a5 100644 --- a/pkg/cloud/scope/machine.go +++ b/pkg/cloud/scope/machine.go @@ -23,7 +23,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" "k8s.io/utils/ptr" @@ -44,7 +43,6 @@ import ( type MachineScopeParams struct { Client client.Client Logger *logger.Logger - ControlPlane *unstructured.Unstructured Cluster *clusterv1.Cluster Machine *clusterv1.Machine InfraCluster EC2Scope @@ -69,9 +67,6 @@ func NewMachineScope(params MachineScopeParams) (*MachineScope, error) { if params.InfraCluster == nil { return nil, errors.New("aws cluster is required when creating a MachineScope") } - if params.ControlPlane == nil { - return nil, errors.New("cluster control plane is required when creating a MachineScope") - } if params.Logger == nil { log := klog.Background() @@ -86,7 +81,6 @@ func NewMachineScope(params MachineScopeParams) (*MachineScope, error) { Logger: *params.Logger, client: params.Client, patchHelper: helper, - ControlPlane: params.ControlPlane, Cluster: params.Cluster, Machine: params.Machine, InfraCluster: params.InfraCluster, @@ -102,7 +96,6 @@ type MachineScope struct { Cluster *clusterv1.Cluster Machine *clusterv1.Machine - ControlPlane *unstructured.Unstructured InfraCluster EC2Scope AWSMachine *infrav1.AWSMachine } @@ -377,8 +370,22 @@ func (m *MachineScope) IsEKSManaged() bool { return m.InfraCluster.InfraCluster().GetObjectKind().GroupVersionKind().Kind == ekscontrolplanev1.AWSManagedControlPlaneKind } +// IsControlPlaneExternallyManaged checks if the control plane is externally managed. +// +// This is determined by the kind of the control plane object (EKS for example), +// or if the control plane referenced object is reporting as externally managed. func (m *MachineScope) IsControlPlaneExternallyManaged() bool { - return util.IsExternalManagedControlPlane(m.ControlPlane) + if m.IsEKSManaged() { + return true + } + + // Check if the control plane is externally managed. + u, err := m.InfraCluster.UnstructuredControlPlane() + if err != nil { + m.Error(err, "failed to get unstructured control plane") + return false + } + return util.IsExternalManagedControlPlane(u) } // IsExternallyManaged checks if the machine is externally managed. diff --git a/pkg/cloud/scope/machine_test.go b/pkg/cloud/scope/machine_test.go index 9cad370f35..f34790d061 100644 --- a/pkg/cloud/scope/machine_test.go +++ b/pkg/cloud/scope/machine_test.go @@ -22,7 +22,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -133,8 +132,7 @@ func setupMachineScope() (*MachineScope, error) { InfraCluster: &ClusterScope{ AWSCluster: awsCluster, }, - ControlPlane: &unstructured.Unstructured{}, - AWSMachine: awsMachine, + AWSMachine: awsMachine, }, ) } @@ -225,10 +223,9 @@ func TestGetRawBootstrapDataWithFormat(t *testing.T) { machineScope, err := NewMachineScope( MachineScopeParams{ - Client: client, - Machine: machine, - Cluster: cluster, - ControlPlane: &unstructured.Unstructured{}, + Client: client, + Machine: machine, + Cluster: cluster, InfraCluster: &ClusterScope{ AWSCluster: awsCluster, }, diff --git a/pkg/cloud/scope/managedcontrolplane.go b/pkg/cloud/scope/managedcontrolplane.go index 56e5bd59c5..948f3fa511 100644 --- a/pkg/cloud/scope/managedcontrolplane.go +++ b/pkg/cloud/scope/managedcontrolplane.go @@ -27,6 +27,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -440,3 +441,9 @@ func (s *ManagedControlPlaneScope) Partition() string { func (s *ManagedControlPlaneScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRule { return nil } + +// UnstructuredControlPlane returns the unstructured object for the control plane, if any. +// When the reference is not set, it returns an empty object. +func (s *ManagedControlPlaneScope) UnstructuredControlPlane() (*unstructured.Unstructured, error) { + return getUnstructuredControlPlane(context.TODO(), s.Client, s.Cluster) +} diff --git a/pkg/cloud/scope/shared.go b/pkg/cloud/scope/shared.go index 76e1ec91d8..865ebfaf52 100644 --- a/pkg/cloud/scope/shared.go +++ b/pkg/cloud/scope/shared.go @@ -17,13 +17,18 @@ limitations under the License. package scope import ( + "context" "fmt" "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/controllers/external" ) var ( @@ -127,3 +132,24 @@ func (p *defaultSubnetPlacementStrategy) getSubnetsForAZs(azs []string, controlP return subnetIDs, nil } + +// getUnstructuredControlPlane returns the unstructured object for the control plane, if any. +// When the reference is not set, it returns an empty object. +func getUnstructuredControlPlane(ctx context.Context, client client.Client, cluster *clusterv1.Cluster) (*unstructured.Unstructured, error) { + if cluster.Spec.ControlPlaneRef == nil { + // If the control plane ref is not set, return an empty object. + // Not having a control plane ref is valid given API contracts. + return &unstructured.Unstructured{}, nil + } + + namespace := cluster.Spec.ControlPlaneRef.Namespace + if namespace == "" { + namespace = cluster.Namespace + } + + u, err := external.Get(ctx, client, cluster.Spec.ControlPlaneRef, namespace) + if err != nil { + return nil, errors.Wrapf(err, "failed to retrieve control plane object %s/%s", cluster.Spec.ControlPlaneRef.Namespace, cluster.Spec.ControlPlaneRef.Name) + } + return u, nil +} diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go index 1fbcce3c90..36e6661f29 100644 --- a/pkg/cloud/services/ec2/instances.go +++ b/pkg/cloud/services/ec2/instances.go @@ -183,7 +183,6 @@ func (s *Service) CreateInstance(scope *scope.MachineScope, userData []byte, use if !scope.IsControlPlaneExternallyManaged() && !scope.IsExternallyManaged() && !scope.IsEKSManaged() && s.scope.Network().APIServerELB.DNSName == "" { record.Eventf(s.scope.InfraCluster(), "FailedCreateInstance", "Failed to run controlplane, APIServer ELB not available") - return nil, awserrors.NewFailedDependency("failed to run controlplane, APIServer ELB not available") } diff --git a/pkg/cloud/services/ec2/instances_test.go b/pkg/cloud/services/ec2/instances_test.go index 9ccf5a67ba..f68e4a5f5e 100644 --- a/pkg/cloud/services/ec2/instances_test.go +++ b/pkg/cloud/services/ec2/instances_test.go @@ -31,7 +31,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -4036,7 +4035,6 @@ func TestCreateInstance(t *testing.T) { machineScope, err := scope.NewMachineScope(scope.MachineScopeParams{ Client: client, Cluster: cluster, - ControlPlane: &unstructured.Unstructured{}, Machine: machine, AWSMachine: awsMachine, InfraCluster: clusterScope, diff --git a/pkg/cloud/services/secretsmanager/secret_test.go b/pkg/cloud/services/secretsmanager/secret_test.go index df4976ea4e..87cf7e958a 100644 --- a/pkg/cloud/services/secretsmanager/secret_test.go +++ b/pkg/cloud/services/secretsmanager/secret_test.go @@ -26,7 +26,6 @@ import ( "github.com/golang/mock/gomock" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -281,7 +280,6 @@ func getClusterScope(client client.Client) (*scope.ClusterScope, error) { func getMachineScope(client client.Client, clusterScope *scope.ClusterScope) (*scope.MachineScope, error) { return scope.NewMachineScope(scope.MachineScopeParams{ Client: client, - ControlPlane: &unstructured.Unstructured{}, Cluster: clusterScope.Cluster, Machine: &clusterv1.Machine{}, InfraCluster: clusterScope, diff --git a/pkg/cloud/services/ssm/secret_test.go b/pkg/cloud/services/ssm/secret_test.go index 4e82494848..04afa9e1d4 100644 --- a/pkg/cloud/services/ssm/secret_test.go +++ b/pkg/cloud/services/ssm/secret_test.go @@ -28,7 +28,6 @@ import ( "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -274,7 +273,6 @@ func getClusterScope(client client.Client) (*scope.ClusterScope, error) { func getMachineScope(client client.Client, clusterScope *scope.ClusterScope) (*scope.MachineScope, error) { return scope.NewMachineScope(scope.MachineScopeParams{ Client: client, - ControlPlane: &unstructured.Unstructured{}, Cluster: clusterScope.Cluster, Machine: &clusterv1.Machine{}, InfraCluster: clusterScope, diff --git a/test/mocks/capa_clusterscoper_mock.go b/test/mocks/capa_clusterscoper_mock.go index 0fb8d7b2e3..54b9ee81eb 100644 --- a/test/mocks/capa_clusterscoper_mock.go +++ b/test/mocks/capa_clusterscoper_mock.go @@ -26,6 +26,7 @@ import ( client "github.com/aws/aws-sdk-go/aws/client" logr "github.com/go-logr/logr" gomock "github.com/golang/mock/gomock" + unstructured "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" v1beta2 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" cloud "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud" throttle "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/throttle" @@ -375,6 +376,21 @@ func (mr *MockClusterScoperMockRecorder) Trace(arg0 interface{}, arg1 ...interfa return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Trace", reflect.TypeOf((*MockClusterScoper)(nil).Trace), varargs...) } +// UnstructuredControlPlane mocks base method. +func (m *MockClusterScoper) UnstructuredControlPlane() (*unstructured.Unstructured, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UnstructuredControlPlane") + ret0, _ := ret[0].(*unstructured.Unstructured) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UnstructuredControlPlane indicates an expected call of UnstructuredControlPlane. +func (mr *MockClusterScoperMockRecorder) UnstructuredControlPlane() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UnstructuredControlPlane", reflect.TypeOf((*MockClusterScoper)(nil).UnstructuredControlPlane)) +} + // Warn mocks base method. func (m *MockClusterScoper) Warn(arg0 string, arg1 ...interface{}) { m.ctrl.T.Helper()