Skip to content
This repository has been archived by the owner on Feb 9, 2022. It is now read-only.

Commit

Permalink
Add flag to control source/origin lookups when destination.service is…
Browse files Browse the repository at this point in the history 
… istio ingress (#1349)

Automatic merge from submit-queue

Add flag to control source/origin lookups when destination.service is istio ingress

As noted in istio/istio#879, the source information that the Mixer receives (`source.ip` values) are problematic for attempting to lookup (in-cluster). This PR addresses provides a workaround for compounding that issue within Mixer by preventing lookup of source/origin information in the kubernetes adapter when `destination.service == ingress.istio-system.svc.cluster.local`.

This behavior is configurable, through options in the config params passed into the adapter builder.

This PR also cleans up a related bit of the kubernetes adapter behavior related to handling and generation of IP_ADDRESS values. As noted in istio/old_mixerclient_repo#112 (and also in istio/proxy#483), handling of IP_ADDRESS values is completely inconsistent throughout Istio. The changes here remove any inconsistencies on the part of the kubernetes adapter.

It also fixes the incorrect IP-related config in `testdata/configroot`.

**Release note**:

```release-note-none
NONE
```
  • Loading branch information
@douglas-reid @istio-merge-robot
douglas-reid authored and istio-merge-robot committed Sep 29, 2017
1 parent c2c6b0a commit 20fbea5
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 66 deletions.
15 changes: 14 additions & 1 deletion adapter/kubernetes/config/config.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ option (gogoproto.gostring_all) = false;
// that includes a key of "sourcePodIP" (assuming parameter defaults).
message Params {
reserved 17;
// next field id: 24
// next field id: 26

// File path to discover kubeconfig. For in-cluster configuration,
// this should be left unset. For local configuration, this should
Expand Down Expand Up @@ -167,4 +167,17 @@ message Params {
//
// Default: Service
string service_value_name = 16;

// Whether or not to execute source and origin value lookup
// for Mixer requests when Istio ingress is the destination
// service.
//
// Default: false
bool lookup_ingress_source_and_origin_values = 24;

// Istio ingress service string. This is used to identify the
// ingress service in requests.
//
// Default: "ingress.istio-system.svc.cluster.local"
string fully_qualified_istio_ingress_service_name = 25;
}
73 changes: 44 additions & 29 deletions adapter/kubernetes/kubernetes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,11 @@ const (
serviceVal = "Service"

// value extraction
clusterDomain = "svc.cluster.local"
podServiceLabel = "app"
istioPodServiceLabel = "istio"
clusterDomain = "svc.cluster.local"
podServiceLabel = "app"
istioPodServiceLabel = "istio"
lookupIngressSourceAndOriginValues = false
istioIngressSvc = "ingress.istio-system.svc.cluster.local"

// cache invaliation
// TODO: determine a reasonable default
Expand All @@ -97,27 +99,29 @@ const (

var (
conf = &config.Params{
KubeconfigPath: "",
CacheRefreshDuration: defaultRefreshPeriod,
SourceUidInputName: sourceUID,
DestinationUidInputName: destinationUID,
OriginUidInputName: originUID,
SourceIpInputName: sourceIP,
DestinationIpInputName: destinationIP,
OriginIpInputName: originIP,
ClusterDomainName: clusterDomain,
PodLabelForService: podServiceLabel,
PodLabelForIstioComponentService: istioPodServiceLabel,
SourcePrefix: sourcePrefix,
DestinationPrefix: destinationPrefix,
OriginPrefix: originPrefix,
LabelsValueName: labelsVal,
PodNameValueName: podNameVal,
PodIpValueName: podIPVal,
HostIpValueName: hostIPVal,
NamespaceValueName: namespaceVal,
ServiceAccountValueName: serviceAccountVal,
ServiceValueName: serviceVal,
KubeconfigPath: "",
CacheRefreshDuration: defaultRefreshPeriod,
SourceUidInputName: sourceUID,
DestinationUidInputName: destinationUID,
OriginUidInputName: originUID,
SourceIpInputName: sourceIP,
DestinationIpInputName: destinationIP,
OriginIpInputName: originIP,
ClusterDomainName: clusterDomain,
PodLabelForService: podServiceLabel,
PodLabelForIstioComponentService: istioPodServiceLabel,
SourcePrefix: sourcePrefix,
DestinationPrefix: destinationPrefix,
OriginPrefix: originPrefix,
LabelsValueName: labelsVal,
PodNameValueName: podNameVal,
PodIpValueName: podIPVal,
HostIpValueName: hostIPVal,
NamespaceValueName: namespaceVal,
ServiceAccountValueName: serviceAccountVal,
ServiceValueName: serviceVal,
FullyQualifiedIstioIngressServiceName: istioIngressSvc,
LookupIngressSourceAndOriginValues: lookupIngressSourceAndOriginValues,
}
)

Expand Down Expand Up @@ -199,6 +203,9 @@ func (*builder) ValidateConfig(c adapter.Config) (ce *adapter.ConfigErrors) {
if len(params.PodLabelForIstioComponentService) == 0 {
ce = ce.Appendf("podLabelForIstioComponentService", "field must be populated")
}
if len(params.FullyQualifiedIstioIngressServiceName) == 0 {
ce = ce.Appendf("fullyQualifiedIstioIngressServiceName", "field must be populated")
}
if len(params.ClusterDomainName) == 0 {
ce = ce.Appendf("clusterDomainName", "field must be populated")
} else if len(strings.Split(params.ClusterDomainName, ".")) != 3 {
Expand Down Expand Up @@ -264,12 +271,15 @@ func (k *kubegen) Close() error { return nil }

func (k *kubegen) Generate(inputs map[string]interface{}) (map[string]interface{}, error) {
values := make(map[string]interface{})
if id, found := serviceIdentifier(inputs, k.params.SourceUidInputName, k.params.SourceIpInputName); found && len(id) > 0 {
k.addValues(values, id, k.params.SourcePrefix)
}
if id, found := serviceIdentifier(inputs, k.params.DestinationUidInputName, k.params.DestinationIpInputName); found && len(id) > 0 {
k.addValues(values, id, k.params.DestinationPrefix)
}
if k.skipIngressLookups(values) {
return values, nil
}
if id, found := serviceIdentifier(inputs, k.params.SourceUidInputName, k.params.SourceIpInputName); found && len(id) > 0 {
k.addValues(values, id, k.params.SourcePrefix)
}
if id, found := serviceIdentifier(inputs, k.params.OriginUidInputName, k.params.OriginIpInputName); found && len(id) > 0 {
k.addValues(values, id, k.params.OriginPrefix)
}
Expand All @@ -286,6 +296,11 @@ func (k *kubegen) addValues(vals map[string]interface{}, uid, valPrefix string)
addPodValues(vals, valPrefix, k.params, pod)
}

func (k *kubegen) skipIngressLookups(values map[string]interface{}) bool {
destSvcParam := k.params.DestinationPrefix + k.params.ServiceValueName
return !k.params.LookupIngressSourceAndOriginValues && values[destSvcParam] == k.params.FullyQualifiedIstioIngressServiceName
}

func keyFromUID(uid string) string {
if ip := net.ParseIP(uid); ip != nil {
return uid
Expand Down Expand Up @@ -314,10 +329,10 @@ func addPodValues(m map[string]interface{}, prefix string, params config.Params,
m[valueName(prefix, params.ServiceAccountValueName)] = p.Spec.ServiceAccountName
}
if len(p.Status.PodIP) > 0 {
m[valueName(prefix, params.PodIpValueName)] = p.Status.PodIP
m[valueName(prefix, params.PodIpValueName)] = net.ParseIP(p.Status.PodIP)
}
if len(p.Status.HostIP) > 0 {
m[valueName(prefix, params.HostIpValueName)] = p.Status.HostIP
m[valueName(prefix, params.HostIpValueName)] = net.ParseIP(p.Status.HostIP)
}
if app, found := p.Labels[params.PodLabelForService]; found {
n, err := canonicalName(app, p.Namespace, params.ClusterDomainName)
Expand Down
78 changes: 51 additions & 27 deletions adapter/kubernetes/kubernetes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,11 @@ func TestBuilder(t *testing.T) {

func TestBuilder_ValidateConfigErrors(t *testing.T) {
val := reflect.ValueOf(&config.Params{}).Elem()
// currently two non-validated fields: kubeconfig and cache refresh duration
expectedConfigErrs := val.NumField() - 2
// currently three non-validated fields:
// - kubeconfig
// - cache refresh duration
// - lookup_ingress_source_and_origin_values
expectedConfigErrs := val.NumField() - 3
tests := []struct {
name string
conf *config.Params
Expand Down Expand Up @@ -228,12 +231,10 @@ func TestKubegen_Generate(t *testing.T) {
"testns/alt-pod": {ObjectMeta: metav1.ObjectMeta{Name: "alt-pod", Namespace: "testns", Labels: map[string]string{"app": "alt-svc.testns"}}},
"testns/bad-svc-pod": {ObjectMeta: metav1.ObjectMeta{Name: "bad-svc-pod", Namespace: "testns", Labels: map[string]string{"app": ":"}}},
"192.168.234.3": {ObjectMeta: metav1.ObjectMeta{Name: "ip-svc-pod", Namespace: "testns", Labels: map[string]string{"app": "ipAddr"}}},
"testns/istio-ingress": {ObjectMeta: metav1.ObjectMeta{Name: "istio-ingress", Namespace: "testns", Labels: map[string]string{"istio": "ingress"}}},
"istio-system/ingress": {ObjectMeta: metav1.ObjectMeta{Name: "ingress", Namespace: "istio-system", Labels: map[string]string{"istio": "ingress"}}},
"testns/ipApp": {ObjectMeta: metav1.ObjectMeta{Name: "ipApp", Namespace: "testns", Labels: map[string]string{"app": "10.1.10.1"}}},
}

kg := &kubegen{log: test.NewEnv(t).Logger(), params: *conf, pods: fakeCache{pods: pods}}

sourceUIDIn := map[string]interface{}{
"sourceUID": "kubernetes://test-pod.testns",
"destinationUID": "kubernetes://badsvcuid",
Expand All @@ -245,8 +246,8 @@ func TestKubegen_Generate(t *testing.T) {
"app": "test",
"something": "",
},
"sourcePodIP": "10.10.10.1",
"sourceHostIP": "10.1.1.10",
"sourcePodIP": net.ParseIP("10.10.10.1"),
"sourceHostIP": net.ParseIP("10.1.1.10"),
"sourceNamespace": "testns",
"sourcePodName": "test-pod",
"sourceService": "test.testns.svc.cluster.local",
Expand Down Expand Up @@ -296,19 +297,19 @@ func TestKubegen_Generate(t *testing.T) {
"destinationPodName": "empty",
}

baddestinationSvcIn := map[string]interface{}{"destinationUID": "kubernetes://bad-svc-pod.testns"}
badDestinationSvcIn := map[string]interface{}{"destinationUID": "kubernetes://bad-svc-pod.testns"}

baddestinationOut := map[string]interface{}{
badDestinationOut := map[string]interface{}{
"destinationLabels": map[string]string{
"app": ":",
},
"destinationNamespace": "testns",
"destinationPodName": "bad-svc-pod",
}

ipdestinationSvcIn := map[string]interface{}{"destinationIP": []uint8(net.ParseIP("192.168.234.3"))}
ipDestinationSvcIn := map[string]interface{}{"destinationIP": []uint8(net.ParseIP("192.168.234.3"))}

ipdestinationOut := map[string]interface{}{
ipDestinationOut := map[string]interface{}{
"destinationLabels": map[string]string{
"app": "ipAddr",
},
Expand All @@ -317,49 +318,72 @@ func TestKubegen_Generate(t *testing.T) {
"destinationService": "ipAddr.testns.svc.cluster.local",
}

istiodestinationSvcIn := map[string]interface{}{
"destinationUID": "kubernetes://istio-ingress.testns",
istioDestinationSvcIn := map[string]interface{}{
"destinationUID": "kubernetes://ingress.istio-system",
"sourceUID": "kubernetes://test-pod.testns",
}

istiodestinationOut := map[string]interface{}{
istioDestinationOut := map[string]interface{}{
"destinationLabels": map[string]string{
"istio": "ingress",
},
"destinationNamespace": "testns",
"destinationPodName": "istio-ingress",
"destinationService": "ingress.testns.svc.cluster.local",
"destinationNamespace": "istio-system",
"destinationPodName": "ingress",
"destinationService": "ingress.istio-system.svc.cluster.local",
}

istioDestinationWithSrcOut := map[string]interface{}{
"destinationLabels": map[string]string{"istio": "ingress"},
"destinationNamespace": "istio-system",
"destinationPodName": "ingress",
"destinationService": "ingress.istio-system.svc.cluster.local",
"sourceServiceAccountName": "test",
"sourceService": "test.testns.svc.cluster.local",
"sourceLabels": map[string]string{"app": "test", "something": ""},
"sourceNamespace": "testns",
"sourcePodIP": net.ParseIP("10.10.10.1"),
"sourceHostIP": net.ParseIP("10.1.1.10"),
"sourcePodName": "test-pod",
}

ipAppSvcIn := map[string]interface{}{
"destinationUID": "kubernetes://ipApp.testns",
}

ipAppdestinationOut := map[string]interface{}{
ipAppDestinationOut := map[string]interface{}{
"destinationLabels": map[string]string{
"app": "10.1.10.1",
},
"destinationNamespace": "testns",
"destinationPodName": "ipApp",
}

confWithIngressLookups := *conf
confWithIngressLookups.LookupIngressSourceAndOriginValues = true

tests := []struct {
name string
inputs map[string]interface{}
want map[string]interface{}
params config.Params
}{
{"source pod and destination service", sourceUIDIn, sourceUIDOut},
{"alternate service canonicalization (namespace)", nsAppLabelIn, nsAppLabelOut},
{"alternate service canonicalization (svc cluster)", svcClusterIn, svcClusterOut},
{"alternate service canonicalization (long svc)", longSvcClusterIn, longSvcClusterOut},
{"empty service", emptySvcIn, emptyServiceOut},
{"bad destination service", baddestinationSvcIn, baddestinationOut},
{"destination ip pod", ipdestinationSvcIn, ipdestinationOut},
{"istio service", istiodestinationSvcIn, istiodestinationOut},
{"ip app", ipAppSvcIn, ipAppdestinationOut},
{"source pod and destination service", sourceUIDIn, sourceUIDOut, *conf},
{"alternate service canonicalization (namespace)", nsAppLabelIn, nsAppLabelOut, *conf},
{"alternate service canonicalization (svc cluster)", svcClusterIn, svcClusterOut, *conf},
{"alternate service canonicalization (long svc)", longSvcClusterIn, longSvcClusterOut, *conf},
{"empty service", emptySvcIn, emptyServiceOut, *conf},
{"bad destination service", badDestinationSvcIn, badDestinationOut, *conf},
{"destination ip pod", ipDestinationSvcIn, ipDestinationOut, *conf},
{"istio ingress service (no lookup source)", istioDestinationSvcIn, istioDestinationOut, *conf},
{"istio ingress service (lookup source)", istioDestinationSvcIn, istioDestinationWithSrcOut, confWithIngressLookups},
{"ip app", ipAppSvcIn, ipAppDestinationOut, *conf},
}

for _, v := range tests {
t.Run(v.name, func(t *testing.T) {

kg := &kubegen{log: test.NewEnv(t).Logger(), params: v.params, pods: fakeCache{pods: pods}}

got, err := kg.Generate(v.inputs)
if err != nil {
t.Errorf("Unexpected error: %v", err)
Expand Down
15 changes: 14 additions & 1 deletion pkg/aspect/attrgenmgr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package aspect

import (
"fmt"
"net"

"github.com/golang/glog"
rpc "github.com/googleapis/googleapis/google/rpc"
Expand Down Expand Up @@ -106,7 +107,19 @@ func (e *attrGenExec) Execute(attrs attribute.Bag, mapper expr.Evaluator) (*Prep
for key, val := range out {
if attrName, found := e.bindings[key]; found {
// TODO: type validation?
bag.Set(attrName, val)
switch v := val.(type) {
case net.IP:
// conversion to []byte necessary based on current IP_ADDRESS handling within Mixer
// TODO: remove
glog.Warningf("---- converting net.IP to []byte")
if v4 := v.To4(); v4 != nil {
bag.Set(attrName, []byte(v4))
continue
}
bag.Set(attrName, []byte(v.To16()))
default:
bag.Set(attrName, val)
}
continue
}
glog.Warningf("Generated value '%s' was not mapped to an attribute.", key)
Expand Down
24 changes: 18 additions & 6 deletions pkg/aspect/attrgenmgr_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package aspect

import (
"errors"
"net"
"reflect"
"testing"

dpb "istio.io/api/mixer/v1/config/descriptor"
Expand Down Expand Up @@ -217,26 +219,36 @@ func (t testAttrGen) Generate(map[string]interface{}) (map[string]interface{}, e
func TestAttributeGeneratorExecutor_Execute(t *testing.T) {

genParams := &apb.AttributesGeneratorParams{
InputExpressions: map[string]string{"pod.ip": "source_ip"},
AttributeBindings: map[string]string{"service_found": "found", "source_service": "srcSvc"},
InputExpressions: map[string]string{"pod.ip": "source_ip"},
AttributeBindings: map[string]string{
"service_found": "found",
"source_service": "srcSvc",
"destination_ip": "destIP",
"ip_v6": "v6IP",
},
}

bMap := map[string]string{"found": "service_found", "srcSvc": "source_service"}
bMap := map[string]string{"found": "service_found", "srcSvc": "source_service", "destIP": "destination_ip", "v6IP": "ip_v6"}

inBag := attribute.GetMutableBag(nil)
inBag.Set("source_ip", "10.1.1.10")
inBag := attribute.GetFakeMutableBagForTesting(map[string]interface{}{"source_ip": []byte(net.IP("10.1.1.10").To4())})

outMap := map[string]interface{}{
"found": true,
"srcSvc": "service1",
"destIP": net.ParseIP("10.34.23.3"),
"v6IP": net.ParseIP("2001:db8::1"),
}
wantBag := attribute.GetMutableBag(nil)
wantBag.Set("service_found", true)
wantBag.Set("source_service", "service1")
wantBag.Set("destination_ip", []byte{0xa, 0x22, 0x17, 0x3})
wantBag.Set("ip_v6", []byte{0x20, 0x1, 0xd, 0xb8, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1})

extraOutMap := map[string]interface{}{
"found": true,
"srcSvc": "service1",
"destIP": net.ParseIP("10.34.23.3"),
"v6IP": net.ParseIP("2001:db8::1"),
"shouldBeStripped": "never_used",
}

Expand Down Expand Up @@ -272,7 +284,7 @@ func TestAttributeGeneratorExecutor_Execute(t *testing.T) {
if !ok {
t.Errorf("Generated attribute.Bag missing attribute %s", n)
}
if gotVal != wantVal {
if !reflect.DeepEqual(gotVal, wantVal) {
t.Errorf("For attribute '%s': got value %v, want %v", n, gotVal, wantVal)
}
}
Expand Down
Loading

0 comments on commit 20fbea5

Please sign in to comment.