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

Fix to pass quoted unsafe strings (with characters like *,<,%) correctly to kubelet #19951

Merged

Conversation

aveshagarwal
Copy link
Contributor

@aveshagarwal aveshagarwal commented Jun 8, 2018

Fix rhbz https://bugzilla.redhat.com/show_bug.cgi?id=1587824. With experimental-allowed-unsafe-sysctls,
"*" is a valid character now as part of kubelet arguments. For example:

kubeletArguments:
  experimental-allowed-unsafe-sysctls:
  - 'kernel.shm*,kernel.msg*,kernel.sem,fs.mqueue.*,net.*'

@sjenning @smarterclayton

Related PR to openshift-ansible: openshift/openshift-ansible#8772

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 8, 2018
@aveshagarwal aveshagarwal changed the title Allow * as valid character when passing arguments to kubelet. Allow * as a safe character when passing arguments to kubelet. Jun 9, 2018
@aveshagarwal
Copy link
Contributor Author

/test extended_clusterup
/test extended_conformance_install

@aveshagarwal
Copy link
Contributor Author

@sjenning @smarterclayton any comment?

@aveshagarwal aveshagarwal force-pushed the master-rhbz-1587824 branch from 59b425b to 479d1d4 Compare June 13, 2018 17:22
@aveshagarwal
Copy link
Contributor Author

aveshagarwal commented Jun 14, 2018

@sjenning @rphillips @smarterclayton

So here is an update related to this issue: As discussed, here are the changes that quote only the (unsafe) value/token part:

aveshagarwal@eb660ba

However, it also fails with following error, and kubelet does not start:

ERROR: logging before flag.Parse: F0614 04:47:57.354808 6223 server.go:233] failed to run Kubelet: failed to create kubelet: sysctl "kernel.shm*,kernel.msg*,kernel.sem,fs.mqueue.,net." must have at most 253 characters and match regex (a-z0-9?.)([a-z0-9][-_a-z0-9])?[a-z0-9*]

In summary, so far the main issue is that quotes are being passed to kubelet: 1) when quoting both(key, token), it is ignored by kubelet; 2) when quoting only the token/value part, it is being rejected by kubelet, as quotes are invalid as per their regex.

Seems like with the 2nd approach, it should help "unquoting" the token part at kubelet label. I have not tried it but can if you think doing it should be fine.

In case anyone interested about the code flow/testing, here it: atomic-openshift-node.service file calls a local script /usr/local/bin/openshift-node. This script starts kubelet as follows:

flags=$( /usr/bin/openshift-node-config-avesh --config=${config} )
exec /usr/bin/hyperkube kubelet --v=${DEBUG_LOGLEVEL:-2} ${flags}

@smarterclayton
Copy link
Contributor

This is a bash problem. If we can't correctly unmarshal flags from the flags command into the kubelet call, that's where the problem should stop.

@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 14, 2018 via email

@smarterclayton
Copy link
Contributor

I think the following should work:

flags=$( /usr/bin/openshift-node-config-avesh --config=${config} )
eval "exec /usr/bin/hyperkube kubelet --v=${DEBUG_LOGLEVEL:-2} ${flags}"

@aveshagarwal aveshagarwal force-pushed the master-rhbz-1587824 branch from 479d1d4 to 998f5fd Compare June 14, 2018 18:33
@aveshagarwal aveshagarwal changed the title Allow * as a safe character when passing arguments to kubelet. Fix to pass quoted unsafe strings (with characters like *,<,%) correctly to kubelet Jun 14, 2018
@aveshagarwal
Copy link
Contributor Author

/test unit
/test gcp

@smarterclayton
Copy link
Contributor

/retest

@aveshagarwal
Copy link
Contributor Author

@smarterclayton all tests passed. so lgtm/approve whenever you get a chance.

@smarterclayton
Copy link
Contributor

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, smarterclayton

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2018
@openshift-merge-robot openshift-merge-robot merged commit c20e215 into openshift:master Jun 15, 2018
@aveshagarwal aveshagarwal deleted the master-rhbz-1587824 branch June 15, 2018 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants