Skip to content

Added proxy setup service#696

Merged
teclator merged 11 commits intomasterfrom
proxy_setup
Aug 21, 2023
Merged

Added proxy setup service#696
teclator merged 11 commits intomasterfrom
proxy_setup

Conversation

@teclator
Copy link
Copy Markdown
Contributor

@teclator teclator commented Aug 9, 2023

Problem

Currently Agama does not have support for setting up a proxy

In case of a network installation dracut url lib uses the one provided by the kernel command line option:

https://github.com/openSUSE/dracut/blob/5e324584a14377f52425be589ab53299609f066b/modules.d/45url-lib/url-lib.sh#L62

We should write a service that continue writing the /etc/sysconfig/proxy and use the microos-tools proxy setup service to make it available to the systemd units if we wanted, or own systemd service to do that before running agama.

The service should read the /proc/cmdline in case it was given as a kernel commandline option or check the configuration written by dracut when asking for more kernel paramaters

Solution

An agama-proxy-setup service and executable will take care of parsing the /proc/cmdline writing the information to /etc/sysconfig/proxy

The setup-systemd-proxy-env path provided by microos-tools is enabled in order to write the information as environment variables to systemd units.

See https://build.opensuse.org/package/rdiff/home:teclator:branches:systemsmanagement:Agama:Devel/agama-live?opackage=agama-live&oproject=systemsmanagement%3AAgama%3ADevel&rev=5

Testing

  • Tested manually

ProxySetup

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 9, 2023

Coverage Status

Changes unknown
when pulling cc3c6b7 on proxy_setup
into ** on master**.

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it looks good. However, I miss better documentation and tests.

@teclator
Copy link
Copy Markdown
Contributor Author

@teclator teclator requested a review from imobachgs August 14, 2023 06:50
def write
return unless proxy

Proxy.Read
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the tool will merge with any existing proxy configuration?
We should document it if it's intentional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it follows the same approach we have for YaST based installers (https://github.com/yast/yast-network/blob/master/src/lib/network/install_inf_convertor.rb#L80).

So, it merges the configuration but in most of the cases it should be empty. I guess by now we could just write what is defined instead of merging it as we are not doing any upgrade or something like that where an existent config could be useful at all.

@mvidner
Copy link
Copy Markdown
Contributor

mvidner commented Aug 16, 2023

I wonder why CI fails (at building nokogiri gem) here but not at #703 ...

Ah that's because Imo has figured out AWK is needed for building it: 5aaac0e

teclator and others added 5 commits August 16, 2023 15:05
Applied fixes suggested from code review

Co-authored-by: Martin Vidner <mvidner@suse.cz>
Co-authored-by: Martin Vidner <mvidner@suse.cz>
Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good. However I am not sure if we should keep the indentation in the setup-service.sh script.

teclator and others added 3 commits August 17, 2023 23:58
Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>
Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I forgot about the changes file. Please, add an entry and do not forget the bsc and/or github references.

@teclator teclator requested a review from imobachgs August 18, 2023 15:14
@teclator teclator merged commit 40e46fa into master Aug 21, 2023
@teclator teclator deleted the proxy_setup branch August 21, 2023 06:50
@teclator teclator mentioned this pull request Aug 22, 2023
This was referenced Sep 26, 2023
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.

4 participants