Repository: osbuild/osbuild-composer
Pull request #2457 (2022-03-18 12:50:11)

workflows: Fix Gitlab CI trigger + revert debug
Apparently Gitlab does not like single quotes. 


This pull request includes:

- [ ] adequate testing for the new functionality or fixed issue
- [ ] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`

Information regarding our GitLab pipeline (Schutzbot):

CI will not be ran automatically if WIP label is applied or the PR is in DRAFT state, instead only
a link will be provided to the pipeline which can then be triggered manually if desired. To run the
CI automatically either switch the PR to ready or apply WIP+test label.

Outside contributors need manual approval from one of the osbuild-composer maintainers.

Schutzbot will only be triggered if all Tests jobs in GitHub workflow succeed.
-->


jrusz: I updated it and changed the commit message. There were single quotes added for some reason to the push options and so I changed it to classic bash if condition which should work.

Patch:
diff --git a/.github/workflows/trigger-gitlab.yml b/.github/workflows/trigger-gitlab.yml
index e05c6bce8..78b37c6f2 100644
--- a/.github/workflows/trigger-gitlab.yml
+++ b/.github/workflows/trigger-gitlab.yml
@@ -76,7 +76,6 @@ jobs:
 
       - name: Push to gitlab
         run: |
-          set -x
           mkdir -p ~/.ssh
           echo "${SCHUTZBOT_SSH_KEY}" > ~/.ssh/id_rsa
           chmod 400 ~/.ssh/id_rsa
@@ -84,5 +83,8 @@ jobs:
           ssh-keyscan -t rsa gitlab.com >> ~/.ssh/known_hosts
           git remote add ci git@gitlab.com:osbuild/ci/osbuild-composer.git
           SKIP_CI=$(cat SKIP_CI.txt)
-          [[ "${SKIP_CI}" == true ]] && PUSH_OPTION='-o ci.variable="SKIP_CI=true"' || PUSH_OPTION=""
-          git push -f ${PUSH_OPTION} ci
+          if [[ "${SKIP_CI}" == true ]];then
+            git push -f -o ci.variable="SKIP_CI=true" ci
+          else
+            git push -f ci 
+          fi
diff --git a/cmd/osbuild-worker/main.go b/cmd/osbuild-worker/main.go
index 11ce8fa27..e6cacb7d6 100644
--- a/cmd/osbuild-worker/main.go
+++ b/cmd/osbuild-worker/main.go
@@ -142,6 +142,8 @@ func main() {
 		Authentication *struct {
 			OAuthURL         string `toml:"oauth_url"`
 			OfflineTokenPath string `toml:"offline_token"`
+			ClientId         string `toml:"client_id"`
+			ClientSecretPath string `toml:"client_secret"`
 		} `toml:"authentication"`
 		RelaxTimeoutFactor uint   `toml:"RelaxTimeoutFactor"`
 		BasePath           string `toml:"base_path"`
@@ -201,8 +203,11 @@ func main() {
 
 	var client *worker.Client
 	if unix {
-		client = worker.NewClientUnix(address, config.BasePath)
-	} else if config.Authentication != nil && config.Authentication.OfflineTokenPath != "" {
+		client = worker.NewClientUnix(worker.ClientConfig{
+			BaseURL:  address,
+			BasePath: config.BasePath,
+		})
+	} else if config.Authentication != nil {
 		var conf *tls.Config
 		conConf := &connectionConfig{
 			CACertFile: "/etc/osbuild-composer/ca-crt.pem",
@@ -214,21 +219,33 @@ func main() {
 			}
 		}
 
-		var token *string
-		var oAuthURL *string
-		t, err := ioutil.ReadFile(config.Authentication.OfflineTokenPath)
-		if err != nil {
-			logrus.Fatalf("Could not read offline token: %v", err)
+		token := ""
+		if config.Authentication.OfflineTokenPath != "" {
+			t, err := ioutil.ReadFile(config.Authentication.OfflineTokenPath)
+			if err != nil {
+				logrus.Fatalf("Could not read offline token: %v", err)
+			}
+			token = strings.TrimSpace(string(t))
 		}
-		t2 := strings.TrimSpace(string(t))
-		token = &t2
 
-		if config.Authentication.OAuthURL == "" {
-			logrus.Fatal("OAuth URL should be specified together with the offline token")
+		clientSecret := ""
+		if config.Authentication.ClientSecretPath != "" {
+			cs, err := ioutil.ReadFile(config.Authentication.ClientSecretPath)
+			if err != nil {
+				logrus.Fatalf("Could not read client secret: %v", err)
+			}
+			clientSecret = strings.TrimSpace(string(cs))
 		}
-		oAuthURL = &config.Authentication.OAuthURL
 
-		client, err = worker.NewClient(fmt.Sprintf("https://%s", address), conf, token, oAuthURL, config.BasePath)
+		client, err = worker.NewClient(worker.ClientConfig{
+			BaseURL:      fmt.Sprintf("https://%s", address),
+			TlsConfig:    conf,
+			OfflineToken: token,
+			OAuthURL:     config.Authentication.OAuthURL,
+			ClientId:     config.Authentication.ClientId,
+			ClientSecret: clientSecret,
+			BasePath:     config.BasePath,
+		})
 		if err != nil {
 			logrus.Fatalf("Error creating worker client: %v", err)
 		}
@@ -246,7 +263,11 @@ func main() {
 			}
 		}
 
-		client, err = worker.NewClient(fmt.Sprintf("https://%s", address), conf, nil, nil, config.BasePath)
+		client, err = worker.NewClient(worker.ClientConfig{
+			BaseURL:   fmt.Sprintf("https://%s", address),
+			TlsConfig: conf,
+			BasePath:  config.BasePath,
+		})
 		if err != nil {
 			logrus.Fatalf("Error creating worker client: %v", err)
 		}
diff --git a/internal/worker/client.go b/internal/worker/client.go
index 87d2574e3..3e1384784 100644
--- a/internal/worker/client.go
+++ b/internal/worker/client.go
@@ -12,27 +12,32 @@ import (
 	"net/http"
 	"net/url"
 	"sync"
-	"time"
 
 	"github.com/google/uuid"
 
 	"github.com/osbuild/osbuild-composer/internal/worker/api"
 )
 
-type bearerToken struct {
-	AccessToken     string `json:"access_token"`
-	ValidForSeconds int    `json:"expires_in"`
+type Client struct {
+	server       *url.URL
+	requester    *http.Client
+	offlineToken string
+	oAuthURL     string
+	accessToken  string
+	clientId     string
+	clientSecret string
+
+	tokenMu sync.RWMutex
 }
 
-type Client struct {
-	server           *url.URL
-	requester        *http.Client
-	offlineToken     *string
-	oAuthURL         *string
-	lastTokenRefresh *time.Time
-	bearerToken      *bearerToken
-
-	tokenMu *sync.Mutex
+type ClientConfig struct {
+	BaseURL      string
+	TlsConfig    *tls.Config
+	OfflineToken string
+	OAuthURL     string
+	ClientId     string
+	ClientSecret string
+	BasePath     string
 }
 
 type Job interface {
@@ -58,36 +63,56 @@ type job struct {
 	dynamicArgs      []json.RawMessage
 }
 
-func NewClient(baseURL string, conf *tls.Config, offlineToken, oAuthURL *string, basePath string) (*Client, error) {
-	server, err := url.Parse(baseURL)
+type tokenResponse struct {
+	AccessToken string `json:"access_token"`
+}
+
+func NewClient(conf ClientConfig) (*Client, error) {
+	server, err := url.Parse(conf.BaseURL)
 	if err != nil {
 		return nil, err
 	}
 
-	api.BasePath = basePath
+	api.BasePath = conf.BasePath
 
 	server, err = server.Parse(api.BasePath + "/")
 	if err != nil {
 		panic(err)
 	}
 
+	if conf.OAuthURL != "" {
+		if conf.ClientId == "" {
+			return nil, fmt.Errorf("OAuthURL token url specified but no client id")
+		}
+		if conf.OfflineToken == "" && conf.ClientSecret == "" {
+			return nil, fmt.Errorf("OAuthURL token url specified but no client secret or offline token")
+		}
+	}
+
 	requester := &http.Client{}
 	transport := http.DefaultTransport.(*http.Transport).Clone()
-	if conf != nil {
-		transport.TLSClientConfig = conf
+	if conf.TlsConfig != nil {
+		transport.TLSClientConfig = conf.TlsConfig
 	}
 	requester.Transport = transport
 
-	return &Client{server, requester, offlineToken, oAuthURL, nil, nil, &sync.Mutex{}}, nil
+	return &Client{
+		server:       server,
+		requester:    requester,
+		offlineToken: conf.OfflineToken,
+		oAuthURL:     conf.OAuthURL,
+		clientId:     conf.ClientId,
+		clientSecret: conf.ClientSecret,
+	}, nil
 }
 
-func NewClientUnix(path string, basePath string) *Client {
+func NewClientUnix(conf ClientConfig) *Client {
 	server, err := url.Parse("http://localhost/")
 	if err != nil {
 		panic(err)
 	}
 
-	api.BasePath = basePath
+	api.BasePath = conf.BasePath
 
 	server, err = server.Parse(api.BasePath + "/")
 	if err != nil {
@@ -97,67 +122,83 @@ func NewClientUnix(path string, basePath string) *Client {
 	requester := &http.Client{
 		Transport: &http.Transport{
 			DialContext: func(context context.Context, network, addr string) (net.Conn, error) {
-				return net.Dial("unix", path)
+				return net.Dial("unix", conf.BaseURL)
 			},
 		},
 	}
 
-	return &Client{server, requester, nil, nil, nil, nil, nil}
+	return &Client{
+		server:    server,
+		requester: requester,
+	}
 }
 
-// Note: Only call this function with Client.tokenMu locked!
-func (c *Client) refreshBearerToken() error {
-	if c.offlineToken == nil || c.oAuthURL == nil {
-		return fmt.Errorf("No offline token or oauth url available")
-	}
+func (c *Client) refreshAccessToken() error {
+	c.tokenMu.Lock()
+	defer c.tokenMu.Unlock()
 
 	data := url.Values{}
-	data.Set("grant_type", "refresh_token")
-	data.Set("client_id", "rhsm-api")
-	data.Set("refresh_token", *c.offlineToken)
+	if c.offlineToken != "" {
+		data.Set("grant_type", "refresh_token")
+		data.Set("client_id", c.clientId)
+		data.Set("refresh_token", c.offlineToken)
+	}
+	if c.clientSecret != "" {
+		data.Set("grant_type", "client_credentials")
+		data.Set("client_id", c.clientId)
+		data.Set("client_secret", c.clientSecret)
+	}
 
-	t := time.Now()
-	resp, err := http.PostForm(*c.oAuthURL, data)
+	resp, err := http.PostForm(c.oAuthURL, data)
 	if err != nil {
 		return err
 	}
 
-	var bt bearerToken
-	err = json.NewDecoder(resp.Body).Decode(&bt)
+	var tr tokenResponse
+	err = json.NewDecoder(resp.Body).Decode(&tr)
 	if err != nil {
 		return err
 	}
 
-	c.bearerToken = &bt
-	c.lastTokenRefresh = &t
+	c.accessToken = tr.AccessToken
 	return nil
 }
 
-func (c *Client) NewRequest(method, url string, body io.Reader) (*http.Request, error) {
+func (c *Client) NewRequest(method, url string, headers map[string]string, body io.Reader) (*http.Response, error) {
+	token := func() string {
+		c.tokenMu.RLock()
+		defer c.tokenMu.RUnlock()
+		return c.accessToken
+	}
+
 	req, err := http.NewRequest(method, url, body)
 	if err != nil {
 		return nil, err
 	}
 
-	// If we're using OAUTH, add the Bearer token
-	if c.offlineToken != nil {
-		// make sure we have a valid token
-		var d time.Duration
-		c.tokenMu.Lock()
-		defer c.tokenMu.Unlock()
-		if c.lastTokenRefresh != nil {
-			d = time.Since(*c.lastTokenRefresh)
-		}
-		if c.bearerToken == nil || d.Seconds() >= (float64(c.bearerToken.ValidForSeconds)*0.8) {
-			err = c.refreshBearerToken()
-			if err != nil {
-				return nil, err
-			}
+	if c.oAuthURL != "" {
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token()))
+	}
+
+	for k, v := range headers {
+		req.Header.Add(k, v)
+	}
+
+	resp, err := c.requester.Do(req)
+	if err != nil {
+		return nil, err
+	}
+
+	if resp.StatusCode == http.StatusUnauthorized && c.oAuthURL != "" {
+		err = c.refreshAccessToken()
+		if err != nil {
+			return nil, err
 		}
 
-		req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.bearerToken.AccessToken))
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token()))
+		resp, err = c.requester.Do(req)
 	}
-	return req, nil
+	return resp, err
 }
 
 func (c *Client) RequestJob(types []string, arch string) (Job, error) {
@@ -176,16 +217,10 @@ func (c *Client) RequestJob(types []string, arch string) (Job, error) {
 		panic(err)
 	}
 
-	req, err := c.NewRequest("POST", url.String(), &buf)
+	response, err := c.NewRequest("POST", url.String(), map[string]string{"Content-Type": "application/json"}, &buf)
 	if err != nil {
 		return nil, err
 	}
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := c.requester.Do(req)
-	if err != nil {
-		return nil, fmt.Errorf("error requesting job: %v", err)
-	}
 	defer response.Body.Close()
 
 	if response.StatusCode == http.StatusNoContent {
@@ -273,14 +308,7 @@ func (j *job) Update(result interface{}) error {
 		panic(err)
 	}
 
-	req, err := j.client.NewRequest("PATCH", j.location, &buf)
-	if err != nil {
-		panic(err)
-	}
-
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("PATCH", j.location, map[string]string{"Content-Type": "application/json"}, &buf)
 	if err != nil {
 		return fmt.Errorf("error fetching job info: %v", err)
 	}
@@ -294,12 +322,7 @@ func (j *job) Update(result interface{}) error {
 }
 
 func (j *job) Canceled() (bool, error) {
-	req, err := j.client.NewRequest("GET", j.location, nil)
-	if err != nil {
-		return false, err
-	}
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("GET", j.location, map[string]string{}, nil)
 	if err != nil {
 		return false, fmt.Errorf("error fetching job info: %v", err)
 	}
@@ -333,14 +356,7 @@ func (j *job) UploadArtifact(name string, reader io.Reader) error {
 		panic(err)
 	}
 
-	req, err := j.client.NewRequest("PUT", loc.String(), reader)
-	if err != nil {
-		return fmt.Errorf("cannot create request: %v", err)
-	}
-
-	req.Header.Add("Content-Type", "application/octet-stream")
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("PUT", loc.String(), map[string]string{"Content-Type": "application/octet-stream"}, reader)
 	if err != nil {
 		return fmt.Errorf("error uploading artifact: %v", err)
 	}
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh
new file mode 100755
index 000000000..10cbb6cda
--- /dev/null
+++ b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh
@@ -0,0 +1,26 @@
+#!/bin/bash
+set -eo pipefail
+source /tmp/cloud_init_vars
+
+echo "Writing client credentials."
+
+if [[ -z "$CLIENT_CREDENTIALS_ARN" ]]; then
+  echo "CLIENT_CREDENTIALS_ARN not defined, skipping."
+  exit 0
+fi
+
+# get client credentials
+/usr/local/bin/aws secretsmanager get-secret-value \
+  --endpoint-url "${SECRETS_MANAGER_ENDPOINT_URL}" \
+  --secret-id "${CLIENT_CREDENTIALS_ARN}" | jq -r ".SecretString" > /tmp/client-credentials.json
+
+CLIENT_ID=$(jq -r ".client_id" /tmp/client-credentials.json)
+jq -r ".client_secret" /tmp/client-credentials.json > /etc/osbuild-worker/client-secret
+rm -f /tmp/client-credentials.json
+
+sudo tee -a /etc/osbuild-worker/osbuild-worker.toml > /dev/null << EOF
+[authentication]
+oauth_url = "https://identity.api.openshift.com/auth/realms/rhoas/protocol/openid-connect/token"
+client_id = "${CLIENT_ID}"
+client_secret = "/etc/osbuild-worker/client-secret"
+EOF
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
index ba6e5a470..c47f1b563 100755
--- a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
+++ b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
@@ -1,9 +1,14 @@
 #!/bin/bash
-set -euo pipefail
+set -eo pipefail
 source /tmp/cloud_init_vars
 
 echo "Writing offline token."
 
+if [[ -z "$OFFLINE_TOKEN_ARN" ]]; then
+  echo "OFFLINE_TOKEN_ARN not defined, skipping."
+  exit 0
+fi
+
 # get offline token
 /usr/local/bin/aws secretsmanager get-secret-value \
   --endpoint-url "${SECRETS_MANAGER_ENDPOINT_URL}" \
@@ -15,5 +20,6 @@ rm -f /tmp/offline-token.json
 sudo tee -a /etc/osbuild-worker/osbuild-worker.toml > /dev/null << EOF
 [authentication]
 oauth_url = "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token"
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/offline-token"
 EOF
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization.service b/templates/packer/ansible/roles/common/files/worker-initialization.service
index 7bc04f481..cbb6f7558 100644
--- a/templates/packer/ansible/roles/common/files/worker-initialization.service
+++ b/templates/packer/ansible/roles/common/files/worker-initialization.service
@@ -10,6 +10,7 @@ ExecStart=touch /etc/worker-first-boot
 ExecStart=/usr/local/libexec/worker-initialization-scripts/set_hostname.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/vector.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/offline_token.sh
+ExecStart=/usr/local/libexec/worker-initialization-scripts/client_credentials.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/subscription_manager.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/get_aws_creds.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/get_azure_creds.sh


Test patch:
diff --git a/internal/worker/server_test.go b/internal/worker/server_test.go
index 1de2391a2..36bf653ba 100644
--- a/internal/worker/server_test.go
+++ b/internal/worker/server_test.go
@@ -342,7 +342,13 @@ func TestOAuth(t *testing.T) {
 	defer workSrv.Close()
 
 	/* Check that the worker supplies the access token  */
+	calls := 0
 	proxySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if calls == 0 {
+			require.Equal(t, "Bearer", r.Header.Get("Authorization"))
+			w.WriteHeader(http.StatusUnauthorized)
+			return
+		}
 		require.Equal(t, "Bearer accessToken!", r.Header.Get("Authorization"))
 		handler.ServeHTTP(w, r)
 	}))
@@ -351,6 +357,7 @@ func TestOAuth(t *testing.T) {
 	offlineToken := "someOfflineToken"
 	/* Start oauth srv supplying the bearer token */
 	oauthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		calls += 1
 		require.Equal(t, "POST", r.Method)
 		err = r.ParseForm()
 		require.NoError(t, err)
@@ -360,11 +367,9 @@ func TestOAuth(t *testing.T) {
 		require.Equal(t, offlineToken, r.FormValue("refresh_token"))
 
 		bt := struct {
-			AccessToken     string `json:"access_token"`
-			ValidForSeconds int    `json:"expires_in"`
+			AccessToken string `json:"access_token"`
 		}{
 			"accessToken!",
-			900,
 		}
 
 		w.WriteHeader(http.StatusOK)
@@ -391,7 +396,14 @@ func TestOAuth(t *testing.T) {
 	_, err = workerServer.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
 	require.NoError(t, err)
 
-	client, err := worker.NewClient(proxySrv.URL, nil, &offlineToken, &oauthSrv.URL, "/api/image-builder-worker/v1")
+	client, err := worker.NewClient(worker.ClientConfig{
+		BaseURL:      proxySrv.URL,
+		TlsConfig:    nil,
+		ClientId:     "rhsm-api",
+		OfflineToken: offlineToken,
+		OAuthURL:     oauthSrv.URL,
+		BasePath:     "/api/image-builder-worker/v1",
+	})
 	require.NoError(t, err)
 	job, err := client.RequestJob([]string{"osbuild"}, arch.Name())
 	require.NoError(t, err)
diff --git a/test/cases/api.sh b/test/cases/api.sh
index 852e6c053..bb7ddd209 100755
--- a/test/cases/api.sh
+++ b/test/cases/api.sh
@@ -1294,6 +1294,7 @@ EOF
 cat <<EOF | sudo tee "/etc/osbuild-worker/osbuild-worker.toml"
 [authentication]
 oauth_url = http://localhost:8081/token
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/token"
 EOF
 
diff --git a/test/cases/multi-tenancy.sh b/test/cases/multi-tenancy.sh
index 8a76fe211..00455e2db 100755
--- a/test/cases/multi-tenancy.sh
+++ b/test/cases/multi-tenancy.sh
@@ -90,6 +90,7 @@ EOF
 sudo tee "/etc/osbuild-worker/osbuild-worker.toml" >/dev/null <<EOF
 [authentication]
 oauth_url = "http://localhost:8081/token"
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/token"
 
 [koji.localhost.kerberos]