diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index ebf13c0..789cc3d 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,13 +1,13 @@ ack_generate_info: - build_date: "2024-03-06T21:39:35Z" - build_hash: a5ba3c851434263128a1464a2c41e528779eeefa + build_date: "2024-03-12T15:06:30Z" + build_hash: d86061f5f579fe5d3a07528917d95e34e79c4dc0 go_version: go1.22.0 - version: v0.32.1 + version: v0.32.1-1-gd86061f-dirty api_directory_checksum: 761a2c708651b0273bf39d98dddaf029de23d337 api_version: v1alpha1 aws_sdk_go_version: v1.49.0 generator_config_info: - file_checksum: ee030a9c18c7fa34d8f7f4777997df355c029971 + file_checksum: 8f85dee132779d172c0fd19537c3bbd709e079d6 original_file_name: generator.yaml last_modification: reason: API generation diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 87e1cd7..51a8a2f 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -148,6 +148,13 @@ resources: - InvalidInput - MalformedPolicyDocument fields: + # Left for historical purposes. It looks like late_initialize is was + # causing the controller to infinitely requeue (every 5 seconds) when the + # description was set to nil. Not it looks like this is not needed + # anymore. + # Note(a-hilaly): Very likely the API behavior has changed and the + # late_initialize is no longer needed. + # Description: # You might be wondering why description is late-initialized, since # there isn't a default server-side value for description. @@ -165,7 +172,10 @@ resources: # we set the late initialize property of the Description field here to # override the Spec.Description to the original value we set in the # CreatePolicy *input* shape. - late_initialize: {} + #late_initialize: {} + set: + - ignore: true + method: Create Path: late_initialize: {} Tags: @@ -211,9 +221,13 @@ resources: set: # The input and output shapes are different... - from: PermissionsBoundary.PermissionsBoundaryArn + # Left for historical purposes. Description: + set: + - ignore: true + method: Create # See above in Policy resource about why this is here. - late_initialize: {} + # late_initialize: {} Path: late_initialize: {} # In order to support attaching zero or more policies to a role, we use diff --git a/generator.yaml b/generator.yaml index 87e1cd7..51a8a2f 100644 --- a/generator.yaml +++ b/generator.yaml @@ -148,6 +148,13 @@ resources: - InvalidInput - MalformedPolicyDocument fields: + # Left for historical purposes. It looks like late_initialize is was + # causing the controller to infinitely requeue (every 5 seconds) when the + # description was set to nil. Not it looks like this is not needed + # anymore. + # Note(a-hilaly): Very likely the API behavior has changed and the + # late_initialize is no longer needed. + # Description: # You might be wondering why description is late-initialized, since # there isn't a default server-side value for description. @@ -165,7 +172,10 @@ resources: # we set the late initialize property of the Description field here to # override the Spec.Description to the original value we set in the # CreatePolicy *input* shape. - late_initialize: {} + #late_initialize: {} + set: + - ignore: true + method: Create Path: late_initialize: {} Tags: @@ -211,9 +221,13 @@ resources: set: # The input and output shapes are different... - from: PermissionsBoundary.PermissionsBoundaryArn + # Left for historical purposes. Description: + set: + - ignore: true + method: Create # See above in Policy resource about why this is here. - late_initialize: {} + # late_initialize: {} Path: late_initialize: {} # In order to support attaching zero or more policies to a role, we use diff --git a/pkg/resource/policy/manager.go b/pkg/resource/policy/manager.go index a900803..5f8ba4c 100644 --- a/pkg/resource/policy/manager.go +++ b/pkg/resource/policy/manager.go @@ -51,7 +51,7 @@ var ( // +kubebuilder:rbac:groups=iam.services.k8s.aws,resources=policies,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=iam.services.k8s.aws,resources=policies/status,verbs=get;update;patch -var lateInitializeFieldNames = []string{"Description", "Path"} +var lateInitializeFieldNames = []string{"Path"} // resourceManager is responsible for providing a consistent way to perform // CRUD operations in a backend AWS service API for Book custom resources. @@ -249,9 +249,6 @@ func (rm *resourceManager) incompleteLateInitialization( res acktypes.AWSResource, ) bool { ko := rm.concreteResource(res).ko.DeepCopy() - if ko.Spec.Description == nil { - return true - } if ko.Spec.Path == nil { return true } @@ -266,9 +263,6 @@ func (rm *resourceManager) lateInitializeFromReadOneOutput( ) acktypes.AWSResource { observedKo := rm.concreteResource(observed).ko.DeepCopy() latestKo := rm.concreteResource(latest).ko.DeepCopy() - if observedKo.Spec.Description != nil && latestKo.Spec.Description == nil { - latestKo.Spec.Description = observedKo.Spec.Description - } if observedKo.Spec.Path != nil && latestKo.Spec.Path == nil { latestKo.Spec.Path = observedKo.Spec.Path } diff --git a/pkg/resource/policy/sdk.go b/pkg/resource/policy/sdk.go index a315537..b245c18 100644 --- a/pkg/resource/policy/sdk.go +++ b/pkg/resource/policy/sdk.go @@ -254,11 +254,6 @@ func (rm *resourceManager) sdkCreate( } else { ko.Status.DefaultVersionID = nil } - if resp.Policy.Description != nil { - ko.Spec.Description = resp.Policy.Description - } else { - ko.Spec.Description = nil - } if resp.Policy.IsAttachable != nil { ko.Status.IsAttachable = resp.Policy.IsAttachable } else { diff --git a/pkg/resource/role/manager.go b/pkg/resource/role/manager.go index b853a1a..52cd5b7 100644 --- a/pkg/resource/role/manager.go +++ b/pkg/resource/role/manager.go @@ -51,7 +51,7 @@ var ( // +kubebuilder:rbac:groups=iam.services.k8s.aws,resources=roles,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=iam.services.k8s.aws,resources=roles/status,verbs=get;update;patch -var lateInitializeFieldNames = []string{"Description", "MaxSessionDuration", "Path"} +var lateInitializeFieldNames = []string{"MaxSessionDuration", "Path"} // resourceManager is responsible for providing a consistent way to perform // CRUD operations in a backend AWS service API for Book custom resources. @@ -249,9 +249,6 @@ func (rm *resourceManager) incompleteLateInitialization( res acktypes.AWSResource, ) bool { ko := rm.concreteResource(res).ko.DeepCopy() - if ko.Spec.Description == nil { - return true - } if ko.Spec.MaxSessionDuration == nil { return true } @@ -269,9 +266,6 @@ func (rm *resourceManager) lateInitializeFromReadOneOutput( ) acktypes.AWSResource { observedKo := rm.concreteResource(observed).ko.DeepCopy() latestKo := rm.concreteResource(latest).ko.DeepCopy() - if observedKo.Spec.Description != nil && latestKo.Spec.Description == nil { - latestKo.Spec.Description = observedKo.Spec.Description - } if observedKo.Spec.MaxSessionDuration != nil && latestKo.Spec.MaxSessionDuration == nil { latestKo.Spec.MaxSessionDuration = observedKo.Spec.MaxSessionDuration } diff --git a/pkg/resource/role/sdk.go b/pkg/resource/role/sdk.go index 13c37d9..1ce24a2 100644 --- a/pkg/resource/role/sdk.go +++ b/pkg/resource/role/sdk.go @@ -259,11 +259,6 @@ func (rm *resourceManager) sdkCreate( } else { ko.Status.CreateDate = nil } - if resp.Role.Description != nil { - ko.Spec.Description = resp.Role.Description - } else { - ko.Spec.Description = nil - } if resp.Role.MaxSessionDuration != nil { ko.Spec.MaxSessionDuration = resp.Role.MaxSessionDuration } else { diff --git a/test/e2e/resources/policy_no_description.yaml b/test/e2e/resources/policy_no_description.yaml new file mode 100644 index 0000000..1ae2c37 --- /dev/null +++ b/test/e2e/resources/policy_no_description.yaml @@ -0,0 +1,7 @@ +apiVersion: iam.services.k8s.aws/v1alpha1 +kind: Policy +metadata: + name: $POLICY_NAME +spec: + name: $POLICY_NAME + policyDocument: '{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:ListAllMyBuckets","Resource":"arn:aws:s3:::*"},{"Effect":"Allow","Action":["s3:List*"],"Resource":["*"]}]}' \ No newline at end of file diff --git a/test/e2e/resources/role_no_description.yaml b/test/e2e/resources/role_no_description.yaml new file mode 100644 index 0000000..c352cd6 --- /dev/null +++ b/test/e2e/resources/role_no_description.yaml @@ -0,0 +1,19 @@ +apiVersion: iam.services.k8s.aws/v1alpha1 +kind: Role +metadata: + name: $ROLE_NAME +spec: + name: $ROLE_NAME + assumeRolePolicyDocument: > + { + "Version":"2012-10-17", + "Statement": [{ + "Effect":"Allow", + "Principal": { + "Service": [ + "ec2.amazonaws.com" + ] + }, + "Action": ["sts:AssumeRole"] + }] + } diff --git a/test/e2e/tests/test_descriptions.py b/test/e2e/tests/test_descriptions.py new file mode 100644 index 0000000..9cbd7f9 --- /dev/null +++ b/test/e2e/tests/test_descriptions.py @@ -0,0 +1,155 @@ +# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"). You may +# not use this file except in compliance with the License. A copy of the +# License is located at +# +# http://aws.amazon.com/apache2.0/ +# +# or in the "license" file accompanying this file. This file is distributed +# on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +# express or implied. See the License for the specific language governing +# permissions and limitations under the License. + +"""Integration tests for resource description fields +The reason we are dedicating a test file for this is because the description/lateinitialize +bug is the most common bug we have seen in the ACK project. This test file is dedicated +to testing the description field for policy and role resources. + +See bugs: +- +""" + +import json +import time + +import pytest + +from acktest.k8s import condition +from acktest.k8s import resource as k8s +from acktest.resources import random_suffix_name +from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_resource +from e2e.common.types import POLICY_RESOURCE_PLURAL +from e2e.replacement_values import REPLACEMENT_VALUES +from e2e import policy +from e2e import role + +DELETE_WAIT_SECONDS = 10 +CREATE_WAIT_SECONDS = 10 +MODIFY_WAIT_SECONDS = 10 + + +@pytest.fixture(scope="module") +def policy_with_no_description(): + policy_name = random_suffix_name("my-simple-policy", 24) + + replacements = REPLACEMENT_VALUES.copy() + replacements['POLICY_NAME'] = policy_name + + resource_data = load_resource( + "policy_no_description", + additional_replacements=replacements, + ) + + ref = k8s.CustomResourceReference( + CRD_GROUP, CRD_VERSION, POLICY_RESOURCE_PLURAL, + policy_name, namespace="default", + ) + k8s.create_custom_resource(ref, resource_data) + cr = k8s.wait_resource_consumed_by_controller(ref) + + cr = k8s.get_resource(ref) + assert cr is not None + assert 'status' in cr + assert 'ackResourceMetadata' in cr['status'] + assert 'arn' in cr['status']['ackResourceMetadata'] + policy_arn = cr['status']['ackResourceMetadata']['arn'] + + policy.wait_until_exists(policy_arn) + + assert cr is not None + assert k8s.get_resource_exists(ref) + + yield (ref, cr) + + _, deleted = k8s.delete_custom_resource( + ref, + period_length=DELETE_WAIT_SECONDS, + ) + assert deleted + + policy.wait_until_deleted(policy_arn) + +@pytest.fixture(scope="module") +def role_with_no_description(): + role_name = random_suffix_name("my-simple-role", 24) + + replacements = REPLACEMENT_VALUES.copy() + replacements['ROLE_NAME'] = role_name + + resource_data = load_resource( + "role_no_description", + additional_replacements=replacements, + ) + + ref = k8s.CustomResourceReference( + CRD_GROUP, CRD_VERSION, "roles", + role_name, namespace="default", + ) + k8s.create_custom_resource(ref, resource_data) + cr = k8s.wait_resource_consumed_by_controller(ref) + + role.wait_until_exists(role_name) + + assert cr is not None + assert k8s.get_resource_exists(ref) + + yield (ref, cr) + + _, deleted = k8s.delete_custom_resource( + ref, + period_length=DELETE_WAIT_SECONDS, + ) + assert deleted + + role.wait_until_deleted(role_name) + +@service_marker +@pytest.mark.canary +class TestRole: + def test_role_empty_description(self, role_with_no_description): + ref, res = role_with_no_description + role_name = ref.name + + time.sleep(CREATE_WAIT_SECONDS) + condition.assert_synced(ref) + condition.assert_type_status( + ref, + cond_type_match=condition.CONDITION_TYPE_LATE_INITIALIZED, + cond_status_match=True, + ) + + updates = { + "spec": { + "description": "non empty description", + }, + } + k8s.patch_custom_resource(ref, updates) + time.sleep(MODIFY_WAIT_SECONDS) + condition.assert_synced(ref) + + latest = role.get(role_name) + assert latest is not None + assert latest["Description"] == "non empty description" + + def test_policy_empty_description(self, policy_with_no_description): + ref, res = policy_with_no_description + + time.sleep(CREATE_WAIT_SECONDS) + condition.assert_synced(ref) + condition.assert_type_status( + ref, + cond_type_match=condition.CONDITION_TYPE_LATE_INITIALIZED, + cond_status_match=True, + ) + diff --git a/test/e2e/tests/test_policy.py b/test/e2e/tests/test_policy.py index a51fcfb..ed8637a 100644 --- a/test/e2e/tests/test_policy.py +++ b/test/e2e/tests/test_policy.py @@ -109,6 +109,8 @@ def test_crud(self, simple_policy): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + latest_tags = policy.get_tags(policy_arn) after_update_expected_tags = [ { @@ -129,6 +131,8 @@ def test_crud(self, simple_policy): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + latest_tags = policy.get_tags(policy_arn) after_update_expected_tags = [ { diff --git a/test/e2e/tests/test_role.py b/test/e2e/tests/test_role.py index 0d15204..8ec8626 100644 --- a/test/e2e/tests/test_role.py +++ b/test/e2e/tests/test_role.py @@ -38,7 +38,6 @@ @pytest.fixture(scope="module") def simple_role(): role_name = random_suffix_name("my-simple-role", 24) - role_desc = "a simple role" replacements = REPLACEMENT_VALUES.copy() replacements['ROLE_NAME'] = role_name @@ -109,14 +108,20 @@ def test_crud(self, simple_role): # wait some time and verify that the IAM server-side resource # shows the new value of the field. updates = { - "spec": {"maxSessionDuration": new_max_sess_duration}, + "spec": { + "maxSessionDuration": new_max_sess_duration, + "description": "a simple role with a new max session duration", + }, } k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + latest = role.get(role_name) assert latest is not None assert latest['MaxSessionDuration'] == new_max_sess_duration + assert latest["Description"] == "a simple role with a new max session duration" # Test the code paths that synchronize the attached policies for a role policy_arns = [ @@ -132,6 +137,8 @@ def test_crud(self, simple_role): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + latest_policy_arns = role.get_attached_policy_arns(role_name) assert latest_policy_arns == policy_arns @@ -159,6 +166,8 @@ def test_crud(self, simple_role): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + after_update_expected_tags = [ { "Key": "tag2", @@ -262,6 +271,8 @@ def test_crud(self, simple_role): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + cr = k8s.get_resource(ref) assert cr is not None assert "spec" in cr @@ -317,6 +328,8 @@ def test_crud(self, simple_role): k8s.patch_custom_resource(ref, updates) time.sleep(MODIFY_WAIT_AFTER_SECONDS) + condition.assert_synced(ref) + cr = k8s.get_resource(ref) assert cr is not None assert 'spec' in cr