Proposal: Support Resource Relationships

docs/design/proposals/references/README.md

@@ -0,0 +1,243 @@
+# [Design] Support Resource Relationships
+
+## Problem Description
+
+In micro service world, often users need to be using multiple services to run an application or Job. For example, to create a model in SageMaker, you could use a combination of SageMaker, IAM, S3, ECR and VPC services. Some of these resources need to be created in serial manner because output of one API(e.g. IAM role ARN, vpc-id) is an input to the another API. 
+
+As an ACK user, I want to enable to be able to express the relationship between resources in ACK CRs, so that I can submit multiple Specs related my application/infrastructure at once without waiting for another resources to get created.
+
+## Proposed Implementation
+Kubernetes style of expressing resource relationship is by using [object references](https://godoc.org/k8s.io/api/core/v1#ObjectReference), [typedLocalObjectReference](https://godoc.org/k8s.io/api/core/v1#TypedLocalObjectReference) or [localObjectReference](https://godoc.org/k8s.io/api/core/v1#LocalObjectReference). Some examples of this are [Secrets](https://github.com/kubernetes/api/blob/4e9f5db10201ce6270b160e96e3f99ddb580135e/core/v1/types.go#L861) and [configmaps](https://github.com/kubernetes/api/blob/4e9f5db10201ce6270b160e96e3f99ddb580135e/core/v1/types.go#L1999-L2016) in K8s API. In the following sections, we present how operator pattern can be used in conjunction with references in ACK to support relationsip between resources.
+
+### Modification to CRDs
+Currently, each top level resource in API model of the AWS service has a CRD. The field types in the CRD are structs, lists or primitive datatypes. A resource in K8s can be identified by a combination of metadata - `APIVersion, Kind, Name and Namespace`. Since AWS APIs have a deterministic input shape, APIVersion and Kind can be fixed at code generation time while Namespace and Name of resource will be a user input at runtime. To facilitate this, we propose to extend the K8s style of referencing resources in ACK by introducing a new datatype `AWSResourceReference` in `apis/core/v1alpha1/common.go`:
+
+```
+// AWSResourceReference represents a reference to another CR which refers to a AWS resource in Kubernetes 
+type AWSResourceReference struct {
+    // Namespace of the resource being referred to
+    Namespace string `json:"namespace"`
+    // Name of the resource being referred to
+    Name string `json:"name"`
+}
+```
+
+While we want to allow ACK users to benefit from references, there might be instances where some of the related resources are not created in the same cluster or do not have ACK service controller yet. The following two approaches show how we can account for this flexibility:
+
+Consider Endpoint resource with following input shape where IAMRole needs to be a referencce
+
+```
+type EndpointSpec {
+    EndpointName *string `json:"endpointName,omitempty"`,
+    IAMRole *string `json:"IAMRole,omitempty"`,
+    ...
+}
+```
+
+#### Approach 1
+Change the type of the referred field, `IAMRole` -> `IAMRoleRef`
+
+```
+type EndpointSpec {
+    EndpointName *string `json:"endpointName,omitempty"`,
+    IAMRoleRef *IAMRoleRef `json:"IAMRoleRef,omitempty"`,
+    ...
+}
+```
+
+In `apis/core/v1alpha1/common.go`
+```
+// AWSIdentifiers is used to identify the type of referred field
+type AWSIdentifiers struct {
+    // ARN is the AWS Resource Name for the resource. It is a globally
+    // unique identifier and generated by AWS service.
+    ARN *AWSResourceName `json:"arn"`
+    // Name is a user-supplied string identifier for the resource. It may
+    // or may not be globally unique, depending on the type of resource.
+    Name *string `json:"name"`
+    // ID is an identifier that has been generated by the AWS service for
+    // the resource. ID fields are *usually*, but not always, globally unique.
+    // Sometimes ID fields are numeric, other times they are strings.
+    ID *string `json:"id"`
+}
+```
+
+and then in, `services/<AWS_SERVICE>/apis/v1alpha1/types.go`, we will add:
+
+```
+// IAMRoleReference is a pointer to a IAMRole resource in the Kubernetes API
+// or the AWS API
+type IAMRoleReference struct {
+    // KubernetesReference indicates the Kubernetes internal identifiers for
+    // the IAMRole CR that references the AWS resource. 
+    // If nil, the IAMRole CR has not yet been created by an ACK controller
+    // controller and the `External` field below should be non-nil.
+    KubernetesReference *ackcorev1alpha1.AWSResourceReferencce `json:"kubernetesReference,omitempty"
+
+    // External indicates the AWS external identifiers for the IAMRole. 
+    // This field can be an Name, ARN or id depending on API input shape and 
+    // is used for resources which are not managed by ACK controller.
+    // If nil, the `KubernetesReferences` field must not be nil.
+    External *ackv1alpha1.AWSIdentifiers `json:"external,omitempty"`
+}
+```
+
+External field is beneficial for following reasons:
+
+* Service controller does not exist for this type of resource or is planned in future. This leaves a way for the Go types to be future proof.
+* User wants to use a resource which has been created by a different infrastructure team(using another Kubernetes cluster /CFN/Terraform/console).
+
+#### Approach 2
+Leave the default field for the external case and have an additional field to specify Kubernetes reference.
+
+```
+type EndpointSpec {
+    EndpointName *string `json:"endpointName,omitempty"`,
+    // Use to specify AWS external identifier
+    IAMRole *string `json:"IAMRole,omitempty"`,
+    // Use to specify KubernetesReference
+    IAMRoleRef *ackv1alpha1.AWSResourceReference `json:"IAMRoleRef,omitempty"`,
+    ...
+}
+```
+Either IAMRole or IAMRoleRef needs to be non-nil. If both are specified by mistake, controller will give preference to external i.e. IAMRole. 
+
+Note: The default IAMRole field will be modified to be optional if it is required in AWS API.
+
+### Modifications to Generator Config 
+
+Introduce a new field “references” to generator config under resources which contain instructions to the code-generator about related resources. References would be a map keyed by path to field name which should be converted to a reference and value being a referenceConfig which consists instructions about the value to be substituted. ReferenceConfig will consist of the following fields:
+
+* value: full path to the value from the referenced resource object to be substituted for parent resource field name.
+    * Full path is needed because value might be a generated field like ARN, id (like vpc-id) which will be present in the Status or can be names (like modelName) which are provided by user as part of spec
+* APIVersion and Kind:
+    * Kind is pre-determined based on the AWS API parameter. So we dont need this to be input by the user
+    * APIVersion needs to be fixed at code generation time so that controller is not affected by updates to another service controller. Kubernetes follows a [hub and spoke design](https://book.kubebuilder.io/multiversion-tutorial/conversion-concepts.html#what-does-that-have-to-do-with-webhooks) allows converting CRs from one version to other using webhooks.
+
+```
+# Initial draft, expect to be changed
+
+resources:
+  Endpoint:
+    references:
+      IAMRole:
+          # include Spec./Status. because it depends if
+          # it is generated param e.g. ARN/Id (generally part of Status) or
+          # provided by user e.g. Name (generally part of Spec)
+        value: Status.ackResourceMetadata.arn
+        APIVersion: iam.services.k8s.aws/v1alpha1
+        Kind: IAMRole
+ignore:
+    resource_names:
+      - Algorithm
+      - App
+      - AutoMLJob
+```
+
+### Controller Implemention
+
+A sharedInformer cache will be added to the service controller for each type(APIVersion, Kind) of resource a service can depend on. This cache will use an event listener to store and update the CR objects according to the operations(add/update/delete).
+
+ResourceManagers in controller will have a pointer to the cache which will be used to read the referred resource. This cache will be read in normal reconciliation loop of the referencing/parent resource. If the referred object exists in cache, it will be used to substitute the value in parent resource else the parent resource will requeueAfter interval.
+* Example: X refers to Y. The service controller for X will have a shared informer cache for Y type of CRs. Reconcile loop in X will look if Y exists in cache.
+  * If Y does not exist - X will RequeueAfter a fixed time interval
+  * If Y exists in cache - X will fetch the value to be substituted and proceed with reconciliation
+
+Currently ACK controllers do not requeue after the resource creation is successful, which means if the underlaying resource (Y) is deleted or updated those events will not get trigger any update in X. These updates will only get picked if X reconciles or user explicitly resubmits X
+ * Hence, we propose to add either a controller level or resource level flag in generator config which will add a periodic requeue even after resource creation is successful
+
+#### Advantages:
+
+* Efficient since the sharedInformer will use a push model to maintain the cache compared to pull model where each resource manager querys the API server for the resource
+* Allows submitting multiple CRs simultaneously without waiting for ARN or id to pre-exist. Name is pre-determined user input
+* In addition to supporting references, periodic requeue after resource creation can be used for
+  * Drift detection
+  * Detect destructive operations [#82](https://github.com/aws/aws-controllers-k8s/issues/82)
+
+#### Limitation (to be solved):
+
+Resource exists in cache/etcd not a sufficient condition if X refers to a field in Y.Spec. E.g.
+  * Endpoint resource (X) refers to Spec.ModelName from resource (Y). User submitted X & Y to cluster. Y.Spec has modelName when the CR is submitted but resource does not get created in AWS because of error. Since X is only looking for is Y to exist in shared informer cache, X will run create API by looking at Y.Spec.ModelName & will go into an exponential back off instead of periodic requeue
+  * If X had a dependency on Y's ARN/id (returned by AWS) then it would have requeueAfter() until Y.Status.*.ARN != nil
+
+Although, we do not want to complicate things by mixing the state of the referred-to resource with the state of the referencing resource, if the reference is created from a spec field, it needs to be treated like ARN/id. Is there a better way to solve this ?
+
+#### Exploration
+Taking deployment in K8s as an example to see behaviour
+
+* ConfigMap in Deployment:
+    * This is a referenced resource. [Updating configmap does not update the deployment/restart the pods](https://github.com/kubernetes/kubernetes/issues/22368). Doesnt look like there will be a fix as well (issue opened 4 years ago). There are [custom controllers](https://github.com/stakater/Reloader) to deal with this issue.
+
+* ReplicaSet in Deployment:
+    * This is not a referenced resource but controller watches its resources. If you delete a pod from the replicaset, it gets recreated. This cannot happen unless controller is continuously watching the pods.
+    * ```
+      k create deploy -—image nginx nginx
+      k scale deploy nginx -—replicas=3
+      k get pods
+      k delete pod <>
+      ```
+
+## In Scope
+* Ability to change field type from API specification to object reference. This includes top level fields in CRD as well as nested fields
+* Declarative syntax for generator Config to make this approach scalable and consistent across services
+* Best practice guide which lists when this should be used
+* Unit tests for code generator and integration tests for a specific controller (e.g. SageMaker)
+
+### Sample Specs after implementation:
+
+Using Kubernetes Reference:
+```
+---
+apiVersion: sagemaker.services.k8s.aws/v1alpha1
+kind: EndpointConfig
+metadata:
+  name: xgboost-endpoint-config
+  namespace: xyz-namespace
+spec:
+  endpointConfigName: def-endpoint-config
+  productionVariants:
+    - variantName: AllTraffic
+      modelName: trained-model
+      initialInstanceCount: 1
+      instanceType: ml.r5.large
+      initialVariantWeight: 1
+---
+apiVersion: sagemaker.services.k8s.aws/v1alpha1
+kind: Endpoint
+metadata:
+  name: xgboost-endpoint
+spec:
+  endpointName: my-endpoint
+  endpointConfigNameRef:
+    name: xgboost-endpoint-config
+    namespace: xyz-namespace
+```
+
+Using external:
+```
+apiVersion: sagemaker.services.k8s.aws/v1alpha1
+kind: Endpoint
+metadata:
+  name: xgboost-endpoint
+spec:
+  endpointName: xgboost-endpoint
+  endpointConfigNameRef:
+    external: abcd-endpoint-config
+```
+
+## Out of Scope
+* Upgrading any service controller to use references
+* E2E tests for every service controller
+
+## Test Plan
+TBD
+
+## Discussion
+
+Within an API, there are multiple AWS resources which are from same and across services. e.g. Model resource in SageMaker has modelPackageName from SageMaker and executionRoleARN, subnetId and securityGroupId from IAM, VPC and EC2 services respectively. Which of these should have Kubernetes reference option?
+
+Here are some options:
+
+  1. **Option A**: Produce a best practice guide which suggests(does not mandate) every AWS resource should be a reference even if a service controller is not available since the external field leaves way to enable references in future.
+  1. **Option B (Proposed)**: In addition to a, let service owners choose the resources they want to convert to reference 
+  1. **Option C**: For consistency, requirement for all AWS resources to be converted to references going forward. This can be a breaking change