cli: unify usage of expired-at flag instead of lifetime flag #2521

AliceInHunterland · 2023-08-21T07:03:56Z

The expired-at flag replaces the deprecated lifetime flag.

Closes #1574.

codecov · 2023-08-21T07:07:36Z

Codecov Report

Merging #2521 (bc2d82a) into master (f5d80b0) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head bc2d82a differs from pull request most recent head 3b463c0. Consider uploading reports for the commit 3b463c0 to get more accurate results

@@           Coverage Diff           @@
##           master    #2521   +/-   ##
=======================================
  Coverage   29.68%   29.69%           
=======================================
  Files         404      404           
  Lines       30711    30711           
=======================================
+ Hits         9117     9119    +2     
+ Misses      20825    20823    -2     
  Partials      769      769

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

carpawell

@cthulhu-rider, @roman-khimov, have we decied to deprecate lifetime? I find it usefull if you do not want to look for the current epoch number. I can imagine the future where every cmd has both expire-at and lifetime (or a better naming option).

cmd/neofs-cli/modules/storagegroup/put.go

AliceInHunterland · 2023-08-21T13:00:09Z

@carpawell Okay, I thought nspcc-dev/neofs-testcases#601 and "Unify" lead to lifetime depreciation. By "Unify epoch expiration flag" #1574 u mean creating both options (lifetime and expired-at) for each applicable command?

carpawell · 2023-08-21T13:06:23Z

u mean creating both options (lifetime and expired-at) for each applicable command?

We can disduss it. IMO, both may be helpfull but for every command then cause that is what meant by "unify". Let's wait for other's' opinions.

cthulhu-rider · 2023-08-22T09:14:11Z

from my pov as user, i don't find it convenient to work with an absolute epoch value

for example, current epoch is 18446740073709551615. It'd be very incovenient to specify absolute expiration epoch

if we imagine user or script that requests netmap netinfo first, then calculates final absolute value - it will +E exact value anyway, so lifetime is again more appropriate

absolute value may be useful when user wanna specify "infinite" value (extremely big or even max). We can also cover this with lifetime - +inf is inf

lifetime value saves user from past epoch setting, while absolute value allows to test fault conditions (e.g. in neofs-testcases)

lifetime won't work if command doesn't accept NeoFS network endpoint

in total, lifetime covers 99+% usecases while expires-at is not very usable in real practice

i'd support both flags in all commands and make them mutual exclusive: if both set, command fails (i don't like priorities, pls set one or nothing)

roman-khimov · 2023-08-22T09:18:45Z

expires-at is not very usable in real practice

I don't think so, you may want to add things over time that should expire all at once. Suppose you have some process running for a day producing content to be stored in NeoFS, but that should be deleted eventually. It'd be a bit inconvenient to have it expire piece by piece in different epochs.

i'd support both flags in all commands and make them mutual exclusive: if both set, command fails (i don't like priorities, pls set one or nothing)

But I absolutely agree with that, it's not hard to have both.

cthulhu-rider · 2023-08-22T09:26:46Z

things over time that should expire all at once

yeah, good example for absolute value. Always forget that CLI usage is much wider than i expect

AliceInHunterland · 2023-08-22T10:21:15Z

i'd support both flags in all commands and make them mutual exclusive: if both set, command fails (i don't like priorities, pls set one or nothing)

But I absolutely agree with that, it's not hard to have both.

however, for example, for bearer create command we have "All epoch flags can be specified relative to the current epoch with the +n syntax.", so it means adding the lifetime flag is redundant to this command. is it okay to add a lifetime flag or maybe we can do the same approach with a relative epoch to expired-at flag everywhere and deprecate lifetime?

cthulhu-rider · 2023-08-22T10:44:02Z

i'd make it simple - for each command with expiration context:

if command accepts NeoFS RPC endpoint (-r), provide both lifetime and expires-at flags
otherwise, provide expires-at only

P.S. lifetime w/o -r is obviously unusable

for tokens, only exp claim may be relative, relative setting of iat and nbf makes no sense imo

cmd/neofs-cli/modules/bearer/create.go

cmd/neofs-cli/modules/object/put.go

cmd/neofs-cli/modules/bearer/create.go

carpawell · 2023-08-22T17:40:57Z

cmd/neofs-cli/modules/session/create.go

+		ni, err := internalclient.NetworkInfo(ctx, netInfoPrm)
+		common.ExitOnErr(cmd, "can't get current epoch: %w", err)
+		currEpoch := ni.NetworkInfo().CurrentEpoch()
+		lifetime = exp - currEpoch


i think i would be better to change CreateSession definition cause you do NetworkInfo here to do NetworkInfo one more time inside CreateSession later

or at least check that exp > currEpoch

cmd/neofs-cli/modules/storagegroup/put.go

CHANGELOG.md

roman-khimov · 2023-08-22T18:09:26Z

cmd/neofs-cli/modules/session/create.go

+	flags.StringP(commonflags.Account, commonflags.AccountShorthand, commonflags.AccountDefault, commonflags.AccountUsage)
+	flags.String(outFlag, "", "File to write session token to")
+	flags.Bool(jsonFlag, false, "Output token in JSON")
+	flags.StringP(commonflags.RPC, commonflags.RPCShorthand, commonflags.RPCDefault, commonflags.RPCUsage)


Unrelated as well.

carpawell · 2023-08-22T18:37:18Z

BTW, @roman-khimov, @vvarg229, @evgeniiz321 some issues for testing it?

CHANGELOG.md

cmd/neofs-cli/modules/bearer/create.go

cmd/neofs-cli/modules/session/create.go

CHANGELOG.md

cmd/neofs-cli/modules/session/create.go

CHANGELOG.md

roman-khimov · 2023-08-25T08:22:36Z

Requires a rebase now to solve CHANGELOG conflict.

carpawell · 2023-08-25T12:20:15Z

cmd/neofs-cli/modules/object/util.go

+	currEpoch, err := internal.GetCurrentEpoch(ctx, endpoint)
+	common.ExitOnErr(cmd, "can't fetch current epoch: %w", err)
+	exp := currEpoch + sessionLifetime
+	tok.SetNbf(currEpoch)


well, i see, some my suggestion conflicts with the reallity. i would not set some parts of a token here and some on the CreateSession side cause it bug-prone IMO

can we pass currEpoch as a param instead? or any another alternative that does not make a caller set nbf and iat outside?

Well, we can (made it in the last commit), however, actually it seems that at the beginning (#2521 (comment)) when we were passing lifetime to the CreateSession it was more optimal in numbers of calls of internal.GetCurrentEpoch

up to you then. i just did not like expireAt = expireAt - currEpoch + currEpoch before. making one more method for expireAt case is also ok to me. getting epoch num before every CreateSession too

Refactor CLI usage to standardize the expired-at flag and lifetime flag. This change is aimed at preventing confusion during flag interpretation and empowering users. Closes #1574. Signed-off-by: Ekaterina Pavlova <[email protected]>

AliceInHunterland requested review from roman-khimov, carpawell and cthulhu-rider as code owners August 21, 2023 07:03

AliceInHunterland mentioned this pull request Aug 21, 2023

neofs-cli removal of the lifetime flag #2522

Closed

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch from ef0ba01 to 0966733 Compare August 21, 2023 07:45

carpawell reviewed Aug 21, 2023

View reviewed changes

cmd/neofs-cli/modules/storagegroup/put.go Outdated Show resolved Hide resolved

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch 2 times, most recently from dcd903a to 80eaaa3 Compare August 22, 2023 12:05

carpawell reviewed Aug 22, 2023

View reviewed changes

roman-khimov reviewed Aug 22, 2023

View reviewed changes

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch 2 times, most recently from 46a7b6e to 3f220e2 Compare August 23, 2023 07:26

AliceInHunterland requested a review from carpawell August 23, 2023 07:31

carpawell reviewed Aug 23, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cmd/neofs-cli/modules/bearer/create.go Outdated Show resolved Hide resolved

cmd/neofs-cli/modules/session/create.go Outdated Show resolved Hide resolved

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch 2 times, most recently from aa6d58f to 7fda793 Compare August 23, 2023 15:26

roman-khimov reviewed Aug 23, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cmd/neofs-cli/modules/session/create.go Show resolved Hide resolved

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch from 7fda793 to 9b2edf7 Compare August 23, 2023 16:47

AliceInHunterland requested review from roman-khimov and carpawell August 24, 2023 11:19

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch from 9b2edf7 to df0933c Compare August 24, 2023 12:40

roman-khimov approved these changes Aug 24, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

roman-khimov mentioned this pull request Aug 24, 2023

Add expire-at/lifetime flags check nspcc-dev/neofs-testcases#612

Closed

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch from df0933c to 0223d14 Compare August 24, 2023 20:08

carpawell reviewed Aug 25, 2023

View reviewed changes

AliceInHunterland force-pushed the feature/1574-unify-epoch-expiration-flag branch from 0223d14 to 3b463c0 Compare August 25, 2023 13:03

carpawell approved these changes Aug 25, 2023

View reviewed changes

roman-khimov merged commit af62f2d into master Aug 25, 2023
8 checks passed

roman-khimov deleted the feature/1574-unify-epoch-expiration-flag branch August 25, 2023 18:05

evgeniiz321 mentioned this pull request Sep 1, 2023

Add checks for lifetime, expire_at flags nspcc-dev/neofs-testcases#621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: unify usage of expired-at flag instead of lifetime flag #2521

cli: unify usage of expired-at flag instead of lifetime flag #2521

AliceInHunterland commented Aug 21, 2023

codecov bot commented Aug 21, 2023 •

edited

Loading

carpawell left a comment

AliceInHunterland commented Aug 21, 2023 •

edited

Loading

carpawell commented Aug 21, 2023

cthulhu-rider commented Aug 22, 2023 •

edited

Loading

roman-khimov commented Aug 22, 2023

cthulhu-rider commented Aug 22, 2023

AliceInHunterland commented Aug 22, 2023 •

edited

Loading

cthulhu-rider commented Aug 22, 2023 •

edited

Loading

carpawell Aug 22, 2023

roman-khimov Aug 22, 2023

carpawell commented Aug 22, 2023

roman-khimov commented Aug 25, 2023

carpawell Aug 25, 2023

AliceInHunterland Aug 25, 2023

carpawell Aug 25, 2023

cli: unify usage of expired-at flag instead of lifetime flag #2521

cli: unify usage of expired-at flag instead of lifetime flag #2521

Conversation

AliceInHunterland commented Aug 21, 2023

codecov bot commented Aug 21, 2023 • edited Loading

Codecov Report

carpawell left a comment

Choose a reason for hiding this comment

AliceInHunterland commented Aug 21, 2023 • edited Loading

carpawell commented Aug 21, 2023

cthulhu-rider commented Aug 22, 2023 • edited Loading

roman-khimov commented Aug 22, 2023

cthulhu-rider commented Aug 22, 2023

AliceInHunterland commented Aug 22, 2023 • edited Loading

cthulhu-rider commented Aug 22, 2023 • edited Loading

carpawell Aug 22, 2023

Choose a reason for hiding this comment

roman-khimov Aug 22, 2023

Choose a reason for hiding this comment

carpawell commented Aug 22, 2023

roman-khimov commented Aug 25, 2023

carpawell Aug 25, 2023

Choose a reason for hiding this comment

AliceInHunterland Aug 25, 2023

Choose a reason for hiding this comment

carpawell Aug 25, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2023 •

edited

Loading

AliceInHunterland commented Aug 21, 2023 •

edited

Loading

cthulhu-rider commented Aug 22, 2023 •

edited

Loading

AliceInHunterland commented Aug 22, 2023 •

edited

Loading

cthulhu-rider commented Aug 22, 2023 •

edited

Loading