Add VpcAssociationPolicy doc #411

zijun726911 · 2023-09-26T20:44:47Z

What type of PR is this?

Which issue does this PR fix:

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

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

Testing done on this change:

Automation added to e2e:

Will this PR introduce any new dependencies?:

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

Does this PR introduce any user-facing change?:

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

solmonk · 2023-09-27T17:09:49Z

Haven't had a chance to review the doc yet, but please make sure any new doc change is included in mkdocs.yml to add them on the website.

mrjustind · 2023-09-27T22:58:16Z

docs/reference/vpc-association-policy.md

+
+| Source                      | Protocol                                            | Port Range                                      | Comment                                                   |
+|-----------------------------|-----------------------------------------------------|-------------------------------------------------|-----------------------------------------------------------|
+| Kubernetes cluster VPC CIDR | protocols defined in the gateway's listener section | ports defined in the gateway's listener section | Allow inbound traffic from current cluster vpc to gateway |


Can we add - "or security group reference" - Incase they don't want to use CIDR/IP addresses.

coveralls · 2023-09-27T23:43:48Z

Pull Request Test Coverage Report for Build 6358392373

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 46.026%

Totals
Change from base Build 6355409430:	0.0%
Covered Lines:	3874
Relevant Lines:	8417

💛 - Coveralls

docs/reference/vpc-association-policy.md

xWink · 2023-09-28T00:07:24Z

docs/reference/vpc-association-policy.md

+
+When attaching a policy to a resource, the following restrictions apply:
+
+* A policy can be only attached to `Gateway` resources.


It's typically advised to not use the word "only" when describing limitations.

Alternative wording would be "Policies must be attached to Gateway resources."

xWink · 2023-09-28T00:07:43Z

docs/reference/vpc-association-policy.md

+When attaching a policy to a resource, the following restrictions apply:
+
+* A policy can be only attached to `Gateway` resources.
+* The attached resource should exist in the same namespace as the policy resource.


The attached resource must exist in the same namespace as the policy resource ("should" sounds suggestive, rather than mandatory).

xWink · 2023-09-28T00:09:18Z

docs/reference/vpc-association-policy.md

+The security Group will not take effect if:
+
+* The targetRef `Gateway` does not exist
+* AssociateWithVpc field set to false


What AssociateWithVpc field?

Also, grammar nitpick: "associateWithVpc is set to false"

xWink · 2023-09-28T00:10:44Z

docs/reference/vpc-association-policy.md

+
+**WARNING**
+
+Current VPC Lattice updateServiceNetworkVpcAssociation api have a limitation that it cannot remove all security groups.


"The VPC Lattice UpdateServiceNetworkVpcAssociation API cannot be used to remove all security groups."

xWink · 2023-09-28T00:12:02Z

docs/reference/vpc-association-policy.md

+Current VPC Lattice updateServiceNetworkVpcAssociation api have a limitation that it cannot remove all security groups.
+That means, if you have a VpcAssociationPolicy attached to a gateway that already applied security groups, following operations will NOT take effect to remove the security groups:
+* Update the VPCAssociationPolicy to empty the security group ids (even though the updated VPCAssociationPolicy can be accepted by the API server)
+* Delete the VPCAssociationPolicy (even though the VPCAssociationPolicy can be deleted from k8s successfully)


Just to clarify, if the customer tries these steps, it will NOT work? If so, we should not list them as part of the documentation at all, and instead just give guidance on what they should do.

xWink · 2023-09-28T00:12:54Z

docs/reference/vpc-association-policy.md

+* Delete the VPCAssociationPolicy (even though the VPCAssociationPolicy can be deleted from k8s successfully)
+
+To remove security groups, instead, you should delete VPC Association and then create a new VPC Association without security group ids by following steps:
+1. Update the VPCAssociationPolicy with AssociateWithVpc is false and empty security group ids


"Update the VPCAssociationPolicy by setting associateWithVpc to false and with empty security group ids"

docs/reference/vpc-association-policy.md

xWink · 2023-09-28T00:18:35Z

docs/reference/vpc-association-policy.md

+1. Update the VPCAssociationPolicy with AssociateWithVpc is false and empty security group ids
+2. Update the VPCAssociationPolicy with AssociateWithVpc is true and empty security group ids
+
+Be cautious to set AssociateWithVpc to false. That can break traffic from the current cluster workloads to the gateway.


Perhaps this should be a note that simply says "Note: Setting associateWithVpc to false will disable traffic from the current cluster workloads to the gateway."

xWink · 2023-09-28T00:19:13Z

docs/reference/vpc-association-policy.md

+Be cautious to set AssociateWithVpc to false. That can break traffic from the current cluster workloads to the gateway.
+
+
+| Field	                                                                                                                      | Description	                                        |


This table needs a title

xWink · 2023-09-28T00:35:45Z

docs/reference/vpc-association-policy.md

+
+
+
+| Field	                                                                                                     | Description                                                                                                                                                                                                                |


Possibly a Required column would help with clarifying whether a field is optional, and a Type column would make it easier to see what type of property it is, instead of lumping it in with the property name

docs/reference/vpc-association-policy.md

xWink

Great start, just needs some grammar and format cleanup.

xWink · 2023-09-29T17:37:18Z

docs/reference/vpc-association-policy.md

+
+### Fields of VpcAssociationPolicy
+
+| Field Name	                                                                                                        | Required  | Description	                                        |


Would be good to have a Type column for easier reading

xWink · 2023-09-29T17:39:44Z

docs/reference/vpc-association-policy.md

+
+| Field Name	        | Type                                                                                          | Required | Description                                                                                                                                                                                                                                                                                                       |
+|--------------------|-----------------------------------------------------------------------------------------------|----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `targetRef`        | *[PolicyTargetReference](https://gateway-api.sigs.k8s.io/geps/gep-713/#policy-targetref-api)* | Yes	     | TargetRef points to the kubernetes `Gateway` resource that will have this policy attached. This field is following the guidelines of Kubernetes Gateway API policy attachment.                                                                                                                                    |


"This field is following the guidelines of Kubernetes Gateway API policy attachment." can be shortened to "Follows the guidelines of Kubernetes Gateway API policy attachment", but do we need this sentence if we already link to it in the hyperlink on PolicyTargetReference, or is this a different guideline? If it is a different guideline, we should hyperlink to it

xWink · 2023-09-29T17:40:23Z

docs/reference/vpc-association-policy.md

+|--------------------|-----------------------------------------------------------------------------------------------|----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `targetRef`        | *[PolicyTargetReference](https://gateway-api.sigs.k8s.io/geps/gep-713/#policy-targetref-api)* | Yes	     | TargetRef points to the kubernetes `Gateway` resource that will have this policy attached. This field is following the guidelines of Kubernetes Gateway API policy attachment.                                                                                                                                    |
+| `associateWithVpc` | *bool*	                                                                                       | No       | Indicates whether the targetRef Gateway is associated with the current k8s cluster VPC. By default, the Gateway API controller sets this to true if it's not defined in VpcAssociationPolicy.                                                                                                                     |
+| `securityGroupIds` | *string[]*	                                                                                   | No       | Defines security groups applied to the gateway (ServiceNetworkVpcAssociation), it controls the inbound traffic from current cluster workloads to the gateway listeners. Please check the [VPC lattice doc](https://docs.aws.amazon.com/vpc-lattice/latest/ug/security-groups.html) for more detail of this field. |


VPC Lattice (capitalize the L)

xWink · 2023-09-29T17:40:55Z

docs/reference/vpc-association-policy.md

+* Policies must be attached to *Gateway* resource.
+* The attached resource must exist in the same namespace as the policy resource.
+
+The security Group will not take effect if:


security group (lowercase S, keep capitalization consistent)

xWink · 2023-09-29T17:41:54Z

docs/reference/vpc-association-policy.md

+The security Group will not take effect if:
+
+* The targetRef `Gateway` does not exist
+* AssociateWithVpc field set to false


The associateWithVpc field is set to false

xWink · 2023-09-29T17:42:46Z

docs/reference/vpc-association-policy.md

+**WARNING**
+
+The VPC Lattice `UpdateServiceNetworkVpcAssociation` API cannot be used to remove all security groups.
+That means, if you have a VpcAssociationPolicy attached to a gateway that already applied security groups, update the VPCAssociationPolicy with empty security group ids or delete the whole VPCAssociationPolicy will NOT remove the security groups from this gateway.


VpcAssociationPolicy should be capitalized consistently (there are a bunch of places here and below where it's written as VPCAssociationPolicy)

xWink · 2023-09-29T17:44:29Z

docs/reference/vpc-association-policy.md

+**WARNING**
+
+The VPC Lattice `UpdateServiceNetworkVpcAssociation` API cannot be used to remove all security groups.
+That means, if you have a VpcAssociationPolicy attached to a gateway that already applied security groups, update the VPCAssociationPolicy with empty security group ids or delete the whole VPCAssociationPolicy will NOT remove the security groups from this gateway.


"That means" is redundant. Can replace sentence with "If you have a VpcAssociationPolicy attached to a gateway that already has security groups applied, updating the VpcAssociationPolicy with empty security group ids or deleting the VpcAssociationPolicy will NOT remove the security groups from the gateway."

xWink · 2023-09-29T18:05:12Z

docs/reference/vpc-association-policy.md

+1. Update the VPCAssociationPolicy by setting associateWithVpc to false and empty security group ids
+2. Update the VPCAssociationPolicy by setting associateWithVpc to true and empty security group ids
+
+Be cautious to set AssociateWithVpc to false. It will disable traffic from the current cluster workloads to the gateway.


"Note: Setting associateWithVpc to false will disable traffic from the current cluster workloads to the gateway"

xWink · 2023-09-29T18:06:35Z

docs/reference/vpc-association-policy.md

+
+## Example Configuration
+
+This example shows how to configure a Gateway with associateWithVpc set to true and apply security group sg-1234567890 and sg-0987654321 


Let's stay consistent with capitalization of words like "gateway". I see it always lowercase above.

Also, let's wrap fields as inline code (i.e. associateWithVpc).

xWink

Few more changes and it'll be spotless :)

zijun726911 · 2023-09-29T23:45:50Z

@xWink Thanks for your detailed comments! I think I should have addressed all of them, please review again when you have a time, thank you!

xWink · 2023-10-03T18:46:50Z

docs/reference/vpc-association-policy.md

+
+### Fields of VpcAssociationPolicy
+
+| Field Name	  | Type                                                                                                    |           | Required                                            | Description	 |


Seems to be an extra | character here, breaking the table

xWink · 2023-10-03T18:49:02Z

docs/reference/vpc-association-policy.md

+
+The security group will not take effect if:
+
+* The targetRef `gateway` does not exist.


targetRef is the field that should be inline code, not gateway

xWink · 2023-10-03T18:50:24Z

docs/reference/vpc-association-policy.md

+
+| Field Name	        | Type                                                                                          | Required | Description                                                                                                                                                                                                                                                                                         |
+|--------------------|-----------------------------------------------------------------------------------------------|----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `targetRef`        | *[PolicyTargetReference](https://gateway-api.sigs.k8s.io/geps/gep-713/#policy-targetref-api)* | Yes	     | TargetRef points to the kubernetes `Gateway` resource that will have this policy attached. It follows the guidelines of Kubernetes Gateway API policy attachment                                                                                                                                    |


This sentence could flow better like so:

Points to the Kubernetes Gateway resource that will have this policy attached, following the guidelines of [Kubernetes Gateway API policy attachment](link-to-guidelines).

xWink

One more fix and a couple of nitpicks!

xWink

Stunning!

Add VpcAssociationPolicy doc

5f8bfef

zijun726911 requested review from mikhail-aws and solmonk September 26, 2023 20:44

zijun726911 force-pushed the vpc_association_policy_doc branch from 5f8bfef to aae22e7 Compare September 27, 2023 21:30

Address PR comments

418f164

zijun726911 force-pushed the vpc_association_policy_doc branch from aae22e7 to 418f164 Compare September 27, 2023 21:32

mrjustind reviewed Sep 27, 2023

View reviewed changes

Address PR comments

369d708

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink reviewed Sep 28, 2023

View reviewed changes

docs/reference/vpc-association-policy.md Outdated Show resolved Hide resolved

xWink suggested changes Sep 28, 2023

View reviewed changes

Address PR comments

e3dbb7b

xWink reviewed Sep 29, 2023

View reviewed changes

xWink suggested changes Sep 29, 2023

View reviewed changes

zijun726911 force-pushed the vpc_association_policy_doc branch from 84fddff to 5dee6a3 Compare September 29, 2023 23:41

Address PR comments

0fe3258

zijun726911 force-pushed the vpc_association_policy_doc branch from 5dee6a3 to 0fe3258 Compare September 29, 2023 23:44

xWink reviewed Oct 3, 2023

View reviewed changes

xWink suggested changes Oct 3, 2023

View reviewed changes

Address more PR comments

574751d

xWink approved these changes Oct 3, 2023

View reviewed changes

zijun726911 merged commit 3e156e8 into aws:main Oct 3, 2023

zijun726911 deleted the vpc_association_policy_doc branch October 3, 2023 19:53


		When attaching a policy to a resource, the following restrictions apply:

		* A policy can be only attached to `Gateway` resources.


		WARNING

		Current VPC Lattice updateServiceNetworkVpcAssociation api have a limitation that it cannot remove all security groups.

		Be cautious to set AssociateWithVpc to false. That can break traffic from the current cluster workloads to the gateway.


		\| Field \| Description \|


		### Fields of VpcAssociationPolicy

		\| Field Name \| Required \| Description \|


		## Example Configuration

		This example shows how to configure a Gateway with associateWithVpc set to true and apply security group sg-1234567890 and sg-0987654321


		### Fields of VpcAssociationPolicy

		\| Field Name \| Type \| \| Required \| Description \|


		The security group will not take effect if:

		* The targetRef `gateway` does not exist.

Add VpcAssociationPolicy doc #411

Add VpcAssociationPolicy doc #411

Uh oh!

Conversation

zijun726911 commented Sep 26, 2023

Uh oh!

solmonk commented Sep 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 6358392373

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xWink left a comment

Choose a reason for hiding this comment

Uh oh!

xWink Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xWink left a comment

Choose a reason for hiding this comment

coveralls commented Sep 27, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 28, 2023 •

edited

Loading

xWink Sep 29, 2023 •

edited

Loading

xWink Sep 29, 2023 •

edited

Loading