Changes to support GLP IAM API Client #113
Conversation
eamonnotoole
force-pushed
the
glp-iam
branch
2 times, most recently
from
a92fcc8 to
1aeeded
Compare
pkg/provider/provider.go
Outdated
| // check that v is in iamVersionList | ||
| found := false | ||
| for _, version := range iamVersionList { | ||
| if version == v.(string) { |
There was a problem hiding this comment.
Unlikely to be an issue at the moment, but probably still worth using a two return form type assertion here(e.g. v, ok := v.(string)) to avoid panics.
pkg/provider/provider.go
Outdated
| @@ -11,6 +11,16 @@ import ( | |||
| "github.com/hewlettpackard/hpegl-provider-lib/pkg/registration" | |||
| ) | |||
|
|
|||
| const ( | |||
| // IAMVersionGLCS is the IAM version for GLCS | |||
| IAMVersionGLCS = "glcs" | |||
There was a problem hiding this comment.
Might be worth defining a custom string type for the IAM version and using that later in the code. This should give some useful linting hints in the future to ensure all switch statements are exhaustive and considering all possible IAM versions.
| tc := testcase | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| _, _, err := generateParamsAndURL("clientID", "clientSecret", "identityServiceURL", tc.iamVersion) |
There was a problem hiding this comment.
We should probably also verify that at least the returned URL is correct. Might be good to check that the params are all good too.
There was a problem hiding this comment.
I've added a validation function on the value of iam_service_url. Also checking params here.
pkg/token/serviceclient/handler.go
Outdated
| @@ -149,6 +153,7 @@ func (h *Handler) retrieveToken() common.Result { | |||
| } | |||
|
|
|||
| // If token is about to expire in TimeToTokenExpiry seconds or less generate a new one | |||
| spew.Dump(tokenDetails) | |||
There was a problem hiding this comment.
Is this just a fancy print statement? Do we want to print out the token that may still be valid for TimeToTokenExpiry seconds?
go.mod
Outdated
| @@ -3,6 +3,7 @@ module github.com/hewlettpackard/hpegl-provider-lib | |||
| go 1.18 | |||
|
|
|||
| require ( | |||
| github.com/davecgh/go-spew v1.1.1 | |||
There was a problem hiding this comment.
This should probably be cleaned up as it's not a direct dependency anymore.
pkg/provider/provider.go
Outdated
| return []string{}, []error{fmt.Errorf("Service URL must be a string")} | ||
| } | ||
|
|
||
| _, err := url.ParseRequestURI(v.(string)) |
There was a problem hiding this comment.
Could use the result of the type assertion from line 193 here instead of asserting again.
There was a problem hiding this comment.
Indeed we could.
| @@ -20,7 +20,7 @@ var _ common.TokenChannelInterface = (*Handler)(nil) | |||
|
|
|||
| //go:generate mockgen -build_flags=-mod=mod -destination=../../mocks/IdentityAPI_mocks.go -package=mocks github.com/hewlettpackard/hpegl-provider-lib/pkg/token/serviceclient IdentityAPI | |||
| type IdentityAPI interface { | |||
| GenerateToken(context.Context, string, string, string) (string, error) | |||
| GenerateToken(context.Context, string, string, string, string) (string, error) | |||
There was a problem hiding this comment.
Would it be possible for the final parameter here to be IAMVersion?
There was a problem hiding this comment.
It is, but I'll fix up the follow-up function called to put IAMVersion as the last parameter.
pkg/provider/provider.go
Outdated
| // check that v is in iamVersionList | ||
| found := false | ||
| for _, version := range iamVersionList { | ||
| versn, ok := v.(string) |
There was a problem hiding this comment.
This type assertion should be moved outside of the loop. Also could rename versn (which looks like a typo at first glance) to versionInput or some such.
If we type assert v to IAMVersion instead of string, in line 175 we shouldn't need to cast version to string.
There was a problem hiding this comment.
I'm not sure that we can type assert to IAMVersion since terraform is going to give us a string. We can cast to IAMVersion once we've asserted that that the input is a string.
Unless I've missed something fundamental about golang types.
In this PR we add support for GLP IAM API Clients. We retain support for GLCS API Clients. We also retain support for "non-api vended" GLCS API Clients for backwards compatibility. We've added a new provider parameter "iam_version" with the corresponding env-var HPEGL_IAM_VERSION to designate which version of IAM is being used: - glcs - glp The default value of this parameter is glcs. We've added a function to validate the value of "iam_version". We've updated the "description" field for "api_vended_service_client" and "tenant_id" in light of the support for GLP IAM. The issuertoken package has been modified to generate the correct URL and the correct set of http request parameters depending on which type of IAM is being used. We've had to update various unit-tests and the generated mocks. Signed-off-by: Eamonn O'Toole <[email protected]>
Added a custom IAMVersion string type Added a ValidationFunction for iam_service_url Signed-off-by: Eamonn O'Toole <[email protected]>
Clean up. Signed-off-by: Eamonn O'Toole <[email protected]>
Signed-off-by: Eamonn O'Toole <[email protected]>
In this PR we add support for GLP IAM API Clients. We retain support for GLCS API Clients. We also retain support for "non-api vended" GLCS API Clients for backwards compatibility.
We've added a new provider parameter "iam_version" with the corresponding env-var HPEGL_IAM_VERSION to designate which version of IAM is being used:
The default value of this parameter is glcs. We've added a function to validate the value of "iam_version".
We've updated the "description" field for "api_vended_service_client" and "tenant_id" in light of the support for GLP IAM.
The issuertoken package has been modified to generate the correct URL and the correct set of http request parameters depending on which type of IAM is being used.
We've had to update various unit-tests and the generated mocks.
Signed-off-by: Eamonn O'Toole [email protected]