Skip to content

Commit

Permalink
use alternate endpoint if specified in Infrastructure
Browse files Browse the repository at this point in the history
Respect the cluster's serviceEndpoints specified in the Infrastructure object when building up a client to interact with AWS. If the IAM service endpoint is specified, use it.

Extend oidcdiscoveryendpoint controller to have a controller-runtime Client to simplify building AWS clients.
  • Loading branch information
Joel Diaz committed Jun 3, 2020
1 parent fc27fdf commit 819250e
Show file tree
Hide file tree
Showing 10 changed files with 274 additions and 86 deletions.
66 changes: 31 additions & 35 deletions pkg/aws/actuator/actuator.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
awsannotator "github.com/openshift/cloud-credential-operator/pkg/operator/secretannotator/aws"
annotatorconst "github.com/openshift/cloud-credential-operator/pkg/operator/secretannotator/constants"
"github.com/openshift/cloud-credential-operator/pkg/operator/utils"
awsutils "github.com/openshift/cloud-credential-operator/pkg/operator/utils/aws"

configv1 "github.com/openshift/api/config/v1"

Expand Down Expand Up @@ -63,7 +64,7 @@ var _ actuatoriface.Actuator = (*AWSActuator)(nil)
type AWSActuator struct {
Client client.Client
Codec *minterv1.ProviderCodec
AWSClientBuilder func(accessKeyID, secretAccessKey []byte, region, infraName string) (ccaws.Client, error)
AWSClientBuilder func(accessKeyID, secretAccessKey []byte, c client.Client) (ccaws.Client, error)
Scheme *runtime.Scheme
}

Expand All @@ -78,7 +79,7 @@ func NewAWSActuator(client client.Client, scheme *runtime.Scheme) (*AWSActuator,
return &AWSActuator{
Codec: codec,
Client: client,
AWSClientBuilder: ccaws.NewClient,
AWSClientBuilder: awsutils.ClientBuilder,
Scheme: scheme,
}, nil
}
Expand Down Expand Up @@ -141,7 +142,7 @@ func (a *AWSActuator) Exists(ctx context.Context, cr *minterv1.CredentialsReques

// needsUpdate will return whether the current credentials satisfy what's being requested
// in the CredentialsRequest
func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsRequest, region, infraName string) (bool, error) {
func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsRequest) (bool, error) {
logger := a.getLogger(cr)
// If the secret simply doesn't exist, we definitely need an update
exists, err := a.Exists(ctx, cr)
Expand All @@ -154,7 +155,7 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR

// Various checks for the kinds of reasons that would trigger a needed update
_, accessKey, secretKey := a.loadExistingSecret(cr)
awsClient, err := a.AWSClientBuilder([]byte(accessKey), []byte(secretKey), region, infraName)
awsClient, err := a.AWSClientBuilder([]byte(accessKey), []byte(secretKey), a.Client)
if err != nil {
return true, err
}
Expand All @@ -168,7 +169,7 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
return true, fmt.Errorf("unable to decode ProviderStatus: %v", err)
}

readAWSClient, err := a.buildReadAWSClient(cr, region, infraName)
readAWSClient, err := a.buildReadAWSClient(cr)
if err != nil {
log.WithError(err).Error("error creating read-only AWS client")
return true, fmt.Errorf("unable to check whether AWS user is properly tagged")
Expand All @@ -190,6 +191,10 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
return true, err
}

infraName, err := utils.LoadInfrastructureName(a.Client, logger)
if err != nil {
return true, err
}
if !userHasExpectedTags(logger, user.User, infraName, string(clusterUUID)) {
return true, nil
}
Expand Down Expand Up @@ -223,6 +228,10 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR

} else {
// for passthrough creds, just see if we have the permissions requested in the credentialsrequest
region, err := awsutils.LoadInfrastructureRegion(a.Client, logger)
if err != nil {
return true, err
}
simParams := &ccaws.SimulateParams{
Region: region,
}
Expand Down Expand Up @@ -256,18 +265,8 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest)
logger := a.getLogger(cr)
logger.Debug("running sync")

infraName, err := utils.LoadInfrastructureName(a.Client, logger)
if err != nil {
return err
}

region, err := utils.LoadInfrastructureRegion(a.Client, logger)
if err != nil {
return err
}

// Should we update anything
needsUpdate, err := a.needsUpdate(ctx, cr, region, infraName)
needsUpdate, err := a.needsUpdate(ctx, cr)
if err != nil {
logger.WithError(err).Error("error determining whether a credentials update is needed")
return &actuatoriface.ActuatorError{
Expand Down Expand Up @@ -304,7 +303,7 @@ func (a *AWSActuator) sync(ctx context.Context, cr *minterv1.CredentialsRequest)
}
} else if cloudCredsSecret.Annotations[annotatorconst.AnnotationKey] == annotatorconst.MintAnnotation {
logger.Debugf("provisioning with cred minting")
err := a.syncMint(ctx, cr, region, infraName, logger)
err := a.syncMint(ctx, cr, logger)
if err != nil {
msg := "error syncing creds in mint-mode"
logger.WithError(err).Error(msg)
Expand Down Expand Up @@ -337,7 +336,7 @@ func (a *AWSActuator) syncPassthrough(ctx context.Context, cr *minterv1.Credenti
}

// syncMint handles both create and update idempotently.
func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest, region, infraName string, logger log.FieldLogger) error {
func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest, logger log.FieldLogger) error {
var err error

awsSpec, err := DecodeProviderSpec(a.Codec, cr)
Expand All @@ -350,6 +349,11 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ
return err
}

infraName, err := utils.LoadInfrastructureName(a.Client, logger)
if err != nil {
return err
}

// Generate a randomized User for the credentials:
// TODO: check if the generated name is free
if awsStatus.User == "" {
Expand All @@ -375,12 +379,12 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ
}
}

rootAWSClient, err := a.buildRootAWSClient(cr, region, infraName)
rootAWSClient, err := a.buildRootAWSClient(cr)
if err != nil {
logger.WithError(err).Warn("error building root AWS client, will error if one must be used")
}

readAWSClient, err := a.buildReadAWSClient(cr, region, infraName)
readAWSClient, err := a.buildReadAWSClient(cr)
if err != nil {
logger.WithError(err).Error("error building read-only AWS client")
return err
Expand Down Expand Up @@ -595,16 +599,8 @@ func (a *AWSActuator) Delete(ctx context.Context, cr *minterv1.CredentialsReques
logger = logger.WithField("userName", awsStatus.User)

logger.Info("deleting credential from AWS")
region, err := utils.LoadInfrastructureRegion(a.Client, logger)
if err != nil {
return err
}
infraName, err := utils.LoadInfrastructureName(a.Client, logger)
if err != nil {
return err
}

awsClient, err := a.buildRootAWSClient(cr, region, infraName)
awsClient, err := a.buildRootAWSClient(cr)
if err != nil {
return err
}
Expand Down Expand Up @@ -730,7 +726,7 @@ func (a *AWSActuator) tagUser(logger log.FieldLogger, awsClient minteraws.Client

// buildRootAWSClient will return an AWS client using the "root" AWS creds which are expected to
// live in kube-system/aws-creds.
func (a *AWSActuator) buildRootAWSClient(cr *minterv1.CredentialsRequest, region, infraName string) (minteraws.Client, error) {
func (a *AWSActuator) buildRootAWSClient(cr *minterv1.CredentialsRequest) (minteraws.Client, error) {
logger := a.getLogger(cr).WithField("secret", fmt.Sprintf("%s/%s", rootAWSCredsSecretNamespace, rootAWSCredsSecret))

logger.Debug("loading AWS credentials from secret")
Expand All @@ -742,7 +738,7 @@ func (a *AWSActuator) buildRootAWSClient(cr *minterv1.CredentialsRequest, region
}

logger.Debug("creating root AWS client")
return a.AWSClientBuilder(accessKeyID, secretAccessKey, region, infraName)
return a.AWSClientBuilder(accessKeyID, secretAccessKey, a.Client)
}

// buildReadAWSCreds will return an AWS client using the the scaled down read only AWS creds
Expand All @@ -752,7 +748,7 @@ func (a *AWSActuator) buildRootAWSClient(cr *minterv1.CredentialsRequest, region
//
// If these are not available but root creds are, we will use the root creds instead.
// This allows us to create the read creds initially.
func (a *AWSActuator) buildReadAWSClient(cr *minterv1.CredentialsRequest, region, infraName string) (minteraws.Client, error) {
func (a *AWSActuator) buildReadAWSClient(cr *minterv1.CredentialsRequest) (minteraws.Client, error) {
logger := a.getLogger(cr).WithField("secret", fmt.Sprintf("%s/%s", roAWSCredsSecretNamespace, roAWSCredsSecret))
logger.Debug("loading AWS credentials from secret")

Expand All @@ -765,12 +761,12 @@ func (a *AWSActuator) buildReadAWSClient(cr *minterv1.CredentialsRequest, region
if err != nil {
if errors.IsNotFound(err) {
logger.Warn("read-only creds not found, using root creds client")
return a.buildRootAWSClient(cr, region, infraName)
return a.buildRootAWSClient(cr)
}
}

logger.Debug("creating read AWS client")
client, err := a.AWSClientBuilder(accessKeyID, secretAccessKey, region, infraName)
client, err := a.AWSClientBuilder(accessKeyID, secretAccessKey, a.Client)
if err != nil {
return nil, err
}
Expand All @@ -789,7 +785,7 @@ func (a *AWSActuator) buildReadAWSClient(cr *minterv1.CredentialsRequest, region
switch aerr.Code() {
case "InvalidClientTokenId":
logger.Warn("InvalidClientTokenId for read-only AWS account, likely a propagation delay, falling back to root AWS client")
return a.buildRootAWSClient(cr, region, infraName)
return a.buildRootAWSClient(cr)
}
// Any other error we just let following code sort out.
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/aws/actuator/actuator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

"github.com/openshift/cloud-credential-operator/pkg/apis"
Expand All @@ -53,7 +54,7 @@ type awsClientBuilderRecorder struct {
fakeAWSClientError error
}

func (a *awsClientBuilderRecorder) ClientBuilder(accessKeyID, secretAccessKey []byte, region, infra string) (ccaws.Client, error) {
func (a *awsClientBuilderRecorder) ClientBuilder(accessKeyID, secretAccessKey []byte, client client.Client) (ccaws.Client, error) {
a.accessKeyID = accessKeyID
a.secretAccessKey = secretAccessKey

Expand Down Expand Up @@ -195,7 +196,7 @@ func TestCredentialsFetching(t *testing.T) {
AWSClientBuilder: clientRecorder.ClientBuilder,
}

aClient, err := a.buildReadAWSClient(test.credentialsRequest, "testregion", "testinfra")
aClient, err := a.buildReadAWSClient(test.credentialsRequest)

if test.expectedError {
assert.Error(t, err, "expected error for test case")
Expand Down
28 changes: 24 additions & 4 deletions pkg/aws/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package aws

import (
"github.com/aws/aws-sdk-go/aws"
awssdk "github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/credentials"
"github.com/aws/aws-sdk-go/aws/request"
Expand Down Expand Up @@ -53,6 +54,14 @@ type Client interface {
PutObject(*s3.PutObjectInput) (*s3.PutObjectOutput, error)
}

// ClientParams holds the various optional tunables that can be used to modify the AWS
// client that will be used for API calls.
type ClientParams struct {
InfraName string
Region string
Endpoint string
}

type awsClient struct {
iamClient iamiface.IAMAPI
s3Client s3iface.S3API
Expand Down Expand Up @@ -122,11 +131,17 @@ func (c *awsClient) PutObject(input *s3.PutObjectInput) (*s3.PutObjectOutput, er
}

// NewClient creates our client wrapper object for the actual AWS clients we use.
func NewClient(accessKeyID, secretAccessKey []byte, region, infraName string) (Client, error) {
func NewClient(accessKeyID, secretAccessKey []byte, params *ClientParams) (Client, error) {
awsConfig := &awssdk.Config{}

if region != "" {
awsConfig.Region = &region
if params != nil {
if params.Region != "" {
awsConfig.Region = aws.String(params.Region)
}

if params.Endpoint != "" {
awsConfig.Endpoint = aws.String(params.Endpoint)
}
}

awsConfig.Credentials = credentials.NewStaticCredentials(
Expand All @@ -136,9 +151,14 @@ func NewClient(accessKeyID, secretAccessKey []byte, region, infraName string) (C
if err != nil {
return nil, err
}

agentText := "defaultAgent"
if params != nil && params.InfraName != "" {
agentText = params.InfraName
}
s.Handlers.Build.PushBackNamed(request.NamedHandler{
Name: "openshift.io/cloud-credential-operator",
Fn: request.MakeAddToUserAgentHandler("openshift.io cloud-credential-operator", version.Get().String(), infraName),
Fn: request.MakeAddToUserAgentHandler("openshift.io cloud-credential-operator", version.Get().String(), agentText),
})

return &awsClient{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ func TestCredentialsRequestReconcile(t *testing.T) {
Client: fakeClient,
Codec: codec,
Scheme: scheme.Scheme,
AWSClientBuilder: func(accessKeyID, secretAccessKey []byte, region, infraName string) (minteraws.Client, error) {
AWSClientBuilder: func(accessKeyID, secretAccessKey []byte, c client.Client) (minteraws.Client, error) {
if string(accessKeyID) == testRootAWSAccessKeyID {
return mockRootAWSClient, nil
} else if string(accessKeyID) == testAWSAccessKeyID {
Expand Down
31 changes: 17 additions & 14 deletions pkg/operator/oidcdiscoveryendpoint/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/aws/aws-sdk-go/service/s3"

"github.com/openshift/cloud-credential-operator/pkg/aws"
"github.com/openshift/cloud-credential-operator/pkg/operator/platform"
awsannotator "github.com/openshift/cloud-credential-operator/pkg/operator/secretannotator/aws"
awsutils "github.com/openshift/cloud-credential-operator/pkg/operator/utils/aws"

configv1 "github.com/openshift/api/config/v1"
configset "github.com/openshift/client-go/config/clientset/versioned"
Expand All @@ -34,6 +34,7 @@ import (
"k8s.io/client-go/kubernetes"

"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand Down Expand Up @@ -121,12 +122,13 @@ func Add(mgr manager.Manager, kubeconfig string) error {
}

r := &s3EndpointReconciler{
kubeclientset: kubeclientset,
configclientset: configclientset,
logger: logger,
eventRecorder: eventRecorder,
infrastructureName: infraStatus.InfrastructureName,
region: infraStatus.PlatformStatus.AWS.Region,
controllerRuntimeClient: mgr.GetClient(),
kubeclientset: kubeclientset,
configclientset: configclientset,
logger: logger,
eventRecorder: eventRecorder,
infrastructureName: infraStatus.InfrastructureName,
region: infraStatus.PlatformStatus.AWS.Region,
}

c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: r})
Expand Down Expand Up @@ -180,12 +182,13 @@ func isServiceAccountTokenSigner(meta metav1.Object) bool {
}

type s3EndpointReconciler struct {
kubeclientset *kubernetes.Clientset
configclientset *configset.Clientset
logger log.FieldLogger
eventRecorder events.Recorder
infrastructureName string
region string
controllerRuntimeClient client.Client
kubeclientset *kubernetes.Clientset
configclientset *configset.Clientset
logger log.FieldLogger
eventRecorder events.Recorder
infrastructureName string
region string
}

var _ reconcile.Reconciler = &s3EndpointReconciler{}
Expand Down Expand Up @@ -274,7 +277,7 @@ func (r *s3EndpointReconciler) reconcileS3Resources() error {
return fmt.Errorf("couldn't fetch key containing %s from cloud cred secret", awsannotator.AwsSecretAccessKeyName)
}

awsClient, err := aws.NewClient(accessKey, secretKey, r.region, r.infrastructureName)
awsClient, err := awsutils.ClientBuilder(accessKey, secretKey, r.controllerRuntimeClient)
if err != nil {
return fmt.Errorf("error creating aws client: %v", err)
}
Expand Down

0 comments on commit 819250e

Please sign in to comment.