From d1c726bb9cb758bd9cf0957aa47208ed6c302ab8 Mon Sep 17 00:00:00 2001 From: AliceProxy Date: Fri, 17 Sep 2021 09:53:28 -0700 Subject: [PATCH 1/2] config respecting dns record ttl Signed-off-by: AliceProxy --- .../crds/getambassador.io_mappings.yaml | 2 + docs/yaml/ambassador/ambassador-crds.yaml | 2 + .../ambassador-rbac-prometheus.yaml | 2 + manifests/ambassador/ambassador-crds.yaml | 2 + pkg/api/getambassador.io/v2/mapping_types.go | 1 + .../v2/zz_generated.deepcopy.go | 5 + python/ambassador/envoy/v2/v2cluster.py | 3 + python/ambassador/envoy/v3/v3cluster.py | 3 + python/ambassador/ir/ircluster.py | 1 + python/ambassador/ir/irhttpmapping.py | 1 + python/ambassador/ir/irhttpmappinggroup.py | 3 +- python/schemas/v2/Mapping.schema | 1 + python/tests/kat/t_dns_type.py | 132 ++++++++++++++++++ python/tests/kat/test_ambassador.py | 1 + python/tests/test_cluster_options.py | 89 ++++++++++++ 15 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 python/tests/kat/t_dns_type.py create mode 100644 python/tests/test_cluster_options.py diff --git a/charts/ambassador/crds/getambassador.io_mappings.yaml b/charts/ambassador/crds/getambassador.io_mappings.yaml index 0e8de7cb7f..2c75bbcdb1 100644 --- a/charts/ambassador/crds/getambassador.io_mappings.yaml +++ b/charts/ambassador/crds/getambassador.io_mappings.yaml @@ -369,6 +369,8 @@ spec: - type: array resolver: type: string + respect_dns_ttl: + type: boolean retry_policy: properties: num_retries: diff --git a/docs/yaml/ambassador/ambassador-crds.yaml b/docs/yaml/ambassador/ambassador-crds.yaml index f303605177..9ac5467a5c 100644 --- a/docs/yaml/ambassador/ambassador-crds.yaml +++ b/docs/yaml/ambassador/ambassador-crds.yaml @@ -1076,6 +1076,8 @@ spec: - type: array resolver: type: string + respect_dns_ttl: + type: boolean retry_policy: properties: num_retries: diff --git a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml index dc70f9028f..1d72cc06e8 100644 --- a/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml +++ b/docs/yaml/ambassador/ambassador-rbac-prometheus.yaml @@ -1140,6 +1140,8 @@ spec: - type: array resolver: type: string + respect_dns_ttl: + type: boolean retry_policy: properties: num_retries: diff --git a/manifests/ambassador/ambassador-crds.yaml b/manifests/ambassador/ambassador-crds.yaml index f303605177..9ac5467a5c 100644 --- a/manifests/ambassador/ambassador-crds.yaml +++ b/manifests/ambassador/ambassador-crds.yaml @@ -1076,6 +1076,8 @@ spec: - type: array resolver: type: string + respect_dns_ttl: + type: boolean retry_policy: properties: num_retries: diff --git a/pkg/api/getambassador.io/v2/mapping_types.go b/pkg/api/getambassador.io/v2/mapping_types.go index 31de6fb4d7..b11586c1d2 100644 --- a/pkg/api/getambassador.io/v2/mapping_types.go +++ b/pkg/api/getambassador.io/v2/mapping_types.go @@ -48,6 +48,7 @@ type MappingSpec struct { KeepAlive *KeepAlive `json:"keepalive,omitempty"` CORS *CORS `json:"cors,omitempty"` RetryPolicy *RetryPolicy `json:"retry_policy,omitempty"` + RespectDNSTTL *bool `json:"respect_dns_ttl,omitempty"` GRPC *bool `json:"grpc,omitempty"` HostRedirect *bool `json:"host_redirect,omitempty"` HostRewrite string `json:"host_rewrite,omitempty"` diff --git a/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go b/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go index cc1184813a..4c30f50ebb 100644 --- a/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go +++ b/pkg/api/getambassador.io/v2/zz_generated.deepcopy.go @@ -1696,6 +1696,11 @@ func (in *MappingSpec) DeepCopyInto(out *MappingSpec) { *out = new(RetryPolicy) (*in).DeepCopyInto(*out) } + if in.RespectDNSTTL != nil { + in, out := &in.RespectDNSTTL, &out.RespectDNSTTL + *out = new(bool) + **out = **in + } if in.GRPC != nil { in, out := &in.GRPC, &out.GRPC *out = new(bool) diff --git a/python/ambassador/envoy/v2/v2cluster.py b/python/ambassador/envoy/v2/v2cluster.py index b355a97231..9c643db4ed 100644 --- a/python/ambassador/envoy/v2/v2cluster.py +++ b/python/ambassador/envoy/v2/v2cluster.py @@ -62,6 +62,9 @@ def __init__(self, config: 'V2Config', cluster: IRCluster) -> None: 'dns_lookup_family': dns_lookup_family } + if cluster.respect_dns_ttl: + fields['respect_dns_ttl'] = cluster.respect_dns_ttl + if ctype == 'EDS': fields['eds_cluster_config'] = { 'eds_config': {'ads': {}}, 'service_name': cmap_entry['endpoint_path']} diff --git a/python/ambassador/envoy/v3/v3cluster.py b/python/ambassador/envoy/v3/v3cluster.py index 5b7a5a8454..60bc52bafa 100644 --- a/python/ambassador/envoy/v3/v3cluster.py +++ b/python/ambassador/envoy/v3/v3cluster.py @@ -62,6 +62,9 @@ def __init__(self, config: 'V3Config', cluster: IRCluster) -> None: 'dns_lookup_family': dns_lookup_family } + if cluster.respect_dns_ttl: + fields['respect_dns_ttl'] = cluster.respect_dns_ttl + if ctype == 'EDS': fields['eds_cluster_config'] = { 'eds_config': { diff --git a/python/ambassador/ir/ircluster.py b/python/ambassador/ir/ircluster.py index 7b012f252c..67228c4fb3 100644 --- a/python/ambassador/ir/ircluster.py +++ b/python/ambassador/ir/ircluster.py @@ -61,6 +61,7 @@ def __init__(self, ir: 'IR', aconf: Config, parent_ir_resource: 'IRResource', load_balancer: Optional[dict] = None, keepalive: Optional[dict] = None, circuit_breakers: Optional[list] = None, + respect_dns_ttl: Optional[bool] = False, rkey: str="-override-", kind: str="IRCluster", diff --git a/python/ambassador/ir/irhttpmapping.py b/python/ambassador/ir/irhttpmapping.py index 88e47ed272..7286b75db4 100644 --- a/python/ambassador/ir/irhttpmapping.py +++ b/python/ambassador/ir/irhttpmapping.py @@ -116,6 +116,7 @@ class IRHTTPMapping (IRBaseMapping): "remove_request_headers": True, "remove_response_headers": True, "resolver": False, + "respect_dns_ttl": False, "retry_policy": False, # Do not include rewrite "service": False, # See notes above diff --git a/python/ambassador/ir/irhttpmappinggroup.py b/python/ambassador/ir/irhttpmappinggroup.py index 60f28dc231..51492d4a80 100644 --- a/python/ambassador/ir/irhttpmappinggroup.py +++ b/python/ambassador/ir/irhttpmappinggroup.py @@ -249,7 +249,8 @@ def add_cluster_for_mapping(self, mapping: IRBaseMapping, cluster_idle_timeout_ms=mapping.get('cluster_idle_timeout_ms', None), cluster_max_connection_lifetime_ms=mapping.get('cluster_max_connection_lifetime_ms', None), circuit_breakers=mapping.get('circuit_breakers', None), - marker=marker) + marker=marker, + respect_dns_ttl=mapping.get('respect_dns_ttl', False)) # Make sure that the cluster is actually in our IR... stored = self.ir.add_cluster(cluster) diff --git a/python/schemas/v2/Mapping.schema b/python/schemas/v2/Mapping.schema index 85c99055e4..3a062090c1 100644 --- a/python/schemas/v2/Mapping.schema +++ b/python/schemas/v2/Mapping.schema @@ -206,6 +206,7 @@ "host_regex": { "type": "boolean" }, "headers": { "$ref": "#/definitions/mapStrStr" }, "regex_headers": { "$ref": "#/definitions/mapStrStr" }, + "respect_dns_ttl": { "type": "boolean" }, "labels": { "type": "object" }, diff --git a/python/tests/kat/t_dns_type.py b/python/tests/kat/t_dns_type.py new file mode 100644 index 0000000000..e9200b7e4f --- /dev/null +++ b/python/tests/kat/t_dns_type.py @@ -0,0 +1,132 @@ +from kat.harness import Query +from abstract_tests import AmbassadorTest, ServiceType, HTTP +import json + +# tests that using logical_dns does not impact requests +# strict_dns is already the default setting so we only need to validate it's config pytest +class LogicalDnsType(AmbassadorTest): + target: ServiceType + + def init(self): + self.target = HTTP(name="target") + + def config(self): + yield self, self.format(""" +--- +apiVersion: ambassador/v2 +kind: Mapping +name: {self.target.path.k8s}-foo +prefix: /foo/ +service: {self.target.path.fqdn} +dns_type: logical_dns +""") + + def queries(self): + yield Query(self.url("foo/")) + + def check(self): + # Not getting a 400 bad request is confirmation that this setting works as long as the request can reach the upstream + assert self.results[0].status == 200 + +# this test is just to confirm that when both dns_type and the endpoint resolver are in use, the endpoint resolver wins +class LogicalDnsTypeEndpoint(AmbassadorTest): + target: ServiceType + + def init(self): + self.target = HTTP(name="target") + + def config(self): + yield self, self.format(""" +--- +apiVersion: ambassador/v2 +kind: Mapping +name: {self.target.path.k8s}-foo +prefix: /foo/ +service: {self.target.path.fqdn} +dns_type: logical_dns +resolver: endpoint +""") + + def queries(self): + yield Query(self.url("foo/")) + + def check(self): + # Not getting a 400 bad request is confirmation that this setting works as long as the request can reach the upstream + assert self.results[0].status == 200 + +# Tests Respecting DNS TTL alone +class DnsTtl(AmbassadorTest): + target: ServiceType + + def init(self): + self.target = HTTP(name="target") + + def config(self): + yield self, self.format(""" +--- +apiVersion: ambassador/v2 +kind: Mapping +name: {self.target.path.k8s}-foo +prefix: /foo/ +service: {self.target.path.fqdn} +respect_dns_ttl: true +""") + + def queries(self): + yield Query(self.url("foo/")) + + def check(self): + # Not getting a 400 bad request is confirmation that this setting works as long as the request can reach the upstream + assert self.results[0].status == 200 + +# Tests the DNS TTL with the endpoint resolver +class DnsTtlEndpoint(AmbassadorTest): + target: ServiceType + + def init(self): + self.target = HTTP(name="target") + + def config(self): + yield self, self.format(""" +--- +apiVersion: ambassador/v2 +kind: Mapping +name: {self.target.path.k8s}-foo +prefix: /foo/ +service: {self.target.path.fqdn} +respect_dns_ttl: true +resolver: endpoint +""") + + def queries(self): + yield Query(self.url("foo/")) + + def check(self): + # Not getting a 400 bad request is confirmation that this setting works as long as the request can reach the upstream + assert self.results[0].status == 200 + +# Tests the DNS TTL with Logical DNS type +class DnsTtlDnsType(AmbassadorTest): + target: ServiceType + + def init(self): + self.target = HTTP(name="target") + + def config(self): + yield self, self.format(""" +--- +apiVersion: ambassador/v2 +kind: Mapping +name: {self.target.path.k8s}-foo +prefix: /foo/ +service: {self.target.path.fqdn} +respect_dns_ttl: true +dns_type: logical_dns +""") + + def queries(self): + yield Query(self.url("foo/")) + + def check(self): + # Not getting a 400 bad request is confirmation that this setting works as long as the request can reach the upstream + assert self.results[0].status == 200 \ No newline at end of file diff --git a/python/tests/kat/test_ambassador.py b/python/tests/kat/test_ambassador.py index 2161105ac7..d5d3f550c8 100644 --- a/python/tests/kat/test_ambassador.py +++ b/python/tests/kat/test_ambassador.py @@ -43,6 +43,7 @@ import t_regexrewrite_forwarding import t_error_response import t_chunked_length +import t_dns_type import t_bufferlimitbytes # pytest will find this because Runner is a toplevel callable object in a file diff --git a/python/tests/test_cluster_options.py b/python/tests/test_cluster_options.py new file mode 100644 index 0000000000..82bb29f259 --- /dev/null +++ b/python/tests/test_cluster_options.py @@ -0,0 +1,89 @@ +from utils import econf_compile, econf_foreach_cluster, module_and_mapping_manifests, SUPPORTED_ENVOY_VERSIONS + +import pytest + +# Tests if `setting` exists within the cluster config and has `expected` as the value for that setting +# Use `exists` to test if you expect a setting to not exist +def _test_cluster_setting(yaml, setting, expected, exists=True, envoy_version="V2"): + econf = econf_compile(yaml, envoy_version=envoy_version) + + def check(cluster): + if exists: + assert setting in cluster + assert cluster[setting] == expected + else: + assert setting not in cluster + + econf_foreach_cluster(econf, check) + +# Tests a setting in a cluster that has it's own fields. Example: common_http_protocol_options has multiple subfields +def _test_cluster_subfields(yaml, setting, expectations={}, exists=True, envoy_version="V2"): + econf = econf_compile(yaml, envoy_version=envoy_version) + + def check(cluster): + if exists: + assert setting in cluster + else: + assert setting not in cluster + for key, expected in expectations.items(): + print("Checking key: {} for the {} setting in Envoy cluster".format(key, setting)) + assert key in cluster[setting] + assert cluster[setting][key] == expected + + econf_foreach_cluster(econf, check) + +# Test dns_type setting in Mapping +@pytest.mark.compilertest +def test_logical_dns_type(): + yaml = module_and_mapping_manifests(None, ["dns_type: logical_dns"]) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="type", + expected="LOGICAL_DNS", exists=True, envoy_version=v) + +@pytest.mark.compilertest +def test_logical_dns_type(): + # Make sure we can configure strict dns as well even though it's the default + yaml = module_and_mapping_manifests(None, ["dns_type: strict_dns"]) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="type", + expected="STRICT_DNS", exists=True, envoy_version=v) + +@pytest.mark.compilertest +def test_logical_dns_type_wrong(): + # Ensure we fallback to strict_dns as the setting when an invalid string is passed + # This is preferable to invalid config and an error is logged + yaml = module_and_mapping_manifests(None, ["dns_type: something_new"]) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="type", + expected="STRICT_DNS", exists=True, envoy_version=v) + +@pytest.mark.compilertest +def test_logical_dns_type_wrong(): + # Ensure we use endpoint discovery instead of this value when using the endpoint resolver + yaml = module_and_mapping_manifests(None, ["dns_type: logical_dns", "resolver: endpoint"]) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="type", + expected="EDS", exists=True, envoy_version=v) + + +@pytest.mark.compilertest +def test_dns_ttl(): + # Test configuring the respect_dns_ttl generates an Envoy config + yaml = module_and_mapping_manifests(None, ["respect_dns_ttl: true"]) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="respect_dns_ttl", + expected="true", exists=True, envoy_version=v) + +@pytest.mark.compilertest +def test_dns_ttl(): + # Test dns_ttl is not configured when not applied in the Mapping + yaml = module_and_mapping_manifests(None, None) + for v in SUPPORTED_ENVOY_VERSIONS: + # The dns type is listed as just "type" + _test_cluster_setting(yaml, setting="respect_dns_ttl", + expected="false", exists=False, envoy_version=v) \ No newline at end of file From 6abd8807685361b130e583200401ba8568cdb92b Mon Sep 17 00:00:00 2001 From: AliceProxy Date: Mon, 20 Sep 2021 13:05:05 -0700 Subject: [PATCH 2/2] set unset variable Signed-off-by: AliceProxy --- python/ambassador/ir/ircluster.py | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ambassador/ir/ircluster.py b/python/ambassador/ir/ircluster.py index 7b560d93ea..860f988467 100644 --- a/python/ambassador/ir/ircluster.py +++ b/python/ambassador/ir/ircluster.py @@ -285,6 +285,7 @@ def __init__(self, ir: 'IR', aconf: Config, parent_ir_resource: 'IRResource', 'connect_timeout_ms': connect_timeout_ms, 'cluster_idle_timeout_ms': cluster_idle_timeout_ms, 'cluster_max_connection_lifetime_ms': cluster_max_connection_lifetime_ms, + 'respect_dns_ttl': respect_dns_ttl, } if grpc: