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

Add a UREPO_DELETE variable ? #3

Open
floreo opened this issue Aug 26, 2017 · 30 comments
Open

Add a UREPO_DELETE variable ? #3

floreo opened this issue Aug 26, 2017 · 30 comments

Comments

@floreo
Copy link
Contributor

floreo commented Aug 26, 2017

Hi,

using urepo behind nginx I noticed that the following line in cgi/process-file (l 36):
[ "$HTTP_HOST" != "127.0.0.1" ] && log "Error: delete from $HTTP_HOST is prohibited" && exit
could probably be a bit "improved" (it's not always 127.0.0.1, my nginx configuration has several different IP to limit POST/GET from specific range), rather than being executed if the IP is "correct", I'd suggest adding a parameter to urepo/urepo.conf such as UREPO_DELETE to allow deleting.
If the idea is interesting to you, I can code it.
Thanks.

@kt97679
Copy link
Contributor

kt97679 commented Aug 26, 2017

This was done to allow deletion of packages only from the urepo host itself. I did this because I wanted to limit ability to delete packages only for users who can actually login to the urepo host (in my case admins). This definitely can be improved, but proper implementation should probably be pretty sophisticated and should track package ownership allowing deletion only for package owners. Please let me know what use case you try to address and we can define requirements for implementation.

@floreo
Copy link
Contributor Author

floreo commented Aug 26, 2017

Thanks again for your fast answer!
Actually we're setting up a build process, we do have one Gitlab (machine1), when you push changes there, it will run a Docker instance to create .deb and .rpm on several Debian/Ubuntu/CentOS flavors (machine2), and upload them to our urepo instance (machine 3). That whole process is under a private network between the 3 machines. Then we had the repo to every cusmoters' machines to update our packages, and it's done over a public IP.

So actually, our urepo instance has 2 IPs:

  • private ip: only machine2 can POST/DELETE packages, I commented the line above in cgi/process-file so it could work, and it's our nginx setting that allows or not deletion and post.
  • public ip: only GET can be done from any customers' machines

So as a matter of fact I don't really need an association of a package to an ower, but it's a fair and good point your raise.

Actually, it all works well with my comment and our nginx configuration but it's not quite standard and we're not even with your repo.

This said, maybe a small sqlite DB keeping tracks of owner/package/ip (or login/password with basic auth) could be enough. I think it should stay simple, yet secure. :)

@kt97679
Copy link
Contributor

kt97679 commented Aug 26, 2017

Thanks for the clarification. As an intermediate solution I was thinking of having list of ips which are allowed to delete packages and I think that will cover your needs. Those ips can be hold by the UREPO_ALLOW_DELETE_IPS variable in the urepo.conf and instead of checking for the 127.0.0.1 at https://github.com/hulu/urepo/blob/master/var/urepo/cgi/process-file#L36 process-file will check if source ip is in the list. If UREPO_ALLOW_DELETE_IPS is not defined source ip should be checked if it is 127.0.0.1 as it is done now. What do you think about this logic?

@kt97679
Copy link
Contributor

kt97679 commented Aug 26, 2017

I also think that this is a good time to make /etc/urepo/urepo.conf not part of the package (but provide example in /usr/share/...). To do this configuration logic from the after install script should be moved to the separate urepo configuration script that should be called each time you update configuration.

@floreo
Copy link
Contributor Author

floreo commented Aug 26, 2017

You're welcome, it seems like a good compromise to have that variable, thanks for your idea. :)
About moving urepo.conf I'm not sure I follow you. When you build urepo.deb, it wouldn't "build" the /etc/urepo/urepo.conf ? So one would have to define everything from scratch in /etc/urepo/urepo.conf ? It makes one thing "complicated" since when you build the conf, it generates index.html which contains some select containing XXX_RELEASES and XXX_COMPONENTS.
Maybe I misunderstood.

@kt97679
Copy link
Contributor

kt97679 commented Aug 26, 2017

Sorry, I was not clear enough regarding /etc/urepo/urepo.conf. The main problem is that current file content has settings used by me at Hulu. This was ok while I was the only user of the package. But now you decided to use it as well and had to change this file to use urepo in your environment. If there will be more people everybody will need to change their settings and this is not good. Because of that I suggest to remove urepo.conf from package so that everybody will need to create customized configuration after installation of the package. After creating configuration user will need to run script (e.g. urepo-config), that will create necessary directory structure and will update index.html.

The downside of this approach is that urepo will not work immediately after installation.

Alternative approach will be to have basic urepo.conf that will be suitable for using with some popular distribution (ubuntu or debian) with standard settings. E.g. urepo.conf can have settings for jessie main (instead of current Hulu specific settings).

My main idea here is to make urepo easier to use since your experience demonstrates, that this package is not very user friendly. And I definitely want to improve this.

@floreo
Copy link
Contributor Author

floreo commented Aug 27, 2017

Alrighty, I get it now, thanks for your clarification!

I agree with your idea, how about within the urepo.deb that one would generate that we add a menu (a bit like when you generate locales, or when you install Debian without graphical installation, so using curses) when it's installing asking for DEB/RPM_CODENAMES/RELEASES and DEB/RPM_COMPONENTS. And then generating urepo.conf and index.html. And when you run dpkg-reconfigure urepo one could add new settings. So once it's done, you can start urepo and it's user friendly and it can have default settings too.

I appreciate that you still keep an eye on it. :)

By the way, how many machines approximately are connecting to your urepo ? We're working with around 1K but it will grow, I can upgrade the VM's "hardware" buy do you think it's OK ?

@floreo
Copy link
Contributor Author

floreo commented Aug 27, 2017

Ah I remembered the name of the package to create a "clean" menu, it's dialog.

@kt97679
Copy link
Contributor

kt97679 commented Aug 27, 2017

This is a very good suggestion, thank you! Regarding configuration same approach can be applied we see currently e.g. with postfix package where you can fill in parameters or leave configuration empty and configure it manually later. I must confess I never used dialog. Do you think you can implement this approach and send me PR?

Regarding your capacity question I currently run urepo on a single vm with 2 cores and 2gb of ram. I have 9k+ clients all of which query urepo and install updates in the time period of 9am-10am. I see no performance issues so far.

@floreo
Copy link
Contributor Author

floreo commented Aug 27, 2017

I'm glad you like, even though I never used it I'll try to do something and let you know, I guess I'll need your help to follow your global idea. ;) No idea when I'll do a PR though, for now I finish an ansible role for urepo then I'll start that evolution after some research.

Thanks for your answer, I set ours with lower capacity (1 vcore, 512MB RAM) but we have quite less clients for now, I'm confident.

@kt97679
Copy link
Contributor

kt97679 commented Aug 27, 2017

Sounds good, looking forward to review your PR :)!

@floreo
Copy link
Contributor Author

floreo commented Aug 30, 2017

Hi,

I hope you're fine, I've started to work on that issue and I was wondering if you would consider to rename DEB_CODENAMES to DEB_RELEASES, just like RPM_RELEASES, it's the only variable that's different between DEB*/RPM* and it would simplify the menu generation/check, I can iterate over an array like that:

 # hotfix // dirty
 DEB_RELEASES=${DEB_CODENAMES}
 for _OS in "DEB" "RPM"
 do
      _OS_VARS=( "${_OS}_REPO_ROOT" "${_OS}_RELEASES" "${_OS}_ARCHITECTURES" "${_OS}_COMPONENTS" )
      dialog --title "urepo - configure ${_OS}'s parameters" \
      --form "\nThe following parameters cannot be left empy" 25 60 16 \
      "${_OS_VARS[0]}" 1 1 "${!_OS_VARS[0]}" 1 25 25 30 \
      "${_OS_VARS[1]}" 2 1 "${!_OS_VARS[1]}" 2 25 25 30 \
      "${_OS_VARS[2]}" 3 1 "${!_OS_VARS[2]}" 3 25 25 30 \
      "${_OS_VARS[3]}" 4 1 "${!_OS_VARS[3]}" 4 25 25 30 \
      2>/tmp/urepo.form${_OS}.$$
  done

If you follow me, it's way easier to generate the menu here. :)

Below a work in progress for a part of the menu (I have no idea why there is a shade but it's not in the real menu, I guess my screenshot software is not working properly)
urepo_menu

@kt97679
Copy link
Contributor

kt97679 commented Aug 30, 2017

Renaming looks reasonable, please ago ahead with it.

@floreo
Copy link
Contributor Author

floreo commented Aug 30, 2017

Thanks, it's done, I move on. :)
Do you know by any chance how to trigger a part of the script in the deb when you run dpkg-reconfigure urepo ?

@kt97679
Copy link
Contributor

kt97679 commented Aug 30, 2017

Sorry, I don't know. I suggest to create dummy package with package scripts that log command line arguments and environment variables. After that run different use cases: install package, upgrade package, run dpkg-reconfigure. After that you can analyze logs and see if there is something you can use to identify that script was run via dpkg-reconfigure.

@floreo
Copy link
Contributor Author

floreo commented Aug 31, 2017

No problem and thanks for the idea ! I lack some time now but I'll try to finish it any time soon. :)

@kt97679
Copy link
Contributor

kt97679 commented Aug 31, 2017

Sounds good, thanks :)!

@floreo
Copy link
Contributor Author

floreo commented Sep 3, 2017

Hi,

you're welcome, so I've learned quite a lot about debconf (it's used to make the menu, keep a database of answer to the questions, dpkg-reconfigure, etc ...). It's different from what I showed you previously which was using dialog, and I was naive enough to believe that it was what postfix used, but nope. So with debconf since there is a database keeping all the answers, I was wondering if /etc/urepo/urepo.conf was still relevant to keep. I know you you were talking about removing it and moving it to /usr/share/doc/urepo/... or something like that. I guess it's a good idea, and same goes for urepo-nginx, I mean it could be cp from /usr/share/doc/urepo/... and done.
Not sure if I'm being clear, but to be more specific, in after-install.sh rather than sourcing the file, I can ask debconf to give me the answer of any questions previously asked, so it makes the sourcing obsolete, I'll try to update my branch to show you what I mean.
Cheers.

@kt97679
Copy link
Contributor

kt97679 commented Sep 3, 2017

Thank you for the update. I think that urepo still should use /etc/urepo/urepo.conf. This will allow both configuration via interactive dialog or configuration via providing complete urepo.conf. Last use case is useful while using unattended installations (e.g. via system configuration tools like salt).

@floreo
Copy link
Contributor Author

floreo commented Sep 4, 2017

Alrighty then, thanks for your answer. :)

@floreo
Copy link
Contributor Author

floreo commented Sep 18, 2017

Hi,
sorry long time, could you have a look at that and give me your feedback ?
I didn't move yet the urepo.conf into /usr/share/doc, so it's commented within the code for now.
Also I kinda moved it all to follow "debconf standard", it's still possible to install urepo without following the menu.
I'm waiting for your advices, for example the menu could be improved so when you pres ESC it goes back rather than validating the current answer.
Thanks.

@kt97679
Copy link
Contributor

kt97679 commented Sep 18, 2017

No worries and thanks a lot for you changes, I commented them. For menu improvements I don't have good feedback at this point, let's try and see. We always can improve this later. Have you tried to install package in the unattended mode?

@floreo
Copy link
Contributor Author

floreo commented Sep 18, 2017

Thanks I'll have a look. What do you mean by the unattended mode ? "By hand" ? If so, yet my only suggestion is to set the urepo.conf before to install urepo, for example using ansible it wouldn't be a problem, even so I do agree, it's ugly.

@kt97679
Copy link
Contributor

kt97679 commented Sep 18, 2017

My comments are here: floreo@db1636f

By unattended install I mean installation using following command:

DEBIAN_FRONTEND=noninteractive apt-get -y -o Dpkg::Options::="--force-confdef" -o Dpkg::Options::="--force-confnew" install ...

Also if /etc/urepo/urepo.conf exists installation should probably ask if we want to use existing config or want to generate new one.

@floreo
Copy link
Contributor Author

floreo commented Sep 18, 2017

Right, thanks I check it all and keep you up to date ASAP.

@floreo
Copy link
Contributor Author

floreo commented Sep 18, 2017

I followed your comments, and it's fixed.

Where would you put urepo.conf and other stuff for documentation ? It could be in the makefile rather than copying to /etc/urepo.

I will try unattended install to check how it goes.

Right about the presence of /etc/urepo/urepo.conf, I'll add that.

@kt97679
Copy link
Contributor

kt97679 commented Sep 18, 2017

Changes look good, thanks, added one more comment.

Sorry, what do you mean by "it can be in the makefile"?

@floreo
Copy link
Contributor Author

floreo commented Sep 18, 2017

NP, it's done for your comment.

Sorry I wasn't clear, I mean that we could cp urepo.conf and other documentation things like it's done now for var/ and etc/ l32 of the makefile.

@kt97679
Copy link
Contributor

kt97679 commented Sep 18, 2017

Oh, sure, let's do it. Btw, I've created develop branch in the hulu/urepo. You can submit your changes as PRs for this branch.

@floreo
Copy link
Contributor Author

floreo commented Sep 20, 2017

Alright then I take care of that ASAP.
OK that's cool for the develop branch, I'll do that too. :)

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

2 participants