Determine ingress backend service by parsing addressable URI #10197
Conversation
knative-prow-robot
added
the
size/L
Codecov Report
@@ Coverage Diff @@
## master #10197 +/- ##
==========================================
+ Coverage 88.04% 88.10% +0.06%
==========================================
Files 184 184
Lines 8621 8611 -10
==========================================
- Hits 7590 7587 -3
+ Misses 789 787 -2
+ Partials 242 237 -5
Continue to review full report at Codecov.
|
| // TODO(julz) in the future we may support addressables that are not created | ||
| // from Services, in which case we would need to dynamically create the | ||
| // an ExternalName Service for the KIngress to use. | ||
| requiredSuffix := ".svc." + network.GetClusterDomainName() |
There was a problem hiding this comment.
this looks like something that can be precomputed as global var requiredSuffix ...
There was a problem hiding this comment.
theoretically yeah but in practice the method has a once.Do inside it anyway. Doesn't look like we pre-compute in any of the other various places we use it, e.g.
,serving/pkg/reconciler/route/config/domain.go
Line 104 in 7398ed7
There was a problem hiding this comment.
One has to start somewhere :)
| @@ -138,26 +138,43 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, dm *v1alpha1.DomainMa | |||
| return ingress, err | |||
| } | |||
|
|
|||
| func (r *Reconciler) resolveRef(ctx context.Context, dm *v1alpha1.DomainMapping) (*apis.URL, error) { | |||
| func (r *Reconciler) resolveRef(ctx context.Context, dm *v1alpha1.DomainMapping) (host string, backendSvc string, err error) { | |||
There was a problem hiding this comment.
| func (r *Reconciler) resolveRef(ctx context.Context, dm *v1alpha1.DomainMapping) (host string, backendSvc string, err error) { | |
| func (r *Reconciler) resolveRef(ctx context.Context, dm *v1alpha1.DomainMapping) (host, backendSvc string, err error) { |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, vagababov 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 |
knative-prow-robot
added
the
approved
…#10197) * Determine ingress backend service by parsing URI * nit
…#10197) * Determine ingress backend service by parsing URI * nit
Part of #9713.
Properly parses the backend service to use from the resolved Addressable URI rather than assuming it's named the same as the ref. This assumes the resolved address is of the form
{name}.{namespace}.svc.{cluster.local.suffix}, and validates that this is the case. I think this should be fine for most things we care about - when we want to support other types of targets we'll need to synthesise an ExternalName, but that's left for a future PR.Note: as of now the validation will still (now unnecessarily) insist the target is a ksvc; I'll relax that in the next PR.
/assign @mattmoor @vagababov