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

Change buildUrl behavior #77

Closed
tloader11 opened this issue Sep 5, 2021 · 1 comment · Fixed by #79
Closed

Change buildUrl behavior #77

tloader11 opened this issue Sep 5, 2021 · 1 comment · Fixed by #79
Assignees

Comments

@tloader11
Copy link

tloader11 commented Sep 5, 2021

With the current buildUrl behavior, some environments cannot be used (kubernetes with proxy, for example)

Detailed description

Our development environment makes use of the kubernetes proxy to access the PowerDNS API. Unfortunately, that means that to access the API our URL is somewhat 'strange'.

Example of the URL on which we can access the API:
http://127.0.0.1:8001/api/v1/namespaces/dns/services/powerdns-api:8081/proxy/

As you can see, we already need to define port 8001, as this is the kubernetes proxy port. We also need to define the kubernetes service port (8081) to proxy to the developer.

As you can see, /proxy/ needs to be added to the end of the URL as well.

This results in a scenario where the buildUrl function would try to append the port number (when null, the port "0" would be added) to a ready-to-go URL...

Context

Everybody who wants to use some kind of proxy like kubectl proxy would benefit from this change.

Possible implementation

Changing the code in Connector.php , rule 177 - 186 to:

        $host_with_port = $config['port'] === 0 ? $config['host'] : sprintf("%s:%d",$config['host'], $config['port']);
        
        return rtrim(
            sprintf(
                '%s/api/v1/servers/%s/%s',
                $host_with_port,
                $config['server'],
                $path
            ),
            '/'
        );

would resolve the issue.

Unfortunately, as is, there is a default port set in Powerdns.php ($port = 8001) and the constructor explicitly disallows "null" to be inserted here. Due to these limitations and to ensure backwards compatibility for people who rely on the default port, this would be the least invasive fix.

To remove the :port from the url, one would simply put "0" in the constructor of the Powerdns class.

A better fix would be to allow null as port and change the check I've created above to:
$host_with_port = isset($config['port']) ? sprintf("%s:%d",$config['host'], $config['port']) : $config['host']

Your environment

PHP 7.4
Windows / Linux

@trizz trizz self-assigned this Sep 6, 2021
@trizz
Copy link
Member

trizz commented Oct 5, 2021

Hi @tloader11,

I've discussed this issue with our team, but we won't be adding such functionality to the package. There are probably more cases where the buildUrl method doesn't suffice and adding exceptions isn't maintainable.

With that being said, it is possible to create your own connector class with the URL structure you'd like. Just pass it as argument to the Powerdns client constructor to use your own connector class instead of the provided one. The only requirement is that it implements the ConnectorInterface.

To prevent writing all the logic in your own class, I've changed the private methods in the Connector class in the upcoming release to be protected. This way, your own Connector class can be pretty simple by only overwriting the buildUrl method:

class CustomConnector extends Connector
{
    protected function buildUrl(string $path): string
    {
        // Your own logic.
    }
}

And use it like this:

$powerdns = new Powerdns();
$connector = new CustomConnector($powerdns);

// Because the connector needs the Powerdns client instance (because its based on the original connector), the new 'setConnector' method must be used:
$powerdns->setConnector($connector);

// If you don't extend the existing connector, you can pass your own implementation when constructing the client:
$powerdns = new Powerdns(null, null, null, null, new CustomConnector());

Of course, if you're using dependency injection you can use that to load the correct instance of the ConnectorInterface.

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 a pull request may close this issue.

3 participants