Feature/v1beta1 #20

rejoshed · 2022-01-26T01:23:51Z

Issue # Bump to v1beta1 -- can't get tickets to load atm, so can't get number.

Description of changes:
Upgraded to support CAPI v1beta1 resources. See comments in Files Changed tab for further descriptions.

Testing performed:
Created a cluster in a shared network.
Deleted the cluster.
Created a cluster in an isolated network.
Scaled from 1:1 nodes to 2:3 back to 1:1.
Deleted the cluster.

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

aws-paris-bot · 2022-01-26T01:23:53Z

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

aws-paris-bot · 2022-01-26T01:23:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rejoshed

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

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

maxdrib

Given the CAPI version upgrade, should we do a minor version bump to 0.4, with an updated entry in metadata.yaml? That should make it easier to work with clusterctl as well. Otherwise code LGTM, I'd just be curious to see how it was tested.

maxdrib · 2022-01-26T13:23:53Z

config/rbac/service_account.yaml

@@ -1,5 +1,5 @@
 apiVersion: v1
 kind: ServiceAccount
 metadata:
-  name: controller-manager


why not controller manager? I believe other CAPI providers have separate docker images for a controller vs manager. In our case, they're combined. Shouldn't the name reflect that?

I was following along with the Cluster Api book's upgrade destructions. They used the name manager, and I wasn't sure whether it was relevant or not.

This change wasn't intended for the final PR unless it was necessary.

I just swapped it back as it wasn't.

rejoshed · 2022-01-27T20:30:57Z

@maxdrib

Otherwise code LGTM, I'd just be curious to see how it was tested.

It wasn't! Not at the time you reviewed anyway. It's still draft atm, but I'll get a description and write about some of my testing in the final PR. : )

Currently I've created and deleted a cluster. I'll probably try scaling up and down before the final PR.

Given the CAPI version upgrade, should we do a minor version bump to 0.4, with an updated entry in metadata.yaml?

Sure! I'll get'err done.

rejoshed · 2022-01-27T23:17:45Z

Makefile

@@ -134,7 +134,7 @@ undeploy: bin/kustomize ## Undeploy controller from the K8s cluster specified in
 .PHONY: binaries
 binaries: bin/controller-gen bin/kustomize bin/ginkgo bin/golangci-lint bin/mockgen bin/kubectl ## Locally install all needed bins.
 bin/controller-gen: ## Install controller-gen to bin.
-	GOBIN=$(PROJECT_DIR)/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.2.9
+	GOBIN=$(PROJECT_DIR)/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.8.0


Latest release is 0.80.

rejoshed · 2022-01-27T23:18:40Z

Makefile

@@ -181,19 +181,12 @@ tilt-up: cluster-api kind-cluster cluster-api/tilt-settings.json manifests cloud
 	export CLOUDSTACK_B64ENCODED_SECRET=$$(base64 -i cloud-config) && cd cluster-api && tilt up

 .PHONY: kind-cluster
-kind-cluster: cluster-api cluster-api/hack/kind-install-for-capd.sh # Create a kind cluster with a local Docker repository.
+kind-cluster: cluster-api # Create a kind cluster with a local Docker repository.


Since we're now on v1.+, we no longer have to checkout the kind install file as it's present by default.

rejoshed · 2022-01-27T23:20:02Z

api/v1beta1/cloudstackcluster_webhook.go

@@ -37,7 +37,7 @@ func (r *CloudStackCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
 		Complete()
 }

-//+kubebuilder:webhook:path=/mutate-infrastructure-cluster-x-k8s-io-v1alpha3-cloudstackcluster,mutating=true,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=cloudstackclusters,verbs=create;update,versions=v1alpha3,name=mcloudstackcluster.kb.io
+//+kubebuilder:webhook:path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-cloudstackcluster,mutating=true,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=cloudstackclusters,verbs=create;update,versions=v1beta1,name=mcloudstackcluster.kb.io,admissionReviewVersions=v1beta1


I did not see admissionReviewVersion in the CAPI book's upgrade destructions, so it took me a bit to realize it was needed. Grr...

rejoshed · 2022-01-27T23:22:09Z

api/v1beta1/webhook_suite_test.go

 	ctx, cancel = context.WithCancel(context.TODO())

 	By("bootstrapping test environment")
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "config", "crd", "bases")},
 		ErrorIfCRDPathMissing: false,
 		WebhookInstallOptions: envtest.WebhookInstallOptions{
-			DirectoryPaths: []string{filepath.Join("..", "..", "config", "webhook")},


API name change apparently.

rejoshed · 2022-01-27T23:22:34Z

api/v1beta1/webhook_suite_test.go

@@ -35,8 +35,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"


Consolidated our logging to use only klog. This seems to be the default across providers.

config/dev/insecure_manager.yaml

rejoshed · 2022-01-27T23:29:22Z

config/webhook/manifests.yaml

 ---
-apiVersion: admissionregistration.k8s.io/v1beta1
+apiVersion: admissionregistration.k8s.io/v1
 kind: MutatingWebhookConfiguration
 metadata:


These file changes were generated by controller-gen.

rejoshed · 2022-01-27T23:31:04Z

controllers/cloudstackcluster_controller.go

@@ -63,8 +63,7 @@ type CloudStackClusterReconciler struct {
 //
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
-func (r *CloudStackClusterReconciler) Reconcile(req ctrl.Request) (retRes ctrl.Result, retErr error) {
-	ctx := context.Background()
+func (r *CloudStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ctrl.Result, retErr error) {


Interface method signature is different for v1beta1. Context is passed instead.

rejoshed · 2022-01-27T23:31:55Z

controllers/cloudstackcluster_controller.go

 		&source.Kind{Type: &capiv1.Cluster{}},
-		&handler.EnqueueRequestsFromMapFunc{
-			ToRequests: util.ClusterToInfrastructureMapFunc(infrav1.GroupVersion.WithKind("CloudStackCluster"))},
+		handler.EnqueueRequestsFromMapFunc(


EnqueuRequestsFromMapFunc is now a method and not a struct.

rejoshed · 2022-01-27T23:37:07Z

main.go

@@ -51,33 +52,70 @@ func init() {
 	//+kubebuilder:scaffold:scheme
 }

-func main() {
+type managerOpts struct {


I really didn't like that all the options passed via args were just individual vars.

In use these vars did not indicate that they were input from the user at startup.

Now instead of cloudConfigFile we have opts.CloudConfigFile. This also helps to remove the opt vars from main, so for clarity alone, this is a win!

rejoshed · 2022-01-27T23:38:17Z

main.go

-	// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
-	// to ensure that exec-entrypoint and run can make use of them.
+	"github.com/aws/cluster-api-provider-cloudstack/pkg/cloud"
+	flag "github.com/spf13/pflag"


pflag is a 'drop in' replacement for the builtin flag that supports posix compatible flags.

So --metrics-addr instead of -metrics-addr.

rejoshed · 2022-01-27T23:39:15Z

main.go

+		"cloud-config-file",
+		"/config/cloud-config",
+		"Overrides the default path to the cloud-config file that contains the CloudStack credentials.")
+	flag.StringVar(


There are a few changes to input params as the upgrade desctructions required renaming and or adding and or change of default values.

Nothing to important though.

rejoshed · 2022-01-27T23:49:29Z

/retest

jweite-amazon · 2022-01-28T00:16:48Z

metadata.yaml

+    contract: v1alpha3
+  - major: 0


Should we still be including the v1alpha3 refs if we no longer support it?

I'm not really sure. I just made this match other providers in the way they set this.

Gotcha. They offer backward-compatible v1alpha3/4 APIs, though. If/since we're no longer offering those API versions, I'd rather we just make like they never existed. I figure this data is here to drive some sort of compatibility mechanism, which I'd rather not inadvertantly activate if we're not compatible to v1alpha3 anymore.

Okay, I'll just delete the v1alpha3 stuffs with early versionings.

to be clear, this commit itself won't serve as a version bump/release, right? We'll keep developing against it before we cut a final tag/release?

Yes, for sure. : )

jweite-amazon · 2022-01-28T00:21:35Z

The one surprise for me here was that we aren't offering v1alpha3 as an alternative supported API version anymore. I had imagined you were adding v1beta1 as a new hub, with conversion to/from v1alpha3.

I'm on the fence about whether it's needed / worth the effort, given how cross-version applications seem to be blocked by other CAPI-isms like clusterctl.

maxdrib · 2022-01-28T14:38:17Z

go.sum

 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/daviddengcn/go-colortext v0.0.0-20160507010035-511bcaf42ccd/go.mod h1:dv4zxwHi5C/8AeI+4gX4dCWOIvNi7I6JCSX0HvlKPgE=
+github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=


this library is bad according to dependabot. Let's keep the replace in the go.mod

maxdrib · 2022-01-28T14:40:24Z

metadata.yaml

+    contract: v1alpha3
+  - major: 0


to be clear, this commit itself won't serve as a version bump/release, right? We'll keep developing against it before we cut a final tag/release?

maxdrib · 2022-01-28T15:01:00Z

Can we confirm with the stakeholders that not supporting v1alpha3 is acceptable before we merge this change?

…rollers.

jweite-amazon · 2022-01-29T00:11:06Z

/lgtm

aws-paris-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2022

aws-paris-bot requested review from jweite-amazon and maxdrib January 26, 2022 01:23

aws-paris-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2022

maxdrib reviewed Jan 26, 2022

View reviewed changes

rejoshed commented Jan 27, 2022

View reviewed changes

config/dev/insecure_manager.yaml Show resolved Hide resolved

rejoshed commented Jan 27, 2022

View reviewed changes

rejoshed requested a review from maxdrib January 27, 2022 23:42

rejoshed marked this pull request as ready for review January 27, 2022 23:42

aws-paris-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2022

jweite-amazon reviewed Jan 28, 2022

View reviewed changes

maxdrib reviewed Jan 28, 2022

View reviewed changes

Bumped underlying CAPI tool versions per CAPI book upgrade instructions.

175298a

Joshua Reed added 17 commits January 28, 2022 08:22

Removed zap logger. Bumped CAPI to 1.0. Things are _very_ broken atm.

d5848f9

Followed instructions to unify manager role between webhooks and cont…

109bf27

…rollers.

Changes to enable individual service accounts for controllers.

87934dd

Renamed api dir to v1beta1

bab52b7

Bumped all refs from v1alpha3 to v1beta1.

afa78c7

Build, but probably doesn't actually work yet.

a98165f

Remove additional steps needed for older CAPC.

939b829

Remove zap logger.

d998c14

Generated via newer controller-gen binary.

f1c20ca

Updated to use new style watch functions.

52c8ffe

Fixup some defaults for args.

51fedfe

Point to local image.

5b181b6

Bumped version.

6f0cbf0

Some final cleanup.

628b58b

Bump v1beta1 in yaml.

5e994ac

Removed insecure package workaround as I believe it is no longer needed.

4f06083

Trying to pass prow.

2af5502

rejoshed force-pushed the feature/v1beta1 branch from bc4f5f3 to 2af5502 Compare January 28, 2022 16:22

Joshua Reed added 2 commits January 28, 2022 08:44

Revert to use secure jwt lib.

c7f8924

Remove unsupported versions.

f103954

aws-paris-bot assigned jweite-amazon Jan 29, 2022

aws-paris-bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2022

aws-paris-bot merged commit 48c5905 into main Jan 29, 2022

wongni deleted the feature/v1beta1 branch February 21, 2022 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/v1beta1 #20

Feature/v1beta1 #20

rejoshed commented Jan 26, 2022 •

edited

Loading

aws-paris-bot commented Jan 26, 2022

aws-paris-bot commented Jan 26, 2022

maxdrib left a comment

maxdrib Jan 26, 2022

rejoshed Jan 27, 2022

rejoshed commented Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022

rejoshed Jan 27, 2022

rejoshed commented Jan 27, 2022

jweite-amazon Jan 28, 2022

rejoshed Jan 28, 2022

jweite-amazon Jan 28, 2022 •

edited

Loading

rejoshed Jan 28, 2022

maxdrib Jan 28, 2022

rejoshed Jan 28, 2022

jweite-amazon commented Jan 28, 2022

maxdrib Jan 28, 2022

maxdrib Jan 28, 2022

maxdrib commented Jan 28, 2022 •

edited

Loading

jweite-amazon commented Jan 29, 2022

Feature/v1beta1 #20

Feature/v1beta1 #20

Conversation

rejoshed commented Jan 26, 2022 • edited Loading

aws-paris-bot commented Jan 26, 2022

aws-paris-bot commented Jan 26, 2022

maxdrib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejoshed commented Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejoshed Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejoshed Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejoshed Jan 27, 2022 • 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

rejoshed commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jweite-amazon Jan 28, 2022 • 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

jweite-amazon commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxdrib commented Jan 28, 2022 • edited Loading

jweite-amazon commented Jan 29, 2022

rejoshed commented Jan 26, 2022 •

edited

Loading

rejoshed commented Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022 •

edited

Loading

rejoshed Jan 27, 2022 •

edited

Loading

jweite-amazon Jan 28, 2022 •

edited

Loading

maxdrib commented Jan 28, 2022 •

edited

Loading