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

t1.After(t2), t1.Before(t2), t1.Equal(t2) build-in function support #1918

Closed
KeisukeYamashita opened this issue Nov 22, 2019 · 16 comments
Closed

Comments

@KeisukeYamashita
Copy link
Contributor

KeisukeYamashita commented Nov 22, 2019

Expected Behavior

Like golang, we want to operations for time, comparing two datetime.

t.After(u)
t.Before(u)
t.Equal(u)

IMO, this should support RFC3339.

Actual Behavior

  • Not support yet.

Workaround

Now we can compare datetimes by comparing timestamps .

Additional Info

@tsandall
Copy link
Member

@KeisukeYamashita OPA can parse date/time strings into nanoseconds represented as numbers. Once you have them as numbers you can use normal comparison operators on them. For example: https://play.openpolicyagent.org/p/rMp1rVd5im.

Is there a compelling reason to add additional built-ins?

@elliots
Copy link

elliots commented Nov 25, 2019

One reason is it would allow easier use of date math like "now/d+10d" (now, rounded to the day, plus 10 days) (via https://github.com/timberio/go-datemath or similar)

@KeisukeYamashita
Copy link
Contributor Author

KeisukeYamashita commented Nov 26, 2019

The background of this issue is, it would be great if rego supports Datetime types.
Math in nanoseconds units is very confusing like below.

Check if the due_date is in two weeks policy

due_date_str := input.due_date
due_date := time.parse_rfc3339_ns(sprintf("%vT00:00:00-00:00", [due_date_str]))
now := time.now_ns()
now > due_date - 60 * 60 * 24 * 7 * 2 * 1000000000

Adding days, months, years is more confusing. Are there any plans for improving time calculations?

Thank you.

@tsandall
Copy link
Member

We don't have any short-term plans to add new data types into the language. I can see how saying now + 3d would be nice but it's just not high priority at the moment. This is an area where rules can be used to improve readability:

ns_per_second = 1000 * 1000 * 1000
ns_per_week = 24 * 7 * 60 * 60 * ns_per_second
two_weeks = ns_per_week * 2

allow {
  time.now_ns() > due_date - two_weeks
}

@srenatus
Copy link
Contributor

srenatus commented Dec 15, 2019

Math in nanoseconds units is very confusing like below. [...] Adding days, months, years is more confusing. Are there any plans for improving time calculations?

💭Maybe some useful (rego) helpers could be added to the library? I don't believe that would require new builtins. 😃

Update: open-policy-agent/library#33 -- but I was also convinced that handling stuff like one month after is very icky in nanoseconds...

@rickj33
Copy link

rickj33 commented Dec 19, 2019

Having rules to improve readability can help in some cases, but I do not see how to create a rule to address the use case I have.
My use case is as follows.
I have a contract that is valid for x number of months from the date of purchase. I need to validate the contract is still valid given a specific date.

I would accomplish this by calculating the contract end date by adding the number of months to the sales date. Then check if the current date is less than the calculated
What I would like to do in the policy is

contract_date_valid(contractSaleDate, contractDuration){
	
   startDate = time.parse_ns("2006-01-02",contractSaleDate)
   contract_end_date = startDate.AddDate(0,contractDuration,0)

   currentDate := time.now_ns()
   
   currentDate < contract_end_date
}

I could do this if some more of the go time functions were exposed as built ins.

@KeisukeYamashita
Copy link
Contributor Author

KeisukeYamashita commented Dec 20, 2019

Update: open-policy-agent/library#33 -- but I was also convinced that handling stuff like one month after is very icky in nanoseconds...

Yeah...Let's say you want to add 3 months... It will be hard to write in rego because the number of days is different.

But YAML, JSON, etc doesn't have Datetime objects so if the Rego supports, that will be great.

@srenatus
Copy link
Contributor

Yeah...Let's say you want to add 3 months

I'm not aware of any golang stdlib functions making something like this simple. Am I missing something?

@rickj33
Copy link

rickj33 commented Dec 20, 2019

Yeah...Let's say you want to add 3 months

I'm not aware of any golang stdlib functions making something like this simple. Am I missing something?

I am not a golang developer, but I found this reference to a golang function in the time package to add months.
https://golang.org/pkg/time/#Time.AddDate

The way I understand it, If I wanted to add 3 months to a start I would use
contract_end_date = startDate.AddDate(0,3,0)

@srenatus
Copy link
Contributor

@rickj33 thanks, good find. It's interesting since under the hood, time.Time is roughly nanoseconds + location (time zone). And yeah, if you look at the implementation, there's a lot going on that I personally wouldn't want to replicate in Rego.

However, to not introduce new objects (#1918 (comment)), maybe something like

time.add_date(ns, [years, months, days])

would be feasible? The first parameter (t) would be an integer of nanoseconds, so it could play nicely with time.parse_ns, time.now_ns, etc. The return value would be nanoseconds, as well; which can be compared to other ns; or fed into time.date().

In the implementation of the builtin, t is converted into time.Date and AddDate would be called on it. I think this also needs to accept [ns, tz], like time.date().

Of course, another way without new objects would be the string-based datemath stuff mentioned in #1918 (comment).

Also, to be explicit, I'm just trying to help finding a good path, I'm not likely to pick this up anytime soon.

@elliots
Copy link

elliots commented Dec 20, 2019

Of course, another way without new objects would be the string-based datemath stuff mentioned in #1918 (comment).

This is what we're currently using...

var nowOverride *time.Time

func setNow(t time.Time) {
	log.Printf("WARNING: OVERRIDING 'NOW' WITH %s. THIS SHOULD BE ONLY DONE IN A TEST.", t.String())
	nowOverride = &t
}

var datemathFn = rego.Function1(
	&rego.Function{
		Name: "i8.datemath",
		Decl: types.NewFunction(types.Args(types.S), types.N),
	},
	func(bctx rego.BuiltinContext, exprT *ast.Term) (*ast.Term, error) {

		exist, ok := bctx.Cache.Get(nowKey)

		var now time.Time
		if !ok {
			now = time.Now().UTC()
			bctx.Cache.Put(nowKey, now)
		} else {
			now = exist.(time.Time)
		}

		if nowOverride != nil {
			now = *nowOverride
		}

		out, err := datemath.ParseAndEvaluate(string(exprT.Value.(ast.String)), datemath.WithNow(now), datemath.WithLocation(time.UTC)) //, datemath.WithBusinessDayFunc(tt.businessDayFunc))
		if err != nil {
			return nil, fmt.Errorf("failed to evaluate datemath expression %s: %s", exprT.String(), err)
		}

		return ast.NewTerm(ast.Number(int64ToJSONNumber(out.UnixNano()))), nil
	})

var datemathWithFn = rego.Function2(
	&rego.Function{
		Name: "i8.datemath_with",
		Decl: types.NewFunction(types.Args(types.N, types.S), types.N),
	},
	func(bctx rego.BuiltinContext, date *ast.Term, exprT *ast.Term) (*ast.Term, error) {

		dateInt, err := strconv.Atoi(string(date.Value.(ast.Number)))
		if err != nil {
			return nil, fmt.Errorf("failed to parse time from json.Number %s: %s", date.Value.String(), err)
		}

		dateTime := time.Unix(0, int64(dateInt)).UTC()

		expr := "now" + string(exprT.Value.(ast.String))

		out, err := datemath.ParseAndEvaluate(expr, datemath.WithNow(dateTime), datemath.WithLocation(time.UTC)) //, datemath.WithBusinessDayFunc(tt.businessDayFunc))
		if err != nil {
			return nil, fmt.Errorf("failed to evaluate datemath expression '%s' %s", exprT.String(), err)
		}

		return ast.NewTerm(ast.Number(int64ToJSONNumber(out.UnixNano()))), nil
	})

which allows

reporting_deadline := time.parse_ns("20060102", input.details.reporting_deadline)

today := i8.datemath("now/d")

escalate[action] {
    today >= i8.datemath_with(reporting_deadline, "-3b")

    action = {
        "type": "email",
        "rule": "ER03",
        "template": "sla-notification",
        "to": ["[email protected]"],
        "from": ["[email protected]"],
        "subject": "Transactions within 3 business days of SLA"
    }
}

It isn't currently possible to get the cached "now" value the built-in time functions use, but that would be useful.

@rickj33
Copy link

rickj33 commented Dec 31, 2019

@elliots How did you get the added functionality into opa? Did you fork it and add the code? Also question on

today >= i8.datemath_with(reporting_deadline, "-3b")

what does time unit does b mean in "-3b"

@elliots
Copy link

elliots commented Jan 1, 2020

@rickj33 I'm using opa from my go application, and the first code block are the built-ins i added so i could use them in the second block.

The 'b' is for "business days"... by default it excludes sat+sun, but you can provide a function to decide if it is a business day or not. (see: https://godoc.org/github.com/timberio/go-datemath )

@rickj33
Copy link

rickj33 commented Jan 2, 2020

@elliots Thank you for the information.

@tsandall
Copy link
Member

tsandall commented Jan 9, 2020

Sorry for the delayed response on this.

I like the suggestion from @rickj33 and @srenatus to use time.AddDate to expose a time.add_date(ns, [years,months,days]) function in Rego (because it's easy to implement and doesn't require a thirdparty dependency). I will open another issue to track that enhancement.

@tsandall
Copy link
Member

tsandall commented Jan 9, 2020

I've opened #1990 to cover the enhancement to add time.add_date. I'm going to close this for now. If enough people want/need support for "business days" we can explore a similar solution. If there are other use cases that need to be covered feel free to file follow up issues.

@tsandall tsandall closed this as completed Jan 9, 2020
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