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

OpenStack Designate provider #305

Merged
merged 3 commits into from
Apr 5, 2018
Merged

Conversation

istalker2
Copy link
Contributor

OpenStack Designate provider

Implements #244

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2017
@istalker2 istalker2 force-pushed the designate branch 2 times, most recently from 8978b66 to f875b1c Compare August 5, 2017 11:35
@linki
Copy link
Member

linki commented Aug 14, 2017

@istalker2 Thank you!

Could you also provide some documentation on how to use it? Something like https://github.com/kubernetes-incubator/external-dns/blob/v0.4.3/docs/tutorials/gke.md

@istalker2
Copy link
Contributor Author

@linki done! Also, support for TLS configuration was added

@istalker2 istalker2 force-pushed the designate branch 2 times, most recently from ca7a7d2 to be2db38 Compare August 15, 2017 18:06
@hjacobs
Copy link
Contributor

hjacobs commented Aug 17, 2017

What is missing to get this merged?

@istalker2
Copy link
Contributor Author

@hjacobs whom are you asking? Nothing that I'm aware of

certFile := os.Getenv(fmt.Sprintf("%s_CERT_FILE", prefix))
keyFile := os.Getenv(fmt.Sprintf("%s_KEY_FILE", prefix))
serverName := os.Getenv(fmt.Sprintf("%s_TLS_SERVER_NAME", prefix))
isInsecureStr := strings.ToLower(os.Getenv(fmt.Sprintf("%s_TLS_INSECURE", prefix)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to define these variables along with the rest here: https://github.com/kubernetes-incubator/external-dns/blob/master/pkg/apis/externaldns/types.go

That way they would be automatically documented under --help and it would also be possible to configure via flags, if preferred, like all the other configuration options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. As you can see those are not variables with a fix name, but rather a variable templates (with generated names)
  2. This is a helper code to be used for providers that need to establish connection to their backend with TLS. Exact variable names are specific to provider. Some providers going to have this set of variables, some are not, some may even have several of them if there is more than one connection to different services with different TLS parameters needed
  3. Those variables are part of provider configuration. --help doesn't show you all the environment variables that can be used to control AWS/GCE etc providers, so why should it do it for designate?
  4. For designate there are about 15 environment variables that affect how and where it connects and authenticates. For some of other providers it is not better. I don't think that extracting parameters of all providers and merging them into one flat list and then printing in --help screen is a good idea.
  5. I'd rather think about extending provider interface so that providers can print their own help information. Then I'd be able to say something like external-dns --help --provider designate and get designate specific help with all those variables listed. Alternatively it can be just a formally structured documentation. Current list of tutorials do help, but it is not a documentation in my understanding and they don't give all the provider options

Copy link
Contributor

@mikkeloscar mikkeloscar Aug 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those variables are part of provider configuration. --help doesn't show you all the environment variables that can be used to control AWS/GCE etc providers, so why should it do it for designate?

I don't see any special environment variables in any of those provides. I only see something in the Cloudflare provider, which I also believe should be moved to the main config.

For designate there are about 15 environment variables that affect how and where it connects and authenticates. For some of other providers it is not better. I don't think that extracting parameters of all providers and merging them into one flat list and then printing in --help screen is a good idea.

I think it's better than having them appear magically in the middle of a provider without any information about it in --help

I'd rather think about extending provider interface so that providers can print their own help information. Then I'd be able to say something like external-dns --help --provider designate and get designate specific help with all those variables listed.

This could be a good solution, maybe a bit tricky with the current CLI library.

In the end this is a great PR and I'm just pointing out something I believe could be done in a better way. If you don't agree it's fine :). I'll let @linki and @ideahitme review and decide.

Copy link
Contributor Author

@istalker2 istalker2 Aug 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I'm wrong, but I just opened the first provider in the list, which happened to be AWS Route53 and one of the first things that it does is call to session.NewSessionWithOptions(...) which executes code in aws-go-sdk that load environment variables like AWS_ACCESS_KEY, AWS_ACCESS_TOKEN, AWC_CA_BUNDLE, AWS_REGION and so on. This is just hidden inside SDK. And this seems logical since I didn't see --help printing any flags that I can use to provide my AWS credentials/region/etc. Here the picture is similar - most of designate environment variables are hidden inside gophercloud library.
But generally, I agree with you as having hidden settings that are not even some minor tweaks is not something that one can like. I don't think that it need to become external-dns CLI flags for two reasons:

  1. It is likely to be too many of them. Some of them are mutually exclusive or must be provided in pairs etc. This goes beyond simple CLI parsing capabilities. Without refactoring to existing code, it will turn into a function that knows all the settings of all providers and validates if the flags were provided in valid configuration which defeats the whole purpose of provider separations. And it makes help screen just to complex
  2. For some of parameters (secrets, passwords) it is generally a bad idea to have them in commandline. For the same reasons that you cannot specify password in ssh CLI flag. On the other hand, having them in environment variables makes it easy to automatically populate them from k8s Secrets/ConfigMaps.

That's why I think that --help --provider name is the best solution here. But it goes beyond the scope of this PR and need to be implemented consistently for all providers


## Authenticating with OpenStack

OpenStack Designate does not have a web dashboard yet. Thus the only way to do something in Designate is to use OpenStack CLI - `openstack` utility,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linki
Copy link
Member

linki commented Aug 25, 2017

👍

Stan Lagun added 3 commits September 22, 2017 20:55
TLS parameters can be passed through the following environment
variables:
* OPENSTACK_CA_FILE - CA file path
* OPENSTACK_CERT_FILE - cert file path
* OPENSTACK_KEY_FILE - key file path. Either both or none of cert/key
  must be specified
* OPENSTACK_TLS_SERVER_NAME - (optional) expected CN of the server
  certificate if differs from domain in the URL
* OPENSTACK_TLS_INSECURE - if set to yes|true|1 disables validation of
  the server certificate

Code that loads tls.Config from environment variables was extracted
into dedicated package so that it could be reused by different providers
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 23, 2017
@thomasjungblut
Copy link

Guys, what's left to merge this? Seems like this effort got a little stale.

@ConnorJC3
Copy link

Guys, what's left to merge this? Seems like this effort got a little stale.

+1

@grahamhayes
Copy link

Are we still waiting for #228 to merge? If so what needs to happen for that?

@ConnorJC3
Copy link

#228 is confirmed as not needed.

As a result, I hope we can get this merged :) - can someone confirm that this PR is ready to go with the current version of External DNS?

cc @istalker2 @linki @ideahitme

@njuettner njuettner merged commit 09a6f6b into kubernetes-sigs:master Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. new-provider size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants