Add support for Equinix Metal #283

detiber · 2021-01-13T18:53:18Z

Start adding support for at least documenting the secret format for Equinix Metal.

Related to: openshift/machine-api-operator#753 (comment)

openshift-ci-robot · 2021-01-13T18:53:40Z

Hi @detiber. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2021-01-13T18:53:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: detiber
To complete the pull request process, please assign twiest after the PR has been reviewed.
You can assign the PR to them by writing /assign @twiest in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

detiber · 2021-01-13T18:55:53Z

README.md

+  namespace: kube-system
+  name: equinix-metal-credentials
+data:
+  PACKET_API_KEY: EquinixMetalAPIKey


Seeing this, we should likely change the name of this variable to EQUINIX_METAL_API_KEY or similar. That would require changes to the cluster-api-provider-equinix-metal PR: openshift/cluster-api-provider-equinix-metal#1 (comment) and installer PR: openshift/installer#4472

+1, i think that makes sense for clarity

detiber · 2021-01-13T18:57:00Z

README.md

@@ -212,6 +224,7 @@ Cloud | Mint | Mint + Remove Admin Cred | Passthrough | Manual | Token
 --- | --- | --- | --- | --- | ---
 AWS | Y | 4.4+ | Y | 4.3+ | 4.6+ (expected)
 Azure | Y | N | Y | unknown | N
+EquinixMetal | ? | ? | ? | ?


@elmiko does it make sense to add more functional support beside just documenting the format? If so, it would help to get some additional context into how CCO fits into the process and what methods might make sense for Equinix Metal to support.

that's a good question, what were you thinking?

EM doesn't currently have fine grained credentials, so I suspect Passthrough may make the most sense, and then as additional support is added for finer grained credentials, we can likely add support for the other methods.

passthrough seems to make sense (i don't a ton about EM though), but as long as fits those restrictions (eg can't mint new credentials) then i would agree.

i think it makes sense to add an asterisk with a note about EM. but i don't want to create an extra maintenance burden, my main goal was to make sure we have a document about what the credentials need in the resource.

get some additional context into how CCO fits into the process

just to make sure i don't skip on this part. as i understand it, the CCO holds the credentials that the Machine-API bits will use(the sensitive bits are in secrets). when calls to the cloud provider API are made then this object will get referenced to ensure we have the proper creds.

what methods might make sense for Equinix Metal to support

another good question, i would like to confer with the installer team to see if there is anything specific we should be highlighting here.

in general, i think proceeding with the passthrough option makes sense to me but i will gather some more information from our teams.

@detiber i followed up with some folks internally and passthrough is fine for now. in the future it would be nice if equinix supported the more granular options, but as it sounds like that does not currently exist we should be fine passthrough.

detiber · 2021-01-14T17:18:22Z

README.md

@@ -212,6 +224,7 @@ Cloud | Mint | Mint + Remove Admin Cred | Passthrough | Manual | Token
 --- | --- | --- | --- | --- | ---
 AWS | Y | 4.4+ | Y | 4.3+ | 4.6+ (expected)
 Azure | Y | N | Y | unknown | N
+EquinixMetal | N | N | 4.x+ (expected) | N | N


I wasn't sure the right way to do this, whether it was to just add a 'Y' here or try to add the version like some of the fields for AWS and GCP...

Similar to my overall comment on this PR, we want to not add any in-cluster processing of CredentialsRequest CRs which means that the only column to be supported would be the Manual column.
This doesn't mean that you don't implement what is effectively Passthrough mode (where each Secret for each in-cluster component contains the same API key), but Passthrough mode implies in-cluster handling of CredentialsRequests...

Suggested change

EquinixMetal | N | N | 4.x+ (expected) | N | N

EquinixMetal | N | N | N | 4.x+ (expected) | N

detiber · 2021-01-14T17:19:11Z

go.mod

 	sigs.k8s.io/controller-runtime v0.6.2
 )
+
+replace github.com/openshift/api => github.com/detiber/api v0.0.0-20210113181726-19e9a251beff


Temporary workaround until openshift/api#784 is merged

detiber · 2021-01-14T17:20:09Z

go.mod

+	k8s.io/api v0.20.0
+	k8s.io/apimachinery v0.20.0
+	k8s.io/client-go v0.20.0
+	k8s.io/code-generator v0.20.0


This was required when I added the replace for github.com/openshift/api below.

I believe the other updates came in transitively through the replace and these updates

yeah, seems like you got caught in the middle of some changes

detiber · 2021-01-14T17:21:58Z

bindata/bootstrap/cloudcredential_v1_operator_config_custresdef.yaml

@@ -4,6 +4,7 @@ metadata:
  name: cloudcredentials.operator.openshift.io
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
+    include.release.openshift.io/single-node-developer: "true"


Updating this through make update-vendored-crds was required to pass make verify

detiber · 2021-01-14T17:22:03Z

manifests/00-config-custresdef.yaml

@@ -4,6 +4,7 @@ metadata:
  name: cloudcredentials.operator.openshift.io
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
+    include.release.openshift.io/single-node-developer: "true"


Updating this through make update-vendored-crds was required to pass make verify

here's a little background on this annotation, https://github.com/openshift/enhancements/blob/master/enhancements/single-node-developer-cluster-profile.md

detiber · 2021-01-14T17:31:15Z

pkg/equinixmetal/actuator/actuator.go

@@ -0,0 +1,364 @@
+/*


Mostly copy/paste from pkg/vsphere/actuator/actuator.go

This is one of those pieces that we don't want to add to the in-cluster operator. As an example of what we would like, you can view the PRs that were done for IBMCloud (the first pass they made at this effectively implemented Passthrough mode, but their current - as of today - PR removes this Passthrough with support for fine-grained creds) #356

detiber · 2021-01-14T17:31:36Z

pkg/operator/secretannotator/equinixmetal/reconciler.go

@@ -0,0 +1,183 @@
+/*


Mostly copy/paste from pkg/operator/secretannotator/vsphere/reconciler.go

Same thing here. We don't need in-cluster processing of the Secret that idealy should not even exist.

detiber · 2021-01-14T17:32:39Z

pkg/operator/secretannotator/equinixmetal/reconciler_test.go

@@ -0,0 +1,198 @@
+/*


Mostly copy/paste from pkg/operator/secretannotator/vsphere/reconciler_test.go

detiber · 2021-01-14T17:35:01Z

@elmiko Updated to add support for passthrough creds using the vsphere implementation as a guide

elmiko · 2021-01-14T17:38:41Z

thanks @detiber, i will be following up with some folks internally next week to make sure this is correct.

pkg/equinixmetal/actuator/actuator.go

pkg/operator/secretannotator/equinixmetal/reconciler_test.go

pkg/operator/secretannotator/secretannotator_controller.go

detiber · 2021-02-25T21:31:29Z

Rebased on top of the latest changes, still caught in the middle of a 1.19 to 1.20 rebase.

detiber · 2021-04-15T19:32:50Z

Rebased on top of the latest changes, still caught in the middle of a 1.19 to 1.20 rebase.

openshift-bot · 2021-07-14T20:16:57Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci · 2021-07-14T20:17:05Z

@detiber: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2021-08-14T00:05:33Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

joelddiaz

In general for new platforms, we want to move away from having CCO's in-cluster operator handling/processing CredentialsRequest CRs. We want to have cloud credentials provisioning/handling to be handled outside the cluster. This goes back to many customer discussions where they have concerns about a highly-privileged credential sitting inside the cluster.

As an alternative, there is a relatively new ccoctl binary in this repo that is meant to handle creating the cloud/platform resources (not applicable for Equinix b/c of supporting shared/passthrough credentials) and the manifests that need to be passed to the installer.

Also, would it be possible to break the vendoring into its own commit?

joelddiaz · 2021-09-01T18:57:37Z

README.md

@@ -59,6 +59,18 @@ data:
  azure_region: Base64encodeRegion
 ```

+### Equinix Metal


In an ideal world, this Secret should not even exist on a cluster as the individual components that need to make cloud API calls would receive their credentials via Secrets directly. This allows for a future where we can introduce fine-grained creds/permissions if/when the platform/cloud allows it.

joelddiaz · 2021-09-01T19:00:24Z

README.md

@@ -212,6 +224,7 @@ Cloud | Mint | Mint + Remove Admin Cred | Passthrough | Manual | Token
 --- | --- | --- | --- | --- | ---
 AWS | Y | 4.4+ | Y | 4.3+ | 4.6+ (expected)
 Azure | Y | N | Y | unknown | N
+EquinixMetal | N | N | 4.x+ (expected) | N | N


Similar to my overall comment on this PR, we want to not add any in-cluster processing of CredentialsRequest CRs which means that the only column to be supported would be the Manual column.
This doesn't mean that you don't implement what is effectively Passthrough mode (where each Secret for each in-cluster component contains the same API key), but Passthrough mode implies in-cluster handling of CredentialsRequests...

Suggested change

EquinixMetal | N | N | 4.x+ (expected) | N | N

EquinixMetal | N | N | N | 4.x+ (expected) | N

joelddiaz · 2021-09-01T19:09:58Z

pkg/equinixmetal/actuator/actuator.go

@@ -0,0 +1,364 @@
+/*


This is one of those pieces that we don't want to add to the in-cluster operator. As an example of what we would like, you can view the PRs that were done for IBMCloud (the first pass they made at this effectively implemented Passthrough mode, but their current - as of today - PR removes this Passthrough with support for fine-grained creds) #356

joelddiaz · 2021-09-01T19:11:17Z

pkg/operator/secretannotator/equinixmetal/reconciler.go

@@ -0,0 +1,183 @@
+/*


Same thing here. We don't need in-cluster processing of the Secret that idealy should not even exist.

openshift-bot · 2021-10-01T20:55:44Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-10-01T20:56:09Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2021

detiber mentioned this pull request Jan 13, 2021

[WIP] Equinix Metal Support openshift/machine-api-operator#753

Closed

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 13, 2021

openshift-ci-robot requested review from abutcher and dgoodwin January 13, 2021 18:53

detiber commented Jan 13, 2021

View reviewed changes

detiber force-pushed the metal branch 2 times, most recently from bc13f83 to 43b2426 Compare January 14, 2021 17:30

detiber changed the title ~~[WIP] Document secret format for Equinix Metal~~ [WIP] Add support for Equinix Metal Jan 14, 2021

detiber commented Jan 14, 2021

View reviewed changes

displague reviewed Jan 14, 2021

View reviewed changes

pkg/equinixmetal/actuator/actuator.go Outdated Show resolved Hide resolved

displague reviewed Jan 14, 2021

View reviewed changes

pkg/equinixmetal/actuator/actuator.go Outdated Show resolved Hide resolved

displague reviewed Jan 14, 2021

View reviewed changes

pkg/operator/secretannotator/equinixmetal/reconciler_test.go Outdated Show resolved Hide resolved

displague reviewed Jan 14, 2021

View reviewed changes

pkg/operator/secretannotator/secretannotator_controller.go Outdated Show resolved Hide resolved

detiber force-pushed the metal branch from 43b2426 to 88fc248 Compare January 14, 2021 22:10

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2021

detiber force-pushed the metal branch from 88fc248 to c8480d1 Compare January 19, 2021 16:10

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2021

detiber force-pushed the metal branch from c8480d1 to 82401f1 Compare January 21, 2021 20:09

This was referenced Feb 9, 2021

[WIP] Add the Packet provider openshift/installer#3914

Closed

Add the Equinix Metal provider openshift/installer#4472

Closed

detiber force-pushed the metal branch from 82401f1 to 449793c Compare February 25, 2021 21:26

detiber changed the title ~~[WIP] Add support for Equinix Metal~~ Add support for Equinix Metal Feb 25, 2021

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 Feb 25, 2021

Add support for Equinix Metal

bf03d3d

detiber force-pushed the metal branch from 449793c to bf03d3d Compare April 15, 2021 19:26

openshift-ci bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 14, 2021

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 14, 2021

joelddiaz reviewed Sep 1, 2021

View reviewed changes

openshift-ci bot closed this Oct 1, 2021

	EquinixMetal \| N \| N \| 4.x+ (expected) \| N \| N
	EquinixMetal \| N \| N \| N \| 4.x+ (expected) \| N

Add support for Equinix Metal #283

Add support for Equinix Metal #283

Conversation

detiber commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

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

elmiko Jan 13, 2021 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detiber commented Jan 14, 2021

elmiko commented Jan 14, 2021

detiber commented Feb 25, 2021

detiber commented Apr 15, 2021

openshift-bot commented Jul 14, 2021

openshift-ci bot commented Jul 14, 2021

openshift-bot commented Aug 14, 2021

joelddiaz left a 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

openshift-bot commented Oct 1, 2021

openshift-ci bot commented Oct 1, 2021

elmiko Jan 13, 2021 •

edited

Loading