Add VpcAssociationPolicy CRD #396
Conversation
|
Shall we use a different name, e.g. GatewayAWSPolicy? And we can add other parameters to them, e.g.
|
I think we decide to do them in 2 CRDs,
For the name of VpcAssociationPolicy we could discuss, GatewayAWSPolicy can be a option |
| // +kubebuilder:validation:MinItems=1 | ||
| SecurityGroupIds []SecurityGroupId `json:"securityGroupIds,omitempty"` | ||
|
|
||
| // AssociateWithVpc indicates whether the VpcServiceNetworkAssociation should be created for current the VPC of EKS cluster. |
There was a problem hiding this comment.
typo: current the -> the current
I would avoid saying "EKS", I think this product is supposed to work for non-eks k8s clusters
There was a problem hiding this comment.
Ok.. but k8s cluster in other cloud provider don't have vpc lattice?
There was a problem hiding this comment.
could be plain, un-managed k8s cluster installed on ec2 instance
|
|
||
| type Policy interface { | ||
| GetNamespacedName() types.NamespacedName | ||
| GetTargetRef() *v1alpha2.PolicyTargetReference |
There was a problem hiding this comment.
I'm okay with having this interface but not sure how we could benefit from it
There was a problem hiding this comment.
Actually my later code change will use that:
func (r *resourceMapper) TargetGroupPolicyToService(ctx context.Context, tgp *v1alpha1.TargetGroupPolicy) *corev1.Service {
if obj := r.policyToTargetRefObj(ctx, tgp, &corev1.Service{}); obj != nil {
return obj.(*corev1.Service)
} else {
return nil
}
}
func (r *resourceMapper) VpcAssociationPolicyToGateway(ctx context.Context, vap *v1alpha1.VpcAssociationPolicy) *gateway_api.Gateway {
if obj := r.policyToTargetRefObj(ctx, vap, &gateway_api.Gateway{}); obj != nil {
return obj.(*gateway_api.Gateway)
} else {
return nil
}
}
func (r *resourceMapper) policyToTargetRefObj(ctx context.Context, policy core.Policy, retObj client.Object) client.Object { }
| // Either one of them set to true or both of them undefined will result in the VpcServiceNetworkAssociation created. | ||
| // +optional | ||
| // +kubebuilder:default=true | ||
| AssociateWithVpc *bool `json:"associateWithVpc,omitempty"` |
There was a problem hiding this comment.
looking at this I'm thinking it is quite not straightforward to have optional value true by default. Could we have an inverse of this option?
There was a problem hiding this comment.
I can remove the // +kubebuilder:default=true here
I will do that in the controller code logic: Either one of them set to true or both of them undefined will result in the VpcServiceNetworkAssociation created.
| type VpcAssociationPolicySpec struct { | ||
|
|
||
| // SecurityGroupIds defines the security groups enforced on the VpcServiceNetworkAssociation. | ||
| // For more details, please check the VPC Lattice documentation https://docs.aws.amazon.com/vpc-lattice/latest/ug/security-groups.html |
There was a problem hiding this comment.
Worth mentioning that it does not take any effect when it is not associated with vpc
zijun726911
force-pushed
the
vap-crd
branch
from
6dec234 to
d47344b
Compare
Pull Request Test Coverage Report for Build 6191268261
💛 - Coveralls |
4 participants
Which issue does this PR fix: Add VpcAssociationPolicy CRD to support SNVA security Group API
What does this PR do / Why do we need it: Add VpcAssociationPolicy CRD to support SNVA security Group API
If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
Testing done on this change:
Installed this CRD in my own cluster succeeded by kubectl apply -f config/crds/bases/application-networking.k8s.aws_vpcassociationpolicies.yaml
Automation added to e2e:
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.