Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pluralizes Listener Ports of Proxy Infra IR #183

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Aug 1, 2022

Previously, a listener could only have a single port. This PR pluralizes infra.proxy.listeners[].ports so a listener can have multiple ports.

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested a review from a team as a code owner August 1, 2022 17:02
internal/ir/infra.go Outdated Show resolved Hide resolved
@lizan
Copy link
Member

lizan commented Aug 1, 2022

We might want to pluralize the combination of address and port, it is a common use case in IPv6/v4 dual stack etc.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Contributor Author

danehans commented Aug 1, 2022

@lizan there are some nuances in Kube to support IPv6. In an effort to focus on the v0.2.0 goals, I prefer to concentrate the PR on IPv4. I created #184 to track the need to support IPv6.

@danehans danehans requested a review from arkodg August 1, 2022 17:49
@lizan
Copy link
Member

lizan commented Aug 1, 2022

I'm not saying we should do IPv6 support right now, but if we pluralize ports only now it will be messy when we pluralize address. Since this is just IR not an API so it is up to you.

@danehans
Copy link
Contributor Author

danehans commented Aug 1, 2022

@lizan understood. I think it's best not to pluralize addresses right now b/c I'm unsure if that's the right approach to supporting dual-stack. Let's make that decision as part of #184.

@danehans danehans merged commit c83b7b0 into envoyproxy:main Aug 1, 2022
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