Add support for custom endpoints #327
Conversation
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
pkg/client/client.go
Outdated
| return nil | ||
| } | ||
|
|
||
| customEndpointsMap := buildCustomEndpointsMap(infra.Spec.PlatformSpec.AWS.ServiceEndpoints) |
There was a problem hiding this comment.
Use the status and not the spec for these values.
There was a problem hiding this comment.
cc @alexander-demichev can we please address this?
openshift-ci-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Danil-Grigorev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
| @@ -44,6 +46,8 @@ const ( | |||
| AwsCredsSecretIDKey = "aws_access_key_id" | |||
| // AwsCredsSecretAccessKey is secret key containing AWS Secret Key | |||
| AwsCredsSecretAccessKey = "aws_secret_access_key" | |||
|
|
|||
| globalInfrastuctureName = "cluster" | |||
There was a problem hiding this comment.
can you reference where this magic "cluster" is coming from so others have context? i.e the line of code in the installer where this is instantiated
There was a problem hiding this comment.
| return nil | ||
| } | ||
|
|
||
| customEndpointsMap := buildCustomEndpointsMap(infra.Status.PlatformStatus.AWS.ServiceEndpoints) |
There was a problem hiding this comment.
this would panic if either PlatformStatus or PlatformStatus.AWS are nil?
There was a problem hiding this comment.
we have a check on lines 227-229
There was a problem hiding this comment.
as far as I can see that's checking infra.Spec.PlatformSpec.AWS. That won't prevent this line from panicking.
I think the check above should include all the pointers that can be nil that being dereferenced here infra.Status.PlatformStatus !=nil, infra.Status.PlatformStatus.AWS != nil
There was a problem hiding this comment.
PlatformSpec is not a pointer here, it can't be nil
There was a problem hiding this comment.
PlatformStatus is a pointer, PlatformStatus.AWS is another pointer.
You are dereferencing them without checking they are nil.
If for any reason they are nil this code will panic.
am I missing anything?
There was a problem hiding this comment.
Sorry, you are right. I was looking at PlatformSpec :) I pushed a fix for that.
pkg/client/client.go
Outdated
| // Do nothing when custom endpoints are missing | ||
| if infra.Status.PlatformStatus == nil && infra.Status.PlatformStatus.AWS == nil { | ||
| return nil | ||
| } |
| return nil | ||
| } | ||
|
|
||
| func buildCustomEndpointsMap(customEndpoints []configv1.AWSServiceEndpoint) map[string]string { |
There was a problem hiding this comment.
nit: add comment that explains the key values of the returned map.
|
PTAL @abhinavdahiya for the final lgtm |
|
/retest |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
This PR adds support for custom endpoints, in order to do so we need to:
https://docs.aws.amazon.com/sdk-for-go/v2/api/aws/endpoints/
https://github.com/openshift/enhancements/pull/163/files
https://github.com/openshift/installer/pull/3277/files