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.

Sign up for GitHub

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 externally managed predicate #2383

Merged
merged 1 commit into from
May 25, 2021
Merged

Add externally managed predicate #2383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/v1alpha4/awscluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
"sigs.k8s.io/cluster-api/util/annotations"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -104,6 +105,13 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
)
}

if annotations.IsExternallyManaged(oldC) && !annotations.IsExternallyManaged(r) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to check the other way around as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the user can add this annotation and disable infrastructure management, am I wrong?

Copy link
Member

@enxebre enxebre May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going from managed to externally managed should be allowed. The other way around is not allowed.

allErrs = append(allErrs,
field.Invalid(field.NewPath("metadata", "annotations"),
r.Annotations, "removal of externally managed annotation is not allowed"),
)
}

allErrs = append(allErrs, r.Spec.Bastion.Validate()...)

return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs)
Expand Down
28 changes: 24 additions & 4 deletions api/v1alpha4/awscluster_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,18 +152,38 @@ func TestAWSCluster_ValidateUpdate(t *testing.T) {
},
wantErr: false,
},
{
name: "removal of externally managed annotation is not allowed",
oldCluster: &AWSCluster{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{clusterv1.ManagedByAnnotation: ""},
},
},
newCluster: &AWSCluster{},
wantErr: true,
},
{
name: "adding externally managed annotation is allowed",
oldCluster: &AWSCluster{},
newCluster: &AWSCluster{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{clusterv1.ManagedByAnnotation: ""},
},
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.TODO()
cluster := tt.oldCluster.DeepCopy()
cluster.ObjectMeta = metav1.ObjectMeta{
GenerateName: "cluster-",
Namespace: "default",
}
cluster.ObjectMeta.GenerateName = "cluster-"
cluster.ObjectMeta.Namespace = "default"

if err := testEnv.Create(ctx, cluster); err != nil {
t.Errorf("failed to create cluster: %v", err)
}
cluster.ObjectMeta.Annotations = tt.newCluster.Annotations
cluster.Spec = tt.newCluster.Spec
if err := testEnv.Update(ctx, cluster); (err != nil) != tt.wantErr {
t.Errorf("ValidateUpdate() error = %v, wantErr %v", err, tt.wantErr)
Expand Down
4 changes: 2 additions & 2 deletions api/v1alpha4/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,16 @@ import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

ctrl "sigs.k8s.io/controller-runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/cluster-api-provider-aws/test/helpers"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
)

var (
testEnv *helpers.TestEnvironment
ctx = ctrl.SetupSignalHandler()
ctx = ctrl.SetupSignalHandler()
)

func TestAPIs(t *testing.T) {
Expand Down
19 changes: 17 additions & 2 deletions controllers/awscluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/go-logr/logr"
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/record"
infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha4"
"sigs.k8s.io/cluster-api-provider-aws/feature"
Expand Down Expand Up @@ -304,14 +305,15 @@ func (r *AWSClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
},
},
).
WithEventFilter(predicates.ResourceIsNotExternallyManaged(log)).
Build(r)
if err != nil {
return errors.Wrap(err, "error creating controller")
}

return controller.Watch(
&source.Kind{Type: &clusterv1.Cluster{}},
handler.EnqueueRequestsFromMapFunc(r.requeueAWSClusterForUnpausedCluster(log)),
handler.EnqueueRequestsFromMapFunc(r.requeueAWSClusterForUnpausedCluster(ctx, log)),
predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
oldCluster := e.ObjectOld.(*clusterv1.Cluster)
Expand Down Expand Up @@ -354,7 +356,7 @@ func (r *AWSClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
)
}

func (r *AWSClusterReconciler) requeueAWSClusterForUnpausedCluster(log logr.Logger) handler.MapFunc {
func (r *AWSClusterReconciler) requeueAWSClusterForUnpausedCluster(ctx context.Context, log logr.Logger) handler.MapFunc {
return func(o client.Object) []ctrl.Request {
c, ok := o.(*clusterv1.Cluster)
if !ok {
Expand All @@ -380,6 +382,19 @@ func (r *AWSClusterReconciler) requeueAWSClusterForUnpausedCluster(log logr.Logg
return nil
}

awsCluster := &infrav1.AWSCluster{}
key := types.NamespacedName{Namespace: c.Spec.InfrastructureRef.Namespace, Name: c.Spec.InfrastructureRef.Name}

if err := r.Get(ctx, key, awsCluster); err != nil {
log.V(4).Error(err, "Failed to get AWS cluster")
return nil
}

if annotations.IsExternallyManaged(awsCluster) {
log.V(4).Info("AWSCluster is externally managed, skipping mapping.")
return nil
}

log.V(4).Info("Adding request.", "awsCluster", c.Spec.InfrastructureRef.Name)
return []ctrl.Request{
{
Expand Down
5 changes: 5 additions & 0 deletions pkg/cloud/scope/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"sigs.k8s.io/cluster-api/controllers/noderefutil"
capierrors "sigs.k8s.io/cluster-api/errors"
"sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/annotations"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/cluster-api/util/patch"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -343,6 +344,10 @@ func (m *MachineScope) IsEKSManaged() bool {
return m.InfraCluster.InfraCluster().GetObjectKind().GroupVersionKind().Kind == "AWSManagedControlPlane"
}

func (m *MachineScope) IsExternallyManaged() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can actually use kubernetes-sigs/cluster-api@3065a92#diff-e25fb86695a8022fa1611100bd244f9b4185f28629d19830e83d18b4fa2ce710R34 here

return annotations.IsExternallyManaged(m.InfraCluster.InfraCluster())
}

// SetInterruptible sets the AWSMachine status Interruptible
func (m *MachineScope) SetInterruptible() {
if m.AWSMachine.Spec.SpotMarketOptions != nil {
Expand Down
14 changes: 11 additions & 3 deletions pkg/cloud/services/ec2/instances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (s *Service) CreateInstance(scope *scope.MachineScope, userData []byte) (*i
}
input.SubnetID = subnetID

if !scope.IsEKSManaged() && s.scope.Network().APIServerELB.DNSName == "" {
if !scope.IsExternallyManaged() && !scope.IsEKSManaged() && s.scope.Network().APIServerELB.DNSName == "" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that it being externally managed means there should be no reconciliation at all? So do we not need something like

if scope.IsExternallyManaged() {
  // Should not reconcile
  return nil, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is related to instance creation, we are skipping logic that is relying on infrastructure created by the managed cluster.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that if we are using external but still provide the APIServer DNSName then we still work as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is only used to check that a load balancer was created.

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")
Expand Down Expand Up @@ -210,7 +210,9 @@ func (s *Service) CreateInstance(scope *scope.MachineScope, userData []byte) (*i
// fallback to AWSCluster.Spec.SSHKeyName if it is defined
prioritizedSSHKeyName = *scope.InfraCluster.SSHKeyName()
default:
prioritizedSSHKeyName = defaultSSHKeyName
if !scope.IsExternallyManaged() {
prioritizedSSHKeyName = defaultSSHKeyName
}
}

// Only set input.SSHKeyName if the user did not explicitly request no ssh key be set (explicitly setting "" on either the Machine or related Cluster)
Expand Down Expand Up @@ -291,7 +293,9 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) {
case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.Filters != nil:
criteria := []*ec2.Filter{
filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable),
filter.EC2.VPC(s.scope.VPC().ID),
}
if !scope.IsExternallyManaged() {
criteria = append(criteria, filter.EC2.VPC(s.scope.VPC().ID))
}
if failureDomain != nil {
criteria = append(criteria, filter.EC2.AvailabilityZone(*failureDomain))
Expand Down Expand Up @@ -355,6 +359,10 @@ func (s *Service) getFilteredSubnets(criteria ...*ec2.Filter) ([]*ec2.Subnet, er
// GetCoreSecurityGroups looks up the security group IDs managed by this actuator
// They are considered "core" to its proper functioning
func (s *Service) GetCoreSecurityGroups(scope *scope.MachineScope) ([]string, error) {
if scope.IsExternallyManaged() {
return nil, nil
}
Comment on lines +362 to +364
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have the same effect as the changes on lines 191 right?

Also, do we not still need security groups? Or are they just fetched explicitly from the AWSCluster in this case?

Copy link
Member

@enxebre enxebre May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have the same effect as the changes on lines 191 right?

I think this is called in more places, so we possibly want to drop the other one and keep this logic here within the func.

Also, do we not still need security groups? Or are they just fetched explicitly from the AWSCluster in this case?

I expect machines with externallyManaged infra to set their desired security groups explicitly through the API contract /~https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/api/v1alpha4/awsmachine_types.go#L100
and don't do any infra naming convention assumptions which would be driven by the not running infra controller. In fact without this change this would fail

cluster-api-provider-aws/pkg/cloud/services/ec2/instances.go

Line 381 in 72f4dad

return nil, awserrors.NewFailedDependency(fmt.Sprintf("%s security group not available", sg))


// These are common across both controlplane and node machines
sgRoles := []infrav1.SecurityGroupRole{
infrav1.SecurityGroupNode,
Expand Down