add cloud-creds-secret #427

sallyom · 2018-10-05T22:56:23Z

Various components will require access to credentials to create AWS resources (such as registry - s3 bucket). This PR adds a secret aws-creds or openstack-creds to kube-system namesapce that holds the cloud-provider credentials plus a role aws|openstack-creds-secret-reader in -n kube-system to bind to to access the secret.

Made the creds secret generic and added logic to get ~~libvirt and~~ aws or openstack creds to generate a secret and role. Note, add whatever is required for libvirt when this lands: #296

TODO: verify the OpenStack credentials secret is what's required @flaper87. Currently the secret is encoded from /etc/openstack/clouds.yaml.
TODO in followup: Add libvirt credentials once they become necessary.

This PR adds (showing aws here):

$ oc describe secret aws-creds -n kube-system
Name:         aws-creds
Namespace:    kube-system
Labels:       <none>
Annotations:  <none>

Type:  Opaque

Data
====
aws_access_key_id:      20 bytes
aws_secret_access_key:  40 bytes

and

$ oc describe role aws-creds-secret-reader -n kube-system
Name:         aws-creds-secret-reader
Labels:       <none>
Annotations:  <none>
PolicyRule:
  Resources  Non-Resource URLs  Resource Names  Verbs
  ---------  -----------------  --------------  -----
  secrets    []                 [aws-creds]     [get]

docs/user/environment-variables.md

pkg/asset/manifests/tectonic.go

wking · 2018-10-07T04:01:29Z

TODO: create secret/role only if cloud provider is aws, and/or make secret more generically 'cloud-creds-secret'.

The libvirt analog is probably the libvird URI (and maybe a client cert/key, #296).

steveej

I wonder if we could make the secret provider-agnostic on the storage level and have the type embedded in the data instead. A use-case which makes this a bit more difficult by requiring the simultaneous storage of different provider credentials is a multi-cloud cluster.

docs/user/environment-variables.md

steveej · 2018-10-08T18:19:50Z

@wking

The libvirt analog is probably the libvird URI

Does the libvirt URI need to be part of the secret? It's not sensitive information and it's already handled by general cluster information.

wking · 2018-10-08T18:33:09Z

The libvirt analog is probably the libvird URI

Does the libvirt URI need to be part of the secret?

No, but once you need a client cert/key, the URI won't be any use without the secret key. It seems easier to just keep the cert/key and URI in the same place. But none of this needs to happen in this PR; just something to think about when planning ahead.

sallyom · 2018-10-08T18:37:39Z

@steveej @wking is a multi-cloud cluster possible atm? I'm going with installconfig platform (aws|libvirt|openstack) as the only options. Is there/will there be an option for more than 1 provider?

wking · 2018-10-08T18:47:32Z

Is there/will there be an option for more than 1 provider?

Not to my knowledge. And the installer code currently assumes one provider in a few places, so I'd just roll with that approach for now.

smarterclayton · 2018-10-09T04:28:43Z

/retest

staebler · 2018-10-11T19:52:38Z

pkg/asset/manifests/content/tectonic/aws-creds-secret-and-reader-role.go

+  namespace: kube-system
+  name: cloud-creds-secret
+data:
+  aws_access_key_id: {{.CloudCreds.AwsCredsData.Base64encodeAWSaccessKeyID}}


Do we expect that these resources will be identical for all platforms save for the data in the secret? If so, it may be better to use an if in the template that checks the platform type to determine which type of data to add rather than having a separate, mostly duplicated go file for each platform.

👍 makes sense thanks, will push a change to that effect.. er...easier said than done but looking into it

pkg/asset/manifests/tectonic.go

staebler · 2018-10-11T19:57:35Z

pkg/asset/manifests/tectonic.go

@@ -80,6 +121,19 @@ func (t *Tectonic) Generate(dependencies asset.Parents) error {
 			Data:     data,
 		})
 	}
+	switch {
+	case installConfig.Config.Platform.AWS != nil:
+		t.files = remove(t.files, "99_openstack-creds-secret-and-reader-role.yaml")


Why don't we just add conditionally rather than adding everything and removing based on the platform?

yup, that is silly.. fixing now..

staebler · 2018-10-11T20:01:44Z

pkg/asset/manifests/template.go

+}
+
+type cloudCredsTemplateData struct {
+	AwsCredsData


Give explicit names to these fields (e.g., AWS, OpenStack, Libvirt). That will (1) make the name in the template file shorter and (2) allow for fields in the various cred data struct to have the same names.

flaper87 · 2018-10-15T11:28:31Z

@sallyom thanks for including OpenStack in this PR. cc'ing @tomassedovic @hardys @russellb too.

Let us know if we can do anything to help on the OpenStack side of things.

dmage · 2018-10-23T15:59:59Z

pkg/asset/manifests/content/tectonic/cloud-creds-secret.go

+  namespace: kube-system
+  name: cloud-creds
+data:
+{{- if .CloudCreds.AWS}}


Can we add the type of the cluster (AWS, OpenStack, etc.) so that consumers know what to expect?

and while we're at it (providing AWS coordinates), the registry operator would also like to know what s3 bucket name should be used for the registry (if you don't supply a name we'll create one, but it's likely users may want to create their own bucket).

@dmage made it so, yes

@bparees, makes sense, might not be within the scope of this PR, I'll discuss with the installer team. In a follow-up PR, we can add registry s3 bucket name to the secret if/when necessary.

sallyom · 2018-10-25T10:20:16Z

/retest

sallyom · 2018-10-25T12:00:34Z

@staebler @wking, I think this is ready for approval, ptal. I've tested in libvirt, no secret/role is created and in aws aws-creds secret and aws-creds-secret-reader role are created

@flaper87 I've made the change to name os secret data clouds.yaml instead of credentials

flaper87 · 2018-10-25T14:38:40Z

@flaper87 I've made the change to name os secret data clouds.yaml instead of credentials

Perfect, thanks!

FWIW, I've tested this with openstack and openshift/release#1824 It works fine!

wking · 2018-10-26T16:19:14Z

pkg/asset/manifests/tectonic.go

@@ -16,12 +20,17 @@ import (

 const (
 	tectonicManifestDir = "tectonic"
+	// TODO: Verify this is expected os creds file


Looks like we can drop this comment?

wking · 2018-10-26T16:22:59Z

pkg/asset/manifests/tectonic.go

 )

 var (
 	tectonicConfigPath = filepath.Join(tectonicManifestDir, "00_cluster-config.yaml")

 	_ asset.WritableAsset = (*Tectonic)(nil)
+
+	// TODO: Verify which creds file to expect
+	//openStackCredsFile = os.Getenv("HOME") + "/.config/openstack"


I'm fine with a GitHub issue or commit-message comment about wanting this, but I'd rather not have commented-out code stubs committed to the repo. They just make refactoring harder, e.g. if we renamed openStackCredsFile, this would be one more thing to update.

wking · 2018-10-26T16:26:53Z

pkg/asset/manifests/tectonic.go

+	var cloudCreds cloudCredsSecretData
+	platform := ""
+	switch {
+	case installConfig.Config.Platform.AWS != nil:


To DRY up the naming, maybe use Platform.Name:

platform := installConfig.Config.Platform.Name() switch platform { case "aws": ...

👍 thanks, got it. I missed the addn of func (p *Platform) Name() string :)

wking · 2018-10-26T16:30:50Z

pkg/asset/manifests/tectonic.go

+		}
+
+		for file, content := range conditionalAssetData {
+			assetData[file] = content


conditionalAssetData seems like more trouble than it's worth. Why not just:

assetData["99_..."] = applyTemplate...

because doh-duh thank you!

wking · 2018-10-26T21:48:01Z

When you get a moment, can you squash the commits and drop the WIP?

$ git log --oneline -2 origin/pr/427
f77fda4 trevor's feedback
47b84f0 WIP-add aws-creds-secret

is probably not a useful distinction for future readers ;). While you're editing the commit message, pasting in whatever is relevant from your initial PR comment (e.g. "Various components will require access...") will make it easier for folks running blame to understand the motivation (otherwise they have to figure out the PR number and check here ;).

…st launch

sallyom · 2018-10-27T11:53:05Z

@wking squashed, re-worded commit msg :)

wking

/lgtm

openshift-ci-robot · 2018-10-27T13:18:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, 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~~ [wking]

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 5, 2018

openshift-ci-robot requested review from rajatchopra and staebler October 5, 2018 22:56

sallyom force-pushed the cloud-credentials-secret branch from ba5aef4 to cf0c81d Compare October 6, 2018 12:30

sallyom commented Oct 6, 2018

View reviewed changes

docs/user/environment-variables.md Show resolved Hide resolved

sallyom commented Oct 6, 2018

View reviewed changes

pkg/asset/manifests/tectonic.go Outdated Show resolved Hide resolved

sallyom force-pushed the cloud-credentials-secret branch from 1e19dc9 to 0aa62e6 Compare October 6, 2018 14:15

sallyom force-pushed the cloud-credentials-secret branch from 0aa62e6 to 89036b6 Compare October 8, 2018 16:43

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2018

sallyom force-pushed the cloud-credentials-secret branch 3 times, most recently from 84d59dd to 3d0ec1f Compare October 8, 2018 17:22

steveej reviewed Oct 8, 2018

View reviewed changes

docs/user/environment-variables.md Show resolved Hide resolved

sallyom changed the title ~~WIP-add aws-creds-secret~~ WIP-add cloud-creds-secret Oct 8, 2018

This was referenced Oct 8, 2018

openshift-install inconsistent AWS credentials usage #336

Closed

Use TLS authentication for cluster-api libvirt connection #296

Closed

sallyom force-pushed the cloud-credentials-secret branch from 3d0ec1f to a299995 Compare October 9, 2018 01:38

sallyom force-pushed the cloud-credentials-secret branch from a299995 to 06b370f Compare October 9, 2018 18:59

staebler reviewed Oct 11, 2018

View reviewed changes

sallyom force-pushed the cloud-credentials-secret branch 2 times, most recently from 37d62e0 to 27293f8 Compare October 15, 2018 18:05

dmage reviewed Oct 23, 2018

View reviewed changes

sallyom force-pushed the cloud-credentials-secret branch 3 times, most recently from 956dbbd to 32f6e1e Compare October 25, 2018 09:16

sallyom force-pushed the cloud-credentials-secret branch from 32f6e1e to 0dae560 Compare October 25, 2018 11:23

sallyom changed the title ~~WIP-add cloud-creds-secret~~ add cloud-creds-secret Oct 25, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2018

sallyom force-pushed the cloud-credentials-secret branch from 0dae560 to 2a54dc4 Compare October 26, 2018 14:52

wking reviewed Oct 26, 2018

View reviewed changes

sallyom mentioned this pull request Oct 26, 2018

Verify openstackCredsFile #550

Closed

sallyom force-pushed the cloud-credentials-secret branch 4 times, most recently from 379a604 to f77fda4 Compare October 26, 2018 20:06

add cloud creds secret & reader role for cluster components to use po…

28f1dae

…st launch

sallyom force-pushed the cloud-credentials-secret branch from f77fda4 to 28f1dae Compare October 27, 2018 11:51

wking approved these changes Oct 27, 2018

View reviewed changes

openshift-ci-robot assigned wking Oct 27, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2018

openshift-merge-robot merged commit 762ae17 into openshift:master Oct 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cloud-creds-secret #427

add cloud-creds-secret #427

sallyom commented Oct 5, 2018 •

edited

Loading

wking commented Oct 7, 2018

steveej left a comment

steveej commented Oct 8, 2018 •

edited

Loading

wking commented Oct 8, 2018

sallyom commented Oct 8, 2018

wking commented Oct 8, 2018

smarterclayton commented Oct 9, 2018

staebler Oct 11, 2018

sallyom Oct 15, 2018 •

edited

Loading

staebler Oct 11, 2018

sallyom Oct 15, 2018

staebler Oct 11, 2018

sallyom Oct 15, 2018

flaper87 commented Oct 15, 2018

dmage Oct 23, 2018

bparees Oct 23, 2018

sallyom Oct 23, 2018 •

edited

Loading

sallyom Oct 23, 2018 •

edited

Loading

sallyom commented Oct 25, 2018

sallyom commented Oct 25, 2018

flaper87 commented Oct 25, 2018

wking Oct 26, 2018

wking Oct 26, 2018

sallyom Oct 26, 2018

wking Oct 26, 2018

sallyom Oct 26, 2018

wking Oct 26, 2018

sallyom Oct 26, 2018

wking commented Oct 26, 2018 •

edited

Loading

sallyom commented Oct 27, 2018

wking left a comment

openshift-ci-robot commented Oct 27, 2018

add cloud-creds-secret #427

add cloud-creds-secret #427

Conversation

sallyom commented Oct 5, 2018 • edited Loading

wking commented Oct 7, 2018

steveej left a comment

Choose a reason for hiding this comment

steveej commented Oct 8, 2018 • edited Loading

wking commented Oct 8, 2018

sallyom commented Oct 8, 2018

wking commented Oct 8, 2018

smarterclayton commented Oct 9, 2018

Choose a reason for hiding this comment

sallyom Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sallyom Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

sallyom Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

sallyom commented Oct 25, 2018

sallyom commented Oct 25, 2018

flaper87 commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Oct 26, 2018 • edited Loading

sallyom commented Oct 27, 2018

wking left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 27, 2018

sallyom commented Oct 5, 2018 •

edited

Loading

steveej commented Oct 8, 2018 •

edited

Loading

sallyom Oct 15, 2018 •

edited

Loading

sallyom Oct 23, 2018 •

edited

Loading

sallyom Oct 23, 2018 •

edited

Loading

wking commented Oct 26, 2018 •

edited

Loading