Skip to content

Set Proxy variables for all systemd services #39

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.

Sign up for GitHub

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

Closed
zaneb wants to merge 3 commits into from
Closed

Set Proxy variables for all systemd services #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d4b11d
Unit tests: make expected systemd service easier to read
zaneb Feb 4, 2022
289470b
Set Proxy variables for all systemd services
zaneb Feb 4, 2022
4a0f6af
Group Proxy vars together
zaneb Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cmd/static-server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ func loadStaticNMState(fsys fs.FS, env *env.EnvInputs, nmstateDir string, imageS
pullSecret,
env.IronicRAMDiskSSHKey,
env.IpOptions,
env.HttpProxy,
env.HttpsProxy,
env.NoProxy,
env.Proxy,
hostname,
)
if err != nil {
Expand Down
10 changes: 7 additions & 3 deletions pkg/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import (
"github.com/pkg/errors"
)

type ProxyConfig struct {
HttpProxy string `envconfig:"HTTP_PROXY"`
HttpsProxy string `envconfig:"HTTPS_PROXY"`
NoProxy string `envconfig:"NO_PROXY"`
}

type EnvInputs struct {
DeployISO string `envconfig:"DEPLOY_ISO" required:"true"`
DeployInitrd string `envconfig:"DEPLOY_INITRD" required:"true"`
Expand All @@ -16,9 +22,7 @@ type EnvInputs struct {
IronicRAMDiskSSHKey string `envconfig:"IRONIC_RAMDISK_SSH_KEY"`
RegistriesConfPath string `envconfig:"REGISTRIES_CONF_PATH"`
IpOptions string `envconfig:"IP_OPTIONS"`
HttpProxy string `envconfig:"HTTP_PROXY"`
HttpsProxy string `envconfig:"HTTPS_PROXY"`
NoProxy string `envconfig:"NO_PROXY"`
Proxy ProxyConfig
}

func New() (*EnvInputs, error) {
Expand Down
34 changes: 27 additions & 7 deletions pkg/ignition/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ignition

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand All @@ -9,6 +10,8 @@ import (

ignition_config_types_32 "github.com/coreos/ignition/v2/config/v3_2/types"
vpath "github.com/coreos/vcontext/path"

"github.com/openshift/image-customization-controller/pkg/env"
)

const (
Expand All @@ -28,13 +31,11 @@ type ignitionBuilder struct {
ironicRAMDiskSSHKey string
networkKeyFiles []byte
ipOptions string
httpProxy string
httpsProxy string
noProxy string
proxy env.ProxyConfig
hostname string
}

func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey, ipOptions string, httpProxy, httpsProxy, noProxy string, hostname string) (*ignitionBuilder, error) {
func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey, ipOptions string, proxy env.ProxyConfig, hostname string) (*ignitionBuilder, error) {
if ironicBaseURL == "" {
return nil, errors.New("ironicBaseURL is required")
}
Expand All @@ -50,9 +51,7 @@ func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ir
ironicAgentPullSecret: ironicAgentPullSecret,
ironicRAMDiskSSHKey: ironicRAMDiskSSHKey,
ipOptions: ipOptions,
httpProxy: httpProxy,
httpsProxy: httpsProxy,
noProxy: noProxy,
proxy: proxy,
hostname: hostname,
}, nil
}
Expand Down Expand Up @@ -115,6 +114,11 @@ func (b *ignitionBuilder) Generate() ([]byte, error) {
})
}

config.Storage.Files = append(config.Storage.Files, ignitionFileEmbed(
Copy link
Member

@dtantsur dtantsur Mar 8, 2022

Choose a reason for hiding this comment

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

Let's please only create this file if there are settings to apply. We're working on integration with the assisted service, I'd like to be able to minimize the impact on them (e.g. if they have their own default environment).

Copy link
Member Author

@zaneb zaneb Mar 8, 2022

Choose a reason for hiding this comment

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

If there is a conflict, better that we have to deal with it all the time IMHO. Test coverage for proxy variables is notoriously poor.

Copy link
Member

@dtantsur dtantsur Mar 10, 2022

Choose a reason for hiding this comment

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

I'm not sure I get it... why do we need to create this file when no proxy settings are provided?

Copy link
Member Author

@zaneb zaneb Mar 10, 2022

Choose a reason for hiding this comment

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

We don't, but if creating it causes a bug then we don't want to find out only when it is used in the field, which is how we've found bugs with the proxy config in the past.

"/etc/systemd/system.conf.d/10-default-env.conf",
0644, false,
b.defaultEnv()))

config.Storage.Files = append(config.Storage.Files, ignitionFileEmbed(
"/etc/NetworkManager/conf.d/clientid.conf",
0644, false,
Expand Down Expand Up @@ -144,3 +148,19 @@ func (b *ignitionBuilder) Generate() ([]byte, error) {

return json.Marshal(config)
}

func (b *ignitionBuilder) defaultEnv() []byte {
buf := bytes.NewBufferString("[Manager]\n")

setEnv := func(envVar, value string) {
if value != "" {
buf.WriteString(fmt.Sprintf("DefaultEnvironment=%s=\"%s\"\n",
envVar, value))
}
}

setEnv("HTTP_PROXY", b.proxy.HttpProxy)
setEnv("HTTPS_PROXY", b.proxy.HttpsProxy)
setEnv("NO_PROXY", b.proxy.NoProxy)
return buf.Bytes()
}
50 changes: 49 additions & 1 deletion pkg/ignition/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package ignition
import (
"strings"
"testing"

"github.com/openshift/image-customization-controller/pkg/env"
)

func TestGenerateRegistries(t *testing.T) {
Expand All @@ -18,7 +20,7 @@ func TestGenerateRegistries(t *testing.T) {
builder, err := New([]byte{}, []byte(registries),
"http://ironic.example.com",
"quay.io/openshift-release-dev/ironic-ipa-image",
"", "", "", "", "", "", "virthost")
"", "", "", env.ProxyConfig{}, "virthost")
if err != nil {
t.Fatalf("Unexpected error %v", err)
}
Expand All @@ -33,3 +35,49 @@ func TestGenerateRegistries(t *testing.T) {
t.Fatalf("Registries data not found in ignition:\n%s", string(ignition))
}
}

func TestDefaultEnv(t *testing.T) {
builder, _ := New([]byte{}, []byte{},
"http://ironic.example.com",
"quay.io/openshift-release-dev/ironic-ipa-image",
"", "", "", env.ProxyConfig{}, "virthost")

envConfig := builder.defaultEnv()
if string(builder.defaultEnv()) != "[Manager]\n" {
t.Errorf("Unexpected default env file:\n%s", envConfig)
}

builder, _ = New([]byte{}, []byte{},
"http://ironic.example.com",
"quay.io/openshift-release-dev/ironic-ipa-image",
"", "", "", env.ProxyConfig{
HttpProxy: "http.example.com",
HttpsProxy: "https.example.com",
NoProxy: "no_proxy.example.com",
}, "virthost")

envConfig = builder.defaultEnv()
expected := `[Manager]
DefaultEnvironment=HTTP_PROXY="http.example.com"
DefaultEnvironment=HTTPS_PROXY="https.example.com"
DefaultEnvironment=NO_PROXY="no_proxy.example.com"
`
if string(envConfig) != expected {
t.Errorf("Invalid default env file with all vars set:\n%s", envConfig)
}

builder, _ = New([]byte{}, []byte{},
"http://ironic.example.com",
"quay.io/openshift-release-dev/ironic-ipa-image",
"", "", "", env.ProxyConfig{
HttpsProxy: "https.example.com",
}, "virthost")

envConfig = builder.defaultEnv()
expected = `[Manager]
DefaultEnvironment=HTTPS_PROXY="https.example.com"
`
if string(envConfig) != expected {
t.Errorf("Invalid default env file with one var set:\n%s", envConfig)
}
}
5 changes: 1 addition & 4 deletions pkg/ignition/service_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,14 @@ Description=Ironic Agent
After=network-online.target
Wants=network-online.target
[Service]
Environment="HTTP_PROXY=%s"
Environment="HTTPS_PROXY=%s"
Environment="NO_PROXY=%s"
TimeoutStartSec=0
Restart=on-failure
ExecStartPre=/bin/podman pull %s %s
ExecStart=/bin/podman run --privileged --network host --mount type=bind,src=/etc/ironic-python-agent.conf,dst=/etc/ironic-python-agent/ignition.conf --mount type=bind,src=/dev,dst=/dev --mount type=bind,src=/sys,dst=/sys --mount type=bind,src=/run/dbus/system_bus_socket,dst=/run/dbus/system_bus_socket --mount type=bind,src=/,dst=/mnt/coreos --env "IPA_COREOS_IP_OPTIONS=%s" --env IPA_COREOS_COPY_NETWORK=%v --name ironic-agent %s
[Install]
WantedBy=multi-user.target
`
contents := fmt.Sprintf(unitTemplate, b.httpProxy, b.httpsProxy, b.noProxy, b.ironicAgentImage, flags, b.ipOptions, copyNetwork, b.ironicAgentImage)
contents := fmt.Sprintf(unitTemplate, b.ironicAgentImage, flags, b.ipOptions, copyNetwork, b.ironicAgentImage)

return ignition_config_types_32.Unit{
Name: "ironic-agent.service",
Expand Down
17 changes: 14 additions & 3 deletions pkg/ignition/service_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,20 @@ func TestIronicAgentService(t *testing.T) {
ironicAgentImage: "http://example.com/foo:latest",
ironicAgentPullSecret: "foo",
want: ignition_config_types_32.Unit{
Name: "ironic-agent.service",
Enabled: pointer.BoolPtr(true),
Contents: pointer.StringPtr("[Unit]\nDescription=Ironic Agent\nAfter=network-online.target\nWants=network-online.target\n[Service]\nEnvironment=\"HTTP_PROXY=\"\nEnvironment=\"HTTPS_PROXY=\"\nEnvironment=\"NO_PROXY=\"\nTimeoutStartSec=0\nRestart=on-failure\nExecStartPre=/bin/podman pull http://example.com/foo:latest --tls-verify=false --authfile=/etc/authfile.json\nExecStart=/bin/podman run --privileged --network host --mount type=bind,src=/etc/ironic-python-agent.conf,dst=/etc/ironic-python-agent/ignition.conf --mount type=bind,src=/dev,dst=/dev --mount type=bind,src=/sys,dst=/sys --mount type=bind,src=/run/dbus/system_bus_socket,dst=/run/dbus/system_bus_socket --mount type=bind,src=/,dst=/mnt/coreos --env \"IPA_COREOS_IP_OPTIONS=ip=dhcp6\" --env IPA_COREOS_COPY_NETWORK=false --name ironic-agent http://example.com/foo:latest\n[Install]\nWantedBy=multi-user.target\n"),
Name: "ironic-agent.service",
Enabled: pointer.BoolPtr(true),
Contents: pointer.StringPtr(`[Unit]
Description=Ironic Agent
After=network-online.target
Wants=network-online.target
[Service]
TimeoutStartSec=0
Restart=on-failure
ExecStartPre=/bin/podman pull http://example.com/foo:latest --tls-verify=false --authfile=/etc/authfile.json
ExecStart=/bin/podman run --privileged --network host --mount type=bind,src=/etc/ironic-python-agent.conf,dst=/etc/ironic-python-agent/ignition.conf --mount type=bind,src=/dev,dst=/dev --mount type=bind,src=/sys,dst=/sys --mount type=bind,src=/run/dbus/system_bus_socket,dst=/run/dbus/system_bus_socket --mount type=bind,src=/,dst=/mnt/coreos --env "IPA_COREOS_IP_OPTIONS=ip=dhcp6" --env IPA_COREOS_COPY_NETWORK=false --name ironic-agent http://example.com/foo:latest
[Install]
WantedBy=multi-user.target
`),
},
}}
for _, tt := range tests {
Expand Down
4 changes: 1 addition & 3 deletions pkg/imageprovider/rhcos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ func (ip *rhcosImageProvider) buildIgnitionConfig(networkData imageprovider.Netw
ip.EnvInputs.IronicAgentPullSecret,
ip.EnvInputs.IronicRAMDiskSSHKey,
ip.EnvInputs.IpOptions,
ip.EnvInputs.HttpProxy,
ip.EnvInputs.HttpsProxy,
ip.EnvInputs.NoProxy,
ip.EnvInputs.Proxy,
hostname,
)
if err != nil {
Expand Down