Rufio options #6741

jacobweinstock · 2023-09-28T03:15:53Z

Issue #, if available:

Description of changes:
This incorporates Rufio options per machine. The only options currently supported are for the Rufio/bmclib RPC provider. All options are configurable via env vars and cli flags.

Testing (if applicable):

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eks-distro-bot · 2023-09-28T03:15:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-09-28T03:26:55Z

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (8c247c8) 71.78% compared to head (97fde97) 71.97%.
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6741      +/-   ##
==========================================
+ Coverage   71.78%   71.97%   +0.19%     
==========================================
  Files         531      532       +1     
  Lines       41295    41590     +295     
==========================================
+ Hits        29642    29934     +292     
+ Misses       9993     9982      -11     
- Partials     1660     1674      +14

Files	Coverage Δ
cmd/eksctl-anywhere/cmd/aflag/aflag.go	`100.00% <100.00%> (ø)`
cmd/eksctl-anywhere/cmd/aflag/marker.go	`100.00% <100.00%> (ø)`
cmd/eksctl-anywhere/cmd/upgradecluster.go	`7.18% <100.00%> (+0.43%)`	⬆️
pkg/dependencies/factory.go	`76.29% <100.00%> (+0.06%)`	⬆️
pkg/providers/tinkerbell/create.go	`72.88% <100.00%> (ø)`
pkg/providers/tinkerbell/hardware/machine.go	`75.00% <ø> (ø)`
pkg/providers/tinkerbell/hardware/validator.go	`91.61% <100.00%> (+0.05%)`	⬆️
pkg/providers/tinkerbell/tinkerbell.go	`61.29% <ø> (ø)`
pkg/providers/tinkerbell/upgrade.go	`65.67% <100.00%> (+3.01%)`	⬆️
pkg/providers/vsphere/vsphere.go	`68.47% <ø> (-1.41%)`	⬇️
... and 14 more

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jacobweinstock · 2023-09-29T02:20:31Z

pkg/dependencies/factory.go

@@ -381,8 +382,20 @@ func (f *Factory) WithExecutableBuilder() *Factory {
 	return f
 }

+// ProviderOptions contains per provider options.
+type ProviderOptions struct {


this allows us to add more options in the future without continually breaking the WithProvider function signature.

chrisdoherty4 · 2023-10-09T14:07:25Z

@jacobweinstock Is this awaiting the rebase since the other PR merged?

This allows us to not have to import Rufio. Rufio uses controller-runtime v0.15.0 (and soon v0.16.2). EKS Anywhere use v0.14.2. Upgrading EKS Anywhere means all other dependent libraries will need upgraded too. This is not feasible at the moment from a time perspective, there are too many to update. For example: capv, capc, capd, abhay-krishna/cluster-api, aws/etcdadm-bootstrap-provider, etc. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

CSV and yaml unmarshalling and writing secrets from the hardware.Machine RPC options were updated and added. This sets the stage for allowing the Rufio RPC provider options to be pulled in. These options will all come from env vars. Signed-off-by: Jacob Weinstock <[email protected]>

This plumbs through non empty options from hardware.Machine to v1alpha1.Machine. Signed-off-by: Jacob Weinstock <[email protected]>

The "Machine" in BMCMachineOptions didnt provide any additional value over just BMCOptions. Signed-off-by: Jacob Weinstock <[email protected]>

This enables passing env vars instead of cli flags. Will be used for BMC secrets.

Adds cli flags for all bmc options. The flags are hidden by default but the cli mechanism we use also makes them available as env vars. The dependencies factory is updated with a new func parameter for provider options that will allow future modifications without breaking the fn signature. the cmd/eksctl-anywhere/cmd/flags pkg was renamed to be singular. It was named similar to the pflags library but with "a" for anywhere. The csv reader was modified to take in bmc options so that the options can be added during a Read. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

TinkerbellBMCCustomPayload and TinkerbellBMCCustomPayloadDotLocation. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock · 2023-10-09T15:44:57Z

@jacobweinstock Is this awaiting the rebase since the other PR merged?

Sorry, yes it is. Just rebased. Thanks!

jacobweinstock · 2023-10-09T16:05:35Z

/retest

chrisdoherty4 · 2023-10-11T22:51:13Z

pkg/providers/tinkerbell/hardware/catalogue_bmc.go

@@ -87,21 +90,147 @@ func toRufioMachine(m Machine) *v1alpha1.Machine {
 	// TODO(chrisdoherty4)


Oops 🤦🏻

chrisdoherty4 · 2023-10-11T22:54:44Z

pkg/providers/tinkerbell/hardware/csv.go

 }

 // NewCSVReader returns a new CSVReader instance that consumes csv data from r. r should return io.EOF when no more
 // records are available.
-func NewCSVReader(r io.Reader) (CSVReader, error) {
+func NewCSVReader(r io.Reader, opts *BMCOptions) (CSVReader, error) {


Are the BMC options not per hardware? I'm also giving thought to BMC options not being present at all in the hardware input (CSV) but we set this anyway?

chrisdoherty4 · 2023-10-11T22:57:04Z

pkg/providers/tinkerbell/hardware/machine.go

@@ -27,6 +28,74 @@ type Machine struct {
 	BMCUsername  string `csv:"bmc_username, omitempty"`
 	BMCPassword  string `csv:"bmc_password, omitempty"`
 	VLANID       string `csv:"vlan_id, omitempty"`
+
+	// BMCOptions are the options used for Rufio providers.
+	BMCOptions *BMCOptions `csv:"-"`


How does a user specify the opts? This looks like (though probably isn't) the options can't be specified in the CSV?

Correct, you cannot set these via the hardware csv. They are all set via env vars only.

chrisdoherty4 · 2023-10-11T22:59:37Z

I left some inline comments but broadly I'm not clear how this is meant to work. Do we expect all hardware to use the same settings? What happens if we want to plumb through different settings per hardware as supported by Rufio?

jacobweinstock · 2023-10-11T23:00:41Z

I left some inline comments but broadly I'm not clear how this is meant to work. Do we expect all hardware to use the same settings? What happens if we want to plumb through different settings per hardware as supported by Rufio?

Thanks.
Do we expect all hardware to use the same settings? Yes.
What happens if we want to plumb through different settings per hardware as supported by Rufio? Not supported right now. Don't have a use case for that yet either.

chrisdoherty4 · 2023-10-11T23:06:03Z

Understood. I don't think there's anything in the 'interface' to configuring this that would prevent future expansion: does that sound accurate? Specifically, we're not adding anything to the hardware CSV and we're just offering a bunch of CLI/ENV options for configuring a single rpc endpoint - does that sound right?

chrisdoherty4 · 2023-10-11T23:14:01Z

cmd/eksctl-anywhere/cmd/createcluster.go

-	flags.MarkRequired(createClusterCmd.Flags(), flags.ClusterConfig.Name)
+func tinkerbellFlags(fs *pflag.FlagSet, r *hardware.RPCOpts) {
+	aflag.String(aflag.TinkerbellBMCConsumerURL, &r.ConsumerURL, fs)
+	aflag.MarkHidden(fs, aflag.TinkerbellBMCConsumerURL.Name)


What's the reason for hiding these?

I am OK with the env/cli variables. Curious if we thought about including these in the cluster config somehow? Historically there's been a strong preference for using the cluster config and this seems like something that could feature on the datacenter config?

@vivek-koppuru @jiayiwang7 @abhinavmpandey08

What's the reason for hiding these? These are what i would consider advanced options and not something I would recommend we clutter a cli help message with.

Curious if we thought about including these in the cluster config somehow? Yeah, i did consider it. The concern i have for adding to the cluster config is that i believe we don't understand enough about how/if/when customers will use this functionality. I would prefer to hide it like i did and learn more before adding it to the cluster config where deprecating feels a lot more difficult.

Great points, agreed.

chrisdoherty4

LGTM.

The inline comment on cluster config vs use of CLI/env vars should probably be circulated some.

jacobweinstock · 2023-10-13T15:06:03Z

/approve

eks-distro-bot · 2023-10-13T15:06:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jacobweinstock

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~~ [jacobweinstock]

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

Signed-off-by: Jacob Weinstock <[email protected]>

eks-distro-bot · 2023-10-13T19:04:23Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock · 2023-10-13T20:21:09Z

/retest

jacobweinstock · 2023-10-13T20:48:19Z

/test eks-anywhere-cli-attribution-presubmit

jacobweinstock · 2023-10-13T21:16:39Z

/test eks-anywhere-cli-attribution-presubmit

eks-distro-bot added the do-not-merge/work-in-progress label Sep 28, 2023

eks-distro-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 28, 2023

eks-distro-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2023

jacobweinstock commented Sep 29, 2023

View reviewed changes

jacobweinstock mentioned this pull request Sep 29, 2023

Add setting webhook data via env and cli flags: #5992

Closed

jacobweinstock requested review from chrisdoherty4, g-gaston and a team October 2, 2023 17:29

jacobweinstock marked this pull request as ready for review October 2, 2023 17:30

eks-distro-bot removed the do-not-merge/work-in-progress label Oct 2, 2023

jacobweinstock force-pushed the rufio-options branch from 0c29f39 to 070196a Compare October 4, 2023 18:50

jacobweinstock added 11 commits October 9, 2023 09:43

Fix linting issues

2eb3549

Signed-off-by: Jacob Weinstock <[email protected]>

Add population of options in toRufioMachine:

330f3f5

This plumbs through non empty options from hardware.Machine to v1alpha1.Machine. Signed-off-by: Jacob Weinstock <[email protected]>

Name change:

0c86fe8

The "Machine" in BMCMachineOptions didnt provide any additional value over just BMCOptions. Signed-off-by: Jacob Weinstock <[email protected]>

Enable CLI flags to be mapped to env vars:

0651f64

This enables passing env vars instead of cli flags. Will be used for BMC secrets.

Remove TODO code comment

816bf05

Signed-off-by: Jacob Weinstock <[email protected]>

Add http.Header env/flag parser, update code comments:

10034b1

Signed-off-by: Jacob Weinstock <[email protected]>

Fix linting issues

70bac63

Signed-off-by: Jacob Weinstock <[email protected]>

Add BMC env opts:

77f0b4e

TinkerbellBMCCustomPayload and TinkerbellBMCCustomPayloadDotLocation. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock force-pushed the rufio-options branch from 070196a to 77f0b4e Compare October 9, 2023 15:44

chrisdoherty4 reviewed Oct 11, 2023

View reviewed changes

chrisdoherty4 approved these changes Oct 11, 2023

View reviewed changes

eks-distro-bot assigned chrisdoherty4 Oct 11, 2023

eks-distro-bot added the lgtm label Oct 11, 2023

eks-distro-bot added the approved label Oct 13, 2023

jacobweinstock removed the request for review from g-gaston October 13, 2023 16:08

Add header flag test

9783af5

Signed-off-by: Jacob Weinstock <[email protected]>

eks-distro-bot removed the lgtm label Oct 13, 2023

jacobweinstock added 3 commits October 13, 2023 13:16

Add test for MarkHidden

11f319f

Signed-off-by: Jacob Weinstock <[email protected]>

Add test case for MarkHidden

32d7e71

Signed-off-by: Jacob Weinstock <[email protected]>

Add tests for the basic aflag types

97fde97

Signed-off-by: Jacob Weinstock <[email protected]>

chrisdoherty4 added the lgtm label Oct 13, 2023

eks-distro-bot merged commit 1b2084d into aws:main Oct 13, 2023
5 checks passed

jacobweinstock deleted the rufio-options branch October 13, 2023 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rufio options #6741

Rufio options #6741

jacobweinstock commented Sep 28, 2023 •

edited

Loading

eks-distro-bot commented Sep 28, 2023

codecov bot commented Sep 28, 2023 •

edited

Loading

jacobweinstock Sep 29, 2023

chrisdoherty4 commented Oct 9, 2023

jacobweinstock commented Oct 9, 2023

jacobweinstock commented Oct 9, 2023

chrisdoherty4 Oct 11, 2023

chrisdoherty4 Oct 11, 2023 •

edited

Loading

chrisdoherty4 Oct 11, 2023 •

edited

Loading

jacobweinstock Oct 11, 2023

chrisdoherty4 commented Oct 11, 2023

jacobweinstock commented Oct 11, 2023

chrisdoherty4 commented Oct 11, 2023

chrisdoherty4 Oct 11, 2023 •

edited

Loading

jacobweinstock Oct 11, 2023

chrisdoherty4 Oct 12, 2023

chrisdoherty4 left a comment

jacobweinstock commented Oct 13, 2023

eks-distro-bot commented Oct 13, 2023

eks-distro-bot commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

		@@ -87,21 +90,147 @@ func toRufioMachine(m Machine) *v1alpha1.Machine {
		// TODO(chrisdoherty4)

Rufio options #6741

Rufio options #6741

Conversation

jacobweinstock commented Sep 28, 2023 • edited Loading

eks-distro-bot commented Sep 28, 2023

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

jacobweinstock Sep 29, 2023

Choose a reason for hiding this comment

chrisdoherty4 commented Oct 9, 2023

jacobweinstock commented Oct 9, 2023

jacobweinstock commented Oct 9, 2023

chrisdoherty4 Oct 11, 2023

Choose a reason for hiding this comment

chrisdoherty4 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jacobweinstock Oct 11, 2023

Choose a reason for hiding this comment

chrisdoherty4 commented Oct 11, 2023

jacobweinstock commented Oct 11, 2023

chrisdoherty4 commented Oct 11, 2023

chrisdoherty4 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jacobweinstock Oct 11, 2023

Choose a reason for hiding this comment

chrisdoherty4 Oct 12, 2023

Choose a reason for hiding this comment

chrisdoherty4 left a comment

Choose a reason for hiding this comment

jacobweinstock commented Oct 13, 2023

eks-distro-bot commented Oct 13, 2023

eks-distro-bot commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

jacobweinstock commented Oct 13, 2023

jacobweinstock commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

chrisdoherty4 Oct 11, 2023 •

edited

Loading

chrisdoherty4 Oct 11, 2023 •

edited

Loading

chrisdoherty4 Oct 11, 2023 •

edited

Loading