Added Support for Updating Access Log Policies #442

xWink · 2023-10-19T02:53:46Z

What type of PR is this?

feature

Which issue does this PR fix:
#200 partially

What does this PR do / Why do we need it:

Enables support for updating Access Log Policies (ALPs)
- When an ALP is updated to have a different destinationArn of the same destination type (i.e. S3 Bucket, CloudWatch Log Group, or Firehose Delivery Stream), the underlying Access Log Subscription (ALS) is updated
- When an ALP is updated to have a different destinationArn of a different destination type, a new ALS is created to replace it, then the previous one is deleted
  - When an ALS is replaced, the ALP's VpcLatticeAccessLogSubscription annotation is updated with the new ARN
- When an ALP is updated to have a different targetRef, a new ALS is created to replace the existing one, then the old one is deleted and the ALP's VpcLatticeAccessLogSubscription annotation is updated
- When an ALP is updated to have a destinationArn for a destination that does not exist, the status of the ALP is set to Invalid
- When an ALP is updated to have a targetRef for a non-existent target, the status of the ALP is set to TargetNotFound
- When an ALP is updated to have a destinationArn for a destination type that is already in use for that target, the status of the ALP is set to Conflicted
- Whenever an ALP update fails, the underlying ALS is not modified
  - This decision was made to reduce blast radius of mistaken updates, users should not lose their monitoring because of a typo in ALP spec
  - Status will still indicate the state of the current ALP configuration, regardless of underlying ALS
  - User can update the ALP again to fix it, or delete/recreate if they prefer

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

Ran 47 of 47 Specs in 3079.116 seconds
SUCCESS! -- 47 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (3080.10s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   3081.006s

Automation added to e2e:

New test: update properly changes or replaces Access Log Subscription and sets Access Log Policy status

This runs all the documented update scenarios and verifies that the controller behaves as expected for each of them (as described above)

Will this PR introduce any new dependencies?:

No new dependencies, but one new CloudWatch Log Group is created/deleted as part of the e2e test. This is needed since we need two different destinations of the same type to verify the conflict scenario.

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No.

Does this PR introduce any user-facing change?:

Yes, but release note will be added in the final PR, which will include the API spec

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pkg/deploy/lattice/access_log_subscription_synthesizer.go

coveralls · 2023-10-19T03:00:00Z

Pull Request Test Coverage Report for Build 6593477191

123 of 147 (83.67%) changed or added relevant lines in 4 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 45.779%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/accesslogpolicy_controller.go	0	6	0.0%
pkg/deploy/lattice/access_log_subscription_manager.go	105	123	85.37%

Files with Coverage Reduction	New Missed Lines	%
pkg/deploy/lattice/access_log_subscription_manager.go	2	86.34%

Totals
Change from base Build 6569460474:	0.4%
Covered Lines:	4100
Relevant Lines:	8956

💛 - Coveralls

mikhail-aws · 2023-10-19T03:06:01Z

pkg/deploy/lattice/access_log_subscription_manager.go

+	if err != nil {
+		switch e := err.(type) {
+		case *vpclattice.AccessDeniedException:
+			return nil, services.NewInvalidError(e.Message())
+		case *vpclattice.ResourceNotFoundException:
+			if *e.ResourceType == "SERVICE_NETWORK" || *e.ResourceType == "SERVICE" {
+				return nil, services.NewNotFoundError(string(accessLogSubscription.Spec.SourceType), accessLogSubscription.Spec.SourceName)
+			}
+			return nil, services.NewInvalidError(e.Message())
+		case *vpclattice.ConflictException:
+			/*
+			 * A conflict can happen when the destination type of the new ALS is different from the original.
+			 * To gracefully handle this, we create a new ALS with the new destination, then delete the old one.
+			 */
+			alsStatus, err := m.Create(ctx, accessLogSubscription)
+			if err != nil {
+				return nil, err
+			}
+			err = m.Delete(ctx, accessLogSubscription)
+			if err != nil {
+				return nil, err
+			}
+			return alsStatus, nil
+		}
+	}
+
+	return &lattice.AccessLogSubscriptionStatus{
+		Arn: *updateALSOutput.Arn,
+	}, nil


Since you are getting into de-nesting code. You can change this one too.

if err == nil { return &lattice.AccessLogSubscriptionStatus{ Arn: *updateALSOutput.Arn, }, nil } swtich ...

Good point, will do

xWink · 2023-10-19T16:32:27Z

Had to fixed some git issues, but commit history should be cleaner now. Addressed above comments.

mikhail-aws · 2023-10-19T20:25:40Z

pkg/deploy/lattice/access_log_subscription_manager_test.go

+			ResourceType: aws.String("ACCESS_LOG_SUBSCRIPTION"),
+		}
+
+		mockLattice.EXPECT().UpdateAccessLogSubscriptionWithContext(ctx, updateALSInput).Return(nil, updateALSError)


Just a thought, which can reduce code for unit tests, makes it easier to read and change in future.

In this mock we do 2 things, 1. assert input into lattice client is correct and 2. it produces output which use later in manager.

And every test here asserts that input to lattice is correct. I dont think we need that, we can create a single test that makes sure we call Lattice with expected fields populated. For remaining tests we use any() in place of input. For example:

mockLattice.EXPECT(). FindService(gomock.Any(), gomock.Any()). Return(&vpclattice.ServiceSummary{ Arn: aws.String("svc-arn"), Id: aws.String("svc-id"), Name: aws.String(svc.LatticeServiceName()), }, nil)

it was a minor comment, I'm ok with current version

I think I prefer the more explicit nature of the existing code

mikhail-aws · 2023-10-19T20:33:32Z

pkg/deploy/lattice/access_log_subscription_manager.go

+		/*
+		 * A conflict can happen when the destination type of the new ALS is different from the original.
+		 * To gracefully handle this, we create a new ALS with the new destination, then delete the old one.
+		 */
+		alsStatus, err := m.Create(ctx, accessLogSubscription)
+		if err != nil {
+			return nil, err
+		}
+		err = m.Delete(ctx, accessLogSubscription)
+		if err != nil {
+			return nil, err
+		}


Looks like we create and delete same accessLogSubscription, not "new" and "old" ones.

Create uses the ALS Spec to call VPC Lattice Create API

Delete uses the ARN of the ALS to call VPC Lattice Delete API

I could change Delete function to simply accept an ARN, I think that would make more sense anyways

…cy targetRef

xWink · 2023-10-20T23:01:43Z

New functionality:

For create/update, if namespace of the policy does not match namespace of the targetRef, policy status is set to Invalid
For update, previously, targetRef changes were ignored, this was a bug and is now fixed
- Changing targetRef results in creating a new ALS and deleting the old one
e2e tests were added for both new behaviours and pass:

Ran 47 of 47 Specs in 3026.234 seconds
SUCCESS! -- 47 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (3027.48s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   3028.331s

xWink requested review from mikhail-aws, solmonk and zijun726911 October 19, 2023 02:54

xWink commented Oct 19, 2023

View reviewed changes

pkg/deploy/lattice/access_log_subscription_synthesizer.go Outdated Show resolved Hide resolved

mikhail-aws reviewed Oct 19, 2023

View reviewed changes

xWink force-pushed the update-alp branch from 8c06ded to 6b73202 Compare October 19, 2023 16:21

Added support for updating Access Log Policy

de37839

xWink force-pushed the update-alp branch from 6b73202 to de37839 Compare October 19, 2023 16:31

mikhail-aws suggested changes Oct 19, 2023

View reviewed changes

Addressed PR comments, added update functionality for Access Log Poli…

c67790d

…cy targetRef

mikhail-aws approved these changes Oct 23, 2023

View reviewed changes

xWink merged commit 48d984a into aws:main Oct 23, 2023
5 checks passed

xWink deleted the update-alp branch October 23, 2023 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Support for Updating Access Log Policies #442

Added Support for Updating Access Log Policies #442

xWink commented Oct 19, 2023 •

edited

Loading

coveralls commented Oct 19, 2023 •

edited

Loading

mikhail-aws Oct 19, 2023

xWink Oct 19, 2023

xWink commented Oct 19, 2023 •

edited

Loading

mikhail-aws Oct 19, 2023

mikhail-aws Oct 19, 2023

xWink Oct 20, 2023

mikhail-aws Oct 19, 2023 •

edited

Loading

xWink Oct 20, 2023

xWink commented Oct 20, 2023 •

edited

Loading

Added Support for Updating Access Log Policies #442

Added Support for Updating Access Log Policies #442

Conversation

xWink commented Oct 19, 2023 • edited Loading

coveralls commented Oct 19, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6593477191

💛 - Coveralls

mikhail-aws Oct 19, 2023

Choose a reason for hiding this comment

xWink Oct 19, 2023

Choose a reason for hiding this comment

xWink commented Oct 19, 2023 • edited Loading

mikhail-aws Oct 19, 2023

Choose a reason for hiding this comment

mikhail-aws Oct 19, 2023

Choose a reason for hiding this comment

xWink Oct 20, 2023

Choose a reason for hiding this comment

mikhail-aws Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

xWink Oct 20, 2023

Choose a reason for hiding this comment

xWink commented Oct 20, 2023 • edited Loading

xWink commented Oct 19, 2023 •

edited

Loading

coveralls commented Oct 19, 2023 •

edited

Loading

xWink commented Oct 19, 2023 •

edited

Loading

mikhail-aws Oct 19, 2023 •

edited

Loading

xWink commented Oct 20, 2023 •

edited

Loading