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

[ATTENTION] Complete project refactoring #46

Closed
ifree92 opened this issue Jun 29, 2021 · 22 comments
Closed

[ATTENTION] Complete project refactoring #46

ifree92 opened this issue Jun 29, 2021 · 22 comments

Comments

@ifree92
Copy link
Contributor

ifree92 commented Jun 29, 2021

Hi there,

We want to update our community about the further development of the project.

Short history: about a month before we have transferred the project under our support at @namecheap from @adamdecaf.
Really thanks to the author and all contributors for your time and efforts, much appreciated. Now we will do our best to get it stable asap :)

Upon our learning and debugging of the existing codebase, we have faced a few critical issues that require the following complete refactoring. Before going through we want to receive some feedback from our community.

Current approach & issues

Example:

resource "namecheap_record" "www-example-com" {
  name = "www"
  domain = "example.com"
  address = "127.0.0.1"
  type = "A"
}

resource "namecheap_record" "www-example2-com" {
  name = "www"
  domain = "example2.com"
  address = "127.0.0.1"
  type = "A"
}

resource "namecheap_ns" "www-mydomain-com" {
  domain = "mydomain.com"
  servers = ["ns1.domain.com", "ns2.domain.com"]
}

Currently, we have the approach where each resource item reflects a record for a specific domain (including nameserver record). Our API "setHosts" requires to send back all existing records with(out) amended/created/removed one. This compels to use a mutex to prevent racing, decreases general performance, and probably might be a part of bug #31. One more issue #37 related to requirements of "setHosts" API to provide the EmailType mode with possible values NONE/FWD/MXE/MX/OX that will allow to set up email behavior. In case we will create MX record, we might face an infinity loop (probably one more reason for #31 when we have added an MX record, we're sending a request to create, then upon reading record still doesn't exist and here we have the problem).

Refactoring proposal

Example:

resource "namecheap_domain_records" "www-example-com" {
  domain = "example.com"
  email_type = "MX"

  record {
    hostname = "host1"
    type = "A"
    address = "10.11.12.13"
    ttl = 1800
  }

  record {
    hostname = "host2"
    type = "A"
    address = "11.12.13.14"
  }

  record {
    hostname = "mail"
    type = "MX"
    address = "mail.domain.com"
    mx_pref = 10
    ttl = 1800
  }
}

resource "namecheap_domain_records" "www-example2-com" {
  domain = "example2.com"
  nameservers = ["ns1.domain.com", "ns2.domain.com"]
}

Since terraform provider is processing all resources in parallel, so we can get rid of mutexes and approaches to handle records synchronously, the provider will work much faster and we will have less complexity from a technical perspective to get it very stable (most likely we won't face #31)
email_type will solve the issue when we need to set up some specific email behavior (most likely we will solve #37).
All nuances will be reflected in the detailed documentation later.

Most likely, we will face a challenge to develop a correct migration process. Even though we will provide detailed instructions about how to migrate the existing resource syntax so after apply the requests will be made but nothing will be changed in fact.

One more controversial question is about "resource writing mode". I mean, when you have set up some records in the Domain Control Panel, upon applying new records, should we completely rewrite existing records or create new records despite existing records added manually? (except default parking records).
We decided to go through in "merge" mode. So that might be a default behavior where we're creating new records without completely removing manually added ones. If this question is reasonable, we might add an additional optional mode parameter:

resource "namecheap_domain_records" "www-example2-com" {
  domain = "example2.com"
  mode = "overwrite"
  ...
}

First of all, I'm requesting the comments from the author @adamdecaf (please tag someone else if we have missed someone).

We will do our best to provide a convenient solution.
Cheers 🥂

@awoimbee
Copy link

This makes a lot of sense.

I don't know the details of the API, but to me it looks a lot like aws_security_group and aws_security_group_rule, you could have the same system with namecheap_domain and namecheap_domain_record (the latter being a hypothetical future improvement).
Having namecheap_domain instead of namecheap_domain_records also allows more extensibility, like setting custom name servers (SetNS in the SDK).
The mode argument is a great idea, I can already think of merge, overwrite, abort.

@ifree92
Copy link
Contributor Author

ifree92 commented Jun 30, 2021

@awoimbee Thank you for your inputs. Much appreciated.

Just to be sure I understand correctly, did you mean by abort the cancellation of the process with appropriate error in case we have conflicts between the current state and the "reality" upon reading?

Having namecheap_domain instead of namecheap_domain_records also allows more extensibility, like setting custom name servers (SetNS in the SDK).

Yah, was thinking in the same way. The only reason to keep namecheap_domain_records naming for me is that it means the only list of records for a specific domain (including nameservers if a user wants to use their own name servers). The only concern with namecheap_domain is that someone probably might interpret it as an ability to register/create the domain.
Anyway, we noted your suggestion, thank you.

@awoimbee
Copy link

@ifree92 thanks for doing all the work :)

did you mean by abort the cancellation of the process with appropriate error in case we have conflicts between the current state and the "reality" upon reading?

Exactly, but now that I think about it, it's dumb, the user has to write "yes" in the prompt he does not need additional protection. Anyways, this is a detail / not important.

The only concern with namecheap_domain is that someone probably might interpret it as an ability to register/create the domain.

A Terraform resource that doesn't actually create the resource would be nothing new but yes, definitely not the norm !

@adamdecaf
Copy link
Collaborator

I could see being safe and defaulting mode to "abort" so that even under terraform apply -auto-approve actions are always approved/updated by a human, but it would force everyone to update their .tf files.

If there is a merge mode would it be deprecated in the future?

I like the idea of multiple record{ .. } stanzas. That's a really nice change.

@ifree92
Copy link
Contributor Author

ifree92 commented Jun 30, 2021

@adamdecaf thanks for the input

but it would force everyone to update their .tf files.

Agree, I like the idea.

If there is a merge mode would it be deprecated in the future?

Very good question. I was thinking about leaving exactly merge by default. The major justification for that is our API has some restrictions, e.g. there's impossible to add SRV records. So, user probably wants to add SRV records manually, but without merge they will overwrite manual settings of a domain.
Probably, abort might be a "lazy" version of merge, where before going through the user will be informed about the following changes to provide final approval.

For the first iteration, I was thinking about adding only overwrite and merge. Later, we will see.

@adamdecaf
Copy link
Collaborator

The SRV limitation makes merge a good idea. Perhaps the provider should error when mode is blank, which means the human hasn't updated/reviewed their HCL yet.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 1, 2021

@adamdecaf makes sense.
I was thinking about marking the mode as required to warn the user about setting the behavior explicitly.

@diraol
Copy link

diraol commented Jul 1, 2021

Hi all!

First of all, thanks for all your efforts with this provider! :)

I have only one comment here so far.

Regarding the given proposal:

resource "namecheap_domain_records" "www-example-com" {
  domain = "example.com"
  email_type = "MX"

  record {
    hostname = "host1"
    type = "A"
    address = "10.11.12.13"
    ttl = 1800
  }

  record {
    hostname = "host2"
    type = "A"
    address = "11.12.13.14"
  }

  record {
    hostname = "mail"
    type = "MX"
    address = "mail.domain.com"
    mx_pref = 10
    ttl = 1800
  }
}

The only possible drawback I can see is that it may be a bit hard to "reuse" specific domain record resource.

For example:
If I had:

resource "namecheap_record" "www-example2-com" {
  name = "www"
  domain = "example2.com"
  address = "127.0.0.1"
  type = "A"
}

I'd be able to use something like namecheap_record.www-example2.com.outputs.full_domain (www.example2.com).

By having everything as blocks within a single namecheap_domain_records resource it would be a bit harder.

That's the only idea I'd like to drop here to contribute with the discussion :)

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 2, 2021

@diraol Thanks for the feedback. Actually, well spotted! :)

Let's see what we can do. Probably, we will provide some examples regarding your concerns.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 14, 2021

Thanks to everyone for this discussion, inputs, comments, much appreciated from our side!

We have been published a new major release v2.0.0! You're welcome to test and provide your feedback.
Documentation will become better with time 😸

https://registry.terraform.io/providers/namecheap/namecheap/latest

I'm closing this issue right now. You're welcome to open new ones.

@tranvansang
Copy link

tranvansang commented Jul 15, 2021

@ifree92 Thank you for maintaining this project officially.
I have been waiting for this so far.

I have one question needed to figure out before migrating my configuration to v2.0.

Currently, I am using namecheap_record to configure records separately. For example, I group SendGrid related records in a separated module.

From v2.0: I need to place all records in a namecheap_domain_records. Does this mean that migrating to v2.0 will lose the ability to separate records in separated modules?

Route53 has the same requirement. I think there may be the same reason for Namecheap to decide to add this new constraint (I was thinking that the ability of separating modules was superior from namecheap compared to route53). I just want to confirm this fact before doing my migration.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 15, 2021

@tranvansang

Please have a look at MERGE mode.

Probably, this might be a solution for your case. Let me provide you with examples.

resource "namecheap_domain_records" "www-domain-resource-one" {
  domain = "domain.com"
  mode = "MERGE"

  record {
    hostname = "dev"
    type = "A"
    address = "10.11.12.13"
  }
}
resource "namecheap_domain_records" "www-domain-resource-two" {
  domain = "domain.com"
  mode = "MERGE"

  record {
    hostname = "blog"
    type = "A"
    address = "11.22.33.44"
  }
}

In different modules, after applying the resources below, your domain will contain two records for dev.* and blog.*.

But this approach has some restrictions: you shouldn't apply them simultaneously, only synchronously. Otherwise, the racing will be a problem. (Under the hood, to apply a record we have to fetch the existing state of domain, modify by adding/updating/removing a record and apply back the whole list of records. In the case of applying two resources simultaneously, the provider will twice fetch the state before two resources, add one and apply back so as a result, we might have only one record/resource applied. Keep it in mind).

@tranvansang
Copy link

@ifree92 GREAT!! <3. Thank you very much.

@tranvansang
Copy link

@ifree92 is there a way to import existing records to a resource? I dont see it in the current docs.

I am going to migrate from v1 by manually importing the existing records created by provider v1.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 15, 2021

@tranvansang

is there a way to import existing records to a resource?
The only way described here: https://registry.terraform.io/providers/namecheap/namecheap/latest/docs/guides/namecheap_provider_migration_v2.0.0

Since we have completely changed the resource schema and now the one resource might contain many records, that's hard to migrate a state correctly automatically. Unfortunately, you have to migrate your terraform code manually.

Let me know if you have more questions.

@ifree92 ifree92 reopened this Jul 15, 2021
@ifree92 ifree92 closed this as completed Jul 15, 2021
@tranvansang
Copy link

tranvansang commented Jul 15, 2021

@ifree92 i didnt mean to ask how to import from v1 code.

I meant to know how to import from an existing record and manage them via terraform resource (regardless of where it was created)

Do you mean that I need to remove all the records once and re-defining them with the terraform resources?

@tranvansang
Copy link

I have one more question.

Under the hood, to apply a record we have to fetch the existing state of domain, modify by adding/updating/removing a record and apply back the whole list of records.

If this is the case. How do you manage to know which records belong to a resource to distinguish them from records created in other places?

Previously, v1 calculated and used hash.

Im curious about the logic in v2 to know any caveat beforehand.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 15, 2021

@tranvansang

Do you mean that I need to remove all the records once and re-defining them with the terraform resources?

Short answer - yes, you should remove old records and re-create them using a new provider. I will revert to this soon.

Im curious about the logic in v2 to know any caveat beforehand.

Basically, the same approach. However, instead of using "hash" function that is creating hash by a combination of hostname, record type, address, we're using just a concatenated string to distinguish records.

However, I see some improvements that I should make for new records. E.g., if a record already exists upon creating, I shouldn't "re-create" it, but mark it as "successfully created" to prevent conflicts and duplicates.

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 15, 2021

@tranvansang could you please provide a bit more details about your case with multiple resources for different modules.
Do you mean you would like to apply multiple resources related to the same domain through the same terraform instance or different?

This is quite mandatory for us to understand your case to make correct improvements. If you're using the same terraform instance and have multiple resources (with different records) related to the same domain, then the possible solution at the moment is to use -parallelism=1 parameter (read more). In this case, we probably need to improve the sequence of records applying using mutexes and some internal mechanisms.

We would much appreciate your inputs on this point. Thank you.

@ifree92 ifree92 reopened this Jul 15, 2021
@tranvansang
Copy link

@ifree92

could you please provide a bit more details about your case with multiple resources for different modules.
Do you mean you would like to apply multiple resources related to the same domain through the same terraform instance or different?

I am using both.

For the same domain, I have multiple services installed to its own sub-domain. Thus, I separated them into separated terraform instances.

For one instance, I use sub-modules which might include a namecheap record.
For example: AWS SES/cloud front/sendgrid, etc.

@adamdecaf
Copy link
Collaborator

adamdecaf commented Jul 15, 2021

We should clarify that depends_on is required when using MERGE mode.

resource "namecheap_domain_records" "www-domain-resource-one" {
  // ... 
  depends_on = [namecheap_domain_records.www-domain-resource-two]
  // ...
}

@ifree92
Copy link
Contributor Author

ifree92 commented Jul 29, 2021

We have made mandatory improvements to allow users to work with different records through the multiple resources that are related to the same domain.

This mandatory improvement will be included in v2.0.1 that will be released in a day or so.

Feel free to re-open this issue.

@ifree92 ifree92 closed this as completed Jul 29, 2021
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

No branches or pull requests

5 participants