Skip to content

Conversation

hbagdi
Copy link
Contributor

@hbagdi hbagdi commented Jun 11, 2020

This commit bundles two changes:

  • pluralizes hostnames to define the same route
  • allows for wildcards in the left-most label (same as Ingress V1)

Fix #80

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2020
@k8s-ci-robot k8s-ci-robot requested review from bowei and thockin June 11, 2020 21:13
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 11, 2020
@hbagdi hbagdi force-pushed the pluralize-hostname branch from 5459ca2 to 26066a3 Compare June 11, 2020 21:14
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 11, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented Jun 11, 2020

While this PR has two different changes, I opened this up as one. Let me know if I should separate this out.

This PR also introduces some inconsistencies in how "wildcard" or "domain" (@jpeach :D) matching is defined at the listener level vs the HTTPRoute level.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2020
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Just a couple nits around pluralization, not sure my suggestions are ideal, but I think there are some places that need to be updated to handle the new potential for plural hostname.

}

// HTTPRouteHost is the configuration for a given host.
// HTTPRouteHost is the configuration for a given host(s).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// HTTPRouteHost is the configuration for a given host(s).
// HTTPRouteHost is the configuration for a given set of hosts.

// HTTPRoute rules. If the host is unspecified, traffic is routed
// based on the HTTPRouteRules.
//
// Host can be "precise" which is a domain name without the terminating
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Host can be "precise" which is a domain name without the terminating
// Hostnames can be "precise" which is a domain name without the terminating

// HTTPRoute with hostname example.com or bar.example.com will not match.
// If Hostname is unspecified, the Gateway routes all traffic based on
// the specified rules.
// Incoming requests are matched against the host before the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Incoming requests are matched against the host before the
// Incoming requests are matched against the hosts before the

@bowei
Copy link
Contributor

bowei commented Jun 24, 2020

looks like this is easy to fix and get merged

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, hbagdi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020
This commit bundles two changes:
- pluralizes hostnames to define the same route
- allows for wildcards in the left-most label (same as Ingress V1)
@hbagdi hbagdi force-pushed the pluralize-hostname branch from 26066a3 to 6886aff Compare June 24, 2020 17:47
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020
@hbagdi
Copy link
Contributor Author

hbagdi commented Jun 24, 2020

@robscott I've incorporated your feedback. PTAL.

@robscott
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit aeaec11 into kubernetes-sigs:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use case: succinctly expose the same service on multiple names

4 participants