DecorationConfig: add pointers to Durations.

[droslean]

After #12100 landed, omitempty is ignored when unmarshaling a time.Duration.
Example:

DecorationConfig: &v1.DecorationConfig{SkipCloning: true}

will be exported as

timeout: 0s
grace_period: 0s
skip_cloning: true

To fix this issue, the Duration should be a pointer.

Related proposal fix: golang/go/issues/11939

[droslean]

/hold
testing if these changes will break the rest of the logic.

[droslean]

/hold cancel

[droslean]

/cc @cjwagner @stevekuznetsov

[stevekuznetsov]

/hold
@cjwagner PTAL

[cjwagner]

If this is going to be changed to a pointer we need to ensure that there is always a value specified once the config is loaded (or explicitly check for a nil pointer and handle defaulting separately everywhere which is not a good idea).
For example, if we merged this PR as is, this line could start segfaulting because the Timeout and GracePeriod fields could be nil:

test-infra/prow/pod-utils/decorate/podspec.go

Line 538 in dda2b80

wrapperOptions, err := InjectEntrypoint(&spec.Containers[0], pj.Spec.DecorationConfig.Timeout.Duration, pj.Spec.DecorationConfig.GracePeriod.Duration, prefix, previous, exitZero, logMount, toolsMount)

The easiest way around this would be to default the Timeout and GracePeriod fields in the DefaultDecorationConfig if they are unspecified. Around here.

[cjwagner]

Why is this defaulting implemented in the build controller? These values should already have been defaulted before the build controller handles them.

[stevekuznetsov]

I was going to make this comment then I channelled my inner @fejta and there is technically nothing stopping someone from making a nil entry with mkpj or something and then the defensive coding here is ok?

[cjwagner]

/shrug
We can keep it if that is a concern, but I'd like to see the proper defaulting as well at least (especially because there is the potential for segfaults with the current implementation).

mkpj will still properly default the field based on the default_decoration_config. The only concern would be hand crafted ProwJob yaml which can break prow in a lot of ways already. I think adding a ProwJob webhook admission controller is the right way to handle that.

[stevekuznetsov]

We could add a defaulting accessor to the config struct and disallow the use of direct references @droslean

[droslean]

addressed

[droslean]

@cjwagner Should the defaults be different than zero?

[stevekuznetsov]

I am not sure this is sufficient -- the issue is, this is for job configuration coming from the YAML on disk, but we want to make sure that when we read the values from a ProwJob CRD in etcd we get no nil pointers. I was thinking we have a

func (c *DecorationConfig) Timeout() Duration {
    if c.Timeout == nil {
        return &Duration{0}
    }
    return c.Timeout
}

And use that in the consumres

[droslean]

addressed

[stevekuznetsov]

/hold
@cjwagner PTAL

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f4b035678f44b46f063edda69217ecff8730788e

[cjwagner]

I just have a couple nits.

[cjwagner]

nit: Return time.Duration instead of *Duration. The time package type is directly usable and returning a pointer doesn't make much sense since this should always return a concrete value. (This applies to GetTimeout as well.)

[cjwagner]

nit: The GetTimeout and GetGracePeriod functions could be replaced with a single func (d *Duration) Get() time.Duration function.

[droslean]

addressed

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: c28475a9cc335a5da4fe4e08aa5f1c0c662cb901

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droslean, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• prow/OWNERS [stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cjwagner]

/hold cancel