Add support for ACM/File listener TLS, backend TLS client policies, backend defaults #130

bigdefect · 2020-02-18T00:09:21Z

Issue #, if available:

End to end encryption of traffic with customer provided certs aws-app-mesh-roadmap#38
End to end encryption of traffic with ACM managed certs aws-app-mesh-roadmap#39

Description of changes:

Note: Currently affected by #133

This adds support for ACM- and File-based listener TLS, backend TLS client policies, and backend defaults for TLS client policies. This is currently a basic pass-through of CRD fields to the SDK.

Update aws-go-sdk in go.mod to use the latest types from App Mesh
Update CRD schema in deploy/all.yaml
Update CRD structs in pkg/apis/appmesh/v1beta1/types.go
Update deepcopy functions using make code-gen
Update App Mesh client wrapper pkg/aws/appmesh.go
Update controller(s) under pkg/controller/

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

bcelenza

Thanks for getting this started!

pkg/apis/appmesh/v1beta1/types.go

pkg/aws/appmesh.go

nckturner

LGTM

pkg/aws/appmesh.go

bigdefect · 2020-03-05T03:16:32Z

Current state is that the implementation of the feature is complete. However as mentioned in the overview this is affected by #133. It can be reviewed as-is, though there may be changes depending on how we handle that issue.

I was able to test this with a spec like:

---
apiVersion: v1
kind: Namespace
metadata:
  labels:
    appmesh.k8s.aws/sidecarInjectorWebhook: enabled
  name: meshy
---
apiVersion: appmesh.k8s.aws/v1beta1
kind: Mesh
metadata:
  namespace: meshy
  name: tlsmesh
---
apiVersion: appmesh.k8s.aws/v1beta1
kind: VirtualNode
metadata:
  name: filetls
  namespace: meshy
spec:
  meshName: tlsmesh
  listeners:
    - portMapping:
        port: 8080
        protocol: http
      tls:
        mode: PERMISSIVE
        certificate:
          file:
            certificateChain: "/path/to/file.pem"
            privateKey: "/path/to/key.pem"
  backends:
    - virtualService:
        virtualServiceName: some.cool.svc
        clientPolicy:
          tls:
            # enforce: true
            ports: [8080, 9090]
            validation:
              trust:
                file:
                  certificateChain: "/path/to/chain.pem"
  backendDefaults:
    clientPolicy:
      tls:
        enforce: false
        ports: []
        validation:
          trust:
            file:
              certificateChain: "/path/to/chain.pem"
  serviceDiscovery:
    dns:
      hostName: front.local
---
apiVersion: appmesh.k8s.aws/v1beta1
kind: VirtualNode
metadata:
  name: acmtls
  namespace: meshy
spec:
  meshName: tlsmesh
  listeners:
    - portMapping:
        port: 8080
        protocol: http
      tls:
        mode: STRICT
        certificate:
          acm:
            certificateArn: "arn:aws:acm:us-east-2:326882241343:certificate/1c2fd791-fa5b-49ef-948d-7cd5c8f57228"
  backends:
    - virtualService:
        virtualServiceName: some.cool.svc
        clientPolicy:
          tls:
            validation:
              trust:
                acm:
                  certificateAuthorityArns:
                    - "arn:aws:acm-pca:us-east-2:326882241343:certificate-authority/351293b2-e08c-4985-a61b-fee6b287a942"
  backendDefaults:
    clientPolicy:
      tls:
        enforce: true
        ports: [1234, 5678]
        validation:
          trust:
            acm:
              certificateAuthorityArns:
                - "arn:aws:acm-pca:us-east-2:326882241343:certificate-authority/351293b2-e08c-4985-a61b-fee6b287a942"
  serviceDiscovery:
    dns:
      hostName: cool.local

kubectl get returns a match. And describing the nodes by the app mesh API returns matching results, except for some inconsistencies where the "default" value is returned by the API when it's not set in the request, e.g. true for enforce.

bigdefect · 2020-03-05T18:14:32Z

We will likely merge this despite #133 though if PR is approved please hold off on merging.

bigdefect · 2020-03-05T23:20:47Z

All right, I was able to apply a workaround using kiran's latest fix as a model. I tested a few combinations of changes but in each case the spec was getting resolved without hitting the update bug. This is ready to review and merge now.

bcelenza · 2020-03-06T01:37:54Z

Tested backend policies, backend defaults, and TLS with ACM on one of my test clusters. Verified everything was configured correctly down to the Envoys. LGTM.

M00nF1sh · 2020-03-06T23:22:30Z

pkg/apis/appmesh/v1beta1/types.go

 	Logging *Logging `json:"logging,omitempty"`
 }

 type Listener struct {
 	PortMapping PortMapping `json:"portMapping"`
 	// +optional
 	HealthCheck *HealthCheckPolicy `json:"healthCheck,omitempty"`
+	// +optional
+	Tls *ListenerTls `json:"tls,omitempty"`


nit: It's better to use TLS as field name to follow k8's convention Ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md
All letters in the acronym should have the same case, using the appropriate case for the situation
TLS in Ingress spec is using TLS

Ok. Are you suggesting the field name is TLS? Orr the yaml/json? Or both?

TLS in Ingress spec is using TLS

What field is this referring to?

Clarification: Ingress appears to actually have this as all lowercase: tls

https://kubernetes.io/docs/concepts/services-networking/ingress/#tls

@bcelenza @efe-selcuk FYI: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/networking/types.go#L264 🤣
we won't need to be stick to this convention, so it's just a nit comment

Haha, well consistently aligned with the formal definition I guess.

I think it makes sense to keep it lower case, because that's what we've done for dns: https://github.com/aws/aws-app-mesh-controller-for-k8s/blob/master/deploy/all.yaml#L156

@bcelenza the attribute in CRD(json tags) is always lower case. it's the field name in golang structure.

Ah, thanks for the clarification.

Ok, so just changing the field name in the struct to TLS, got it.

M00nF1sh · 2020-03-07T00:25:22Z

pkg/apis/appmesh/v1beta1/types.go

+}
+
+type TlsValidationContextAcmTrust struct {
+	CertificateAuthorityArns []*string `json:"certificateAuthorityArns"`


is there a reason for []*string? []string is better to me, normally we only use *string to represent optional semantic.

BTW, I know aws-sdk-go is a hell filled with pointers(and they are fixing it in aws-sdk-go-v2)

I don't have a good reason, I think I was just going of some existing fields. There was an earlier discussion where I settled on exactly the point you're making, just didn't think to apply it to the slices. Will do it to both this and the Ports array.

BTW, I know aws-sdk-go is a hell filled with pointers

And it seems they have conversion helpers for everything because of it.

M00nF1sh · 2020-03-07T00:27:46Z

pkg/apis/appmesh/v1beta1/types.go

+	// +optional
+	Enforce *bool `json:"enforce,omitempty"`
+	// +optional
+	Ports      []*int64             `json:"ports,omitempty"`


same here, []int64 will be better. (if it's not intended to null-check a pointer, don't use a pointer)

M00nF1sh · 2020-03-07T00:46:57Z

pkg/aws/appmesh.go

+// BackendDefaults converts from the SDK types into CRD types
+func (v *VirtualNode) BackendDefaults() *appmeshv1beta1.BackendDefaults {
+	if v.Data.Spec.BackendDefaults == nil {
+		return &appmeshv1beta1.BackendDefaults{}


nit, what's the semantic empty value for BackendDefaults? is it always &appmeshv1beta1.BackendDefaults{} or nil? currently it's fine since we will always do null-checks for BackendDefaults.ClientPolicy(since it's a pointer). But these unclearly defined empty value is typically source of bugs.

Good catch. This should be nil. It's an optional field in the API.

I see why I did it, the other helpers like Listeners() and Backends() define it as an empty object/slice. Possibly due to API semantics.

M00nF1sh · 2020-03-07T00:57:31Z

pkg/controller/virtualnode.go

-		}
-		currSet := target.BackendsSet()
-		if !desiredSet.Equal(currSet) {
+		if !reflect.DeepEqual(desired.Spec.Backends, target.Backends()) {


will the AppMesh api guarantee the backends is ordered using same order when it's created with? guess that's the original intention to use set compare

I suspect it is stable since it's a paginated API, but I don't think there's a guarantee. That makes sense. I don't recall why I removed this (maybe I thought it was redundant), but I'll add it back.

I think what confused me is that Listeners is not handled the same way, though that might be because we have a limit of 1 from the api right now.

M00nF1sh · 2020-03-07T01:08:25Z

pkg/controller/virtualnode.go

@@ -279,6 +274,17 @@ func vnodeNeedsUpdate(desired *appmeshv1beta1.VirtualNode, target *aws.VirtualNo
 		}
 	}

+	if desired.Spec.BackendDefaults != nil {


if we fix target.BackendDefaults() to return 'nil' when target.Data.Spec.BackendDefaults == nil, this can be simplified to just be !reflect.DeepEqual(desired.Spec.BackendDefaults, target.BackendDefaults())

M00nF1sh · 2020-03-07T01:12:15Z

pkg/controller/virtualnode.go

+}
+
+func mergeTlsClientPolicyDefaults(baseClientPolicyTls *appmeshv1beta1.ClientPolicyTls) {
+	if baseClientPolicyTls == nil {


redundant null check

I assume you're suggesting removing the null check at the call site, not here.

i.e. the call site becomes

if vnode.Spec.BackendDefaults != nil { mergeTlsClientPolicyDefaults(vnode.Spec.BackendDefaults.ClientPolicy) }

Or would it be more appropriate to pass in a value and return a new copy?

i suggest remove it from function side. it's more like io.read(buff) where buff is expected to be a valid buffer. (pass in value and return a new copy is good too :D)

bigdefect · 2020-03-09T19:49:56Z

Forgot to put the BackendsSet logic back in, and now a couple TLS tests are failing. Working on it.

…ackend defaults

bcelenza suggested changes Feb 19, 2020

View reviewed changes

bigdefect requested a review from nckturner February 21, 2020 00:27

nckturner reviewed Feb 26, 2020

View reviewed changes

pkg/aws/appmesh.go Show resolved Hide resolved

bigdefect changed the title ~~[Draft] add support for ACM/File listener TLS, backend TLS client policies, backend defaults~~ add support for ACM/File listener TLS, backend TLS client policies, backend defaults Feb 27, 2020

This was referenced Mar 2, 2020

End to end encryption of traffic with ACM managed certs aws/aws-app-mesh-roadmap#39

Closed

End to end encryption of traffic with customer provided certs aws/aws-app-mesh-roadmap#38

Closed

kiranmeduri changed the title ~~add support for ACM/File listener TLS, backend TLS client policies, backend defaults~~ [WIP] add support for ACM/File listener TLS, backend TLS client policies, backend defaults Mar 4, 2020

bigdefect changed the title ~~[WIP] add support for ACM/File listener TLS, backend TLS client policies, backend defaults~~ Add support for ACM/File listener TLS, backend TLS client policies, backend defaults Mar 5, 2020

bigdefect mentioned this pull request Mar 5, 2020

Controller can't reconcile between spec and API response when default values are set #133

Closed

bigdefect requested review from bcelenza and M00nF1sh March 5, 2020 23:20

bcelenza approved these changes Mar 6, 2020

View reviewed changes

bigdefect mentioned this pull request Mar 6, 2020

Add listener tls, client policies, and backend defaults to CRD aws/eks-charts#79

Merged

M00nF1sh reviewed Mar 6, 2020

View reviewed changes

M00nF1sh reviewed Mar 7, 2020

View reviewed changes

M00nF1sh approved these changes Mar 9, 2020

View reviewed changes

Add support for ACM/File listener TLS, backend TLS client policies, b…

fb5dd1e

…ackend defaults

bigdefect merged commit 8e97692 into aws:master Mar 9, 2020

bigdefect deleted the tls branch March 9, 2020 23:21

bigdefect mentioned this pull request Mar 9, 2020

Controller can't reconcile if multiple backends with same name exist #135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ACM/File listener TLS, backend TLS client policies, backend defaults #130

Add support for ACM/File listener TLS, backend TLS client policies, backend defaults #130

bigdefect commented Feb 18, 2020 •

edited

Loading

bcelenza left a comment

nckturner left a comment

bigdefect commented Mar 5, 2020 •

edited

Loading

bigdefect commented Mar 5, 2020

bigdefect commented Mar 5, 2020

bcelenza commented Mar 6, 2020

M00nF1sh Mar 6, 2020

bigdefect Mar 6, 2020

bcelenza Mar 6, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

bcelenza Mar 7, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

bcelenza Mar 7, 2020

bigdefect Mar 7, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

bigdefect Mar 7, 2020 •

edited

Loading

bigdefect Mar 8, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

bigdefect Mar 7, 2020

bigdefect Mar 8, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

bigdefect Mar 7, 2020

bigdefect Mar 7, 2020

M00nF1sh Mar 7, 2020

M00nF1sh Mar 7, 2020

bigdefect Mar 7, 2020

bigdefect Mar 7, 2020

M00nF1sh Mar 7, 2020 •

edited

Loading

bigdefect commented Mar 9, 2020

Add support for ACM/File listener TLS, backend TLS client policies, backend defaults #130

Add support for ACM/File listener TLS, backend TLS client policies, backend defaults #130

Conversation

bigdefect commented Feb 18, 2020 • edited Loading

bcelenza left a comment

Choose a reason for hiding this comment

nckturner left a comment

Choose a reason for hiding this comment

bigdefect commented Mar 5, 2020 • edited Loading

bigdefect commented Mar 5, 2020

bigdefect commented Mar 5, 2020

bcelenza commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

bigdefect Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

bigdefect commented Mar 9, 2020

bigdefect commented Feb 18, 2020 •

edited

Loading

bigdefect commented Mar 5, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

bigdefect Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading

M00nF1sh Mar 7, 2020 •

edited

Loading