Direct-code APIKeys Key #1177
Direct-code APIKeys Key #1177
Conversation
|
cc @cheftako ... it's the "APIKeysKey: Create direct controller" commit that I think is interesting. I actually don't love the version of the mapping that is on this branch, I have another one I'm going to swap in, but wanted to give you a sneak peek! |
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
b64442b to
e11642a
Compare
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
e11642a to
ae7f29c
Compare
mockgcp/common/httpmux/json.go
Outdated
| @@ -0,0 +1,59 @@ | |||
| package httpmux | |||
There was a problem hiding this comment.
Good catch ... hopefully our tests will also yell at me about that!
mockgcp/common/operations/http.go
Outdated
| forwardResponseOptions := mux.GetForwardResponseOptions() | ||
|
|
||
| // GET /{prefix}/operations/{name} | ||
| if err := mux.HandlePath("GET", "/"+prefix+"/operations/{name}", func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) { |
There was a problem hiding this comment.
Any reason for string concatenation over sprintf?
There was a problem hiding this comment.
My personal style is to use concatenation when it is just concatenation of a reasonable number of strings (go doesn't have string auto-conversion, so it has to be strings). When there's a int -> string involved, then I find %d much more readable than strconv.Itoa etc
mockgcp/common/httpmux/json.go
Outdated
|
|
||
| func (r *protoResolver) FindMessageByURL(url string) (protoreflect.MessageType, error) { | ||
| if strings.HasPrefix(url, "type.googleapis.com/google.") { | ||
| s := "type.googleapis.com/mockgcp." + strings.TrimPrefix(url, "type.googleapis.com/google.") |
There was a problem hiding this comment.
Is the intent for swapping google. for mockgcp. (when we later swap it back) to ensure that we don't accidentally send to a live system during the test? I think it would be good to add a comment about how/why we temporarily make this change.
There was a problem hiding this comment.
I'll add a comment. This is a workaround for proto oddities, where you can't load two proto messages with the same proto name but different go packages. (Now I think about it, that's probably precisely to support Any, which is what we're dealing with here!)
|
|
||
| obj := &pb.TagKey{} | ||
| if err := s.storage.Get(ctx, fqn, obj); err != nil { | ||
| if apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
With #1221 I don't think we need this special handling here.
There was a problem hiding this comment.
Good call (and in fact it messes things up now, because we no longer return a 404!)
| @@ -0,0 +1,296 @@ | |||
| // Copyright 2022 Google LLC | |||
| }, nil | ||
| } | ||
|
|
||
| func ValueOf[T any](p *T) T { |
There was a problem hiding this comment.
Could this be moved to a library file rather then being kept in a specific controller?
There was a problem hiding this comment.
In theory yes... depending on how we feel about the . import of the mappings we could put it there...
There was a problem hiding this comment.
Moved to a utils.go file, I also think we can group services by package. So it might be repeated 100x, but not 1000x.
| return false, nil | ||
| } | ||
|
|
||
| req := &pb.GetKeyRequest{ |
There was a problem hiding this comment.
Would it make more sense to add things like keyID != "" check into the fullyQualifiedName() method and add an error return?
There was a problem hiding this comment.
My 2c: fullyQualifiedName isn't that complicated, and if we had to check errors we'd be tempted not to use it.
| func (a *adapter) Delete(ctx context.Context) error { | ||
| // TODO: Delete via status selfLink? | ||
| req := &pb.DeleteKeyRequest{ | ||
| Name: a.fullyQualifiedName(), |
There was a problem hiding this comment.
As per above I realize we would have to populate this outside but it would centralize/catch errors to do with the name,
There was a problem hiding this comment.
There is a call sequence that is part of the (evolving) contract, I'll document the contract as-is.
| } | ||
| op, err := a.gcp.DeleteKey(ctx, req) | ||
| if err != nil { | ||
| return fmt.Errorf("deleting key: %w", err) |
There was a problem hiding this comment.
Should we return an error if the object matching the key has already been deleted?
There was a problem hiding this comment.
Good call. I don't think we should return an error, but I do now return a bool to tell the difference between "I really deleted this" and "The object was already gone"
| return err | ||
| } | ||
|
|
||
| op, err := a.gcp.CreateKey(ctx, req) |
There was a problem hiding this comment.
Should we return an error if the key already exists?
There was a problem hiding this comment.
You mean like a special error? Maybe ... we don't have a test for this with the mocks (though we should!). We might have test coverage for this in the "older" tests, my preference would be to beef up our new tests as there are too many silent exceptions in the "old" tests.
There was a problem hiding this comment.
So this should actually be unlikely now, only happening if someone else is racing with us:
By contract we called Find immediately previously and did not find the object. If someone else concurrently created the object, I don't think we can do any better than to return an error from the reconciler, retry (with backoff), and then hopefully Find will find the object next time.
|
|
||
| updateMask := &fieldmaskpb.FieldMask{} | ||
|
|
||
| // TODO: I think we can do this with a helper |
There was a problem hiding this comment.
I do have a helper now in another PR (somewhere!)
| // } | ||
|
|
||
| if len(updateMask.Paths) == 0 { | ||
| // TODO: Log/warn/error? |
There was a problem hiding this comment.
I'd warn for now and we can adjust as we get more experience.
|
|
||
| _, err := a.gcp.UpdateKey(ctx, req) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Seems racy as there should have been a get before this but what if now the resource does not exist?
There was a problem hiding this comment.
I think we treat it like any other not-entirely-expected error, we return the error from the controller and allow it to re-reconcile.
| } | ||
|
|
||
| // HasHTTPCode returns true if the given error is an HTTP response with the given code. | ||
| func HasHTTPCode(err error, code int) bool { |
There was a problem hiding this comment.
This seems like another library function.
There was a problem hiding this comment.
Agreed - moved to a per-package utils.go file for now
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
3 times, most recently
from
bbecd6e to
9e5dc43
Compare
|
As this is now behind a featureflag, removing WIP :-) |
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
9e5dc43 to
1cb7002
Compare
| // UserProjectOverride provides the option to use the resource project for preconditions, quota, and billing, | ||
| // instead of the project the credentials belong to; false by default | ||
| UserProjectOverride bool | ||
|
|
||
| // BillingProject is the project used by the TF provider and DCL client to determine preconditions, | ||
| // quota, and billing if UserProjectOverride is set to true. If this field is empty, | ||
| // but UserProjectOverride is set to true, resource project will be used. | ||
| BillingProject string |
There was a problem hiding this comment.
I know this is not net new but could we get away with just specifying BillingProject to mean the project override? It doesn't seem like there's a mode where UserProjectOverride == false && BillingProject != "" (unless I am misunderstanding)
There was a problem hiding this comment.
Yes, that would make sense but let's not conflate it into this PR. Do you want to open a PR / issue so we don't lose the idea?
| return nil, fmt.Errorf("unable to determine resourceID") | ||
| } | ||
|
|
||
| location := "global" |
There was a problem hiding this comment.
will this need to be a configurable option down the road as part of the adapter?
There was a problem hiding this comment.
Not for this resource, it appears - apikeys are always global, though the location is in the URL. It's an odd one (which is likely why it wasn't implemented previously)
I'll add a comment!
| // var logger = log.Log.WithName(controllerName) | ||
|
|
||
| r := DirectReconciler{ |
There was a problem hiding this comment.
should we throw a named logger in with the DirectReconciler that can be used downstream?
There was a problem hiding this comment.
If we need a logger, we should use log := klog.FromContext(ctx) or similar. The "named logger" idea predates context, and the percolation of context through the logging ecosystem. I think controller-runtime already populates the context logger with the Kind & NamespacedName
| resource, err := toK8sResource(policy) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error converting to k8s resource while handling unresolvable dependencies event: %w", err) | ||
| } |
There was a problem hiding this comment.
this may be an existing pattern but all these handleX funcs do the same work here of calling toK8sResource; it may be worth pulling it one level higher ?
There was a problem hiding this comment.
So this is because directbase_controller is currently a fairly direct mapping of the terraform controller. In about 3 PRs we will switch it to stop doing this k8s.Resource dance, and just work with unstructured.Unstructured. I'll add a comment & TODO!
| // TODO: Do we want to normalize? | ||
| // return FieldID(strings.ToLower(id)) | ||
| return FieldID(id) |
There was a problem hiding this comment.
I updated the comment. It's more a question of whether we want to normalize this here :-)
So far, we haven't need to normalize, we use the json field as the identifier for the field, and our CRD and the API JSON matches exactly. We might get into more trouble with e.g. JSON fields that end in "ID"
There was a problem hiding this comment.
overall thoughts for the mappings package
- sanity check me but it makes sense to roll our own translation logic because there's not something readily available off the shelf right? at least I couldn't find a go library for this,
- if not now, when the shape settles down, could we add some unit tests for all the parsing?
There was a problem hiding this comment.
+1 on unit test coverage.
I think detailed comments will also be helpful on exported variables and types.
There was a problem hiding this comment.
sanity check me but it makes sense to roll our own translation logic because there's not something readily available off the shelf right? at least I couldn't find a go library for this,
Right, I couldn't find one. Also our use case is greatly simplified because we generated the CRD structure and field names to be 1:1 with the API (albeit indirectly). So we don't need a full library, and actually the validation suggestions are more important to us.
if not now, when the shape settles down, could we add some unit tests for all the parsing?
Yes! There is some unit-test level coverage in this PR (apikeyskey_test.go). We'll also get pretty strict coverage in our integration tests once we turn on http validation. I also propose we add tests whenever we find a bug, though personally I prefer higher-level integration tests because they tend to bitrot less (though it can be harder to construct an integration test!)
There was a problem hiding this comment.
I think detailed comments will also be helpful on exported variables and types.
Done!
There was a problem hiding this comment.
is there a cheap skeleton smoke test we can put in place for this reconciler?
There was a problem hiding this comment.
I think our best bet is to plug it into our integration tests. When running against mockgcp they are cheap. When running against mockkubeapiserver they are very cheap.
There was a problem hiding this comment.
Thank you for the implementation of the SDK-based resource, @justinsb ! I have some design related questions:
- Is
apikeyskey_controller.gofully hand-coded? Looks like it's a bit different from the design doc where automation/boilerplate template are mentioned to simplify the process. (Or did I misunderstand when automation will be used?) How much automation do you think we can introduce into the implementation? - Will contributors use the test failures to drive the implementation of the customized controller? Or do we expect contributors to fully understand both the KCC customized controller framework + the GCP resource API before they can write the customized controllers?
- There doesn't seem to be a new CRD generated, is it because there is already a APIKeysKey CRD? In the
apikeyskey_controller.go, it looks like not all the fields in the existing CRD are there, then do we plan to still use the same CRD to talk to either controllers (the customized one and the TF-based one)? For new CRDs, I guess we still need to add a different CRD generation logic for them, right? - Have you tried to run an integration test (CRUD+no change) locally for this resource? I assume we will reuse existing test cases for this resource once we do continuous testing?
In addition, it looks like there are still some missing pieces (in the adaptor functions, field mappings, etc) to make it work E2E, so I guess it is still a WIP feature? As it doesn't really touch much existing logic, have it merged should not be a big issue, though could you clean up the commented out code before you merge the code?
| // Fix our mockgcp hack | ||
| // Fix our mockgcp hack: | ||
| // The protobuf libraries get upset if we have two proto message types | ||
| // with the same proto path, but different go paths. |
There was a problem hiding this comment.
Just for self-education purpose:
- Do we own the two proto message types? Or just one of them?
- How is the proto path computed?
- Is it because we have a copy of the same proto message type in the protobuf lib to implement the mock and caused the potential conflict?
There was a problem hiding this comment.
Is it because we have a copy of the same proto message type in the protobuf lib to implement the mock and caused the potential conflict?
Right.
How is the proto path computed?
It's defined in the proto file.
Do we own the two proto message types? Or just one of them?
One is in the built-in proto library. One is the one we used to generate grpc-gateway. Now I'm wondering if we can use the existing types with grpc-gateway. But I'll look at that in a separate PR if that's OK, this isn't something we're introducing in this PR.
| UserAgent: gcp.KCCUserAgent, | ||
| } | ||
| dclOptions := clientconfig.Options{} | ||
| dclOptions.UserProjectOverride = config.UserProjectOverride |
There was a problem hiding this comment.
Just for self-education purpose: Why do you move the assignment of subfields in the struct outside of the initialization of the struct? It looks like that you've done the same for some of struct initializations but not all.
There was a problem hiding this comment.
Because it's now a nested field, so the struct initialization approach is now less elegant.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package apikeys |
There was a problem hiding this comment.
Should it be a generic util file for all the controllers (in the future), or do we plan to intentionally limit the scope of the code to be within the customized controller package only?
There was a problem hiding this comment.
TBD. We're probably going to refactor so we group reconcilers by service, so we would have 100 copies instead of 1000 copies. But that's still a lot.
The thing we're trying to avoid is a utils package until we understand what we actually need in it, otherwise the utils package inevitably becomes incoherent over time.
|
|
||
| func (a *adapter) buildCreateRequest(ctx context.Context) (*pb.CreateKeyRequest, error) { | ||
| // You can configure only the `display_name`, `restrictions`, and | ||
| // `annotations` fields. |
There was a problem hiding this comment.
Looks like annotations field is ignored in KeyMapping - does it not impact the ability to build create requests?
There was a problem hiding this comment.
This is what terraform gives us. Our next step is to go to infinity and beyond... or at least add some missing fields.
| config controller.Config | ||
| } | ||
|
|
||
| var keyMapping = NewMapping(&pb.Key{}, &krm.APIKeysKey{}, |
There was a problem hiding this comment.
Looks like we'll need to go through all the top-level fields and their subfields to configure the key mappings, right? Do we plan to automate the key mappings generation?
There was a problem hiding this comment.
Yes. Validation already helps. Duet is surprisingly helpful out of the box, sort of magically. But this is one area that is ripe for experimentation!
| "sigs.k8s.io/controller-runtime/pkg/source" | ||
| ) | ||
|
|
||
| // Add creates a new IAM Policy Controller and adds it to the Manager with default RBAC. The Manager will set fields on the Controller |
There was a problem hiding this comment.
I guess the comment needs to be updated as it is not a IAM Policy controller?
There was a problem hiding this comment.
Thanks, fixed. I copied the IAM Policy Controller as the starting point!
| return false, r.handleUpdateFailed(ctx, u, fmt.Errorf("error creating: %w", err)) | ||
| } | ||
| } else { | ||
| if _, err = adapter.Update(ctx); err != nil { |
There was a problem hiding this comment.
Looks like we don't do a diff for the entire spec but only check the fields Update() function cares about?
Not sure if it is applicable to this resource, but overall the immutability checks will be necessary for invalid changes.
| return false, r.handleUpdateFailed(ctx, u, fmt.Errorf("error updating: %w", err)) | ||
| } | ||
| } | ||
| if isAPIServerUpdateRequired(u) { |
There was a problem hiding this comment.
I kinda doubt this is still necessary as we always want to handle UpToDate (and you changed it to return true anyways).
There was a problem hiding this comment.
There's a TODO, I think we need it back! But we don't have failing tests yet.
| type structTypeMapping struct { | ||
| scope *Mapping | ||
|
|
||
| inType *reflectType |
There was a problem hiding this comment.
Could you add some comments for each field? It's a bit hard to understand how they should be used properly.
There was a problem hiding this comment.
Good point - done!
There was a problem hiding this comment.
+1 on unit test coverage.
I think detailed comments will also be helpful on exported variables and types.
Yes, it's hand coded, though Duet and validation are already steering the show. We can likely use @acpana 's linter work to make inline suggestions from validation. I'm pretty sure we can introduce a lot of automation, I'm not sure it's a bottleneck even today.
Yes, we should be very test driven. And yes, contributors should understand the GCP API and at least understand controllers. Once we have state-into-spec absent, I think our controllers are pretty normal controllers.
Right, I started with one where the CRD existed already. Future steps are (1) to add a field that isn't in terraform and (2)
Yes, and right. We were pretty close on the HTTP level. The biggest challenge is that terraform is very chatty (particularly extra reads of the GCP resource!), I wasn't sure if we should add some spurious API calls just to keep the golden http output closer. @gemmahou is driving our http request/response testing. |
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
ed86e42 to
f589efd
Compare
|
To see the changes across the rebase: |
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
f589efd to
9c84ab5
Compare
mockgcp/mockapikeys/service.go
Outdated
| if err := pb.RegisterApiKeysHandler(ctx, mux, conn); err != nil { | ||
| mux, err := httpmux.NewServeMux(ctx, conn, | ||
| pb.RegisterApiKeysHandler, | ||
| s.operations.RegisterOperationsHandler("v2"), |
There was a problem hiding this comment.
I assume this is v2 because https://cloud.google.com/api-keys/docs/reference/rest#rest-resource:-v2.keys is v2. If we plan on this being a template it might be good to document that.
There was a problem hiding this comment.
Yes, that's the path-prefix that the API implements in front of operation.name. I'll a comment ...
It will actually become a true template in a few PRs :-)
| op, err := a.gcp.DeleteKey(ctx, req) | ||
| if err != nil { | ||
| if IsNotFound(err) { | ||
| return false, nil |
There was a problem hiding this comment.
Is false actually the correct return value? If the goal of delete is for resource to not exist, then it being not found seems to indicate a success. Also if we return false on non existence then it would seems likely to trigger an endless series of retries...
There was a problem hiding this comment.
Added some comment-pointers to the interface, where it is documented as returning (true, nil) when we really deleted it, and (false, nil) when we found the object was not found.
We don't actually use that first value today, but I figured we might post different events and that I didn't want to have to go through and manually update each IsNotFound block etc. That said, we don't need it today...
| ) | ||
|
|
||
| func TestBuildCreate(t *testing.T) { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
Unless we believe that TestBuildCreate will add a context parameter this should probably be just context.Background()
There was a problem hiding this comment.
Replaced with ctx := test.WithContext(context.Background(), t)
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
9c84ab5 to
ff50835
Compare
| @@ -0,0 +1,394 @@ | |||
| // Copyright 2022 Google LLC | |||
There was a problem hiding this comment.
Do we know what the linter is complaining about here? Is is the year in the copyright? If so why not in any of the other new files? Does it dislike the redundancy of something prefixed with directbase in a directbase directory? I think this is then transitively causing other lint failures.
There was a problem hiding this comment.
I rebased and fixed up the lint issues. I think the core issue was that one of the intermediate changes renamed the name of one of the error values (NotFoundError -> ErrNotFound)
| func (d DeferredJSON) String() string { | ||
| b, err := json.Marshal(d.O) | ||
| if err != nil { | ||
| return fmt.Sprintf("<error: %v>", err) |
There was a problem hiding this comment.
A little worried that by deferring marshalling we have essentially hidden this error. Wondering if we should at least log this case?
There was a problem hiding this comment.
So this should be called as klog.Infof("foo: %v", debug.JSON(obj)). It will print the object as json. If json conversion fails, the error message is printed instead of the json object. So we do log it :-)
| ctx, cancel := context.WithTimeout(context.TODO(), timeoutPeriod) | ||
| defer cancel() | ||
| logger.Info("starting wait with timeout on resource's reference", "timeout", timeoutPeriod) | ||
| if err := watcher.WaitForResourceToBeReady(ctx, refNN, refGVK); err != nil { |
There was a problem hiding this comment.
Doing this under a weighted semaphore seems like it may cause (?exacerbate) ordering problems? Holding a semaphore, even a weighted semaphore and then blocking indefinitely just seems to be asking for problems. Are we sure this isn't a premature optimization? I've seen KAS and KCM ticking overly fairly happily with hundreds of thousands of goroutines. I'd like to be convinced we actually need to solve this issue. If we do, then I wonder if this is the best way to do it.
There was a problem hiding this comment.
So good news / bad news ... directbase_controller started with a copy of one of our existing go controllers (iampolicy_controller.go, IIRC). I don't think I ever looked at this code, it wasn't in the critical path for the direct controllers yet :-) I agree with you that we can do better, we actually have the logic to do so in our current experimental/composite directory (TrackingDynamicClient).
Do you mind filing a github issue so we don't lose this problem, but I don't think we want to solve it in this PR.
| // Like traditional assertions, assertions are runtime checks that | ||
| // will panic the program, but we only run those checks if assertion | ||
| // checks are enabled. | ||
| var AssertEnabled bool = true |
There was a problem hiding this comment.
Not convinced that bool is the correct type here. I'm betting we end up wanting an enumeration for things like testing only assertion and production assertions.
There was a problem hiding this comment.
Agreed, bool is just all I needed right now!
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
7421db7 to
674913a
Compare
We want some operations support
This enables us to merge the code without activating it yet, and test it against the old code. Current syntax is: `export KCC_USE_DIRECT_RECONCILERS=APIKeysKey,<kind2>` However I think we should consider this syntax provisional and subject to change.
This should help implementors choose the right prefix value.
justinsb
force-pushed
the
handcode_apikeys_apikey
branch
from
674913a to
76adc4d
Compare
| } | ||
| return schemaUpdater, nil | ||
| default: | ||
| klog.Warningf("requested direct reconciler for %v, but it is not supported", gvk.GroupKind()) |
There was a problem hiding this comment.
This at a minimum needs to be an Error. Worried that we could fall through to an alternate reconciler. Worried about that reconciler then doing something we don't want with the customers objects.
There was a problem hiding this comment.
Hmmm, good point, I'll take a look!
| case reflect.Int64: | ||
| return "int64" | ||
| case reflect.Map: | ||
| return "map[todo]todo" |
There was a problem hiding this comment.
Assuming this won't actually work. If true would it be better to fail here?
There was a problem hiding this comment.
We're outputing a proposed line to copy and paste into their go code. We could output "map[todo]todo /* TODO: replace with real types" :-) The problem with failing is that we don't give the caller anything to start from!
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako 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:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit ef47c38
into
GoogleCloudPlatform:master
Building on #1168, taking the APIKeys Key terraform resource and experimenting with working directly with the underlying APIs.