openshift-online / ocm-sdk-go Goto Github PK

https://github.com/osbuild/osbuild-composer/ is a project that's aiming to run on api.openshift.com in future. However it also releases regularly into fedora/epel etc... And in order to depend on the ocm-sdk a few minor changes could help us greatly.

Most of ocm-sdk's dependencies are release into these platforms already, except:

• github.com/grokify/html-strip-tags-go:
  alternative https://github.com/microcosm-cc/bluemonday is packaged, plus it's much better suited to handle untrusted html.
• https://gitlab.com/c0b/go-ordered-json:
  a bit of a weird one, doesn't have go module support and seems really unmaintained (last code update was 3 years ago).
  We could package it alongside, but it has bugs (the tests don't work). Could also just copy the needed file into this project with a reference to it's upstream.

There's also the issue that https://github.com/dgrijalva/jwt-go is unmaintained. https://github.com/golang-jwt/jwt is a maintained fork, which sadly isn't yet packaged, but hopefully will be soon.

I'm happy to do the work on this, but want to discuss if it would be acceptable from your side first.

I would like to be able to create a mock object for some of our status-board structs that we can then use within tests. Currently I cannot create a struct like StatusesListResponse because its members are private.

What I would like is something like NewMockStatusesListResponse(...) and NewMockStatusList(...)

The service_logs API as currently generated makes the Add method available by way of the ClusterLogsClient for the path /api/service_logs/v1/cluster_logs/clusters/{uuid}/cluster_logs. The backend however only implements List for this path making the sdk somewhat confusing.

tokensContext() grabs a mutex:

This is sub-optimal — usually we renew some time before previous token expires, and could be used meanwhile without delaying requests.
Even the original request that triggered the renewal could proceed without blocking!

A second question is what happens with retries and concurrent requests. The mutex is only held for a single try, but if a retry is needed it means we still don't have valid auth, so the 2nd request will immediately take the mutex; the first retry loop doesn't know that happened, so we can get several retry loops running in parallel. 🔀
Once one of them succeeds, the following tokensContext() tries will all see valid auth and succeed immediately, and all retry loops will complete.

• So this sounds internally safe to me;
• however, externally we're not keeping the backoff we intended. Are we risking a "thundering herd" problem where we make an SSO outage worse by amplifying the number of requests?
• also (very minor), this may confuse the retries logging/metrics #231.

I propose we move renewal off the requesting goroutine into a separate dedicated goroutine. One per Connection object. This will simplify management of retries.
We need to decide whether renewal should wait for a request that needs auth (as done now, meaning zero traffic when unused), or be timer-based (minimizes latency).

@igoihman @nimrodshn @tzvatot @vkareh @zgalor @petli-openshift WDYT?

Currently the sdk supports only application/json content-type and returns error for the others.

It would be great if we could support other content-types such as text/csv

Version 0.1.164 of the SDK sends an incorrect Host header to the API server, containing the name of the SSO server instead of the name of the API server:

2021/03/22 12:40:47 OpenID token URL wasn't provided, will use 'https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token'
2021/03/22 12:40:47 OpenID client identifier wasn't provided, will use 'cloud-services'
2021/03/22 12:40:47 OpenID client secret wasn't provided, will use ''
2021/03/22 12:40:47 Added URL with prefix '', regular expression '^(/.*)?$' and URL 'https://api.openshift.com'
2021/03/22 12:40:47 Resolved URL is 'https://api.openshift.com/api/clusters_mgmt/v1/clusters?page=1&search=name+like+%27my%25%27&size=10'
2021/03/22 12:40:47 OCM auth: Bearer token isn't available
2021/03/22 12:40:47 OCM auth: trying to get new tokens (attempt 1)
2021/03/22 12:40:47 OCM auth: requesting new token using the refresh token grant
2021/03/22 12:40:47 Client for key 'tcp' doesn't exist, will create it
2021/03/22 12:40:47 Request method is POST
2021/03/22 12:40:47 Request URL is 'https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token'
2021/03/22 12:40:47 Request header 'Host' is 'sso.redhat.com'
2021/03/22 12:40:47 Request header 'Accept' is 'application/json'
2021/03/22 12:40:47 Request header 'Content-Type' is 'application/x-www-form-urlencoded'
2021/03/22 12:40:47 Request header 'User-Agent' is 'OCM-SDK/0.1.164'
2021/03/22 12:40:47 Request body follows
2021/03/22 12:40:47 client_id=cloud-services&client_secret=***&grant_type=refresh_token&refresh_token=***
2021/03/22 12:40:48 Response protocol is 'HTTP/1.1'
2021/03/22 12:40:48 Response status is '200 OK'
2021/03/22 12:40:48 Response header 'Cache-Control' is 'no-store'
2021/03/22 12:40:48 Response header 'Content-Type' is 'application/json'
2021/03/22 12:40:48 Response header 'Date' is 'Mon, 22 Mar 2021 11:40:48 GMT'
2021/03/22 12:40:48 Response header 'Pragma' is 'no-cache'
2021/03/22 12:40:48 Response header 'Set-Cookie' is 'BIGipServer~prod~keycloak-webssl-https=862389514.64288.0000; path=/; Httponly; Secure'
2021/03/22 12:40:48 Response header 'Strict-Transport-Security' is 'max-age=31536000; includeSubDomains'
2021/03/22 12:40:48 Response header 'Vary' is 'Accept-Encoding'
2021/03/22 12:40:48 Response header 'X-Content-Type-Options' is 'nosniff'
2021/03/22 12:40:48 Response header 'X-Frame-Options' is 'SAMEORIGIN'
2021/03/22 12:40:48 Response header 'X-Site' is 'phx2'
2021/03/22 12:40:48 Response header 'X-Xss-Protection' is '1; mode=block'
2021/03/22 12:40:48 Response body follows
2021/03/22 12:40:48 {
  "access_token": "***",
  "expires_in": 900,
  "refresh_expires_in": 0,
  "refresh_token": "***",
  "token_type": "bearer",
  "id_token": "***",
  "not-before-policy": 0,
  "session_state": "3df8402d-8476-41a5-a128-f7bbc7d03d4b",
  "scope": "openid offline_access"
}
2021/03/22 12:40:48 OCM auth: got tokens on attempt 1.
2021/03/22 12:40:48 Request method is GET
2021/03/22 12:40:48 Request URL is 'https://api.openshift.com/api/clusters_mgmt/v1/clusters?page=1&search=name+like+%27my%25%27&size=10'
2021/03/22 12:40:48 Request header 'Host' is 'api.openshift.com'
2021/03/22 12:40:48 Request header 'Accept' is 'application/json'
2021/03/22 12:40:48 Request header 'Authorization' is omitted
2021/03/22 12:40:48 Request header 'User-Agent' is 'OCM-SDK/0.1.164'
Can't retrieve page 1: can't send request: Get "https://api.openshift.com/api/clusters_mgmt/v1/clusters?page=1&search=name+like+%27my%25%27&size=10": x509: certificate is valid for *.apps.app-sre-prod-04.i5h0.p1.openshiftapps.com, api.app-sre-prod-04.i5h0.p1.openshiftapps.com, rh-api.app-sre-prod-04.i5h0.p1.openshiftapps.com, not sso.redhat.com

That results in a rejected request because the verification of the host name in the TLS certificate fails.

Currently we have some code that looks something like this:

statuses := openapiStatusBoard.StatusList{}
err := json.Unmarshal(response.Bytes(), &statuses) // Bytes() is just the body

Unmarshalling a json response into a struct seems to be a popular idiom in Golang and it's convenient. However, it's not currently possible without using a raw HTTP GET method, somewhat defeating the nice API you've given us.

What I would like is the ability to do something like this:

response, _ := connection.StatusBoard().V1().Products().List().SendContext(ctx)
fmt.Println(response.Body())

Currently, the authentication package has an internal error handler which cannot be overridden.

It would be great if we can add support to override this so that we can handle our errors consistently throughout our application which uses this package. Would this be possible?

The following error is produced when making a successful request to the accountsmgmt Notify() endpoint:

 OCM-CA-9: ReadObject: expect { or , or } or n, but found , error found in #0 byte of ...||..., bigger context ...||...

It would be helpful for automation if the following fields were added to the json output of describe cluster:

• Cluster's InfraID
• Security Groups created (Master and Worker)
• Load Balancers created (API and Apps)
• vpcID

$ go run ./cmd/ocm create cluster beni-6 --parameter=dryRun=true --debug
...
I1115 18:25:44.866889  885691 dump.go:143] Response status is '204 No Content'
I1115 18:25:44.866932  885691 dump.go:155] Response header 'Content-Encoding' is 'gzip'
I1115 18:25:44.866943  885691 dump.go:155] Response header 'Content-Type' is 'application/json'
I1115 18:25:44.866952  885691 dump.go:155] Response header 'Date' is 'Sun, 15 Nov 2020 16:25:44 GMT'
I1115 18:25:44.866964  885691 dump.go:155] Response header 'Vary' is 'Accept-Encoding'
I1115 18:25:44.866977  885691 dump.go:155] Response header 'X-Operation-Id' is '1gvgh21bj38trolqi5qu1b95mrud1umk'
I1115 18:25:44.866987  885691 dump.go:159] Response body follows
I1115 18:25:44.867006  885691 dump.go:249] 
Error: Failed to create cluster: unable to create cluster: ReadObject: expect { or , or } or n, but found , error found in #0 byte of ...||..., bigger context ...||...
exit status 1

This only happens in typed interface (untyped connection.Post() works fine) and only with --debug enabled.
The error happes after dumpRoundTripper completes, but depends on it taking this branch (body != nil but has len 0):

I confirmed checking for 204 and avoiding that branch is enough to solve this issue, but that's probably not the right solution...

Here's an example of a pretty idiomatic SDK usage:
https://github.com/openshift/osde2e/blob/143f353ea48119e9dc3e65868cc2466e738c9943/pkg/osd/cluster.go#L74-L103
(where u.clusters() returns u.conn.ClustersMgmt().V1().Clusters())

This code eventually logged the following error:

failed waiting for cluster ready: Encountered error waiting for cluster: couldn't get cluster '1b9b8un2n10cbv8l5oaq2ek0m5fdf07p': couldn't retrieve cluster '1b9b8un2n10cbv8l5oaq2ek0m5fdf07p': CLUSTERS-MGMT-500

where most of the error is from the calling osde2e code, apparently CLUSTERS-MGMT-500 is the only part that came from the SDK.

In this case, I failed to find matching clusters-service logs to this 500 (interal hive-integration env).
Almost always 500 errors include an "operation_id": "...." field, and these make later searching much easier.
I think it would be good if the SDK included such id, when available, in the error string it returns.

I guess the reason we didn't do this already is our internal code using the SDK tends to log the full response body on errors. In osde2e code, the immediate function returns the body too:

return resp.Body(), resp.Error()

but then the calling function ignores the body because err != nil. Which I think is likely to happen in real-world usage, frequently only the error string propogates out...

@jhernand what do you think? cc @meowfaceman @ethernetdan from osde2e

We've seen errors from SSO/akamai like this:

Access Denied Access Denied You don\'t have permission to access "http://sso.redhat.com/AK_PM_VPATH0/" on this server. Reference #18.46019b8.16008613...

It looks like the reference numbers in your logs are truncated
A full reference number looks like Reference #3.2461aad1.1595016997.1e085d88

As these numbers can help SSO team check up on an error, lets increase the truncation limit here:

Lack of GCPEncryptionKey in cluster builder, it blocks the migration of QE.

I would like to be able to pass a list of parameters to a List method. This would allow users to obtain a subset of data in a single call. The status-board project deliberately does not implement the search= parameter for security reasons, so that is not an option for us.

Within status-board we have an endpoint /api/status-board/v1/status_updates with accepts a list of product ID's. The equivalent command line call, for example:

ocm get /api/status-board/v1/status_updates --parameter product_ids=4aff22fc-b5b9-4863-adfd-92dc92974cd5,33496a9f-f2bc-408a-b503-be726ab04976

In essence, what I would like is to be able to do is something like this:

connection.StatusBoard().V1().Products().Updates().List("xxx", "yyy") or perhaps connection.StatusBoard().V1().Products().Updates().List().ProductIds("xxx", "yyy"), whichever makes more sense.

Currently attempting to add an in ProductIds []String param to the statuses_resource.model struct results in:

E: Method 'StatusBoard.V1.Statuses.List' should have exactly one list parameter but it has 2

There are two issues blocking use of the AccessToken client:

1. It is not possible, through the client, to set and send an empty request body. Attempting to do so results in the following error:

client := connection.AccountsMgmt().V1().AccessToken()
response, err := client.Post().Send()

request body is mandatory for the 'POST' method

2. The response body for AccessToken cannot be defined in ocm-api-model using the current structure. The response body looks like this, with an indeterminate number of auths values:

{
  "auths": {
    "cloud.openshift.com": {
      "auth": "REDACTED"
      "email": "[email protected]"
    },
    "quay.io": {
      "auth": "REDACTED"
      "email": "[email protected]"
    },
    "registry.connect.redhat.com": {
      "auth": "REDACTED"
      "email": "[email protected]"
    },
    "registry.redhat.io": {
      "auth": "REDACTED"
      "email": "[email protected]"
    }
  }
}

We need some way to properly model this response so that the SDK and parse the JSON and return it as map[string]interface{} for consumption.

This will help support any macOS users and any forth coming linux arm users

In osdctl, the code passes both the access token and the refresh token when calling connectionBuilder.Tokens. If in the code below, a long-lived offline access token is set as config.AccessToken and config.RefreshToken is set to an empty string (because it is not needed), the API will generate an error.

connectionBuilder.Tokens(config.AccessToken, config.RefreshToken)

Get \"https://api.stage.openshift.com/api/clusters_mgmt/v1/clusters?search=omitted&size=50\": can't get access token: invalid_grant: Invalid refresh token

There may not be a reason to pass in the refresh token since the user of osdctl could log in again to generate a new temporary access token or only use the long-lived offline access token so I will likely propose that change on that repository. However, I personally believe it would be beneficial for the API to ignore empty tokens and the SDK to ignore (or warn) when passed empty strings.

I am willing to make that change to the SDK, but I wanted to ask the maintainers first so let me know your thoughts. Thank you.

This might be an issue on the Openshift API side. The identityprovider type in the types.go are not accepted by the openshift api.

Example errors:

&{400 map[Content-Type:[application/json] Date:[Fri, 08 Apr 2022 15:45:24 GMT] Server:[envoy] Vary:[Accept-Encoding] X-Envoy-Upstream-Service-Time:[5] X-Operation-Id:[cf6d38bb-ea30-412e-9ce0-a62a6a926851]] 0xc00007ec40 <nil>} status is 400, identifier is '400', code is 'CLUSTERS-MGMT-400' and operation identifier is 'cf6d38bb-ea30-412e-9ce0-a62a6a926851': Unknown identity provider type 'open_id'

&{400 map[Content-Type:[application/json] Date:[Fri, 08 Apr 2022 15:45:24 GMT] Server:[envoy] Vary:[Accept-Encoding] X-Envoy-Upstream-Service-Time:[5] X-Operation-Id:[cf6d38bb-ea30-412e-9ce0-a62a6a926851]] 0xc00007ec40 <nil>} status is 400, identifier is '400', code is 'CLUSTERS-MGMT-400' and operation identifier is 'cf6d38bb-ea30-412e-9ce0-a62a6a926851': Unknown identity provider type 'ldap'

Example code snippet:

package main

import (
	"fmt"
	"os"

	sdk "github.com/openshift-online/ocm-sdk-go"
	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
)

func main() {
	logger, err := sdk.NewGoLoggerBuilder().
		Debug(true).
		Build()
	if err != nil {
		fmt.Println("Can't build logger")
	}
	// Create the connection, and remember to close it:
	connection, err := sdk.NewConnectionBuilder().
		Logger(logger).
		Tokens("INSERT_ROSA_TOKEN_HERE").
		Build()
	if err != nil {
		fmt.Fprintf(os.Stderr, "Can't build connection: %v\n", err)
		os.Exit(1)
	}
	defer connection.Close()

	collection := connection.ClustersMgmt().V1().Clusters()

	if err != nil {
		panic(err)
	}

	var ip = cmv1.NewIdentityProvider()
	var idp = cmv1.NewOpenIDIdentityProvider()
	claims := cmv1.NewOpenIDClaims()
	claims = claims.
		Email("email").
		Name("name").
		PreferredUsername("preffered_name")
	idp = idp.ClientID("redacted").
		ClientSecret("redacted").
		Issuer("redacted").
		Claims(claims).
		ExtraAuthorizeParameters(map[string]string{}).
		ExtraScopes().
		CA("")
	sss, err := ip.Name("myauth").MappingMethod("claim").OpenID(idp).
		Type("OpenIDIdentityProvider").
		// Type("open_id"). // does not work
		// Type(cmv1.IdentityProviderTypeLDAP). // does not work
		Build()
	ipur := cmv1.IdentityProviderUpdateRequest{}
	ipur.Body(sss)
	fmt.Println(ipur)
	// ipur.SendContext(context.TODO())
	body := collection.Cluster("CLUSTERID_GOES_HERE").IdentityProviders().Add().Body(sss)
	fmt.Println(body)
	resource1, err := body.Send()
	fmt.Println(resource1, err)
}

To check what IdP type strings work, test it on a cluster:

func getIdP(clusterid string, connection *sdk.Connection) {

	fmt.Println("Getting IDentity providers")

	collection := connection.ClustersMgmt().V1().Clusters()

	idps, _ := collection.Cluster("CLUSTER_ID_GOES_HERE").IdentityProviders().List().Send()

	idplist, _ := idps.GetItems()
	fmt.Println("number of idps:", len(idplist.Slice()))
	for _, v := range idplist.Slice() {
		n, _ := v.GetName()
		fmt.Println("name:", n)
		t, _ := v.GetType()
		fmt.Println("type:", t)
		// fmt.Println(v.)
	}
}

The output of the above function is as follows:

name: Cluster-Admin
type: HTPasswdIdentityProvider
name: myauth
type: OpenIDIdentityProvider