allow the dns check to be ignored with private hosted zones. #810
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
I have submitted the cla/linuxfoundation CLA. |
| if err != nil { | ||
| return fmt.Errorf("error doing DNS lookup for NS records for %q: %v", dnsName, err) | ||
| } | ||
| if os.Getenv("DNS_IGNORE_NS_CHECK") == "" { |
There was a problem hiding this comment.
Do we want an env varible or to have a yaml option?
There was a problem hiding this comment.
I will be using this env as I am using a private hosted zone. I am unsure if there were other cases when this env var would be used, @justinb?
Maybe we need to be more explicit being a private hosted zone. We could either identify whether the user has given a private or public using the aws API, or have it explicitly defined in the yaml.
|
OK no worries. So I should create a new yaml option called |
|
The DNS lookup is performed when creating a cluster, so will need to also add a CLI option along with the YAML option. |
|
Performed in the create or update? We can ask the user to do the three step process, create, edit, update. On the fence. Your call but we already have a ton of cli switches. |
|
Sorry, in the |
|
+1 for this change. I just had to comment out this section of code yesterday to workaround building with a private zone. |
|
@chrislovecnm I have replaced the env var with a cli switch (and in the yaml) |
|
@justinsb / @kris-nova I need a model review. This looks good, but I want it awesome :) @billyshambrook I am sorry I did not mention this, but what are your thoughts on testing this, or helping improve how we test this. |
|
@billyshambrook great talking with you about test cases. Like I said don't want 16 million lines of unit tests. Keep it simple, clean and elegant. |
|
We need a rebase, and I can merge. |
| if err != nil { | ||
| return fmt.Errorf("error doing DNS lookup for NS records for %q: %v", dnsName, err) | ||
| } | ||
| if !fi.BoolValue(cluster.Spec.IgnoreNSCheck) { |
There was a problem hiding this comment.
This is good, but it doesn't belong in the cluster spec. Yes, this does mean you'll have to pass the flag every time.
A nicer option is probably explicit support for a private DNS HostedZone. That didn't make sense before, but now we have topology=private, it does.
|
We need a rebase and @justinsb has requested changes. |
|
I appreciate your work with this! Can we get a rebase, and @justinsb has some feedback. |
|
@billyshambrook any updates? |
|
Did we fix this? |
|
I think so! Adding the scheduled to close label.. Anyone have anything here? |
|
We haven't seen an update here in some time. Please re open this PR if we still want to follow it through. |
fixes #781
This change is