Support to control/toggle SDS per VirtualNode #418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fawadkhaliq
suggested changes
Jan 13, 2021
|
|
||
| func isSDSDisabled(pod *corev1.Pod) bool { | ||
| if v, ok := pod.ObjectMeta.Annotations[AppMeshSDSAnnotation]; ok { | ||
| if v == "disabled" { |
There was a problem hiding this comment.
Let's log a warning when user configures an unsupported value like "enabled"
pkg/inject/utils.go
Outdated
| func mutateSDSMounts(pod *corev1.Pod, envoyContainer *corev1.Container, SdsUdsPath string) { | ||
| SDSVolumeType := corev1.HostPathSocket | ||
| volume := corev1.Volume{ | ||
| Name: "sds-socket-volume", |
There was a problem hiding this comment.
nit: prefix the name with "appmesh" so the name is not super generic and can be identified for debugging etc
There was a problem hiding this comment.
Sure, will change it.
Comment on lines
+110
to
+111
| envoyContainer.VolumeMounts = append(envoyContainer.VolumeMounts, volumeMount) | ||
| pod.Spec.Volumes = append(pod.Spec.Volumes, volume) |
There was a problem hiding this comment.
To make sure this is idempotent, if a volume is already there, we should not append it again
There was a problem hiding this comment.
Agree, for VirtualGateway scenario. We can do a name based match to make sure we don't already have an existing volume.
| @@ -128,6 +128,9 @@ func (m *virtualGatewayEnvoyConfig) buildTemplateVariables(pod *corev1.Pod) Virt | |||
| meshName := m.getAugmentedMeshName() | |||
| virtualGatewayName := aws.StringValue(m.vg.Spec.AWSName) | |||
| preview := m.getPreview(pod) | |||
| if m.mutatorConfig.enableSDS && isSDSDisabled(pod) { | |||
| m.mutatorConfig.enableSDS = false | |||
There was a problem hiding this comment.
This reads a bit off changing state of m.mutatorConfig.enableSDS for the entire package. i.e. it leaks the bounds of this specific function. It is better to use separate function local variable that is passed as a value of EnableSDS in VirtualGatewayEnvoyVariables
Same applies to the change in envoy.go
There was a problem hiding this comment.
This setting is per Pod (i.e.,) inject config is populated with defaults per pod and we want to toggle the SDS support for Pod level anyways.
kishorj
approved these changes
Jan 13, 2021
achevuru
added a commit
that referenced
this pull request
Jan 22, 2021
* [Preview] App Mesh mTLS support (#394) * App Mesh mTLS support * Additional UT cases * mTLS - SDS based e2e integration tests (#404) * Support to control/toggle SDS per VirtualNode (#418) * Support to control/toggle SDS per VirtualNode * Addressed CR comments * Add a log for unsupported annotation values * Revert CertManager API version changes until CI scripts are updated * Disable Preview Mode * Move master to Preview Mode * Set default envoy image to v1.16.1.0-prod
cgchinmay
pushed a commit
to cgchinmay/aws-app-mesh-controller-for-k8s
that referenced
this pull request
May 24, 2021
[Preview] App Mesh mTLS support (aws#394) * App Mesh mTLS support * Additional UT cases Support to control/toggle SDS per VirtualNode (aws#418) * Support to control/toggle SDS per VirtualNode * Addressed CR comments * Add a log for unsupported annotation values Updated VirtualGateway CRD to support gatewayRouteSelector GatewayRoute changes Added Test case for gateway_validator Update to gatewayroute test case Added matching based on path and query parameters in Route CRD Added path rewrite changes to Gateway Route CRDs Updated 1. gatewayroute_type and virtualrouter_type changes 2. CRD conversion Changes for gatewayroute and virtualrouter 3. Validator changes for gatewayroute Added Rewrite changes to GatewayRoute CRD Added Conversion code for CRD to SDK Added Validation logic for Rewrite Added Validation Tests Added Conversion Tests Fixed race condition test (aws#457) Fixed typos in comments (aws#461) Updated LiveDocs for VGW section (aws#454) Update docs for vgw (aws#467) * Updated LiveDocs for sidecar Injection in VGW section * Added a new section for VGW CRD in LiveDocs reference * Updated Docs to explain the association of VGW and GWRoutes * Updated Live Docs for VGW * minor change Co-authored-by: Chinmay Gadgil <cgadgil@amazon.com> Added a new GatewayRoute selector optional field to VirtualGateway (aws#464) * Added a new GatewayRoute selector optional field to VirtualGateway Updated membership_designator logic for GatewayRoute to VirtualGateway * Select only those virtualGateways which have matching gatewayRoute selector if specified * Updated Logic for GatewayRoute Selector * Updated logic in membership_designator and added test cases * Addressed PR comments * Updated error Message to include gatewayroute name if we cannot find any matching virtualgateways. * Updated CRD for VirtualGateway * Undo crd changes as these must be sent to a preview branch for 1.4.0 release * Added VGW CRD changes Co-authored-by: Chinmay Gadgil <cgadgil@amazon.com> Update jaegar tracing Envoy config to v3 API (aws#468) Added validation logic Updated Test files Changes tested with gamma endpoint Use camel casing for field names instead of underscores reverted go.mod changes Updated CRD's , fixed naming revert go.sum changes Changes to troubleshooting guide Changed HTTPathMatch to a pointer in HTTPRouteMatch GatewayRoute association readme changes Updated Validation logic for Header match filters Reverted header match empty validation of GatewayRoute
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
NA
Description of changes:
Ability to enable SDS based mTLS is currently a controller level flag. This PR, adds support to disable SDS @ VirtualNode level via an annotation on the corresponding deployment spec. AppMesh, currently bootstraps a static SDS cluster purely based on the presence of an environment variable (and not dynamically based on the presence of mTLS SDS config) and this change will help avoid it at the controller level.
If SDS is enabled via the controller flag(and SDS UDS path specified), controller will populate the "APPMESH_SDS_SOCKET_PATH" env variable and mount the UDS in to Envoy. To disable this @ VirtualNode level, one can use annotation "appmesh.k8s.aws/sds" set to disabled on the corresponding deployment spec.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.