Add a cloud credentials checker asset

[rajatchopra]

A cloud creds checker asset just checks upon the credentials. The 'cluster' target
can then depend on it and, thus, cloud creds will be checked again before the
cluster is finally created.
The reason for a double check is that a cluster can be created through an
imported install-config and the credentials may be missing then.
If the cluster were to be created in one go (i.e. no install-config generation
as a sub-step), then the asset creation ensures that they are checked upon only once.
Or rather asked about the creds only once.

Solves: https://jira.coreos.com/projects/CORS/issues/CORS-949

[rajatchopra]

@staebler PTAL. Looks like a lot of code churn but really not. Important part is that cluster asset now includes this new 'platformCredsCheck' as a dependency.

[staebler]

This is not going to work well when the user brings their own install-config.yaml. In that case, the installer will prompt the user for the platform even though the platform has been specified already in the install-config.yaml.

[staebler]

Don't use underscores in the names of the go files. In other words, platform_creds_check.go should be platformcredscheck.go.

[staebler]

There are other places where the AWS creds are used. Those places need to use the creds checker, too. For example, https://github.com/openshift/installer/blob/master/pkg/asset/machines/aws/zones.go.

[rajatchopra]

@staebler credchecker now depends on installconfig, thanks for pointing out the loophole.

There are other places where the AWS creds are used. Those places need to use the creds checker, too. For example, https://github.com/openshift/installer/blob/master/pkg/asset/machines/aws/zones.go.

But it is not reachable directly via a target without going through 'create cluster'. Or so far as I could tell.

[flaper87]

Thanks for this patch. I've dropped a comment on the openstack check.

Please add me or @tomassedovic on patches that have an impact on OpenStack. 😃

Thanks

[staebler]

@staebler credchecker now depends on installconfig, thanks for pointing out the loophole.

There are other places where the AWS creds are used. Those places need to use the creds checker, too. For example, https://github.com/openshift/installer/blob/master/pkg/asset/machines/aws/zones.go.

But it is not reachable directly via a target without going through 'create cluster'. Or so far as I could tell.

@rajatchopra In the particular case of the AvailabilityZones function, that is used by the Master and Worker assets, which are parents of the Openshift Manifest asset, which is targeted by the manifests target. But, even if it were only assets targeted by the cluster target that needed the creds checker, the individual assets need the dependency not the target.

[wking]

I agree with @staebler that ec2Client should be updated to use GetSession (it would still have to insert the region locally). That way we don't have to worry about targets and access paths to that code, and we can also benefit from centralizing the NewSessionWithOptions etc. in a single helper around the vendored AWS code.

[rajatchopra]

I agree with @staebler that ec2Client should be updated to use GetSession (it would still have to insert the region locally). That way we don't have to worry about targets and access paths to that code, and we can also benefit from centralizing the NewSessionWithOptions etc. in a single helper around the vendored AWS code.

I will get the centralizing the GetSession in another PR (the minor difference right now in the two usages is that one is generic and the other is specific to the region).
I have updated the machines/{master,worker} assets meanwhile to depend upon CredsCheck, which is what @staebler was asking as a must I think.

[wking]

I will get the centralizing the GetSession in another PR (the minor difference right now in the two usages is that one is generic and the other is specific to the region).

Adjusting the region will look like this, but with the EC2 client as the wrapper. So:

func ec2Client(region string) (*ec2.EC2, error) {
  ssn, err := installeraws.GetSession()
  if err != nil {
    return nil, err
  }

  client := ec2.New(ssn, aws.NewConfig().WithRegion(region))
  return client, nil
}

I'm ok punting that if you like, but I think it's about as complicated as your current asset-dependency stopgap ;).

[rajatchopra]

I'm ok punting that if you like, but I think it's about as complicated as your current asset-dependency stopgap ;).

You made it look easy :). Done as a separate commit in this PR.
The asset-dependency is not a stop-gap and this change really has no overlap with it though, am I missing some hint?

[staebler]

I'm ok punting that if you like, but I think it's about as complicated as your current asset-dependency stopgap ;).

You made it look easy :). Done as a separate commit in this PR.
The asset-dependency is not a stop-gap and this change really has no overlap with it though, am I missing some hint?

@rajatchopra With the consolidation, I would like at some point (not necessarily for this PR) to go through the Credentials Checker Asset to get the session, making the reliance on the checker explicit.

[wking]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rajatchopra, wking

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:

• OWNERS [rajatchopra,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[rajatchopra]

/retest

[rajatchopra]

/retest

[wking]

e2e-aws:

Flaky tests:

[sig-storage] Dynamic Provisioning DynamicProvisioner should provision storage with different parameters [Suite:openshift/conformance/parallel] [Suite:k8s]

Failing tests:

[sig-storage] Volume limits should verify that all nodes have volume limits [Suite:openshift/conformance/parallel] [Suite:k8s]

The volume failure should be ignored now, but looks like this test [started after that ignore landed][3]. Hmm...

/retest

[wking]

/retest

Pick up openshift/origin#21867.