nns: Add support global domain #102
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-contract#102
Loading…
Reference in a new issue
No description provided.
Delete branch "achuprov/frostfs-contract:feta/global_domain"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Alexander Chuprov a.chuprov@yadro.com
8a472c0f57
toa0388358df
a0388358df
to10cdba59b9
WIP: [#9999] nns: Add support global domainto [#9999] nns: Add support global domain[#9999] nns: Add support global domainto nns: Add support global domaineither
add support for
or justsupport
@ -18,6 +18,8 @@ permissions:
- "register"
- "transferX"
- "update"
- "getGlobalDomain"
Container contract should know nothing about NNS features.
Fixed. Now the
global domain zone
is created withaddRecord
and removed withdeleteRecords
.Need description, it is unclear what problem this PR solves and why.
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.
10cdba59b9
tod0f6a5ef19
@ -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.
The comment is in the wrong place, it was commenting public function.
fixed
d0f6a5ef19
tob586029177
@ -3,3 +3,3 @@
safemethods: ["balanceOf", "decimals", "symbol", "totalSupply", "tokensOf", "ownerOf",
"tokens", "properties", "roots", "getPrice", "isAvailable", "getRecords",
"resolve", "version"]
"resolve", "version","globalDomainInfo"]
Can we make this a separate function instead of contract method?
Do you mean to remove this function from the contract and implement it on the client side?
Yes, like we resolve NNS contract hashes in the frostfs-adm
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 aCNAME
or aTXT
recordcnametgt
.But you are right, this is the more expected behavior of NNS.
Fixed
@ -237,0 +269,4 @@
}
// GlobalDomainInfo provides domain details including global zone aliases and activation status.
func GlobalDomainInfo(name string) string {
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.
Fixed
@ -237,0 +319,4 @@
tokenID := []byte(tokenIDFromName(name))
records := getRecordsByType(ctx, tokenID, name, TXT)
if records == nil {
Do we need this special case here?
Yes, that's possible if there are no
TXT
records.It is possible, but don't we handle this naturally with the following range loop?
We cannot perform a range on
nil
.@ -430,0 +578,4 @@
if status != nil || globalDomain != "" {
storage.Delete(ctx, globalDomainKey)
deleteDomain(ctx, globalDomain)
DeleteRecords(name, CNAME)
Why do we delete all CNAME records here instead of the specific one.
We can have only one
CNAME
record per domain. Therefore, deleting allCNAME
records for the domain means deleting a specificCNAME
record.if typ == CNAME && id != 0 {
@ -577,0 +739,4 @@
recordKey := getIdRecordKey(tokenId, string(tokenId), SOA, id)
recBytes := storage.Get(ctx, recordKey)
if recBytes == nil {
Same here, do we have a test for this branch?
fixed
@ -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")
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.
fixed
@ -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
As you have introduced a new key in the contract storage, please, make it mentioned in the scheme table in
doc.go
Thanks, fixed
b586029177
toad38f6f9ec
ad38f6f9ec
to138ee648b0
138ee648b0
tod2d4ae0c89
d2d4ae0c89
to8884ada827
8884ada827
tocf3c3f36c8
@ -231,9 +240,70 @@ func IsAvailable(name string) bool {
return true
}
checkParentExists(ctx, fragments)
if !checkAvailableGlobalDomain(ctx, fragments) {
Both usages of
check
panic on failure.Can we panic inside? See
checkParentExists
, this is an ok practice in contracts.fixed
@ -232,2 +241,4 @@
}
checkParentExists(ctx, fragments)
if !checkAvailableGlobalDomain(ctx, fragments) {
panic("global domain is already taken")
See #98
It would be nice to mention this global domain name.
fixed
@ -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.
same here
fixed
@ -237,0 +261,4 @@
}
// GetGlobalDomain returns the global domain.
func getGlobalDomain(ctx storage.Context, fragments []string) string {
Doc-comment should start with a function name (
s/G/g/
)fixed
@ -237,0 +265,4 @@
name := ""
for i := 1; i < len(fragments); i++ {
if name != "" {
name = name + "."
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.
fixed
@ -237,0 +294,4 @@
}
if fragments[0] == Cnametgt {
globalDomain = fragments[1]
break
here?fixed
@ -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",
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 toregister
in terms of creating a global domain record.Fixed. You are right; however, we can't create SOA records using
SetRecord
func checkBaseRecords(typ RecordType, data string) bool {
cf3c3f36c8
to3fd80ffa25
@ -237,0 +255,4 @@
if nsBytes != nil {
panic("global domain is already taken: " + globalDomain)
}
return
useless
Fixd
@ -335,0 +430,4 @@
deleteDomain(ctx, globalDomain[0])
}
}
DeleteRecords(name, TXT)
What about
A
records?Fixd
@ -428,2 +530,4 @@
recordsKey := getRecordsKeyByType(tokenID, name, typ)
records := storage.Find(ctx, recordsKey, storage.KeysOnly)
if typ == TXT {
We have
records
iterator, which could containcnametgt=...
value.Why do we have a separate branch here instead of checking iterator value in the loop?
The loop is located here. When we delete all
TXT
records, it means that we have deleted the container.That is ok, my question relates to the execution of this branch for every domain vs only for the one containing
cnametgt=...
TXT recordAlright. 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")
Hmm, why is it available right after we had registered it?
From the RFC: 'When
GlobalCNAMEZone
is enabled, existing buckets will not be registered in the global zone.' This domain zone was created beforeGlobalCNAMEZone
was enabled. Creating a new TXT record should not affect this.3fd80ffa25
to5c46316e4a
Please, implement
DeletDomain
feature in a separate commit.@ -335,0 +430,4 @@
deleteDomain(ctx, globalDomain[0])
}
}
DeleteRecords(name, TXT)
Each of this calls gets its own
storage.Context
. How about reusing it?fixed
5c46316e4a
toc23bc2c7bc
c23bc2c7bc
to39671b9c33
39671b9c33
tod55125e18e
@ -234,0 +245,4 @@
var id byte
tokenId := []byte(tokenIDFromName(name))
recordKey := getIdRecordKey(tokenId, name, CNAME, id)
Why do we check for
CNAME
only and withid=0
?It seems the right way is to check for any records.
Also, if we have a domain
b.c
and recorda.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))
default
Something instead of magic constants?@ -236,0 +285,4 @@
stackitem.NewBuffer([]byte("bober.animals")),
})
cNNS.Invoke(t, false, "isAvailable", "bober.animals")
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?
added tests to container_test
d55125e18e
to40e1b132d5
40e1b132d5
tof005f86acf
f005f86acf
to82e04b6c32