diff --git a/pkg/runtime/adoption_reconciler_test.go b/pkg/runtime/adoption_reconciler_test.go index 2c21fd1..55097b1 100644 --- a/pkg/runtime/adoption_reconciler_test.go +++ b/pkg/runtime/adoption_reconciler_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "context" @@ -33,7 +33,6 @@ import ( ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics" - ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" ackrtcache "github.com/aws-controllers-k8s/runtime/pkg/runtime/cache" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" ) @@ -61,7 +60,7 @@ func mockAdoptionReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclient sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} apiReader := &ctrlrtclientmock.Reader{} - return ackrt.NewAdoptionReconcilerWithClient( + return NewAdoptionReconcilerWithClient( sc, fakeLogger, cfg, diff --git a/pkg/runtime/field_export_reconciler_test.go b/pkg/runtime/field_export_reconciler_test.go index f2d30a3..46dc820 100644 --- a/pkg/runtime/field_export_reconciler_test.go +++ b/pkg/runtime/field_export_reconciler_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "bytes" @@ -35,7 +35,6 @@ import ( ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors" ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics" - ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" ackrtcache "github.com/aws-controllers-k8s/runtime/pkg/runtime/cache" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" @@ -81,7 +80,7 @@ func mockFieldExportReconcilerWithResourceDescriptor(rd *mocks.AWSResourceDescri sc.On("GetResourceManagerFactories").Return(rmFactoryMap) kc := &ctrlrtclientmock.Client{} apiReader := &ctrlrtclientmock.Reader{} - return ackrt.NewFieldExportReconcilerWithClient( + return NewFieldExportReconcilerWithClient( sc, fakeLogger, cfg, @@ -230,7 +229,7 @@ func setupMockUnstructuredConverter() { }, nil, ) // Update the package variable - ackrt.UnstructuredConverter = conv + UnstructuredConverter = conv } func mockSourceResource() ( @@ -594,7 +593,7 @@ func assertPatchedSecretWithKey(expected bool, t *testing.T, ctx context.Context return bytes.Equal(val, []byte("test-book-name")) }) if expected { - kc.AssertCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) + kc.AssertCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) } else { kc.AssertNotCalled(t, "Patch", withoutCancelContextMatcher, dataMatcher, mock.Anything) } diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index ddc94f4..f509474 100644 --- a/pkg/runtime/reconciler.go +++ b/pkg/runtime/reconciler.go @@ -257,11 +257,49 @@ func (r *resourceReconciler) Reconcile(ctx context.Context, req ctrlrt.Request) if err != nil { return r.handleCacheError(ctx, err, desired) } + parsedARN, err := arn.Parse(string(roleARN)) + if err != nil { + return ctrlrt.Result{}, fmt.Errorf("parsing role ARN %q from %q configmap: %v", roleARN, ackrtcache.ACKRoleTeamMap, err) + } + acctID = ackv1alpha1.AWSAccountID(parsedARN.AccountID) } region := r.getRegion(desired) endpointURL := r.getEndpointURL(desired) gvk := r.rd.GroupVersionKind() + + // If the user has specified a region that is different from the + // region the resource currently exists in, we need to fail the + // reconciliation with a terminal error. + if r.regionDrifted(desired) { + msg := fmt.Sprintf( + "Resource already exists in region %s, but the desired state specifies region %s. ", + region, desired.MetaObject().GetAnnotations()[ackv1alpha1.AnnotationRegion], + ) + rlog.Info( + msg, + "current_region", region, + "desired_region", desired.Identifiers().Region(), + ) + return ctrlrt.Result{}, ackerr.NewTerminalError(errors.New(msg)) + } + + // Similarly, if the user has specified an account ID that is different + // from the account ID the resource currently exists in, we need to + // fail the reconciliation with a terminal error. + if desired.Identifiers() != nil && desired.Identifiers().OwnerAccountID() != nil && *desired.Identifiers().OwnerAccountID() != acctID { + msg := fmt.Sprintf( + "Resource already exists in account %s, but the role used for reconciliation is in account %s. ", + *desired.Identifiers().OwnerAccountID(), acctID, + ) + rlog.Info( + msg, + "current_account", *desired.Identifiers().OwnerAccountID(), + "desired_account", acctID, + ) + return ctrlrt.Result{}, ackerr.NewTerminalError(errors.New(msg)) + } + // The config pivot to the roleARN will happen if it is not empty. // in the NewResourceManager clientConfig, err := r.sc.NewAWSConfig(ctx, region, &endpointURL, roleARN, gvk) @@ -285,6 +323,36 @@ func (r *resourceReconciler) Reconcile(ctx context.Context, req ctrlrt.Request) return r.HandleReconcileError(ctx, desired, latest, err) } +// regionDrifted return true if the desired resource region is different +// from the target region. Target region can be derived from the two places +// in the following order: +// 1) the region annotation on the resource +// 2) from the namespace annotation +func (r *resourceReconciler) regionDrifted(desired acktypes.AWSResource) bool { + if desired.Identifiers() == nil || desired.Identifiers().Region() == nil { + return false + } + + currentRegion := desired.Identifiers().Region() + + // look for region in CR metadata annotations + resAnnotations := desired.MetaObject().GetAnnotations() + region, ok := resAnnotations[ackv1alpha1.AnnotationRegion] + if ok { + return ackv1alpha1.AWSRegion(region) == *currentRegion + } + + // look for default region in namespace metadata annotations + ns := desired.MetaObject().GetNamespace() + nsRegion, ok := r.cache.Namespaces.GetDefaultRegion(ns) + if ok { + return ackv1alpha1.AWSRegion(nsRegion) == *currentRegion + } + + // use controller configuration region + return ackv1alpha1.AWSRegion(r.cfg.Region) == *currentRegion +} + func (r *resourceReconciler) handleCacheError( ctx context.Context, err error, diff --git a/pkg/runtime/reconciler_test.go b/pkg/runtime/reconciler_test.go index fb05154..bfdfe39 100644 --- a/pkg/runtime/reconciler_test.go +++ b/pkg/runtime/reconciler_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "context" @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/smithy-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -31,9 +32,15 @@ import ( k8sobj "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" k8srtschema "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + k8sfake "k8s.io/client-go/kubernetes/fake" + ctrlrt "sigs.k8s.io/controller-runtime" ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" + k8srtschemamocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema" + ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" + ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" @@ -41,13 +48,8 @@ import ( "github.com/aws-controllers-k8s/runtime/pkg/featuregate" ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics" "github.com/aws-controllers-k8s/runtime/pkg/requeue" - ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" ackrtcache "github.com/aws-controllers-k8s/runtime/pkg/runtime/cache" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" - - k8srtschemamocks "github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema" - ctrlrtclientmock "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" - ackmocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) // isWithoutCancelContext checks if the context is a WithoutCancel context @@ -125,7 +127,7 @@ func reconcilerMocks( sc.On("GetMetadata").Return(scmd) kc := &ctrlrtclientmock.Client{} - return ackrt.NewReconcilerWithClient( + return NewReconcilerWithClient( sc, kc, rmf, fakeLogger, cfg, metrics, ackrtcache.Caches{}, ), kc, scmd } @@ -150,7 +152,7 @@ func managerFactoryMocks( ) { rd := &ackmocks.AWSResourceDescriptor{} rd.On("GroupVersionKind").Return( - schema.GroupVersionKind{ + k8srtschema.GroupVersionKind{ Group: "bookstore.services.k8s.aws", Kind: "fakeBook", }, @@ -164,7 +166,7 @@ func managerFactoryMocks( rmf.On("ResourceDescriptor").Return(rd) rmf.On("RequeueOnSuccessSeconds").Return(0) - reg := ackrt.NewRegistry() + reg := NewRegistry() reg.RegisterResourceManagerFactory(rmf) return rmf, rd } @@ -505,7 +507,7 @@ func TestReconcilerAdoptOrCreateResource_Adopt(t *testing.T) { latest, latestRTObj, latestMetaObj := resourceMocks() latest.On("Identifiers").Return(ids) latest.On("Conditions").Return([]*ackv1alpha1.Condition{}) - latest.On( + latest.On( "ReplaceConditions", mock.AnythingOfType("[]*v1alpha1.Condition"), ).Return().Run(func(args mock.Arguments) { @@ -1748,3 +1750,146 @@ func TestReconcilerUpdate_EnsureControllerTagsError(t *testing.T) { rm.AssertNotCalled(t, "LateInitialize", ctx, latest) rm.AssertCalled(t, "EnsureTags", ctx, desired, scmd) } + +func TestReconcile_AccountDrifted(t *testing.T) { + require := require.New(t) + + ctx := context.TODO() + req := ctrlrt.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "production", + Name: "mybook", + }, + } + + // Create resource with existing account + existingAccount := ackv1alpha1.AWSAccountID("111111111111") + + desired, _, metaObj := resourceMocks() + metaObj.SetNamespace("production") + + ids := &ackmocks.AWSResourceIdentifiers{} + ids.On("Region").Return(nil) + ids.On("OwnerAccountID").Return(&existingAccount) + desired.On("Identifiers").Return(ids) + desired.On("Conditions").Return([]*ackv1alpha1.Condition{}) + desired.On( + "ReplaceConditions", + mock.AnythingOfType("[]*v1alpha1.Condition"), + ).Return() + desired.On("IsBeingDeleted").Return(false) + + // Setup resource descriptor + rd := &ackmocks.AWSResourceDescriptor{} + rd.On("GroupVersionKind").Return(schema.GroupVersionKind{ + Group: "test.services.k8s.aws", + Kind: "Book", + Version: "v1alpha1", + }) + rd.On("EmptyRuntimeObject").Return(&fakeBook{}) + rd.On("ResourceFromRuntimeObject", mock.Anything).Return(desired) + + // Setup service controller + sc := &ackmocks.ServiceController{} + sc.On("GetMetadata").Return(acktypes.ServiceControllerMetadata{}) + sc.On("NewAWSConfig", + mock.Anything, + mock.AnythingOfType("v1alpha1.AWSRegion"), + mock.Anything, + mock.AnythingOfType("v1alpha1.AWSResourceName"), + mock.AnythingOfType("schema.GroupVersionKind"), + ).Return(aws.Config{}, nil) + + // Get fakeLogger + zapOptions := ctrlrtzap.Options{ + Development: true, + Level: zapcore.InfoLevel, + } + fakeLogger := ctrlrtzap.New(ctrlrtzap.UseFlagOptions(&zapOptions)) + + // Create fake k8s client with namespace that has owner account annotation + k8sClient := k8sfake.NewSimpleClientset() + + // Create namespace with owner account annotation + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "production", + Annotations: map[string]string{ + ackv1alpha1.AnnotationOwnerAccountID: "222222222222", + }, + }, + } + k8sClient.CoreV1().Namespaces().Create(context.Background(), namespace, metav1.CreateOptions{}) + + // Create CARM configmap + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: ackrtcache.ACKRoleAccountMap, + Namespace: "ack-system", + }, + Data: map[string]string{ + "222222222222": "arn:aws:iam::222222222222:role/ACKRole", + }, + } + k8sClient.CoreV1().ConfigMaps("ack-system").Create(context.Background(), configMap, metav1.CreateOptions{}) + + // Create caches with the k8s client + caches := ackrtcache.New(fakeLogger, ackrtcache.Config{}, featuregate.FeatureGates{}) + + // Run the caches + stopCh := make(chan struct{}) + defer close(stopCh) + caches.Run(k8sClient) + + // Wait for caches to sync + time.Sleep(100 * time.Millisecond) + + kc := &ctrlrtclientmock.Client{} + statusWriter := &ctrlrtclientmock.SubResourceWriter{} + kc.On("Status").Return(statusWriter) + statusWriter.On("Patch", mock.Anything, mock.Anything, mock.Anything).Return(nil) + + rm := &ackmocks.AWSResourceManager{} + rmf := &ackmocks.AWSResourceManagerFactory{} + rmf.On("ResourceDescriptor").Return(rd) + rmf.On("ManagerFor", + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + mock.AnythingOfType("v1alpha1.AWSAccountID"), + mock.AnythingOfType("v1alpha1.AWSRegion"), + mock.AnythingOfType("v1alpha1.AWSResourceName"), + ).Return(rm, nil) + rm.On("ResolveReferences", mock.Anything, mock.Anything, mock.Anything).Return( + desired, false, nil, + ) + rm.On("EnsureTags", mock.Anything, mock.Anything, mock.Anything).Return(nil) + + // Create reconciler with namespace cache + r := &resourceReconciler{ + reconciler: reconciler{ + kc: kc, + sc: sc, + log: fakeLogger, + cfg: ackcfg.Config{AccountID: "333333333333"}, + cache: caches, + metrics: ackmetrics.NewMetrics("test"), + }, + rmf: rmf, + rd: rd, + } + + apiReader := &ctrlrtclientmock.Reader{} + apiReader.On("Get", ctx, req.NamespacedName, mock.AnythingOfType("*runtime.fakeBook")).Return(nil) + r.apiReader = apiReader + + // Call Reconcile + _, err := r.Reconcile(ctx, req) + + // Should get terminal error for account drift + require.NotNil(err) + assert.Contains(t, err.Error(), "Resource already exists in account 111111111111") + assert.Contains(t, err.Error(), "but the role used for reconciliation is in account 222222222222") +} diff --git a/pkg/runtime/service_controller_test.go b/pkg/runtime/service_controller_test.go index f4fff04..c341d45 100644 --- a/pkg/runtime/service_controller_test.go +++ b/pkg/runtime/service_controller_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "context" @@ -42,7 +42,6 @@ import ( ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" mocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config" - ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" ) @@ -150,7 +149,7 @@ func TestServiceController(t *testing.T) { rmf.On("ResourceDescriptor").Return(rd) rmf.On("RequeueOnSuccessSeconds").Return(0) - reg := ackrt.NewRegistry() + reg := NewRegistry() reg.RegisterResourceManagerFactory(rmf) vi := acktypes.VersionInfo{ @@ -159,7 +158,7 @@ func TestServiceController(t *testing.T) { BuildDate: "now", } - sc := ackrt.NewServiceController("bookstore", "bookstore.services.k8s.aws", vi) + sc := NewServiceController("bookstore", "bookstore.services.k8s.aws", vi) require.NotNil(sc) zapOptions := ctrlrtzap.Options{ Development: true, diff --git a/pkg/runtime/tags_test.go b/pkg/runtime/tags_test.go index bb99206..bc7dc0e 100644 --- a/pkg/runtime/tags_test.go +++ b/pkg/runtime/tags_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "fmt" @@ -21,7 +21,6 @@ import ( mocks "github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client" "github.com/aws-controllers-k8s/runtime/pkg/config" - "github.com/aws-controllers-k8s/runtime/pkg/runtime" acktags "github.com/aws-controllers-k8s/runtime/pkg/tags" acktypes "github.com/aws-controllers-k8s/runtime/pkg/types" ) @@ -42,27 +41,27 @@ func TestGetDefaultTags(t *testing.T) { } // nil config - assert.Empty(runtime.GetDefaultTags(nil, &obj, md)) + assert.Empty(GetDefaultTags(nil, &obj, md)) // nil object - assert.Empty(runtime.GetDefaultTags(&cfg, nil, md)) + assert.Empty(GetDefaultTags(&cfg, nil, md)) // no resource tags - assert.Empty(runtime.GetDefaultTags(&cfg, &obj, md)) + assert.Empty(GetDefaultTags(&cfg, &obj, md)) // ill formed tags cfg.ResourceTags = []string{"foobar"} - expandedTags := runtime.GetDefaultTags(&cfg, &obj, md) + expandedTags := GetDefaultTags(&cfg, &obj, md) assert.Empty(expandedTags) // ill formed tags cfg.ResourceTags = []string{"foo=bar=baz"} - expandedTags = runtime.GetDefaultTags(&cfg, &obj, md) + expandedTags = GetDefaultTags(&cfg, &obj, md) assert.Empty(expandedTags) // tags without any ack resource tag format cfg.ResourceTags = []string{"foo=bar"} - expandedTags = runtime.GetDefaultTags(&cfg, &obj, md) + expandedTags = GetDefaultTags(&cfg, &obj, md) assert.Equal(1, len(expandedTags)) assert.Equal("bar", expandedTags["foo"]) @@ -80,7 +79,7 @@ func TestGetDefaultTags(t *testing.T) { acktags.ResourceNameTagFormat, ), } - expandedTags = runtime.GetDefaultTags(&cfg, &obj, md) + expandedTags = GetDefaultTags(&cfg, &obj, md) assert.Equal(4, len(expandedTags)) assert.Equal("bar", expandedTags["foo"]) assert.Equal("s3-v0.0.10", expandedTags["services.k8s.aws/controller-version"]) diff --git a/pkg/runtime/util_test.go b/pkg/runtime/util_test.go index 74589e3..52e7875 100644 --- a/pkg/runtime/util_test.go +++ b/pkg/runtime/util_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package runtime_test +package runtime import ( "testing" @@ -21,7 +21,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" - ackrt "github.com/aws-controllers-k8s/runtime/pkg/runtime" mocks "github.com/aws-controllers-k8s/runtime/mocks/pkg/types" ) @@ -35,11 +34,11 @@ func TestIsAdopted(t *testing.T) { ackv1alpha1.AnnotationAdopted: "true", }, }) - require.True(ackrt.IsAdopted(res)) + require.True(IsAdopted(res)) res = &mocks.AWSResource{} res.On("MetaObject").Return(&metav1.ObjectMeta{}) - require.False(ackrt.IsAdopted(res)) + require.False(IsAdopted(res)) } func TestIsSynced(t *testing.T) { @@ -52,7 +51,7 @@ func TestIsSynced(t *testing.T) { Status: corev1.ConditionTrue, }, }) - require.True(ackrt.IsSynced(res)) + require.True(IsSynced(res)) res = &mocks.AWSResource{} res.On("Conditions").Return([]*ackv1alpha1.Condition{ @@ -65,7 +64,7 @@ func TestIsSynced(t *testing.T) { Status: corev1.ConditionFalse, }, }) - require.False(ackrt.IsSynced(res)) + require.False(IsSynced(res)) } func TestIsForcedAdoption(t *testing.T) { @@ -78,7 +77,7 @@ func TestIsForcedAdoption(t *testing.T) { ackv1alpha1.AnnotationAdopted: "false", }, }) - require.True(ackrt.NeedAdoption(res)) + require.True(NeedAdoption(res)) res = &mocks.AWSResource{} res.On("MetaObject").Return(&metav1.ObjectMeta{ @@ -87,15 +86,15 @@ func TestIsForcedAdoption(t *testing.T) { ackv1alpha1.AnnotationAdopted: "true", }, }) - require.False(ackrt.NeedAdoption(res)) + require.False(NeedAdoption(res)) res = &mocks.AWSResource{} res.On("MetaObject").Return(&metav1.ObjectMeta{ Annotations: map[string]string{ - ackv1alpha1.AnnotationAdopted: "true", + ackv1alpha1.AnnotationAdopted: "true", }, }) - require.False(ackrt.NeedAdoption(res)) + require.False(NeedAdoption(res)) } func TestExtractAdoptionFields(t *testing.T) { @@ -115,7 +114,7 @@ func TestExtractAdoptionFields(t *testing.T) { "clusterName": "my-cluster", "name": "ng-1234", } - actual, err := ackrt.ExtractAdoptionFields(res) + actual, err := ExtractAdoptionFields(res) require.NoError(err) require.Equal(expected, actual) }