Skip to content

URI vs URL inconsistency #779

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.

Sign up for GitHub

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

Open
sabrina-ngrok wants to merge 1 commit into
base: main
Choose a base branch
from
Open

URI vs URL inconsistency #779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api/bindings/v1alpha1/boundendpoint_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ import (

// BoundEndpointSpec defines the desired state of BoundEndpoint
type BoundEndpointSpec struct {
// EndpointURI is the unique identifier
// EndpointURL is the unique identifier
// representing the BoundEndpoint + its Endpoints
// Format: <scheme>://<service>.<namespace>:<port>
//
// +kubebuilder:validation:Required
// See: https://regex101.com/r/9QkXWl/1
// +kubebuilder:validation:Pattern=`^((?P<scheme>(tcp|http|https|tls)?)://)?(?P<service>[a-z][a-zA-Z0-9-]{0,62})\.(?P<namespace>[a-z][a-zA-Z0-9-]{0,62})(:(?P<port>\d+))?$`
EndpointURI string `json:"endpointURI"`
EndpointURL string `json:"endpointURL"`

// Scheme is a user-defined field for endpoints that describe how the data packets
// are framed by the pod forwarders mTLS connection to the ngrok edge
Expand Down Expand Up @@ -138,7 +138,7 @@ type BindingEndpoint struct {
// +kubebuilder:subresource:status

// BoundEndpoint is the Schema for the boundendpoints API
// +kubebuilder:printcolumn:name="URI",type="string",JSONPath=".spec.endpointURI"
// +kubebuilder:printcolumn:name="URL",type="string",JSONPath=".spec.endpointURL"
// +kubebuilder:printcolumn:name="Port",type="string",JSONPath=".spec.port"
// +kubebuilder:printcolumn:name="Endpoints",type="string",JSONPath=".status.endpointsSummary"
// +kubebuilder:printcolumn:name="Services",type="string",JSONPath=".status.conditions[?(@.type==\"ServicesCreated\")].status"
Expand Down
10 changes: 5 additions & 5 deletions helm/ngrok-crds/templates/bindings.k8s.ngrok.com_boundendpoints.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions internal/controller/bindings/boundendpoint_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func (r *BoundEndpointReconciler) update(ctx context.Context, cr *bindingsv1alph
if err != nil {
if client.IgnoreNotFound(err) != nil {
// real error
log.Error(err, "Failed to find existing Upstream Service", "name", cr.Name, "uri", cr.Spec.EndpointURI)
log.Error(err, "Failed to find existing Upstream Service", "name", cr.Name, "url", cr.Spec.EndpointURL)
return r.updateStatus(ctx, cr, err)
}

Expand Down Expand Up @@ -284,7 +284,7 @@ func (r *BoundEndpointReconciler) update(ctx context.Context, cr *bindingsv1alph
if err != nil {
if client.IgnoreNotFound(err) != nil {
// real error
log.Error(err, "Failed to find existing Target Service", "name", cr.Name, "uri", cr.Spec.EndpointURI)
log.Error(err, "Failed to find existing Target Service", "name", cr.Name, "url", cr.Spec.EndpointURL)
return r.updateStatus(ctx, cr, err)
}

Expand Down Expand Up @@ -528,7 +528,7 @@ func (r *BoundEndpointReconciler) findBoundEndpointsForService(ctx context.Conte

// tryToBindEndpoint attempts a TCP connection through the provisioned services for the BoundEndpoint
func (r *BoundEndpointReconciler) testBoundEndpointConnectivity(ctx context.Context, boundEndpoint *bindingsv1alpha1.BoundEndpoint) error {
log := ctrl.LoggerFrom(ctx).WithValues("uri", boundEndpoint.Spec.EndpointURI)
log := ctrl.LoggerFrom(ctx).WithValues("url", boundEndpoint.Spec.EndpointURL)

bindErrMsg := fmt.Sprintf("connectivity check failed for BoundEndpoint %s", boundEndpoint.Name)

Expand All @@ -540,17 +540,17 @@ func (r *BoundEndpointReconciler) testBoundEndpointConnectivity(ctx context.Cont
retries := 8

// rely on kube-dns to resolve the targetService's ExternalName
uri, err := url.Parse(boundEndpoint.Spec.EndpointURI)
uri, err := url.Parse(boundEndpoint.Spec.EndpointURL)
if err != nil {
wrappedErr := fmt.Errorf("failed to parse BoundEndpoint URI %s: %w", boundEndpoint.Spec.EndpointURI, err)
log.Error(wrappedErr, bindErrMsg, "uri", boundEndpoint.Spec.EndpointURI)
wrappedErr := fmt.Errorf("failed to parse BoundEndpoint URL %s: %w", boundEndpoint.Spec.EndpointURL, err)
log.Error(wrappedErr, bindErrMsg, "url", boundEndpoint.Spec.EndpointURL)
return wrappedErr
}

for i := range retries {
select {
case <-ctx.Done():
err = errors.New("attempting to connect to BoundEndpoint URI timed out")
err = errors.New("attempting to connect to BoundEndpoint URL timed out")
log.Error(err, bindErrMsg)
return err

Expand Down
58 changes: 29 additions & 29 deletions internal/controller/bindings/boundendpoint_poller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,8 @@ func (r *BoundEndpointPoller) reconcileBoundEndpointAction(ctx context.Context,
// process from list
for _, binding := range remainingBindings {
if err := action(ctx, binding); err != nil {
name := hashURI(binding.Spec.EndpointURI)
log.Error(err, "Failed to reconcile BoundEndpoint", "action", actionMsg, "name", name, "uri", binding.Spec.EndpointURI)
name := hashURL(binding.Spec.EndpointURL)
log.Error(err, "Failed to reconcile BoundEndpoint", "action", actionMsg, "name", name, "url", binding.Spec.EndpointURL)
failedBindings = append(failedBindings, binding)
}
}
Expand All @@ -336,10 +336,10 @@ func (r *BoundEndpointPoller) filterBoundEndpointActions(ctx context.Context, ex
log.V(9).Info("Filtering BoundEndpoints", "existing", existingBoundEndpoints, "desired", desiredEndpoints)

for _, existingBoundEndpoint := range existingBoundEndpoints {
uri := existingBoundEndpoint.Spec.EndpointURI
endpointURL := existingBoundEndpoint.Spec.EndpointURL

if desiredBoundEndpoint, ok := desiredEndpoints[uri]; ok {
expectedName := hashURI(desiredBoundEndpoint.Spec.EndpointURI)
if desiredBoundEndpoint, ok := desiredEndpoints[endpointURL]; ok {
expectedName := hashURL(desiredBoundEndpoint.Spec.EndpointURL)

// if the names match, then they are the same resource and we can update it
if existingBoundEndpoint.Name == expectedName {
Expand All @@ -359,7 +359,7 @@ func (r *BoundEndpointPoller) filterBoundEndpointActions(ctx context.Context, ex

// remove the desired endpoint from the set
// so we can see which endpoints are net-new
delete(desiredEndpoints, uri)
delete(desiredEndpoints, endpointURL)
}

for _, desiredBoundEndpoint := range desiredEndpoints {
Expand All @@ -374,12 +374,12 @@ func (r *BoundEndpointPoller) filterBoundEndpointActions(ctx context.Context, ex
func (r *BoundEndpointPoller) createBinding(ctx context.Context, desired bindingsv1alpha1.BoundEndpoint) error {
log := ctrl.LoggerFrom(ctx)

name := hashURI(desired.Spec.EndpointURI)
name := hashURL(desired.Spec.EndpointURL)

// allocate a port
port, err := r.portAllocator.SetAny()
if err != nil {
r.Log.Error(err, "Failed to allocate port for BoundEndpoint", "name", name, "uri", desired.Spec.EndpointURI)
r.Log.Error(err, "Failed to allocate port for BoundEndpoint", "name", name, "url", desired.Spec.EndpointURL)
return err
}

Expand All @@ -393,7 +393,7 @@ func (r *BoundEndpointPoller) createBinding(ctx context.Context, desired binding
Namespace: r.Namespace,
},
Spec: bindingsv1alpha1.BoundEndpointSpec{
EndpointURI: desired.Spec.EndpointURI,
EndpointURL: desired.Spec.EndpointURL,
Scheme: desired.Spec.Scheme,
Port: port,
Target: bindingsv1alpha1.EndpointTarget{
Expand All @@ -409,26 +409,26 @@ func (r *BoundEndpointPoller) createBinding(ctx context.Context, desired binding
},
}

log.Info("Creating new BoundEndpoint", "name", name, "uri", toCreate.Spec.EndpointURI)
log.Info("Creating new BoundEndpoint", "name", name, "url", toCreate.Spec.EndpointURL)
if err := r.Create(ctx, toCreate); err != nil {
if client.IgnoreAlreadyExists(err) != nil {
log.Error(err, "Failed to create BoundEndpoint", "name", name, "uri", toCreate.Spec.EndpointURI)
log.Error(err, "Failed to create BoundEndpoint", "name", name, "url", toCreate.Spec.EndpointURL)
r.Recorder.Event(toCreate, v1.EventTypeWarning, "Created", fmt.Sprintf("Failed to create BoundEndpoint: %v", err))
return err
}

log.Info("BoundEndpoint already exists, skipping create...", "name", name, "uri", toCreate.Spec.EndpointURI)
log.Info("BoundEndpoint already exists, skipping create...", "name", name, "url", toCreate.Spec.EndpointURL)

if toCreate.Status.HashedName != "" && len(toCreate.Status.Endpoints) > 0 {
// Status is filled, no need to update
return nil
}

// intentionally fall through and fill in status
log.Info("BoundEndpoint already exists, but status is empty, filling in status...", "name", name, "uri", toCreate.Spec.EndpointURI, "toCreate", toCreate)
log.Info("BoundEndpoint already exists, but status is empty, filling in status...", "name", name, "url", toCreate.Spec.EndpointURL, "toCreate", toCreate)

if err := r.Get(ctx, client.ObjectKey{Namespace: r.Namespace, Name: name}, toCreate); err != nil {
log.Error(err, "Failed to get existing BoundEndpoint, skipping status update...", "name", name, "uri", toCreate.Spec.EndpointURI)
log.Error(err, "Failed to get existing BoundEndpoint, skipping status update...", "name", name, "url", toCreate.Spec.EndpointURL)
return nil
}
}
Expand Down Expand Up @@ -463,7 +463,7 @@ func (r *BoundEndpointPoller) createBinding(ctx context.Context, desired binding
func (r *BoundEndpointPoller) updateBinding(ctx context.Context, desired bindingsv1alpha1.BoundEndpoint) error {
log := ctrl.LoggerFrom(ctx)

desiredName := hashURI(desired.Spec.EndpointURI)
desiredName := hashURL(desired.Spec.EndpointURL)

// Attach the metadata fields to the desired boundendpoint
desired.Spec.Target.Metadata.Annotations = r.TargetServiceAnnotations
Expand All @@ -474,16 +474,16 @@ func (r *BoundEndpointPoller) updateBinding(ctx context.Context, desired binding
if err != nil {
if client.IgnoreNotFound(err) == nil {
// BoundEndpoint doesn't exist, create it on the next polling loop
log.Info("Unable to find existing BoundEndpoint, skipping update...", "name", desiredName, "uri", desired.Spec.EndpointURI)
log.Info("Unable to find existing BoundEndpoint, skipping update...", "name", desiredName, "url", desired.Spec.EndpointURL)
return nil // not an error
}
// real error
log.Error(err, "Failed to find existing BoundEndpoint", "name", desiredName, "uri", desired.Spec.EndpointURI)
log.Error(err, "Failed to find existing BoundEndpoint", "name", desiredName, "url", desired.Spec.EndpointURL)
return err
}

if !boundEndpointNeedsUpdate(ctx, *existing, desired) {
log.Info("BoundEndpoint already matches existing state, skipping update...", "name", desiredName, "uri", desired.Spec.EndpointURI)
log.Info("BoundEndpoint already matches existing state, skipping update...", "name", desiredName, "url", desired.Spec.EndpointURL)
return nil
}

Expand All @@ -493,11 +493,11 @@ func (r *BoundEndpointPoller) updateBinding(ctx context.Context, desired binding
toUpdate.Spec.Port = existing.Spec.Port // keep the same port
toUpdate.Spec.Scheme = desired.Spec.Scheme
toUpdate.Spec.Target = desired.Spec.Target
toUpdate.Spec.EndpointURI = desired.Spec.EndpointURI
toUpdate.Spec.EndpointURL = desired.Spec.EndpointURL

log.Info("Updating BoundEndpoint", "name", toUpdate.Name, "uri", toUpdate.Spec.EndpointURI)
log.Info("Updating BoundEndpoint", "name", toUpdate.Name, "url", toUpdate.Spec.EndpointURL)
if err := r.Update(ctx, toUpdate); err != nil {
log.Error(err, "Failed updating BoundEndpoint", "name", toUpdate.Name, "uri", toUpdate.Spec.EndpointURI)
log.Error(err, "Failed updating BoundEndpoint", "name", toUpdate.Name, "url", toUpdate.Spec.EndpointURL)
r.Recorder.Event(toUpdate, v1.EventTypeWarning, "Updated", fmt.Sprintf("Failed to update BoundEndpoint: %v", err))
return err
}
Expand Down Expand Up @@ -533,10 +533,10 @@ func (r *BoundEndpointPoller) deleteBinding(ctx context.Context, boundEndpoint b
log := ctrl.LoggerFrom(ctx)

if err := r.Delete(ctx, &boundEndpoint); err != nil {
log.Error(err, "Failed to delete BoundEndpoint", "name", boundEndpoint.Name, "uri", boundEndpoint.Spec.EndpointURI)
log.Error(err, "Failed to delete BoundEndpoint", "name", boundEndpoint.Name, "url", boundEndpoint.Spec.EndpointURL)
return err
}
log.Info("Deleted BoundEndpoint", "name", boundEndpoint.Name, "uri", boundEndpoint.Spec.EndpointURI)
log.Info("Deleted BoundEndpoint", "name", boundEndpoint.Name, "url", boundEndpoint.Spec.EndpointURL)

// unset the port allocation
r.portAllocator.Unset(boundEndpoint.Spec.Port)
Expand All @@ -561,11 +561,11 @@ func (r *BoundEndpointPoller) updateBindingStatus(ctx context.Context, desired *
current.Status.HashedName = desired.Status.HashedName

if err := r.Status().Update(ctx, current); err != nil {
log.Error(err, "Failed to update BoundEndpoint status", "name", current.Name, "uri", current.Spec.EndpointURI)
log.Error(err, "Failed to update BoundEndpoint status", "name", current.Name, "uri", current.Spec.EndpointURL)
return err
}

log.Info("Updated BoundEndpoint status", "name", current.Name, "uri", current.Spec.EndpointURI)
log.Info("Updated BoundEndpoint status", "name", current.Name, "uri", current.Spec.EndpointURL)
return nil
}

Expand Down Expand Up @@ -625,7 +625,7 @@ func boundEndpointNeedsUpdate(ctx context.Context, existing bindingsv1alpha1.Bou
existing.Spec.Target.Protocol != desired.Spec.Target.Protocol ||
existing.Spec.Target.Service != desired.Spec.Target.Service ||
existing.Spec.Target.Namespace != desired.Spec.Target.Namespace ||
existing.Spec.EndpointURI != desired.Spec.EndpointURI ||
existing.Spec.EndpointURL != desired.Spec.EndpointURL ||
!targetMetadataIsEqual(existing.Spec.Target.Metadata, desired.Spec.Target.Metadata)

if hasSpecChanged {
Expand Down Expand Up @@ -654,8 +654,8 @@ func boundEndpointNeedsUpdate(ctx context.Context, existing bindingsv1alpha1.Bou
return false
}

// hashURI hashes a URI to a unique string that can be used as BoundEndpoint.metadata.name
func hashURI(uri string) string {
uid := uuid.NewSHA1(uuid.NameSpaceURL, []byte(uri))
// hashURL hashes a URL to a unique string that can be used as BoundEndpoint.metadata.name
func hashURL(url string) string {
uid := uuid.NewSHA1(uuid.NameSpaceURL, []byte(url))
return "ngrok-" + uid.String()
}
Loading
Loading