Access Log Policy Controller #424

xWink · 2023-10-04T22:30:40Z

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:

Adds new controller for AccessLogPolicy
- This controller enables creating and deleting AccessLogPolicy resources, and appropriately sets statuses and finalizers. It does not yet support creating VPC Lattice Access Log Subscriptions, and thus has no real functionality for customers, but that will come in bite-sized followup PRs.
Adds example AccessLogPolicy yaml

Output of `kubectl describe alp test-policy` after creation:

Name:         test-policy
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  application-networking.k8s.aws/v1alpha1
Kind:         AccessLogPolicy
Metadata:
  Creation Timestamp:  2023-10-05T02:14:37Z
  Finalizers:
    accesslogpolicy.k8s.aws/resources
  Generation:        1
  Resource Version:  24456853
  UID:               a67bb8b1-7744-4380-a22c-f35bdee92f3f
Spec:
  Destination Arn:  arn:aws:s3:::my-bucket
  Target Ref:
    Group:  gateway.networking.k8s.io
    Kind:   Gateway
    Name:   my-hotel
Status:
  Conditions:
    Last Transition Time:  2023-10-05T02:14:37Z
    Message:               application-networking.k8s.aws/gateway-api-controller
    Observed Generation:   1
    Reason:                Accepted
    Status:                True
    Type:                  Accepted
Events:                    <none>

Output after updating the destinationArn

Name:         test-policy
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  application-networking.k8s.aws/v1alpha1
Kind:         AccessLogPolicy
Metadata:
  Creation Timestamp:  2023-10-05T02:14:37Z
  Finalizers:
    accesslogpolicy.k8s.aws/resources
  Generation:        2
  Resource Version:  24584481
  UID:               a67bb8b1-7744-4380-a22c-f35bdee92f3f
Spec:
  Destination Arn:  arn:aws:s3:::my-bucket-1
  Target Ref:
    Group:  gateway.networking.k8s.io
    Kind:   Gateway
    Name:   my-hotel
Status:
  Conditions:
    Last Transition Time:  2023-10-05T02:14:37Z
    Message:               application-networking.k8s.aws/gateway-api-controller
    Observed Generation:   2
    Reason:                Accepted
    Status:                True
    Type:                  Accepted
Events:                    <none>

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 32 of 32 Specs in 2227.128 seconds
SUCCESS! -- 32 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2228.35s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   2229.062s

Automation added to e2e:

Will this PR introduce any new dependencies?:

No

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

Does this PR introduce any user-facing change?:

Not yet, since use of the AccessLogPolicy doesn't achieve anything

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

…oute reconciler info logs, remove use of struct embedding in logs

…alidation

controllers/accesslogpolicy_controller.go

zijun726911 · 2023-10-05T16:10:56Z

Before, we handle TargetGroupPolicy (model build and stack deploy) as part of route_controller.go and handle VpcAssociation as part of gateway_controller.go. How we decide to handle XXXXXPolicy in the xxxxx_policy_controller or in the existing xxx_k8s_resource_controller?

xWink · 2023-10-05T16:54:35Z

Before, we handle TargetGroupPolicy (model build and stack deploy) as part of route_controller.go and handle VpcAssociation as part of gateway_controller.go. How we decide to handle XXXXXPolicy in the xxxxx_policy_controller or in the existing xxx_k8s_resource_controller?

We opted to handle AccessLogPolicy as its own standalone resource in the Kubernetes setting because it's its own standalone resource in the VPC Lattice setting. Since Access Log Subscription has its own lifecycle, so should Access Log Policy.

This simplifies handling some of the create/delete/update cases vs. only adding event handling to existing controllers, which would increase their complexities substantially.

erikfuller · 2023-10-05T16:57:24Z

examples/access-log-policy-example.yaml

+    group: gateway.networking.k8s.io
+    kind: Gateway
+    name: my-hotel
+  destinationArn: "arn:aws:s3:::my-bucket"


Just a general question on the feature. How are we planning on reporting back the actual log paths? From what I recall, logs are based on unique resource ids, not names. How will folks using the controller know which logs apply to which k8s resources?

Not 100% sure I follow, so please correct me if I'm going down the wrong path here. When a user creates a Gateway or Route, that resource is associated to 1 VPC Lattice Service Network or Service. When they create an AccessLogPolicy with a targetRef of a Gateway or Route, it's the equivalent of creating a VPC Lattice Access Log Subscription for the corresponding ServiceNetwork or Service.

So if I create Gateway my-hotel, which in VPC Lattice is Service Network sn-12345678901234567, and then create an AccessLogPolicy which refers to Gateway my-hotel, then it will create an AccessLogSubscription with resourceIdentifier sn-12345678901234567, and any access logs flowing through that Service Network will wind up in the log destination I specified in the AccessLogPolicy.

Right. So the path in S3 would look like [1]:

[bucket]/[prefix]/AWSLogs/[accountId]/VpcLattice/AccessLogs/[region]/[YYYY/MM/DD]/[resource-id]/[accountId]_VpcLatticeAccessLogs_[region]_[resource-id]_YYYYMMDDTHHmmZ_[hash].json.gz

Let's assume I have a number of gateways I'm managing - how will I know the Lattice resource ID for each? For example, do we patch the gateway with the Lattice service network ID? I see it as a message here: https://github.com/aws/aws-application-networking-k8s/blob/main/controllers/gateway_controller.go#L279. Ideally, I would be able to find out the ID by doing something like kubectl get gateway.

Similarly, it may be difficult to map from a k8s route to a log line, or from a log line back to the k8s objects, since these log entries also use unique IDs rather than k8s-based names. Maybe for now just enabling the logs is enough, then customers can use the Lattice API out of band to determine the ID. Longer term we should look a improving this experience.

Patch the status(or annotation) of gateway and k8sService resource with lattice resource id and Patch AccessLogPolicy with als id seems a good approach to make mapping cleaner

Agreed, this would absolutely improve clarity for users. I'll look into making this an annotation for ALP (don't think status is the right place for this kind of data), and will add it as a followup once the Create workflow is done. We can also create an issue to add this behaviour to other resources, like Gateway and Route.

xWink · 2023-10-05T22:47:05Z

Are there any remaining concerns regarding this PR that need addressing?

zijun726911

LGTM

Shawn Kaplan added 10 commits October 2, 2023 17:53

Change unwanted error logs to debug, remove unwanted info logs, add r…

9a4e9e7

…oute reconciler info logs, remove use of struct embedding in logs

Added AccessLogPolicy CRD

833839a

Merge branch 'main' into access-logs-crd

309cfc5

Fixed alp shortname and destinationArn json, plus added kubebuilder v…

f322ba4

…alidation

Added missing yaml changes

a26ce02

added initial access log policy controller

6beaf45

Fixed documentation for support access log policy target ref kinds

6e0b988

Merge branch 'access-logs-crd' into access-logs-controller

f91f62a

Finished basic access log policy reconciliation with statuses

6c8346a

Merge branch 'main' into access-logs-controller

2697b16

xWink changed the title ~~Access logs controller~~ Access Log Policy Controller Oct 4, 2023

scottlaiaws reviewed Oct 4, 2023

View reviewed changes

controllers/accesslogpolicy_controller.go Show resolved Hide resolved

erikfuller reviewed Oct 5, 2023

View reviewed changes

zijun726911 approved these changes Oct 5, 2023

View reviewed changes

xWink merged commit 165be2c into aws:main Oct 5, 2023
5 checks passed

xWink deleted the access-logs-controller branch October 5, 2023 22:51

xWink mentioned this pull request Oct 11, 2023

Support Creating Access Log Policies #430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access Log Policy Controller #424

Access Log Policy Controller #424

xWink commented Oct 4, 2023 •

edited

Loading

zijun726911 commented Oct 5, 2023

xWink commented Oct 5, 2023 •

edited

Loading

erikfuller Oct 5, 2023

xWink Oct 5, 2023

erikfuller Oct 5, 2023

zijun726911 Oct 5, 2023 •

edited

Loading

xWink Oct 5, 2023 •

edited

Loading

xWink commented Oct 5, 2023

zijun726911 left a comment

Access Log Policy Controller #424

Access Log Policy Controller #424

Conversation

xWink commented Oct 4, 2023 • edited Loading

Output of kubectl describe alp test-policy after creation:

Output after updating the destinationArn

zijun726911 commented Oct 5, 2023

xWink commented Oct 5, 2023 • edited Loading

erikfuller Oct 5, 2023

Choose a reason for hiding this comment

xWink Oct 5, 2023

Choose a reason for hiding this comment

erikfuller Oct 5, 2023

Choose a reason for hiding this comment

zijun726911 Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

xWink Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

xWink commented Oct 5, 2023

zijun726911 left a comment

Choose a reason for hiding this comment

xWink commented Oct 4, 2023 •

edited

Loading

Output of `kubectl describe alp test-policy` after creation:

xWink commented Oct 5, 2023 •

edited

Loading

zijun726911 Oct 5, 2023 •

edited

Loading

xWink Oct 5, 2023 •

edited

Loading