Add space trimming check in ValidateSysctls

[xatier]

This is to catch invalid sysctl configs with extra spacing.

See
containers/common#723 (comment)

Signed-off-by: xatier [email protected]

[unknowndevQwQ]

Cloud you consider adding something like this for ulimit?

[TomSweeneyRedHat]

Might be good to add a test that should pass, perhaps:

	strSlice := []string{
		" net.ipv4.ping_group_range=0 0          ",
		"          net.ipv4.ping_group_range=0 0 ",
	}

Also, can you add a check for the error text you're expecting, or at least "extra spaces found" as the substring?

[xatier]

@TomSweeneyRedHat thanks for suggestions. I was thinking that other tests for ValidateSysctls would still go through my changes, as long as we don't see regressions on the other existing tests I think there's no need to introduce other test cases? I can still add a few if you wish.

Yes, I'll update my PR to check the message as well.

[xatier]

Also, this won't be (and shouldn't be, IMO) passing, as we do arr := strings.Split(val, "=") in ValidateSysctls.

    strSliceGood := []string{                                                    
        " net.ipv4.ping_group_range=0 0          ",                              
        "          net.ipv4.ping_group_range=0 0 ",                             
    }                                                                            
    _, err := ValidateSysctls(strSliceGood)                                     
    assert.Nil(t, err)

[xatier]

@unknowndevQwQ, I didn't look into that code path too closely, sorry.

Cloud you consider adding something like this for ulimit?

---

Update:

Just checked briefly on ulimits. It appears to be we are leveraging docker/go-units [1] in containers/common validateUlimits [2]. You may want to submit a PR there.

[1] https://github.com/docker/go-units/blob/master/ulimit.go#L66
[2] https://github.com/containers/common/blob/6c933f226ed44491d082d6d52cbe31ae6fe0e76f/pkg/config/config_local.go#L69

[xatier]

@TomSweeneyRedHat @rhatdan, also once this is merged, I'll create another PR with the same change in containers/common.

https://github.com/containers/common/blob/main/pkg/sysclt/sysctl.go#L30

[rhatdan]

LGTM
Thanks @xatier

[unknowndevQwQ]

It would be nice if there was some way to allow spaces on both sides of the equal sign
For example, sysctl.d allows this

[xatier]

@unknowndevQwQ, I believe it makes more sense to stay with more restricted formats (i.e., no space). I am aware sysctl.conf would ignore whitespaces, while traditionally the sysctl cli tool has the wildly used variable=value form.

In container/common, we simply treat this as simple []string type [1], disallowing any additional extra spacing for this would make parsing (and any other sanity checks) a lot less painful. Furthermore, we can eliminate future code bugs similar to [2].

[1] https://github.com/containers/common/blob/main/pkg/config/config.go#L90
[2] containers/common#723

[rhatdan]

/approve

I am fine with either format, this is not going to be a common mistake.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, xatier

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:

• OWNERS [rhatdan]

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

[mheon]

/lgtm