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.

Sign up for GitHub

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

Controller gets stuck in a crash loop #314

Merged
merged 3 commits into from
Jul 16, 2020
+176 −4
Merged

Controller gets stuck in a crash loop #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b74eb62
CloudMap config of VirtualNode Spec is made immutable
achevuru Jul 15, 2020
bc86ee9
additional test case for virtualnode validating webhook
achevuru Jul 15, 2020
59f6075
Allow DNS->CloudMap modification
achevuru Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions controllers/appmesh/cloudmap_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ func (r *cloudMapReconciler) reconcileVirtualNodeWithCloudMap(ctx context.Contex

func (r *cloudMapReconciler) cleanupCloudMapResources(ctx context.Context, vNode *appmesh.VirtualNode) error {
if k8s.HasFinalizer(vNode, k8s.FinalizerAWSCloudMapResources) {
// TODO: choose one of the approach in https://github.com/aws/aws-app-mesh-controller-for-k8s/issues/305#issuecomment-654374069
// to completely fix the issue
if vNode.Spec.ServiceDiscovery != nil && vNode.Spec.ServiceDiscovery.AWSCloudMap != nil {
if err := r.cloudMapResourceManager.Cleanup(ctx, vNode); err != nil {
return err
Expand Down
3 changes: 3 additions & 0 deletions webhooks/appmesh/virtualnode_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ func (v *virtualNodeValidator) enforceFieldsImmutability(vn *appmesh.VirtualNode
if !reflect.DeepEqual(vn.Spec.MeshRef, oldVN.Spec.MeshRef) {
changedImmutableFields = append(changedImmutableFields, "spec.meshRef")
}
if oldVN.Spec.ServiceDiscovery.AWSCloudMap != nil && !reflect.DeepEqual(vn.Spec.ServiceDiscovery.AWSCloudMap, oldVN.Spec.ServiceDiscovery.AWSCloudMap) {
changedImmutableFields = append(changedImmutableFields, "spec.serviceDiscovery.awsCloudMap")
}
if len(changedImmutableFields) != 0 {
return errors.Errorf("%s update may not change these fields: %s", "VirtualNode", strings.Join(changedImmutableFields, ","))
}
Expand Down
175 changes: 173 additions & 2 deletions webhooks/appmesh/virtualnode_validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,94 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
Copy link

Choose a reason for hiding this comment

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

Can we please also add coverage on the following:

  • DNS<->CloudMap (should fail)
  • DNS<->DNS (should pass)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CloudMap->DNS is covered where it blocks if anything under awsCloudMap is changed as part of" VirtualNode field awsCloudMap changed". We will allow DNS->CloudMap however. Will add a test case for DNS->DNS.

Copy link

Choose a reason for hiding this comment

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

We will allow DNS->CloudMap however

I don't see how DNS->CloudMap is allowed here. Looking at the diff in virtualnode_validator.go, it'll block all: DNS->CloudMap, CloudMap->DNS and CloudMap->CloudMap(new)

Let's please also add a test for DNS->CloudMap as per the expected behavior

},
},
},
},
oldVN: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
},
wantErr: nil,
},
{
name: "VirtualNode DNS Servicediscovery change is allowed",
args: args{
vn: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
DNS: &appmesh.DNSServiceDiscovery{Hostname: "dns-new-hostname"},
},
},
},
oldVN: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
DNS: &appmesh.DNSServiceDiscovery{Hostname: "dns-hostname"},
},
},
},
},
wantErr: nil,
},
{
name: "VirtualNode Servicediscovery mode change from DNS to CloudMap is allowed",
args: args{
vn: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
oldVN: &appmesh.VirtualNode{
Expand All @@ -46,6 +134,9 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
DNS: &appmesh.DNSServiceDiscovery{Hostname: "dns-hostname"},
},
},
},
},
Expand All @@ -65,6 +156,12 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
oldVN: &appmesh.VirtualNode{
Expand All @@ -78,6 +175,12 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
},
Expand All @@ -97,6 +200,12 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "another-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
oldVN: &appmesh.VirtualNode{
Expand All @@ -110,13 +219,63 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
},
wantErr: errors.New("VirtualNode update may not change these fields: spec.meshRef"),
},
{
name: "VirtualNode fields awsName and meshRef changed",
name: "VirtualNode field awsCloudMap changed",
args: args{
vn: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "new-cloudmap-ns",
ServiceName: "new-cloudmap-svc",
},
},
},
},
oldVN: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "my-vn",
},
Spec: appmesh.VirtualNodeSpec{
AWSName: aws.String("my-vn_awesome-ns"),
MeshRef: &appmesh.MeshReference{
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
},
wantErr: errors.New("VirtualNode update may not change these fields: spec.serviceDiscovery.awsCloudMap"),
},
{
name: "VirtualNode fields awsName, meshRef and awsCloudMap changed",
args: args{
vn: &appmesh.VirtualNode{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -129,6 +288,12 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "another-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "new-cloudmap-ns",
ServiceName: "new-cloudmap-svc",
},
},
},
},
oldVN: &appmesh.VirtualNode{
Expand All @@ -142,10 +307,16 @@ func Test_virtualNodeValidator_enforceFieldsImmutability(t *testing.T) {
Name: "my-mesh",
UID: "408d3036-7dec-11ea-b156-0e30aabe1ca8",
},
ServiceDiscovery: &appmesh.ServiceDiscovery{
AWSCloudMap: &appmesh.AWSCloudMapServiceDiscovery{
NamespaceName: "cloudmap-ns",
ServiceName: "cloudmap-svc",
},
},
},
},
},
wantErr: errors.New("VirtualNode update may not change these fields: spec.awsName,spec.meshRef"),
wantErr: errors.New("VirtualNode update may not change these fields: spec.awsName,spec.meshRef,spec.serviceDiscovery.awsCloudMap"),
},
}
for _, tt := range tests {
Expand Down