Generate APIKeys Key from Terraform #1168

justinsb · 2024-01-30T21:25:00Z

Following the existing (legacy) procedure to generate an APIKeys Key resource from terraform.

Because it's coming from terraform and we intend to rework it, mark it as alpha quality until we have reworked it.

justinsb · 2024-01-31T13:40:54Z

This is just going through the "legacy" terraform-backed generation process at this point. Targeting v1alpha1 because it won't yet be the quality we want.

My plan is to then implement this directly using the API, to verify that it is worthwhile in terms of quality / efficient, that we can migrate existing TF resources etc etc.

I do wonder if I should put this CRD into a separate directory, just so that we can delay adoption for a little longer. It is alpha though.

maqiuyujoyce

Hi @justinsb , thank you for adding the resource!

Most of the comments are questions, though I want to make sure we are on the same page before we merge the change.

There are still some process gaps to support a v1alpha1 resource, so I think we might want to make v1alpha1 resources less noticeable by customers before graduating them to beta/GA.

maqiuyujoyce · 2024-02-01T23:01:06Z

config/servicemappings/apikeys.yaml

+      kind: APIKeysKey
+      metadataMapping:
+        name: name
+        #labels: labels


maqiuyujoyce · 2024-02-01T23:04:50Z

mockgcp/fixup-third-party.sh

 mv google/api/serviceusage/ mockgcp/api/

 cd mockgcp

 find . -type f -print0 | xargs -0 sed -i -e "s@google/iam/@mockgcp/iam/@g"
 find . -type f -print0 | xargs -0 sed -i -e "s@google\[email protected]@g"

+find . -type f -print0 | xargs -0 sed -i -e "s@google/api/apikeys/@mockgcp/api/apikeys/@g"


Just for self-education: Is it a required step when adding a mock API? I'm not sure if I've missed anything, but I didn't find them in the README.

This is just when we add a new API outside of the normal google/cloud group.

However, I'll update the README!

maqiuyujoyce · 2024-02-01T23:06:26Z

mockgcp/Makefile

@@ -26,6 +26,7 @@ gen-proto:
 		--grpc-gateway_opt paths=source_relative \
 		--experimental_allow_proto3_optional \
 		./apis/mockgcp/cloud/resourcemanager/v1/*.proto \
+		./third_party/googleapis/mockgcp/api/apikeys/v2/*.proto \


Nit: Maybe we can add a lint/a unit test to ensure the paths are ordered alphabetically?

We should aim to but I think it's tricky to enforce (unless you know of a suitable tool) and I don't think it changes anything at runtime.

maqiuyujoyce · 2024-02-01T23:11:17Z

config/servicemappings/apikeys.yaml

+  namespace: cnrm-system
+spec:
+  name: APIKeys
+  version: v1alpha1


We don't have a service level version set to v1alpha1 so there might be some rare feature gaps somewhere.

Meanwhile, we support all the other resources via autogen. So APIKeysKey configured here, I think there'll be two routes to promote a resource from alpha to beta. Feels like our code will get even more complicated.

Well we're going to switch this one to direct actuation pretty fast, it looks like! There are some significant gaps in the terraform implementation.

maqiuyujoyce · 2024-02-01T23:11:57Z

mockgcp/mockapikeys/apikeysv2.go

@@ -0,0 +1,191 @@
+// Copyright 2023 Google LLC


maqiuyujoyce · 2024-02-01T23:35:02Z

pkg/test/constants/presubmitconstants.go

@@ -32,6 +32,7 @@ var (
 		"accesscontextmanager": {"accesscontextmanagerserviceperimeter"},
 		"alloydb":              {"fullalloydbcluster"},
 		"apigee":               {"apigeeenvironment"},
+		"apikeys":              {"apikeyskeybasic"},


I think we may not want to add v1alpha1 resources into presubmit (and get distracted by its failures)?

So this is needed because of TestAllServicesInMap.

I do think we want to test it because we're actively developing it, and it's an important evaluation of a new approach. The API itself is relatively old I believe, so I don't think we'll see too many service flakes.

Sure, I'm okay with us keeping it as is. Though we may want to re-evaluate it if it adds more maintenance burden than expected.

maqiuyujoyce · 2024-02-01T23:38:54Z

pkg/test/resourcefixture/testdata/basic/apikeys/v1alpha1/apikeyskey/apikeyskeybasic/create.yaml

+apiVersion: apikeys.cnrm.cloud.google.com/v1alpha1
+kind: APIKeysKey
+metadata:
+  name: apikeyskeybasic-${uniqueId}


Should be name: aapikeyskey-${uniqueId}. The naming convention is [kindinlowercase]-${uniqueId}.

Not sure I agree with this convention (we know the kind from the kind, so we get more information from [test name]-$uniqueId), but ... fixed

Thank you for the change! And feel free to propose new naming conventions!

IIUC, our naming convention was designed like this so that it's

easy to follow (especially when there are dependencies of different kinds),

consistent (most of our existing testdata already follows this naming convention so it minimizes the confusion from external contributors and minimizes any cleanup work), and

trackable in debugging (because unique IDs will be different).

I'll gather more evidence before making a change. Once we split the output by test I think it will be a lot easier to understand each object, so I think I'll focus more on that.

maqiuyujoyce · 2024-02-01T23:39:13Z

pkg/test/resourcefixture/testdata/basic/apikeys/v1alpha1/apikeyskey/apikeyskeybasic/update.yaml

+apiVersion: apikeys.cnrm.cloud.google.com/v1alpha1
+kind: APIKeysKey
+metadata:
+  name: apikeyskeybasic-${uniqueId}


maqiuyujoyce · 2024-02-01T23:41:39Z

scripts/generate-google3-docs/resource-reference/templates/apikeys_apikeyskey.tmpl

@@ -0,0 +1,58 @@
+{{template "headercomment.tmpl" .}}


Mhmm, maybe remove the resource doc?
We didn't add any resource doc for any autogen / v1alpha1 resources because we believe additional information about no resource quality guaranteed, no backwards compatibility support, and etc should be provided on the page, but we never get the chance to prioritize working with PM to finalize the idea.

Removed the alpha docs. We can add them back in a separate PR, we should also make sure they are readable on github for those alpha resources - a good opportunity.

maqiuyujoyce · 2024-02-01T23:42:20Z

pkg/test/resourcefixture/sets.go

 		// Skip v1alpha1 CRDs when testing set cover as they may not yet be
 		// correctly supported.
+
+		// We do allowlist a few that we are hand-coding / checking.
+		// (Needed to get TestGetBasicTypeSetCover to pass)


Hmmm, can we make TestGetBasicTypeSetCover() check for v1beta1 services only?

Changed getResourceConfigIdSet to ignore all v1alpha1 resources.

maqiuyujoyce

LGTM! Comments are non-blockers.

maqiuyujoyce · 2024-02-02T07:02:44Z

pkg/test/constants/presubmitconstants.go

@@ -32,6 +32,7 @@ var (
 		"accesscontextmanager": {"accesscontextmanagerserviceperimeter"},
 		"alloydb":              {"fullalloydbcluster"},
 		"apigee":               {"apigeeenvironment"},
+		"apikeys":              {"apikeyskeybasic"},


Sure, I'm okay with us keeping it as is. Though we may want to re-evaluate it if it adds more maintenance burden than expected.

maqiuyujoyce · 2024-02-02T07:32:38Z

mockgcp/mockapikeys/apikeysv2.go

@@ -0,0 +1,191 @@
+// Copyright 2023 Google LLC


Thank you for the change! Not a blocker, just want to follow up a bit.

To clarify, I think the resource apikeysv2 can be a reasonable name for file: the generated pb.go file has the same name, and v2 is the version in the path, so I think automation can be possible here and it saves us a manual step.
Overall I don't have a strong preference whether it should be key.go or apikeysv2.go as long as the names are consistent under mockgcp services.

So far I've seen a few different naming conventions:

Version name only: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/mockgcp/mockcertificatemanager/v1.go

Partial resource name + version name: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/mockgcp/mockbilling/billingv1.go

Plural form resource name + version name: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/mockgcp/mockcompute/networksv1.go

Resource name only: https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/mockgcp/mockedgenetwork/network.go

Others?

Would be great if the names can be more consistent.

I'd prefer an automated name if possible, or having the version in the filename to avoid conflicts. E.g. Cloud Run API has RunService and will have two service.go.

maqiuyujoyce · 2024-02-02T07:33:16Z

mockgcp/mockapikeys/apikeysv2.go

+	// You can update only the `display_name`, `restrictions`, and `annotations` fields.
+
+	// See docs for UpdateMask
+	updateMask := req.GetUpdateMask()


Gotcha! I had the confusion (and still have a bit of it) because this seems to require the contributor to understand the logic of the API backend, and understand to what extent we need to implement the CRUD methods. Are there any detailed/actionable steps for contributors to follow when they implement the methods? Or are they just expected to do trial and error?

maqiuyujoyce · 2024-02-02T07:35:26Z

pkg/test/resourcefixture/testdata/basic/apikeys/v1alpha1/apikeyskey/apikeyskeybasic/create.yaml

+apiVersion: apikeys.cnrm.cloud.google.com/v1alpha1
+kind: APIKeysKey
+metadata:
+  name: apikeyskeybasic-${uniqueId}


Thank you for the change! And feel free to propose new naming conventions!

IIUC, our naming convention was designed like this so that it's

easy to follow (especially when there are dependencies of different kinds),

consistent (most of our existing testdata already follows this naming convention so it minimizes the confusion from external contributors and minimizes any cleanup work), and

trackable in debugging (because unique IDs will be different).

google-oss-prow · 2024-02-02T07:36:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, maqiuyujoyce

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~~ [justinsb,maqiuyujoyce]

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

This allows us to test our scaffolded API.

Add a basic resourcefixture and sample.

We don't want docs for alpha resources - yet.

…verage Our concern is the alpha/beta stability level, rather than how it came about.

Took a while to show up, but it did show up!

Generated clients and docs

If adding a new API not under google/cloud, we may need a few new sed lines.

justinsb · 2024-02-05T15:15:03Z

Rebased to pick up test fixes

maqiuyujoyce · 2024-02-05T19:50:57Z

/lgtm

google-oss-prow bot added the approved label Jan 30, 2024

justinsb force-pushed the generate_apikeys_apikey_from_terraform branch 2 times, most recently from 9989f96 to f2e9fbd Compare January 31, 2024 02:48

google-oss-prow bot assigned maqiuyujoyce Jan 31, 2024

justinsb force-pushed the generate_apikeys_apikey_from_terraform branch from 9e969f3 to ce8e4a5 Compare January 31, 2024 15:48

This was referenced Jan 31, 2024

Scaffold APIKeys Key resource #866

Open

Direct-code APIKeys Key #1177

Merged

maqiuyujoyce reviewed Feb 1, 2024

View reviewed changes

justinsb force-pushed the generate_apikeys_apikey_from_terraform branch from ce8e4a5 to 41aa4a5 Compare February 2, 2024 02:08

maqiuyujoyce approved these changes Feb 2, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Feb 2, 2024

justinsb added 11 commits February 5, 2024 10:14

APIKeysKey: Add servicemapping

3f1759b

autogen: make manifests

6abd8e3

Fixup resource generation docs

ebfd42b

Implement mock for apikeys

8ea5c8a

This allows us to test our scaffolded API.

APIKeysKey: test cases and samples

970ab9f

Add a basic resourcefixture and sample.

APIKeysKey: populate asset skeleton / uri skeleton

0aa7e9e

docgen: Skip generating alpha resource docs

181beca

We don't want docs for alpha resources - yet.

tests: ignore all alpha resources, not just autogenerated once for co…

685f709

…verage Our concern is the alpha/beta stability level, rather than how it came about.

APIKeysKey: now available in Cloud Asset Inventory

b4f41fe

Took a while to show up, but it did show up!

autogen: make ready-pr

4f00864

Generated clients and docs

mockgcp: update readme for renaming of new services

25ce9b5

If adding a new API not under google/cloud, we may need a few new sed lines.

justinsb force-pushed the generate_apikeys_apikey_from_terraform branch from 41aa4a5 to 25ce9b5 Compare February 5, 2024 15:14

google-oss-prow bot removed the lgtm label Feb 5, 2024

google-oss-prow bot added the lgtm label Feb 5, 2024

google-oss-prow bot merged commit 2391918 into GoogleCloudPlatform:master Feb 5, 2024
5 of 6 checks passed

This was referenced Feb 6, 2024

Fix MockService for mockapikeys #1194

Merged

mockgcp: Fix return type for mockapikeys #1202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate APIKeys Key from Terraform #1168

Generate APIKeys Key from Terraform #1168

justinsb commented Jan 30, 2024

justinsb commented Jan 31, 2024

maqiuyujoyce left a comment

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024 •

edited

Loading

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 2, 2024

justinsb Feb 5, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce Feb 1, 2024

justinsb Feb 2, 2024

maqiuyujoyce left a comment

maqiuyujoyce Feb 2, 2024

maqiuyujoyce Feb 2, 2024 •

edited

Loading

maqiuyujoyce Feb 2, 2024

maqiuyujoyce Feb 2, 2024

google-oss-prow bot commented Feb 2, 2024

justinsb commented Feb 5, 2024

maqiuyujoyce commented Feb 5, 2024

Generate APIKeys Key from Terraform #1168

Generate APIKeys Key from Terraform #1168

Conversation

justinsb commented Jan 30, 2024

justinsb commented Jan 31, 2024

maqiuyujoyce 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

justinsb Feb 2, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqiuyujoyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqiuyujoyce Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Feb 2, 2024

justinsb commented Feb 5, 2024

maqiuyujoyce commented Feb 5, 2024

justinsb Feb 2, 2024 •

edited

Loading

maqiuyujoyce Feb 2, 2024 •

edited

Loading