Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutator and Validator Webhooks for GatewayRoute #192

Merged
merged 2 commits into from May 12, 2020
Merged

Mutator and Validator Webhooks for GatewayRoute #192

merged 2 commits into from May 12, 2020

Conversation

karanvasnani
Copy link
Contributor

Issue #, if available:
aws-app-mesh-roadmap #37

Description of changes:
This PR is separated to two commits:

  1. "Mutating and Validating webhook for GatewayRoute"- Also adds a VirtualGateway MembershipDesignator using Namespace and Pod selector.
  2. "Mocks and auto-generated files" is auto-generated, files within /mocks are autogenerated by scripts/gen_mocks.sh

Testing

  • make docker-build && make docker-push
  • make deploy

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch

func (d *membershipDesignator) DesignateNamespace(ctx context.Context, obj metav1.Object) (*appmesh.VirtualGateway, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check whether the VirtualGateway and the the given Namespace belongs to the same mesh.

// DesignatePod will choose a VirtualGateway for given pod.
DesignatePod(ctx context.Context, pod *corev1.Pod) (*appmesh.VirtualGateway, error)
// DesignateNamespace will choose a VirtualGateway for given namespaced GatewayRoute CR.
DesignateNamespace(ctx context.Context, obj metav1.Object) (*appmesh.VirtualGateway, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd favor like below to be more specific.
DesignateForGatewayRoute(ctx context.Context, obj *appmesh.GatewayRoute)
DesignateForPod(ctx context.Context, pod *corev1.Pod) (*appmesh.VirtualGateway, error)

if err != nil {
return nil, err
}
if virtualGateway.Spec.MeshRef.UID != mesh.UID {
Copy link
Contributor

@M00nF1sh M00nF1sh May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use
(virtualGateway.Spec.MeshRef == nil || !mesh.isMeshReferenced(mesh, *virtualGateway.Spec.MeshRef) to check for both name & UID.
we should check for virtualGateway.Spec.MeshRef == nil, even it will only be nil when webhooks is disabled. (but the consequences is the controller will crash if it's nil)
however need to either rename the "mesh" variable to something like "ms", or rename the mesh package when import.

Copy link
Contributor

@M00nF1sh M00nF1sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm over all :D

Copy link
Contributor

@M00nF1sh M00nF1sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@M00nF1sh M00nF1sh merged commit b706286 into aws:v1beta2 May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants