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

Eventing TLS: Add AddressableFromDestination method on the resolver #2714

Closed
pierDipi opened this issue Apr 3, 2023 · 2 comments · Fixed by #2717
Closed

Eventing TLS: Add AddressableFromDestination method on the resolver #2714

pierDipi opened this issue Apr 3, 2023 · 2 comments · Fixed by #2717
Assignees
Labels
area/API good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature

Comments

@pierDipi
Copy link
Member

pierDipi commented Apr 3, 2023

After #2711 is implemented, as the Eventing TLS feature track describes we should add a new method on the resolver as follow:

AddressableFromDestination(ctx Context, dest Destination, parent interface{}) (*Addressable, error)

The implementation should be similar to URIFromDestination

https://github.com/knative/pkg/blob/main/resolver/addressable_resolver.go#L75-L76C10

but with the following additional algorithm/logic:

  • If status.addresses of the resolved destination is set (has at least 1 element)
    • if dest.ref.address is not specified:
      • Select the first (in order) address from all addresses
      • If no address is found use status.address of the resolved destination
    • else
      • Select the first (in order) address from all addresses with name == dest.ref.address
      • If no address is found return an error explaining that the address with name <address_value> cannot be found/resolved
  • else return status.address of the resolved destination

Since the new AddressableFromDestination is a superset of URIFromDestination, the URIFromDestination should make use of the AddressableFromDestination method but return the resolved URI

Additional Info

/good-first-issue
/kind feature
/area API

@knative-prow
Copy link

knative-prow bot commented Apr 3, 2023

@pierDipi:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

After #2711 is implemented, as the Eventing TLS feature track describes we should add a new method on the resolver as follow:

AddressableFromDestination(ctx Context, dest Destination, parent interface{}) (*Addressable, error)

The implementation should be similar to URIFromDestination

https://github.com/knative/pkg/blob/main/resolver/addressable_resolver.go#L75-L76C10

but with the following additional algorithm/logic:

  • If status.addresses of the resolved destination is set (has at least 1 element)
  • if dest.ref.address is not specified:
    • Select the first (in order) address from all addresses
    • If no address is found use status.address of the resolved destination
  • else
    • Select the first (in order) address from all addresses with name == dest.ref.address
    • If no address is found return an error explaining that the address with name <address_value> cannot be found/resolved
  • else return status.address of the resolved destination

Since the new AddressableFromDestination is a superset of URIFromDestination, the URIFromDestination should make use of the AddressableFromDestination method but return the resolved URI

Additional Info

/good-first-issue
/kind feature
/area API

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow knative-prow bot added kind/feature area/API good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Apr 3, 2023
@vishal-chdhry
Copy link
Member

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants