feat: Add Apigee instance controller #3554
Conversation
jasonvigil
force-pushed
the
apigee-instance-controller
branch
2 times, most recently
from
774c8a3 to
a4eed02
Compare
jasonvigil
changed the title
| return mapCtx.Err() | ||
| } | ||
|
|
||
| if resource.AccessLoggingConfig != nil && !reflect.DeepEqual(resource.AccessLoggingConfig, a.actual.AccessLoggingConfig) { |
There was a problem hiding this comment.
Can we use this function to sort and the compare? https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/controller/direct/apigee/envgroup_controller.go#L278
| op, err := a.instancesClient.Delete(a.id.String()).Context(ctx).Do() | ||
| if err != nil { | ||
| if direct.IsNotFound(err) { | ||
| // Return success if not found (assume it was already deleted) |
There was a problem hiding this comment.
Is this a new idea / pattern to add a log here? Should we update the template / existing controllers for this?
| return true, nil | ||
| } | ||
|
|
||
| func (a *ApigeeInstanceAdapter) waitForOp(ctx context.Context, op *api.GoogleLongrunningOperation) error { |
There was a problem hiding this comment.
This can be an individual function. And suggest moving it to the controller/direct/common lib for sharing purpose.
| out.AccessLoggingConfig = AccessLoggingConfig_ToAPI(mapCtx, in.AccessLoggingConfig) | ||
| out.ConsumerAcceptList = in.ConsumerAcceptList | ||
| out.Description = direct.ValueOf(in.Description) | ||
| // MISSING: DiskEncryptionKeyName |
There was a problem hiding this comment.
Just to double check, do you have a NOTYET comment for this field?
There was a problem hiding this comment.
The mapper for this needs to be implemented. Will fix.
| --api-version apigee.cnrm.cloud.google.com/v1alpha1 \ | ||
| --kind ApigeeInstance \ | ||
| --proto-resource GoogleCloudApigeeV1Instance | ||
|
|
There was a problem hiding this comment.
We don't need to update this file. It is just an example (or is it a requirement now?)
There was a problem hiding this comment.
Yeah let's not add generate-controller command.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
| } | ||
|
|
||
| func (a *ApigeeInstanceAdapter) waitForOp(ctx context.Context, op *api.GoogleLongrunningOperation) error { | ||
| for { |
There was a problem hiding this comment.
We can probably create a helper function which
- respect the ctx timeout
- take a input timeout
Something like this (AI generated)
func waitUntil(ctx context.Context, timeout time.Duration, condition func() bool) error {
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
ticker := time.NewTicker(100 * time.Millisecond)
defer ticker.Stop()
for {
select {
case <-ctx.Done():
if err := ctx.Err(); err == context.DeadlineExceeded {
return fmt.Errorf("timeout exceeded")
}
return ctx.Err()
case <-ticker.C:
if condition() {
return nil
}
}
}
}
The pulling interval could also be customized via input
There was a problem hiding this comment.
The waitForOp respects the context also, no? In the operationsClient.Get(...).Context(ctx).Do(). It may wait for up to an additional 2 seconds, but the function will return (with an error) when the context is cancelled.
There was a problem hiding this comment.
Yeah, the current implementation of waitForOp does respect the context's timeout and cancellation. If we were to create a helper function that accepts an arbitrary input function (not just "a.operationsClient.Get"), that function may or may not respect the context. Not saying we need to create one—just an idea.
There was a problem hiding this comment.
I ended up going with a variant of this idea. Thanks for the recommendation.
| out.PeeringCIDRRange = direct.LazyPtr(in.PeeringCidrRange) | ||
| return out | ||
| } | ||
| func ApigeeInstanceSpec_ToAPI(mapCtx *direct.MapContext, in *krm.ApigeeInstanceSpec) *pb.GoogleCloudApigeeV1Instance { |
There was a problem hiding this comment.
where is the mapping function for diskEncryptionKMSCryptoKeyRef?
There was a problem hiding this comment.
Ah, not implemented yet. Will add it.
There was a problem hiding this comment.
I added this mapper, good catch
jasonvigil
force-pushed
the
apigee-instance-controller
branch
from
a4eed02 to
7dc778b
Compare
google-oss-prow
bot
merged commit 2456040
into
GoogleCloudPlatform:master
This PR adds the mappers, controller, and records GCP interaction for ApigeeInstance. MockGCP tests will come in a separate PR.