Migrate platform reference to Upbound official providers #22

ytsarev · 2022-09-16T00:07:05Z

Description of your changes

Migrate to official provider-gcp
Include CompositeNetwork XR within the Cluster Composition in a consistent manner similar to aws and azure platform-refs
Comply with new GKE authentication requirements and extend abstractions with googleApplicationCredentialsSecret

I have:

Read and followed Upbound's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

kubectl apply -f examples/cluster.yaml

k get managed
NAME                                                              CHART                   VERSION   SYNCED   READY   STATE      REVISION   DESCRIPTION        AGE
release.helm.crossplane.io/platform-ref-gcp-cluster-jvl4j-nb5fw   kube-prometheus-stack   34.5.1    True     True    deployed   1          Install complete   27m

NAME                                                                     READY   SYNCED   EXTERNAL-NAME                                                                                                                       AGE
projectiammember.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster   True    True     crossplane-playground/roles/container.admin/serviceAccount:platform-ref-gcp-cluster@crossplane-playground.iam.gserviceaccount.com   27m

NAME                                                                   READY   SYNCED   EXTERNAL-NAME              AGE
serviceaccount.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster   True    True     platform-ref-gcp-cluster   27m

NAME                                                                      READY   SYNCED   EXTERNAL-NAME                                                                                                                                                         AGE
serviceaccountkey.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster   True    True     projects/crossplane-playground/serviceAccounts/platform-ref-gcp-cluster@crossplane-playground.iam.gserviceaccount.com/keys/62cc39a87cca9bef38028f05bcd4614071461e12   27m

NAME                                                                     READY   SYNCED   EXTERNAL-NAME                          AGE
subnetwork.compute.gcp.upbound.io/platform-ref-gcp-cluster-jvl4j-mlr4l   True    True     platform-ref-gcp-cluster-jvl4j-mlr4l   27m

NAME                                                                  READY   SYNCED   EXTERNAL-NAME                          AGE
network.compute.gcp.upbound.io/platform-ref-gcp-cluster-jvl4j-vznfl   True    True     platform-ref-gcp-cluster-jvl4j-vznfl   27m

NAME                                                                     READY   SYNCED   EXTERNAL-NAME                          AGE
nodepool.container.gcp.upbound.io/platform-ref-gcp-cluster-jvl4j-nbnns   True    True     platform-ref-gcp-cluster-jvl4j-nbnns   27m

NAME                                                                    READY   SYNCED   EXTERNAL-NAME                          AGE
cluster.container.gcp.upbound.io/platform-ref-gcp-cluster-jvl4j-9j7g6   True    True     platform-ref-gcp-cluster-jvl4j-9j7g6   27m

k get composite
NAME                                                                  READY   COMPOSITION                      AGE
gke.gcp.platformref.upbound.io/platform-ref-gcp-cluster-jvl4j-5wvrp   True    gke.gcp.platformref.upbound.io   29m

NAME                                                                               READY   COMPOSITION                                        AGE
compositenetwork.gcp.platformref.upbound.io/platform-ref-gcp-cluster-jvl4j-w5q75   True    gcp.compositenetworks.gcp.platformref.upbound.io   29m

NAME                                                                         READY   COMPOSITION                                        AGE
compositecluster.gcp.platformref.upbound.io/platform-ref-gcp-cluster-jvl4j   True    gke.compositeclusters.gcp.platformref.upbound.io   29m

NAME                                                                       READY   COMPOSITION                           AGE
services.gcp.platformref.upbound.io/platform-ref-gcp-cluster-jvl4j-sjnk8   True    services.gcp.platformref.upbound.io   29m

examples/network.yaml

muvaf · 2022-09-21T09:01:56Z

examples/network.yaml

@@ -0,0 +1,8 @@
+apiVersion: gcp.platformref.upbound.io/v1alpha1


I'm torn between having this as an example given that Cluster already creates a network because I thought we needed to create both. Maybe have example of every definition or only Cluster would make it more clear?

following the strategy in ref-aws and ref-gcp, I renamed example file to network-xr.yaml and cluster-claim.yaml to reflect their kind

cluster/composition.yaml

cluster/gke/definition.yaml

muvaf · 2022-09-21T09:30:31Z

cluster/gke/definition.yaml

                required:
                - nodes
-                - networkRef
+                - googleApplicationCredentialsSecret


Suggested change

- googleApplicationCredentialsSecret

- googleApplicationCredentialsSecret

- compositeNetworkSelector

muvaf · 2022-09-21T10:55:09Z

cluster/gke/composition.yaml

@@ -1,39 +1,28 @@
 apiVersion: apiextensions.crossplane.io/v1
 kind: Composition
 metadata:
-  name: gke.gcp.platformref.crossplane.io
+  name: gke.gcp.platformref.upbound.io


I started with a bare ServiceAccount to be used in my providerconfig and it turns out that a ServiceAccount is necessary for Cluster to create the default node pool. If it's not given under spec.nodeConfig[0].serviceAccount, it falls back to using default in the Google project. I realized this because my fresh service account didn't have permissions to use default service account.

'apply failed: googleapi: Error 400: The user does not have access to service account "283222062215-compute@developer.gserviceaccount.com". Ask a project owner to grant you the iam.serviceAccountUser role on the service account.

I think having a ServiceAccount in Composition and referencing it from both Cluster and NodePool objects would be a better experience.

Composition extended, things to note:

I had to add ServiceAccount management role to the account that is used for GCP provider credentials

Currently I hit

Warning CannotUpdateManagedResource 3m8s (x111 over 8m36s) managed/container.gcp.upbound.io/v1beta1, kind=cluster Operation cannot be fulfilled on clusters.container.gcp.upbound.io "platform-ref-gcp-cluster-fd76b-xbn5d": the object has been modified; please apply your changes to the latest version and try again

Maybe it is due

nodeConfig: - serviceAccountSelector: matchControllerRef: true

and similar to https://github.com/upbound/official-providers/issues/466#issuecomment-1253444978 , though the selector is part of the spec not a patch there, so I do not yet properly understand the root cause

@muvaf should we propagate ServiceAccount ID through XR status here?

@ytsarev Yes, I'm afraid we need to patch through status. It's an array and core crossplane replaces the array item when you try to patch, hence it ends up having only matchControllerRef.

cluster/services/composition.yaml

ytsarev · 2022-09-27T11:58:18Z

Everything works now.

Caveat: ServiceAccount associated with provider-gcp credentials has to be quite powerful to manage ServiceAccounts, ServiceAccountKeys and create ProjectIAMMember

muvaf · 2022-09-27T12:10:40Z

crossplane.yaml

+    - provider: xpkg.upbound.io/upbound/provider-gcp
+      version: ">=v0.13.0-0"
+    - provider: xpkg.upbound.io/crossplane/provider-helm
+      version: ">=v0.10.0-0"


Suggested change

version: ">=v0.10.0-0"

version: ">=v0.11.1-0"

@muvaf , thanks, updated!

During the configuration testing, I got v0.12.rc build installed. Is that expected?

k get providers NAME INSTALLED HEALTHY PACKAGE AGE provider-aws True True xpkg.upbound.io/upbound/provider-aws:v0.14.0 5d15h provider-gcp True True xpkg.upbound.io/upbound/provider-gcp:v0.13.0 16h crossplane-provider-helm True True xpkg.upbound.io/crossplane/provider-helm:v0.12.0-rc.0.5.gb368192 30s

- provider: xpkg.upbound.io/upbound/provider-gcp version: ">=v0.13.0" - provider: xpkg.upbound.io/crossplane/provider-helm version: ">=v0.11.1"

@hasheddan let me know that they shouldn't have the suffix, especially the provider-helm one since it publishes RC builds as well to the original repo as opposed to the big three official providers.

ytsarev · 2022-09-27T13:11:11Z

@muvaf @stevendborrelli this PR is ready for the test, Configuration is also installable via examples/configuration.yaml

apiVersion: pkg.crossplane.io/v1
kind: Configuration
metadata:
  name: platform-ref-gcp-staging
spec:
  package: xpkg.upbound.io/upbound/platform-ref-gcp-staging:v0.1.2
  packagePullSecrets:
    - name: package-pull-secret

muvaf

Tested it a couple of times. Everything works as expected. For some reason, the cluster goes into repairing state first during deletion which makes it take longer to delete but that's likely about GCP API. Thanks a lot @ytsarev !!

muvaf · 2022-09-27T14:40:43Z

cluster/gke/composition.yaml

+        kind: ServiceAccount
+      patches:
+        - fromFieldPath: spec.id
+          toFieldPath: metadata.name


Suggested change

toFieldPath: metadata.name

toFieldPath: metadata.annotations[crossplane.io/external-name]

This is a better way of naming resources in general so that Crossplane can take care of metadata.name, making sure there is no conflict.

Ah, got it. I always had an opposite vector of name -> external-name in mind. That makes sense, will update

muvaf · 2022-09-27T14:41:29Z

cluster/gke/composition.yaml

+        - fromFieldPath: spec.id
+          toFieldPath: metadata.name


Suggested change

- fromFieldPath: spec.id

toFieldPath: metadata.name

Seems like keys don't have names. Not patching it at all would be enough here to prevent conflicts I think.

muvaf · 2022-09-27T14:43:05Z

examples/cluster-claim.yaml

@@ -14,8 +14,6 @@ spec:
    services:


I think using 1 as count would be faster to provision.

muvaf · 2022-09-27T14:43:37Z

examples/configuration.yaml

+metadata:
+  name: platform-ref-gcp-staging
+spec:
+  package: xpkg.upbound.io/upbound/platform-ref-gcp-staging:v0.1.2


Don't forget to replace with the real repo before merging.

donovanmuller · 2022-09-27T14:52:59Z

Tested it a couple of times. Everything works as expected. For some reason, the cluster goes into repairing state first during deletion which makes it take longer to delete but that's likely about GCP API. Thanks a lot @ytsarev !!

@muvaf I actually ran into something similar here as part of https://github.com/upbound/official-providers/pull/657, where if there are two node pools (default and additional NodePool) on creation, it causes this behaviour. See this related post.

muvaf · 2022-09-27T17:24:33Z

Test with latest state. The deletion succeeded, too.

NAME                                                              CHART                   VERSION   SYNCED   READY   STATE      REVISION   DESCRIPTION        AGE
release.helm.crossplane.io/platform-ref-gcp-cluster-4rdlm-mchl6   kube-prometheus-stack   34.5.1    True     True    deployed   1          Install complete   12m

NAME                                                                                 READY   SYNCED   EXTERNAL-NAME                                                                                                                       AGE
projectiammember.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-xfqb8   True    True     crossplane-playground/roles/container.admin/serviceAccount:platform-ref-gcp-cluster@crossplane-playground.iam.gserviceaccount.com   12m

NAME                                                                               READY   SYNCED   EXTERNAL-NAME              AGE
serviceaccount.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-mwz2d   True    True     platform-ref-gcp-cluster   12m

NAME                                                                                  READY   SYNCED   EXTERNAL-NAME                                                                                                                                                         AGE
serviceaccountkey.cloudplatform.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-5xb2p   True    True     projects/crossplane-playground/serviceAccounts/platform-ref-gcp-cluster@crossplane-playground.iam.gserviceaccount.com/keys/33ef64d7fd28e5c02fc3e609ae3d3ffedd96b0fe   12m

NAME                                                                  READY   SYNCED   EXTERNAL-NAME                          AGE
network.compute.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-7gcsm   True    True     platform-ref-gcp-cluster-4rdlm-7gcsm   12m

NAME                                                                     READY   SYNCED   EXTERNAL-NAME                          AGE
subnetwork.compute.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-btdwh   True    True     platform-ref-gcp-cluster-4rdlm-btdwh   12m

NAME                                                                     READY   SYNCED   EXTERNAL-NAME                          AGE
nodepool.container.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-2lj59   True    True     platform-ref-gcp-cluster-4rdlm-2lj59   12m

NAME                                                                    READY   SYNCED   EXTERNAL-NAME                          AGE
cluster.container.gcp.upbound.io/platform-ref-gcp-cluster-4rdlm-jcxbd   True    True     platform-ref-gcp-cluster-4rdlm-jcxbd   12m

jbw976

Thanks for another quick but thorough investment into our major reference platforms @ytsarev!! 💪

A few questions to explore, but mostly looks good - an approval isn't far away 😂

jbw976 · 2022-09-27T17:34:48Z

README.md


 Ensure that the following roles are added to your service account:

 * `roles/compute.networkAdmin`
 * `roles/container.admin`
 * `roles/iam.serviceAccountUser`
+* `roles/iam.securityAdmin`


what caused these extra roles/permissions to now be needed?

@jbw976 it is required to create ProjectIAMMember

I mentioned here #22 (comment)

we need all that machinery to enable automated access in the form of

identity: type: GoogleApplicationCredentials

for the helm ProviderConfig here

aha yes, thanks for clearing that up. So just to tie some loose ends together (for myself), this is the new approach for getting Auth to work with GKE clusters. This support was added to:

provider-helm in Support GKE OAuth Authorization crossplane-contrib/provider-helm#109

provider-kubernetes in Support GKE OAuth Authorization crossplane-contrib/provider-kubernetes#10

And that functionality generally address issues we've seen around this topic, like:

GKECluster kubeconfig relies on deprecated basic auth crossplane-contrib/provider-gcp#343

Generated KubeConfig file cannot perform any operations on the created cluster crossplane-contrib/provider-gcp#415

README.md

jbw976 · 2022-09-27T17:38:22Z

cluster/composition.yaml

+        - fromFieldPath: spec.parameters.googleApplicationCredentialsSecret.name
+        - fromFieldPath: spec.parameters.googleApplicationCredentialsSecret.namespace
+        - fromFieldPath: spec.parameters.googleApplicationCredentialsSecret.key


what do these fromFieldPath patches without a to* destination do? where does the from data go to? 🤔

@jbw976 without toFieldPath it is patching exactly the same destination as mentioned in fromFieldPath

But these patches are actually leftovers from the previous intermediate implementation so I remove them anyway, thanks for the catch!

jbw976 · 2022-09-27T17:43:03Z

cluster/network/definition.yaml

@@ -0,0 +1,25 @@
+apiVersion: apiextensions.crossplane.io/v1


this CompositeNetwork object is now consistently treated the same as the AWS and Azure platform refs?

We've historically had some problems here: When i first built reference platforms, I exposed the Networking as a claim, so app devs could create it on their own. That was a weird experience, because it's not something you'd expect app devs to have exposure to. Instead, the Networking should be created for them under the covers, alongside their cluster. Example issue: upbound/platform-ref-aws#25

We cleaned this up a bit in other reference platforms, but then I think GCP got updated in a different way, resulting in #21.

Would you say they are all consistent now, and #21 is fixed by this approach? 🙏

@jbw976 that's totally right, this PR makes network XR layout consistent with aws and azure refs:

Use Network XR from Cluster Composition

Do not provide Claim, just XR as example so it also appears in the Marketplace

jbw976

Thanks for the quick responses and updates @ytsarev! giving my +1 LGTM now, is there anything else you were waiting on to merge? 🤔

jbw976 · 2022-09-27T18:14:51Z

Do you want to squash the commit history a bit @ytsarev?

* Migrate all XRD and Compositions to OP * Update package meta and examples * Use compositeNetworkSelector with matchLabels propagation * Add ServiceAccount creation within GKE Composition * Add ServiceAccountKey for Helm ProviderConfig * Reliably propagate ServiceAccount to Cluster and NodePool through XR status * Create ProjectIAMMember association * Update prometheus operator version to currently available one * Strict versioning in meta config Signed-off-by: Yury Tsarev <yury@upbound.io>

ytsarev · 2022-09-27T18:36:08Z

@jbw976 I've squashed the commits into one leaving only useful messages from the commit log. I've just updated the Configuration example to point to the semantical v0.1.0 version as we agreed with @muvaf .

The PR is good to merge, thank you so much for the thorough review, folks!

ytsarev requested review from stevendborrelli and elohmrow September 16, 2022 00:07

pull-request-size bot added the size/L label Sep 16, 2022

muvaf reviewed Sep 21, 2022

View reviewed changes

ytsarev force-pushed the op-migration branch from 426a033 to fa617a6 Compare September 26, 2022 22:26

pull-request-size bot added size/XL and removed size/L labels Sep 27, 2022

muvaf reviewed Sep 27, 2022

View reviewed changes

ytsarev requested a review from muvaf September 27, 2022 12:55

muvaf approved these changes Sep 27, 2022

View reviewed changes

ytsarev removed request for stevendborrelli and elohmrow September 27, 2022 16:20

ytsarev force-pushed the op-migration branch from 0a9b629 to 9cf57cf Compare September 27, 2022 16:27

jbw976 reviewed Sep 27, 2022

View reviewed changes

jbw976 approved these changes Sep 27, 2022

View reviewed changes

ytsarev force-pushed the op-migration branch from 6c60bf0 to f5353db Compare September 27, 2022 18:32

jbw976 merged commit cf0514d into main Sep 27, 2022

jbw976 deleted the op-migration branch September 27, 2022 18:38

ytsarev mentioned this pull request Sep 28, 2022

App devs can't trigger creation of networking for their cluster anymore #21

Closed

ytsarev mentioned this pull request Oct 28, 2022

New GKE cluster ends up in repairing state #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate platform reference to Upbound official providers #22

Migrate platform reference to Upbound official providers #22

ytsarev commented Sep 16, 2022 •

edited

muvaf Sep 21, 2022

ytsarev Sep 26, 2022

muvaf Sep 21, 2022

muvaf Sep 21, 2022

ytsarev Sep 26, 2022

muvaf Sep 27, 2022

ytsarev commented Sep 27, 2022

muvaf Sep 27, 2022

ytsarev Sep 27, 2022

muvaf Sep 27, 2022

ytsarev commented Sep 27, 2022 •

edited

muvaf left a comment

muvaf Sep 27, 2022

ytsarev Sep 27, 2022

muvaf Sep 27, 2022

muvaf Sep 27, 2022

muvaf Sep 27, 2022

donovanmuller commented Sep 27, 2022 •

edited

muvaf commented Sep 27, 2022

jbw976 left a comment

jbw976 Sep 27, 2022

ytsarev Sep 27, 2022 •

edited

jbw976 Sep 27, 2022

jbw976 Sep 27, 2022

ytsarev Sep 27, 2022

jbw976 Sep 27, 2022

ytsarev Sep 27, 2022

jbw976 left a comment

jbw976 commented Sep 27, 2022

ytsarev commented Sep 27, 2022

		@@ -0,0 +1,8 @@
		apiVersion: gcp.platformref.upbound.io/v1alpha1

	- googleApplicationCredentialsSecret
	- googleApplicationCredentialsSecret
	- compositeNetworkSelector

	toFieldPath: metadata.name
	toFieldPath: metadata.annotations[crossplane.io/external-name]

Migrate platform reference to Upbound official providers #22

Migrate platform reference to Upbound official providers #22

Conversation

ytsarev commented Sep 16, 2022 • edited

Description of your changes

How has this code been tested

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

ytsarev commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev commented Sep 27, 2022 • edited

muvaf 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

Choose a reason for hiding this comment

donovanmuller commented Sep 27, 2022 • edited

muvaf commented Sep 27, 2022

jbw976 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev Sep 27, 2022 • edited

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

jbw976 left a comment

Choose a reason for hiding this comment

jbw976 commented Sep 27, 2022

ytsarev commented Sep 27, 2022

ytsarev commented Sep 16, 2022 •

edited

ytsarev commented Sep 27, 2022 •

edited

donovanmuller commented Sep 27, 2022 •

edited

ytsarev Sep 27, 2022 •

edited