Skip to content

Conversation

@solmonk
Copy link
Contributor

@solmonk solmonk commented Nov 29, 2023

What type of PR is this?

documentation

Which issue does this PR fix:

#401

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.

metadata:
name: service-1
annotations:
application-networking.k8s.aws/port: "9200"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we show a list of comma-separated port in here, e.g.,
application-networking.k8s.aws/port: "9200,9201,9203"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is almost no chance that users will need comma-separated list behavior here, tbh I don't like that feature and I think it only causes confusion. I don't want to highlight that use case as an example.

### Limitations
* The exported Service can only be used in HTTPRoutes. GRPCRoute is currently not supported.
* Limited to one ServiceExport per Service. If you need multiple exports representing each port,
you should create multiple Service-ServiceExport pairs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this mean?

If you need multiple exports representing each port, you should create multiple Service-ServiceExport pairs.

Sounds like we support multiple ServiceExports per Service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't, we need as many services as serviceexport.

Indicates the cluster name owning the exported service.
* `application-networking.k8s.aws/aws-vpc`
Indicates the VPC ID owning the exported service.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention these 2 annotations are optional? The controller is able to automatically identify cluster name and vpc if not specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update this. btw the fields are optional but it is not like the controller automatically identifies them. They are like an additional sanity check when finding exported TG

### Annotations

- `application-networking.k8s.aws/lattice-assigned-domain-name`
- `application-networking.k8s.aws/lattice-assigned-domain-name`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: trailing space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually intended to create a line break. Seems like backslash doesn't work in mkdocs so this is an alternative to it

### Annotations

- `application-networking.k8s.aws/lattice-assigned-domain-name`
- `application-networking.k8s.aws/lattice-assigned-domain-name`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit as above

Comment on lines +8 to +10
Internally, creating a ServiceExport creates a standalone VPC Lattice [target group](https://docs.aws.amazon.com/vpc-lattice/latest/ug/target-groups.html).
Even without ServiceImports, creating ServiceExports can be useful in case you only need the target groups created;
for example, using target groups in the VPC Lattice setup outside Kubernetes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A second thought on it, I dont think we should advertise TG creation with ServiceExports, because TG now "ephemeral" with random suffix. It will break use of TG outside of controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target groups are not ephemeral, especially when created from ServiceExports. The only exception is when users intentionally change immutable fields, but that happens regardless of random suffix anyways.

@solmonk solmonk merged commit 5553213 into aws:main Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants