Skip to content

Commit

Permalink
Fix logic on apigateway that ignores current annotations on services (#…
Browse files Browse the repository at this point in the history 
…3597)
  • Loading branch information
@missylbytes
missylbytes committed Feb 14, 2024
1 parent 9b3d54b commit fb44cbc
Show file tree
Hide file tree
Showing 3 changed files with 239 additions and 37 deletions.
3 changes: 3 additions & 0 deletions .changelog/3597.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
api-gateway: fix issue where external annotations and labels are being incorrectly deleted on services controlled by the API Gateway
```
209 changes: 193 additions & 16 deletions control-plane/api-gateway/gatekeeper/gatekeeper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,28 @@ var (
dataplaneImage = "hashicorp/consul-dataplane"
name = "test"
namespace = "default"
labels = map[string]string{

labels = map[string]string{
"component": "api-gateway",
"gateway.consul.hashicorp.com/name": name,
"gateway.consul.hashicorp.com/namespace": namespace,
createdAtLabelKey: createdAtLabelValue,
"gateway.consul.hashicorp.com/managed": "true",
}

// These annotations are used for testing that annotations stay on the service after reconcile.
copyAnnotationKey = "copy-this-annotation"
copyAnnotations = map[string]string{
copyAnnotationKey: "copy-this-annotation-value",
}
externalAnnotations = map[string]string{
"external-annotation": "external-annotation-value",
}
externalAndCopyAnnotations = map[string]string{
"external-annotation": "external-annotation-value",
copyAnnotationKey: "copy-this-annotation-value",
}

listeners = []gwv1beta1.Listener{
{
Name: "Listener 1",
Expand Down Expand Up @@ -70,6 +85,10 @@ type testCase struct {

initialResources resources
finalResources resources

// This is used to ignore the timestamp on the service when comparing the final resources
// This is useful for testing an update on a service
ignoreTimestampOnService bool
}

type resources struct {
Expand Down Expand Up @@ -190,7 +209,7 @@ func TestUpsert(t *testing.T) {
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
}, "1"),
}, "1", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{},
},
Expand Down Expand Up @@ -242,7 +261,7 @@ func TestUpsert(t *testing.T) {
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "1"),
}, "1", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{},
},
Expand Down Expand Up @@ -300,7 +319,7 @@ func TestUpsert(t *testing.T) {
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "1"),
}, "1", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
Expand Down Expand Up @@ -426,7 +445,7 @@ func TestUpsert(t *testing.T) {
Protocol: "TCP",
Port: 8080,
},
}, "1"),
}, "1", true, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
Expand Down Expand Up @@ -456,12 +475,13 @@ func TestUpsert(t *testing.T) {
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "2"),
}, "2", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
},
},
ignoreTimestampOnService: true,
},
"update a gateway, removing a listener from a service": {
gateway: gwv1beta1.Gateway{
Expand Down Expand Up @@ -515,7 +535,7 @@ func TestUpsert(t *testing.T) {
Protocol: "TCP",
Port: 8081,
},
}, "1"),
}, "1", true, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
Expand All @@ -539,12 +559,13 @@ func TestUpsert(t *testing.T) {
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
}, "2"),
}, "2", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
},
},
ignoreTimestampOnService: true,
},
"updating a gateway deployment respects the number of replicas a user has set": {
gateway: gwv1beta1.Gateway{
Expand Down Expand Up @@ -587,6 +608,144 @@ func TestUpsert(t *testing.T) {
serviceAccounts: []*corev1.ServiceAccount{},
},
},
"updating a gateway deployment respects the labels and annotations a user has set": {
gateway: gwv1beta1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Annotations: copyAnnotations,
},
Spec: gwv1beta1.GatewaySpec{
Listeners: listeners,
},
},
gatewayClassConfig: v1alpha1.GatewayClassConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "consul-gatewayclassconfig",
},
Spec: v1alpha1.GatewayClassConfigSpec{
DeploymentSpec: v1alpha1.DeploymentSpec{
DefaultInstances: common.PointerTo(int32(5)),
MaxInstances: common.PointerTo(int32(7)),
MinInstances: common.PointerTo(int32(1)),
},
CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: []string{copyAnnotationKey}},
ServiceType: (*corev1.ServiceType)(common.PointerTo("NodePort")),
},
},
helmConfig: common.HelmConfig{
ImageDataplane: dataplaneImage,
},
initialResources: resources{
services: []*corev1.Service{
configureService(name, namespace, labels, externalAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
{
Name: "Listener 1",
Protocol: "TCP",
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
{
Name: "Listener 2",
Protocol: "TCP",
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "1", true, false),
},
},
finalResources: resources{
deployments: []*appsv1.Deployment{},
roles: []*rbac.Role{},
services: []*corev1.Service{
configureService(name, namespace, labels, externalAndCopyAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
{
Name: "Listener 1",
Protocol: "TCP",
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
{
Name: "Listener 2",
Protocol: "TCP",
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "2", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{},
},
ignoreTimestampOnService: true,
},
"updating a gateway that has copy-annotations and labels doesn't panic if another controller has removed them all": {
gateway: gwv1beta1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Annotations: copyAnnotations,
},
Spec: gwv1beta1.GatewaySpec{
Listeners: listeners,
},
},
gatewayClassConfig: v1alpha1.GatewayClassConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "consul-gatewayclassconfig",
},
Spec: v1alpha1.GatewayClassConfigSpec{
DeploymentSpec: v1alpha1.DeploymentSpec{
DefaultInstances: common.PointerTo(int32(5)),
MaxInstances: common.PointerTo(int32(7)),
MinInstances: common.PointerTo(int32(1)),
},
CopyAnnotations: v1alpha1.CopyAnnotationsSpec{Service: []string{copyAnnotationKey}},
ServiceType: (*corev1.ServiceType)(common.PointerTo("NodePort")),
},
},
helmConfig: common.HelmConfig{
ImageDataplane: dataplaneImage,
},
initialResources: resources{
services: []*corev1.Service{
configureService(name, namespace, nil, nil, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
{
Name: "Listener 1",
Protocol: "TCP",
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
{
Name: "Listener 2",
Protocol: "TCP",
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "1", true, false),
},
},
finalResources: resources{
deployments: []*appsv1.Deployment{},
roles: []*rbac.Role{},
services: []*corev1.Service{
configureService(name, namespace, labels, copyAnnotations, (corev1.ServiceType)("NodePort"), []corev1.ServicePort{
{
Name: "Listener 1",
Protocol: "TCP",
Port: 8080,
TargetPort: intstr.FromInt(8080),
},
{
Name: "Listener 2",
Protocol: "TCP",
Port: 8081,
TargetPort: intstr.FromInt(8081),
},
}, "2", false, false),
},
serviceAccounts: []*corev1.ServiceAccount{},
},
ignoreTimestampOnService: true,
},
"update a gateway deployment by scaling it when no min or max number of instances is defined on the GatewayClassConfig": {
gateway: gwv1beta1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -775,7 +934,7 @@ func TestUpsert(t *testing.T) {

err := gatekeeper.Upsert(context.Background(), tc.gateway, tc.gatewayClassConfig, tc.helmConfig)
require.NoError(t, err)
require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources))
require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources, tc.ignoreTimestampOnService))
})
}
}
Expand Down Expand Up @@ -867,7 +1026,7 @@ func TestDelete(t *testing.T) {
Protocol: "TCP",
Port: 8081,
},
}, "1"),
}, "1", true, false),
},
serviceAccounts: []*corev1.ServiceAccount{},
},
Expand Down Expand Up @@ -928,7 +1087,7 @@ func TestDelete(t *testing.T) {
Protocol: "TCP",
Port: 8081,
},
}, "1"),
}, "1", true, false),
},
serviceAccounts: []*corev1.ServiceAccount{
configureServiceAccount(name, namespace, labels, "1"),
Expand Down Expand Up @@ -964,7 +1123,7 @@ func TestDelete(t *testing.T) {
Name: tc.gateway.Name,
})
require.NoError(t, err)
require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources))
require.NoError(t, validateResourcesExist(t, client, tc.helmConfig, tc.finalResources, false))
require.NoError(t, validateResourcesAreDeleted(t, client, tc.initialResources))
})
}
Expand Down Expand Up @@ -994,7 +1153,7 @@ func joinResources(resources resources) (objs []client.Object) {
return objs
}

func validateResourcesExist(t *testing.T, client client.Client, helmConfig common.HelmConfig, resources resources) error {
func validateResourcesExist(t *testing.T, client client.Client, helmConfig common.HelmConfig, resources resources, ignoreTimestampOnService bool) error {
t.Helper()

for _, expected := range resources.deployments {
Expand Down Expand Up @@ -1101,6 +1260,10 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo
actual.Labels[createdAtLabelKey] = createdAtLabelValue
actual.Spec.Selector[createdAtLabelKey] = createdAtLabelValue

if ignoreTimestampOnService {
expected.CreationTimestamp = actual.CreationTimestamp
}

require.Equal(t, expected, actual)
}

Expand Down Expand Up @@ -1322,16 +1485,24 @@ func configureRoleBinding(name, namespace string, labels map[string]string, reso
}
}

func configureService(name, namespace string, labels, annotations map[string]string, serviceType corev1.ServiceType, ports []corev1.ServicePort, resourceVersion string) *corev1.Service {
return &corev1.Service{
func configureService(name, namespace string, labels, annotations map[string]string, serviceType corev1.ServiceType, ports []corev1.ServicePort, resourceVersion string, isInitialResource, addExternalLabel bool) *corev1.Service {

// This is used only to test that any external labels added to the service
// are not removed on reconcile
combinedLabels := labels
if addExternalLabel {
combinedLabels["extra-label"] = "extra-label-value"
}

service := corev1.Service{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "Service",
},
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Labels: labels,
Labels: combinedLabels,
Annotations: annotations,
ResourceVersion: resourceVersion,
OwnerReferences: []metav1.OwnerReference{
Expand All @@ -1350,6 +1521,12 @@ func configureService(name, namespace string, labels, annotations map[string]str
Ports: ports,
},
}

if isInitialResource {
service.ObjectMeta.CreationTimestamp = metav1.Now()
}

return &service
}

func configureServiceAccount(name, namespace string, labels map[string]string, resourceVersion string) *corev1.ServiceAccount {
Expand Down
Loading

0 comments on commit fb44cbc

Please sign in to comment.