Skip to content

Conversation

@alexjh
Copy link

@alexjh alexjh commented Mar 17, 2016

Not all environments allow writing to /proc/sys, allow these to be
controlled via config setting.

There is already a check for running_in_container() but this seems to be bosh-lite specific as it's looking for a specific entry in /proc/self/cgroup.

@cfdreddbot
Copy link

Hey alexjh!

Thanks for submitting this pull request!

All pull request authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions.

Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look.

1 similar comment
@cfdreddbot
Copy link

Hey alexjh!

Thanks for submitting this pull request!

All pull request authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions.

Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/115904293.

@alexjh
Copy link
Author

alexjh commented Mar 17, 2016

Ah, I didn't have my membership for the hpcloud org set to public. Just updated it now.

@alexjh alexjh closed this Mar 18, 2016
@alexjh
Copy link
Author

alexjh commented Mar 18, 2016

Ok dreddbot, trying again.

@alexjh alexjh reopened this Mar 18, 2016
@cfdreddbot
Copy link

Hey alexjh!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

1 similar comment
@cfdreddbot
Copy link

Hey alexjh!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@emalm
Copy link
Contributor

emalm commented Mar 21, 2016

Thanks, @alexjh! Could you elaborate on which BOSH-deployed environments don't allow writing to /proc/sys, other than BOSH-Lite? Is there some way the ctl scripts could determine that programmatically, as they do for BOSH-Lite? I'd like to avoid adding more configuration values to the manifest if possible.

Also, the 'tweak' in the property name sounds too informal. Would you mind changing the property names to 'adjust_proc_sys' or something similar?

Thanks,
Eric, CF Runtime Diego PM

@alexjh alexjh force-pushed the skip-proc-sys branch 2 times, most recently from 8ac0d06 to 5a5903b Compare March 22, 2016 20:07
@alexjh
Copy link
Author

alexjh commented Mar 22, 2016

Hi @ematpl! Renaming is no problem, I've changed the property names to adjust_proc_sys.

Re the environment, we're running the diego-release components in a docker container, so our /proc/self/cgroups looks something like this:

12:name=systemd:/
11:hugetlb:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
10:net_prio:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
9:perf_event:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
8:blkio:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
7:net_cls:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
6:freezer:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
5:devices:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
4:memory:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
3:cpuacct:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
2:cpu:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353
1:cpuset:/docker/62387e51cf21af0f62202ffcb0753e6eb89e4ac021c37c1de95f3b9fc16cc353

There are some recommended ways of checking if you're in a docker container but I didn't want our implementation leaking into the diego-release scripts. But if you think that would be better than adding properties, I can make a change to running_in_container() to handle our case as well.

@emalm
Copy link
Contributor

emalm commented Mar 23, 2016

Thanks for the extra context, @alexjh. Do you think it would be reasonable to change the running_in_container function to check whether the process is in any non-root cgroups at all? For example, when I check proc/self/cgroup on a CF or Diego VM on AWS, all the cgroups are / or unset, while in a BOSH-Lite-deployed garden-linux container some of them are /instance-<warden-handle>, and in a docker container they are all /docker/<container-id>. That check would seem to cover both the BOSH-Lite and Docker cases.

@alexjh
Copy link
Author

alexjh commented Mar 23, 2016

@ematpl That makes sense. I've pushed the change to handle both cases, no new configuration properties are needed.

@emalm
Copy link
Contributor

emalm commented Mar 23, 2016

Thanks, @alexjh! I'll prioritize the story for the Diego team to evaluate this and pull it in.

Best,
Eric

@jfmyers9 jfmyers9 merged commit fd47f5e into cloudfoundry:develop Mar 28, 2016
@alexjh alexjh deleted the skip-proc-sys branch March 29, 2016 16:44
jvshahid added a commit that referenced this pull request Jun 18, 2018
[finishes #155485548](https://www.pivotaltracker.com/story/show/155485548)

Submodule src/code.cloudfoundry.org/diego-ssh 9b3f460..1174556:
  > Upgrade jwt-go library.
Submodule src/code.cloudfoundry.org/uaa-go-client 0c176509..26b271e3:
  > Ignore validation error when token used before issued
  > go back to using submodules from routing-release
  > Provide correct url
  > Update README
  > all tests passing with newer JWT
  > fixing vet errors
  > Merge pull request #10 from cloudfoundry-incubator/vendor-deps
  > Revert "remove incubator"
  > remove incubator
Submodule src/github.com/dgrijalva/jwt-go f62f64ea..06ea1031:
  > documentation around expected key types
  > Merge branch 'master' of github.com:dgrijalva/jwt-go
  > add options to ParseFromRequest
  > fixed a formatting error in a test
  > documenting changes for upcoming 3.2.0 release
  > Merge pull request #152 from pusher/parse-unverified
  > Merge pull request #219 from geertjanvdk/feat/parse
  > Merge pull request #205 from zamicol/icon_godoc
  > Merge pull request #209 from zhyuri/patch-1
  > Merge pull request #220 from polarina/readme-alt-include
  > Notice about upcoming 4.0.0 release
  > 3.1.0 changelog
  > Merge pull request #218 from zoofood/patch-1
  > updated note on alg type vulnerability
  > Merge pull request #183 from hnakamur/support_rs256_in_jwt_command
  > Merge pull request #196 from dgrijalva/dg/cmd_args
  > Merge pull request #190 from jamesrwhite/patch-1
  > Merge pull request #180 from kevinburke/fix-unreachable
  > Merge pull request #166 from johnlockwood-wf/issue-165-missing-arg
  > Merge pull request #151 from zaichang/FixMigrationGuide
  > Merge pull request #146 from pkieltyka/master
  > Merge pull request #140 from kazhuravlev/patch-1
  > Merge pull request #77 from dgrijalva/release_3_0_0
  > v2.7.0
  > notice about imminent 3.0.0
  > Merge pull request #136 from bruston/keyfunc-typo
  > fixes #135 copy/paste error in rsa decoding tools
  > Merge pull request #132 from abourget/master
  > Merge pull request #133 from johnlockwood-wf/expire-delta
  > release notes
  > expose inner error within ValidationError
  > Merge branch 'master' of https://github.com/emanoelxavier/jwt-go-contr into dg/merge_112
  > cleaned up style and added tests
  > Merge branch 'master' of https://github.com/dakom/jwt-go into dg/pr_121
  > version history update
  > Merge pull request #79 from dgrijalva/dg/none
  > Merge pull request #122 from appleboy/patch-1
  > add 1.6 to travis.yml
  > Merge pull request #107 from Snorlock/bearer-verification
  > Merge pull request #111 from matm/master
  > added supported signing methods
  > Added some clarification and (hopefully) helpful documentation
  > version history
  > signature should be populated after parsing a valid token
  > Merge pull request #98 from dgrijalva/dg/parser
  > use cleaner version of prefix checking (thanks shurcooL)
  > fix array OOB panic (#100)
  > Merge pull request #93 from EnerfisTeam/master
  > Merge branch 'master' of github.com:dgrijalva/jwt-go
  > minor refactor of HMAC verify for legibility.  no functional changes
  > updated documenation of SigningMethod interface
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

Successfully merging this pull request may close these issues.

5 participants