Repository: kubermatic/machine-controller
Pull request #708 (2020-03-11 16:04:18)

Use OAuth2 client instead of implementing access token retrieval and refresh
**What this PR does / why we need it**:

This PR uses the oauth2 client from go subrepository in order to avoid implementing the logic for obtaining and refreshing the tokens when talking to RHSM API

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Optional Release Note**:
<!--
Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
NONE
```


kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/708#" title="Author self-approved">irozzo-1A</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [irozzo-1A]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
kubermatic-bot: @irozzo-1A: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-e2e-kubevirt | ff40ee4ac00b7faa85290fa41396399a28af9a93 | [link](https://public-prow.loodse.com/view/gcs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/708/pull-machine-controller-e2e-kubevirt/1237771451847675904) | `/test pull-machine-controller-e2e-kubevirt`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=708)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
moadqassem: /approve 
/lgtm
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: 5638f55ba4f688f890529d369c7ae3367cd52cfa</details>
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/708#" title="Author self-approved">irozzo-1A</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/708#issuecomment-598058796" title="LGTM">moadqassem</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [irozzo-1A,moadqassem]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/cloudprovider/rhsm/subscription_manager.go b/pkg/cloudprovider/rhsm/subscription_manager.go
index 02a9fcb9..f71a19ee 100644
--- a/pkg/cloudprovider/rhsm/subscription_manager.go
+++ b/pkg/cloudprovider/rhsm/subscription_manager.go
@@ -17,20 +17,24 @@ limitations under the License.
 package rhsm
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
-	"strings"
 	"time"
 
+	"golang.org/x/oauth2"
+
 	"k8s.io/klog"
 )
 
+const defaultTimeout = 10 * time.Second
+
 // RedHatSubscriptionManager is responsible for removing redhat subscriptions.
 type RedHatSubscriptionManager interface {
+	//TODO(irozzo) add context in input to give more control to the caller
 	UnregisterInstance(machineName string) error
 }
 
@@ -50,16 +54,9 @@ type systemsResponse struct {
 	Body       []body     `json:"body"`
 }
 
-type credentials struct {
-	AccessToken string `json:"access_token"`
-}
-
 type defaultRedHatSubscriptionManager struct {
-	offlineToken    string
-	authURL         string
 	apiURL          string
 	client          *http.Client
-	credentials     *credentials
 	requestsLimiter int
 }
 
@@ -69,25 +66,37 @@ func NewRedHatSubscriptionManager(offlineToken string) (RedHatSubscriptionManage
 	if offlineToken == "" {
 		return nil, errors.New("RedHatSubscriptionManager offline token cannot be empty")
 	}
-
 	return &defaultRedHatSubscriptionManager{
-		client: &http.Client{
-			Timeout: 10 * time.Second,
-		},
-		authURL:         "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token",
 		apiURL:          "https://api.access.redhat.com/management/v1/systems",
-		offlineToken:    offlineToken,
+		client:          newOAuthClientWithRefreshToken(offlineToken, "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token"),
 		requestsLimiter: 100,
 	}, nil
 }
 
-func (d *defaultRedHatSubscriptionManager) UnregisterInstance(machineName string) error {
-	if d.credentials == nil {
-		klog.Info("access token has been expired or not initialized, refreshing token")
-		if err := d.refreshToken(); err != nil {
-			return fmt.Errorf("failed to refresh offline token: %v", err)
-		}
+func newOAuthClientWithRefreshToken(refreshToken string, tokenURL string) *http.Client {
+	ctx := context.Background()
+	// Use the custom HTTP client when requesting an access token in order to
+	// set a timeout value.
+	// See: https://github.com/golang/oauth2/blob/c85d3e98c914e3a33234ad863dcbff5dbc425bb8/internal/token.go#L232
+	httpClient := &http.Client{Timeout: defaultTimeout}
+	ctx = context.WithValue(ctx, oauth2.HTTPClient, httpClient)
+	conf := &oauth2.Config{
+		ClientID: "rhsm-api",
+		Endpoint: oauth2.Endpoint{
+			TokenURL: tokenURL,
+		},
+	}
+	tok := &oauth2.Token{
+		RefreshToken: refreshToken,
 	}
+	// Set timeout for the client used for API calls as well.
+	c := conf.Client(ctx, tok)
+	c.Timeout = defaultTimeout
+	return c
+}
+
+func (d *defaultRedHatSubscriptionManager) UnregisterInstance(machineName string) error {
+	ctx := context.Background()
 
 	var (
 		retries    = 0
@@ -95,14 +104,8 @@ func (d *defaultRedHatSubscriptionManager) UnregisterInstance(machineName string
 	)
 
 	for retries < maxRetries {
-		machineUUID, err := d.findSystemsProfile(machineName)
+		machineUUID, err := d.findSystemsProfile(ctx, machineName)
 		if err != nil {
-			if err == errUnauthenticatedRequest {
-				if err := d.refreshToken(); err != nil {
-					klog.Errorf("failed to refresh offline token: %v", err)
-					continue
-				}
-			}
 			return fmt.Errorf("failed to find system profile: %v", err)
 		}
 
@@ -111,18 +114,12 @@ func (d *defaultRedHatSubscriptionManager) UnregisterInstance(machineName string
 			return nil
 		}
 
-		err = d.deleteSubscription(machineUUID)
+		err = d.deleteSubscription(ctx, machineUUID)
 		if err == nil {
 			klog.Infof("subscription for vm %v has been deleted successfully", machineUUID)
 			return nil
 		}
 
-		if err == errUnauthenticatedRequest {
-			if err := d.refreshToken(); err != nil {
-				klog.Errorf("failed to refresh offline token: %v", err)
-				continue
-			}
-		}
 		klog.Errorf("failed to delete subscription for system: %s due to: %v", machineUUID, err)
 		time.Sleep(2 * time.Second)
 		retries++
@@ -131,48 +128,10 @@ func (d *defaultRedHatSubscriptionManager) UnregisterInstance(machineName string
 	return errors.New("failed to delete system profile after max retires number has been reached")
 }
 
-func (d *defaultRedHatSubscriptionManager) refreshToken() error {
-	payload := url.Values{}
-	payload.Add("grant_type", "refresh_token")
-	payload.Add("client_id", "rhsm-api")
-	payload.Add("refresh_token", d.offlineToken)
-
-	req, err := http.NewRequest("POST", d.authURL, strings.NewReader(payload.Encode()))
-	if err != nil {
-		return err
-	}
-
-	req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
-
-	res, err := d.client.Do(req)
-	if err != nil {
-		return err
-	}
-
-	defer res.Body.Close()
-
-	var creds = &credentials{}
-	data, err := ioutil.ReadAll(res.Body)
-	if err != nil {
-		return fmt.Errorf("failed while reading response: %v", err)
-	}
-
-	if res.StatusCode == http.StatusOK {
-		if err := json.Unmarshal(data, creds); err != nil {
-			return fmt.Errorf("failed while unmarshalling data: %v", err)
-		}
-		d.credentials = creds
-
-		return nil
-	}
-
-	return fmt.Errorf("error while executing request with status code: %v and message: %s", res.StatusCode, string(data))
-}
-
-func (d *defaultRedHatSubscriptionManager) findSystemsProfile(name string) (string, error) {
+func (d *defaultRedHatSubscriptionManager) findSystemsProfile(ctx context.Context, name string) (string, error) {
 	var offset int
 	for {
-		systemsInfo, err := d.executeFindSystemsRequest(offset)
+		systemsInfo, err := d.executeFindSystemsRequest(ctx, offset)
 		if err != nil {
 			return "", fmt.Errorf("failed to retrieve systems: %v", err)
 		}
@@ -194,13 +153,12 @@ func (d *defaultRedHatSubscriptionManager) findSystemsProfile(name string) (stri
 	return "", nil
 }
 
-func (d *defaultRedHatSubscriptionManager) deleteSubscription(uuid string) error {
+func (d *defaultRedHatSubscriptionManager) deleteSubscription(ctx context.Context, uuid string) error {
 	req, err := http.NewRequest("DELETE", fmt.Sprintf("%s/%s", d.apiURL, uuid), nil)
 	if err != nil {
 		return fmt.Errorf("failed to create delete system request: %v", err)
 	}
-
-	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", d.credentials.AccessToken))
+	req.WithContext(ctx)
 
 	res, err := d.client.Do(req)
 	if err != nil {
@@ -224,13 +182,12 @@ func (d *defaultRedHatSubscriptionManager) deleteSubscription(uuid string) error
 	return nil
 }
 
-func (d *defaultRedHatSubscriptionManager) executeFindSystemsRequest(offset int) (*systemsResponse, error) {
+func (d *defaultRedHatSubscriptionManager) executeFindSystemsRequest(ctx context.Context, offset int) (*systemsResponse, error) {
 	req, err := http.NewRequest("GET", fmt.Sprintf(d.apiURL+"?limit=%v&offset=%v", d.requestsLimiter, offset), nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create fetch systems request: %v", err)
 	}
-
-	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", d.credentials.AccessToken))
+	req.WithContext(ctx)
 
 	res, err := d.client.Do(req)
 	if err != nil {


Test patch:
diff --git a/pkg/cloudprovider/rhsm/subscription_manager_test.go b/pkg/cloudprovider/rhsm/subscription_manager_test.go
index df06a549..f65c7dea 100644
--- a/pkg/cloudprovider/rhsm/subscription_manager_test.go
+++ b/pkg/cloudprovider/rhsm/subscription_manager_test.go
@@ -62,7 +62,7 @@ func TestDefaultRedHatSubscriptionManager_UnregisterInstance(t *testing.T) {
 				t.Fatalf("failed executing test: %v", err)
 			}
 			manager.(*defaultRedHatSubscriptionManager).apiURL = tt.testingServer.URL + apiPath
-			manager.(*defaultRedHatSubscriptionManager).authURL = tt.testingServer.URL + authPath
+			manager.(*defaultRedHatSubscriptionManager).client = newOAuthClientWithRefreshToken(tt.offlineToken, tt.testingServer.URL+authPath)
 			manager.(*defaultRedHatSubscriptionManager).requestsLimiter = tt.requestLimiter
 
 			if err := manager.UnregisterInstance(tt.machineName); err != nil {
@@ -80,6 +80,7 @@ func createTestingServer(pagination, rounded bool) *httptest.Server {
 	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
 		case authPath:
+			w.Header().Set("Content-Type", "application/json")
 			fmt.Fprintln(w, "{\"access_token\":\"test-access-token\"}")
 		case apiPath:
 			if pagination {