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

Export SVCB map #1362

Closed
wants to merge 1 commit into from
Closed

Export SVCB map #1362

wants to merge 1 commit into from

Conversation

ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Apr 12, 2022

The mappings of the numeric SVCB key values to strings are exported, under names
consistent with the already existing exported mappings, to make it easier for
the clients of the module to validate and print SVCB keys.

Split off from #1359.

The mappings of the numeric SVCB key values to strings are exported, under names
consistent with the already existing exported mappings, to make it easier for
the clients of the module to validate and print SVCB keys.

Split off from miekg#1359.
@miekg
Copy link
Owner

miekg commented Apr 12, 2022

I think this requires somewhat stronger than:

to make it easier for the clients of the module to validate and print SVCB keys.

Not sure what that would be, but making something public as a high cost

@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 12, 2022

@miekg, the original reason for this PR is to be able to properly construct an SVCBMandatory from text representation of the keys. That is, receiving something like alpn=h2 mandatory=alpn and being able to do something like:

// keyStr = "alpn"
code, ok := dns.StringToSVCBKey[keyStr]
if !ok {
        return nil, fmt.Errorf("unsupported svcb key %q", keyStr)
}

return &dns.SVCBMandatory{
        Code: []dns.SVCBKey{code},
}, nil

Currently, we have to maintain a copy of that map in AdGuard Home, which obviously leads to issues of having to update the map whenever a draft is updated or a new key is proposed.

An alternative and more minimal API change to solve the same issue would be to export svcbStringToKey, although that would still leave those two maps pretty much the only ones of their kind to be unexported.

@shane-kerr
Copy link
Contributor

Not sure what that would be, but making something public as a high cost

Not strictly related to this particular PR, but... what is the cost for making something public?

We've got a significant amount of code from this library copied into other codebases because we need the functionality. For example, lots of the APL parsing is inaccessible. I was going to open a ticket to make various stuff public, but I wasn't sure if y'all would be open to the idea.

@miekg
Copy link
Owner

miekg commented Apr 13, 2022

anything that has a public interface can't be changed anymore - unless we're doing a v2, but noone is working on that. So once in the open it's basically set in stone - see discussion on the TSIG stuff for example.

OTOH if (enough) user want functions to be opened up, we can always discuss that, problem is getting the API right (although for super new stuff we sometimes made some backwards incompatible changes).

OTOH OTOH copying code is fine as well.

TL;DR: it depends

@miekg miekg closed this Apr 14, 2022
@ainar-g
Copy link
Contributor Author

ainar-g commented Apr 14, 2022

@miekg, I assume that you closing this PR means that exporting svcbStringToKey only, like I proposed in my latest comment, is also out of the question for now?

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.

None yet

3 participants