nns: Add support global domain #102

Merged
fyrchik merged 1 commit from achuprov/frostfs-contract:feta/global_domain into master 2024-09-04 19:51:18 +00:00
Member

global_domain_zone-Page-2.drawio
global_domain_zone-Page-1.drawio

frostfs-adm morph frostfsid  create-namespace    --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml  --namespace="poland"
frostfs-adm morph frostfsid  create-namespace    --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml  --namespace="sweden"


frostfs-adm morph nns register --name="poland" --email="email@email.email"  --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml
frostfs-adm morph nns register --name="sweden" --email="email@email.email"  --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml
frostfs-adm morph nns register --name="animals" --email="email@email.email"  --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml

frostfs-adm morph nns  add-record --name="poland" --data="cnametgt=animals"  --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --type="txt"
frostfs-adm morph nns  add-record --name="sweden" --data="cnametgt=animals"  --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --type="txt"


frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container create json --policy "REP 1 IN X CBF 1 SELECT 1 FROM * AS X" --basic-acl public-read-write  --nns-name="bober" --nns-zone="poland"
frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container create json --policy "REP 1 IN X CBF 1 SELECT 1 FROM * AS X" --basic-acl public-read-write  --nns-name="bober" --nns-zone="sweden"

frostfs-adm morph nns tokens     --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml


frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container delete --cid Gb9ErSoAHEzgnTCJJMiYHGW4gr7D2jmpai4Uj8kC3ZcB
frostfs-adm morph nns tokens     --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml


Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

![global_domain_zone-Page-2.drawio](/attachments/4fe1395d-576d-44c1-9bc1-ecd1225bef34) ![global_domain_zone-Page-1.drawio](/attachments/a49d6e1a-53f3-4aed-92f1-db96c89b2900) <details> <summary></summary> ``` frostfs-adm morph frostfsid create-namespace --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --namespace="poland" frostfs-adm morph frostfsid create-namespace --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --namespace="sweden" frostfs-adm morph nns register --name="poland" --email="email@email.email" --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml frostfs-adm morph nns register --name="sweden" --email="email@email.email" --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml frostfs-adm morph nns register --name="animals" --email="email@email.email" --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml frostfs-adm morph nns add-record --name="poland" --data="cnametgt=animals" --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --type="txt" frostfs-adm morph nns add-record --name="sweden" --data="cnametgt=animals" --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml --type="txt" frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container create json --policy "REP 1 IN X CBF 1 SELECT 1 FROM * AS X" --basic-acl public-read-write --nns-name="bober" --nns-zone="poland" frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container create json --policy "REP 1 IN X CBF 1 SELECT 1 FROM * AS X" --basic-acl public-read-write --nns-name="bober" --nns-zone="sweden" frostfs-adm morph nns tokens --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml frostfs-cli --config /home/achuprov/Documents/work/frostfs-dev-env/wallets/conf.yml container delete --cid Gb9ErSoAHEzgnTCJJMiYHGW4gr7D2jmpai4Uj8kC3ZcB frostfs-adm morph nns tokens --config /home/achuprov/Documents/work/frostfs-dev-env/frostfs-adm.yml ``` </details> Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov added 1 commit 2024-07-31 15:03:45 +00:00
[#9999] nns: Add support global domain
All checks were successful
DCO action / DCO (pull_request) Successful in 56s
Tests / Tests (1.21) (pull_request) Successful in 1m40s
Tests / Tests (1.22) (pull_request) Successful in 1m43s
8a472c0f57
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov force-pushed feta/global_domain from 8a472c0f57 to a0388358df 2024-08-02 18:39:01 +00:00 Compare
achuprov force-pushed feta/global_domain from a0388358df to 10cdba59b9 2024-08-06 11:14:24 +00:00 Compare
achuprov changed title from WIP: [#9999] nns: Add support global domain to [#9999] nns: Add support global domain 2024-08-06 11:14:34 +00:00
achuprov requested review from storage-core-committers 2024-08-06 11:14:42 +00:00
achuprov requested review from storage-core-developers 2024-08-06 11:14:43 +00:00
achuprov requested review from r.loginov 2024-08-06 11:14:56 +00:00
fyrchik changed title from [#9999] nns: Add support global domain to nns: Add support global domain 2024-08-06 11:15:40 +00:00
Owner

either add support for or just support

either `add support for` or just `support`
fyrchik reviewed 2024-08-06 11:16:22 +00:00
@ -18,6 +18,8 @@ permissions:
- "register"
- "transferX"
- "update"
- "getGlobalDomain"
Owner

Container contract should know nothing about NNS features.

Container contract should know nothing about NNS features.
Author
Member

Fixed. Now the global domain zone is created with addRecord and removed with deleteRecords.

Fixed. Now the `global domain zone` is created with `addRecord` and removed with `deleteRecords`.
achuprov marked this conversation as resolved
dstepanov-yadro requested changes 2024-08-06 14:32:23 +00:00
dstepanov-yadro left a comment
Member

Need description, it is unclear what problem this PR solves and why.

Need description, it is unclear what problem this PR solves and why.
Owner

I recommend not introducing a single global domain zone. Instead, consider allowing the use of target CNAME domains for each zone individually. This approach provides the flexibility to maintain a single global CNAME zone while also supporting multiple domains with either individual or grouped CNAME zones. This enhances versatility and adaptability.

I recommend not introducing a single global domain zone. Instead, consider allowing the use of target CNAME domains for each zone individually. This approach provides the flexibility to maintain a single global CNAME zone while also supporting multiple domains with either individual or grouped CNAME zones. This enhances versatility and adaptability.
achuprov force-pushed feta/global_domain from 10cdba59b9 to d0f6a5ef19 2024-08-07 10:10:55 +00:00 Compare
fyrchik reviewed 2024-08-07 11:27:35 +00:00
@ -268,0 +343,4 @@
return register(ctx, name, owner, email, refresh, retry, expire, ttl)
}
// Register registers a new domain with the specified owner and name if it's available.
Owner

The comment is in the wrong place, it was commenting public function.

The comment is in the wrong place, it was commenting public function.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
achuprov force-pushed feta/global_domain from d0f6a5ef19 to b586029177 2024-08-08 09:27:55 +00:00 Compare
fyrchik reviewed 2024-08-08 10:34:04 +00:00
nns/config.yml Outdated
@ -3,3 +3,3 @@
safemethods: ["balanceOf", "decimals", "symbol", "totalSupply", "tokensOf", "ownerOf",
"tokens", "properties", "roots", "getPrice", "isAvailable", "getRecords",
"resolve", "version"]
"resolve", "version","globalDomainInfo"]
Owner

Can we make this a separate function instead of contract method?

Can we make this a separate function instead of contract method?
Author
Member

Do you mean to remove this function from the contract and implement it on the client side?

Do you mean to remove this function from the contract and implement it on the client side?
Owner

Yes, like we resolve NNS contract hashes in the frostfs-adm

Yes, like we resolve NNS contract hashes in the frostfs-adm
Author
Member

For each domain, we will need to make 2-3 queries instead of one. Additionally, we won't have access to the data stored under prefixGlobalDomain. We will only be able to verify the existence of a CNAME or a TXT record cnametgt.

For each domain, we will need to make 2-3 queries instead of one. Additionally, we won't have access to the data stored under `prefixGlobalDomain`. We will only be able to verify the existence of a `CNAME` or a `TXT` record `cnametgt`.
Author
Member

But you are right, this is the more expected behavior of NNS.

But you are right, this is the more expected behavior of NNS.
Author
Member

Fixed

Fixed
achuprov marked this conversation as resolved
fyrchik requested changes 2024-08-08 11:38:07 +00:00
Dismissed
@ -237,0 +269,4 @@
}
// GlobalDomainInfo provides domain details including global zone aliases and activation status.
func GlobalDomainInfo(name string) string {
Owner

I don't like this whole Global thing being seen in the NNS interface.
In was defined in terms of other NNS features, we should be able to work with it via an already existing interface.

I don't like this whole `Global` thing being seen in the NNS interface. In was defined in terms of other NNS features, we should be able to work with it via an already existing interface.
Author
Member

Fixed

Fixed
achuprov marked this conversation as resolved
@ -237,0 +319,4 @@
tokenID := []byte(tokenIDFromName(name))
records := getRecordsByType(ctx, tokenID, name, TXT)
if records == nil {
Owner

Do we need this special case here?

Do we need this special case here?
Author
Member

Yes, that's possible if there are no TXT records.

Yes, that's possible if there are no `TXT` records.
Owner

It is possible, but don't we handle this naturally with the following range loop?

It is possible, but don't we handle this naturally with the following range loop?
Author
Member

We cannot perform a range on nil.

We cannot perform a range on `nil`.
fyrchik marked this conversation as resolved
@ -430,0 +578,4 @@
if status != nil || globalDomain != "" {
storage.Delete(ctx, globalDomainKey)
deleteDomain(ctx, globalDomain)
DeleteRecords(name, CNAME)
Owner

Why do we delete all CNAME records here instead of the specific one.

Why do we delete all CNAME records here instead of the specific one.
Author
Member

We can have only one CNAME record per domain. Therefore, deleting all CNAME records for the domain means deleting a specific CNAME record.

if typ == CNAME && id != 0 {

We can have only one `CNAME` record per domain. Therefore, deleting all `CNAME` records for the domain means deleting a specific `CNAME` record. https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/b586029177069a9fe5b7600ae3fedf5059e4b8a2/nns/nns_contract.go#L757
fyrchik marked this conversation as resolved
@ -577,0 +739,4 @@
recordKey := getIdRecordKey(tokenId, string(tokenId), SOA, id)
recBytes := storage.Get(ctx, recordKey)
if recBytes == nil {
Owner

Same here, do we have a test for this branch?

Same here, do we have a test for this branch?
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -577,0 +745,4 @@
rec := std.Deserialize(recBytes.([]byte)).(RecordState)
split := std.StringSplitNonEmpty(rec.Data, " ")
if len(split) != 7 {
panic("invalid SOA record for the global domain")
Owner

In this case the panic will happen below. Anyway, it seems impossible to have sth here, because we get a valid SOA record from the contract state.

If this line is reachable, I would like to see the test for it, if no -- let's remove.

In this case the panic will happen below. Anyway, it seems impossible to have sth here, because we get a valid SOA record from the contract state. If this line is reachable, I would like to see the test for it, if no -- let's remove.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
aarifullin reviewed 2024-08-08 14:14:32 +00:00
@ -43,1 +43,4 @@
prefixRecord byte = 0x22
//prefixGlobalDomain contains a flag indicating that this domain was created using GlobalDomain.
//This is necessary to distinguish it from regular CNAME records.
prefixGlobalDomain byte = 0x23
Member

As you have introduced a new key in the contract storage, please, make it mentioned in the scheme table in doc.go

As you have introduced a new key in the contract storage, please, make it mentioned in the scheme table in `doc.go`
Author
Member

Thanks, fixed

Thanks, fixed
aarifullin marked this conversation as resolved
achuprov force-pushed feta/global_domain from b586029177 to ad38f6f9ec 2024-08-08 16:37:19 +00:00 Compare
achuprov force-pushed feta/global_domain from ad38f6f9ec to 138ee648b0 2024-08-08 16:43:14 +00:00 Compare
aarifullin approved these changes 2024-08-09 09:25:03 +00:00
achuprov force-pushed feta/global_domain from 138ee648b0 to d2d4ae0c89 2024-08-09 11:38:22 +00:00 Compare
achuprov force-pushed feta/global_domain from d2d4ae0c89 to 8884ada827 2024-08-09 13:31:24 +00:00 Compare
achuprov force-pushed feta/global_domain from 8884ada827 to cf3c3f36c8 2024-08-09 13:40:43 +00:00 Compare
fyrchik requested changes 2024-08-12 07:25:38 +00:00
Dismissed
@ -231,9 +240,70 @@ func IsAvailable(name string) bool {
return true
}
checkParentExists(ctx, fragments)
if !checkAvailableGlobalDomain(ctx, fragments) {
Owner

Both usages of check panic on failure.
Can we panic inside? See checkParentExists, this is an ok practice in contracts.

Both usages of `check` panic on failure. Can we panic inside? See `checkParentExists`, this is an ok practice in contracts.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -232,2 +241,4 @@
}
checkParentExists(ctx, fragments)
if !checkAvailableGlobalDomain(ctx, fragments) {
panic("global domain is already taken")
Owner

See #98

It would be nice to mention this global domain name.

See https://git.frostfs.info/TrueCloudLab/frostfs-contract/pulls/98 It would be nice to mention this global domain name.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -234,3 +246,4 @@
return storage.Get(ctx, append([]byte{prefixName}, getTokenKey([]byte(name))...)) == nil
}
// CheckAvailableGlobalDomain - triggers a panic if the global domain name is occupied.
Owner

same here

same here
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -237,0 +261,4 @@
}
// GetGlobalDomain returns the global domain.
func getGlobalDomain(ctx storage.Context, fragments []string) string {
Owner

Doc-comment should start with a function name (s/G/g/)

Doc-comment should start with a function name (`s/G/g/`)
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -237,0 +265,4 @@
name := ""
for i := 1; i < len(fragments); i++ {
if name != "" {
name = name + "."
Owner

You first split string, and then build it again.
It could be more beneficial (in terms of code size and spent GAS) to just strip the prefix from the whole string.

You first split string, and then build it again. It could be more beneficial (in terms of code size and spent GAS) to just strip the prefix from the whole string.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -237,0 +294,4 @@
}
if fragments[0] == Cnametgt {
globalDomain = fragments[1]
Owner

break here?

`break` here?
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -160,0 +166,4 @@
"testdomain.com", int64(nns.TXT), nns.Cnametgt+"=globaldomain.com")
c.Invoke(t, true, "isAvailable", "dom.testdomain.com")
c3.Invoke(t, true, "register",
Owner

Could you move these cases to a separate Test*?
I also do not see the tests for AddRecord -- I believe the behaviour should be similar to register in terms of creating a global domain record.

Could you move these cases to a separate `Test*`? I also do not see the tests for `AddRecord` -- I believe the behaviour should be similar to `register` in terms of creating a global domain record.
Author
Member

Fixed. You are right; however, we can't create SOA records using SetRecord

func checkBaseRecords(typ RecordType, data string) bool {

Fixed. You are right; however, we can't create SOA records using `SetRecord` https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/49e5270f673e228b6b0dc33e1ff41ea7ef7e08b6/nns/nns_contract.go#L382
achuprov marked this conversation as resolved
achuprov force-pushed feta/global_domain from cf3c3f36c8 to 3fd80ffa25 2024-08-12 12:21:37 +00:00 Compare
fyrchik reviewed 2024-08-12 14:13:01 +00:00
@ -237,0 +255,4 @@
if nsBytes != nil {
panic("global domain is already taken: " + globalDomain)
}
return
Owner

useless

useless
Author
Member

Fixd

Fixd
achuprov marked this conversation as resolved
@ -335,0 +430,4 @@
deleteDomain(ctx, globalDomain[0])
}
}
DeleteRecords(name, TXT)
Owner

What about A records?

What about `A` records?
Author
Member

Fixd

Fixd
achuprov marked this conversation as resolved
@ -428,2 +530,4 @@
recordsKey := getRecordsKeyByType(tokenID, name, typ)
records := storage.Find(ctx, recordsKey, storage.KeysOnly)
if typ == TXT {
Owner

We have records iterator, which could contain cnametgt=... value.
Why do we have a separate branch here instead of checking iterator value in the loop?

We have `records` iterator, which could contain `cnametgt=...` value. Why do we have a separate branch here instead of checking iterator value in the loop?
Author
Member

The loop is located here. When we delete all TXT records, it means that we have deleted the container.

The loop is located [here](https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/3fd80ffa25f60a6511d4014c60a842863ab52cf1/nns/nns_contract.go#L289). When we delete all `TXT` records, it means that we have deleted the container.
Owner

That is ok, my question relates to the execution of this branch for every domain vs only for the one containing cnametgt=... TXT record

That is ok, my question relates to the execution of this branch for every domain vs only for the one containing `cnametgt=...` TXT record
Author
Member

Alright. Now we enter this branch if the globalDomainFlag exists.

Alright. Now we enter this branch if the `globalDomainFlag` exists.
@ -162,0 +183,4 @@
c1.Invoke(t, stackitem.Null{}, "addRecord",
"domik.testdomain.com", int64(nns.TXT), "CID")
c.Invoke(t, true, "isAvailable", "domik.globaldomain.com")
Owner

Hmm, why is it available right after we had registered it?

Hmm, why is it available right after we had registered it?
Author
Member

From the RFC: 'When GlobalCNAMEZone is enabled, existing buckets will not be registered in the global zone.' This domain zone was created before GlobalCNAMEZone was enabled. Creating a new TXT record should not affect this.

From the RFC: 'When `GlobalCNAMEZone` is enabled, existing buckets will not be registered in the global zone.' This domain zone was created before `GlobalCNAMEZone` was enabled. Creating a new TXT record should not affect this.
achuprov marked this conversation as resolved
achuprov force-pushed feta/global_domain from 3fd80ffa25 to 5c46316e4a 2024-08-12 16:27:59 +00:00 Compare
fyrchik approved these changes 2024-08-13 06:54:45 +00:00
Dismissed
fyrchik left a comment
Owner

Please, implement DeletDomain feature in a separate commit.

Please, implement `DeletDomain` _feature_ in a separate commit.
@ -335,0 +430,4 @@
deleteDomain(ctx, globalDomain[0])
}
}
DeleteRecords(name, TXT)
Owner

Each of this calls gets its own storage.Context. How about reusing it?

Each of this calls gets its own `storage.Context`. How about reusing it?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
fyrchik dismissed fyrchik's review 2024-08-13 06:54:56 +00:00
achuprov force-pushed feta/global_domain from 5c46316e4a to c23bc2c7bc 2024-08-13 13:00:25 +00:00 Compare
achuprov force-pushed feta/global_domain from c23bc2c7bc to 39671b9c33 2024-08-15 06:20:36 +00:00 Compare
achuprov force-pushed feta/global_domain from 39671b9c33 to d55125e18e 2024-08-15 06:25:49 +00:00 Compare
fyrchik reviewed 2024-08-15 13:41:16 +00:00
@ -234,0 +245,4 @@
var id byte
tokenId := []byte(tokenIDFromName(name))
recordKey := getIdRecordKey(tokenId, name, CNAME, id)
Owner

Why do we check for CNAME only and with id=0?
It seems the right way is to check for any records.

Why do we check for `CNAME` only and with `id=0`? It seems the right way is to check for any records.
Owner

Also, if we have a domain b.c and record a.b.c, will we behave correctly?

Also, if we have a domain `b.c` and record `a.b.c`, will we behave correctly?
@ -236,0 +247,4 @@
cNNS := c.CommitteeInvoker(nnsHash)
cNNS.Invoke(t, true, "register",
"animals", c.CommitteeHash,
"whateveriwant@world.com", int64(0), int64(0), int64(100_000), int64(0))
Owner

defaultSomething instead of magic constants?

`default`Something instead of magic constants?
achuprov marked this conversation as resolved
@ -236,0 +285,4 @@
stackitem.NewBuffer([]byte("bober.animals")),
})
cNNS.Invoke(t, false, "isAvailable", "bober.animals")
Owner

Not only we do want to have this record, but we also want it to resolve to something (via our resolve method).
Can we add a tests for this?

Not only we do want to have this record, but we also want it to resolve to something (via our `resolve` method). Can we add a tests for this?
Author
Member

added tests to container_test

added tests to container_test
achuprov marked this conversation as resolved
achuprov force-pushed feta/global_domain from d55125e18e to 40e1b132d5 2024-08-16 13:31:33 +00:00 Compare
achuprov force-pushed feta/global_domain from 40e1b132d5 to f005f86acf 2024-08-16 13:33:34 +00:00 Compare
achuprov force-pushed feta/global_domain from f005f86acf to 82e04b6c32 2024-08-16 13:39:03 +00:00 Compare
fyrchik approved these changes 2024-08-16 14:11:48 +00:00
aarifullin approved these changes 2024-08-16 14:16:32 +00:00
fyrchik merged commit 82e04b6c32 into master 2024-08-16 14:18:52 +00:00
fyrchik referenced this pull request from a commit 2024-08-16 14:18:54 +00:00
fyrchik referenced this pull request from a commit 2024-08-16 14:18:54 +00:00
fyrchik referenced this pull request from a commit 2024-08-19 15:23:33 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-contract#102
No description provided.