Support DNS names for egress network policy #13002

pravisankar · 2017-02-17T18:17:28Z

Trello card: https://trello.com/c/yH9l0ouV/289-8-use-dns-names-with-egress-firewall-demo-human-names

pravisankar · 2017-02-17T18:22:27Z

@openshift/networking PTAL
(I think it will be easy to follow if you review one commit at a time)

danwinship · 2017-02-20T15:15:14Z

pkg/sdn/api/v1/types.go

@@ -96,8 +96,12 @@ const (

 // EgressNetworkPolicyPeer specifies a target to apply egress network policy to
 type EgressNetworkPolicyPeer struct {
+	// Either cidrSelector or dnsName should be specified but not both
+


This doesn't get picked up in the docs. You should probably update the docs on each field instead. (eg, "cidrSelector is the CIDR range to allow/deny traffic to. If this is set, dnsName must be unset.") Or else move this comment to be part of the EgressNetworkPolicyPeer doc comment.

(Also, this commit has "Author: Ravi Sankar Penta <[email protected]>")

danwinship · 2017-02-20T15:17:35Z

pkg/sdn/api/validation/validation.go

+
+		if len(rule.To.CIDRSelector) > 0 {
+			if _, _, err := net.ParseCIDR(rule.To.CIDRSelector); err != nil {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("egress").Index(i).Child("to"), rule.To.CIDRSelector, err.Error()))


This is a pre-existing bug in the code, but that field descriptor probably should have .Child("cidrSelector") (and likewise below for the dnsName error)

danwinship · 2017-02-20T15:23:22Z

pkg/sdn/plugin/egress_dns.go

+	}
+
+	for name, ips := range dnsInfo.Get() {
+		if name == dnsName {


dnsInfo.Get() should take a name argument and only return that one record. (Or else call it Lookup() if you still need the get-everything version somewhere else.)

danwinship · 2017-02-20T15:27:38Z

pkg/sdn/plugin/egress_dns.go

+
+	for {
+		<-t.C
+		glog.V(8).Infof("Periodic egress dns sync: update policy rules")


I generally expect that telling people to use --loglevel=5 will get everything interesting... And this is way less verbose than most of what's at V(5) anyway, so I don't think there's any reason to make it V(8).

danwinship · 2017-02-20T15:36:25Z

pkg/sdn/plugin/egress_dns.go

+}
+
+func (proxy *OsdnProxy) SyncEgressDNSProxyFirewall() {
+	t := time.NewTicker(60 * time.Minute)


I worry that the two loops (OVS and proxy) are going to end up getting out of sync...

If this was C#, I'd say that EgressDNS should have a single periodic update loop, and emit an event when DNS results changed, and then OsdnNode and OsdnProxy should each have handlers for that event and then update their own state. I'm not sure what the correspondingly idiomatic way of doing that in golang would be... something with a channel I suppose, but it's tricky because we need a single writer and multiple readers.

(Oh, and the plugin and the proxy each have their own EgressDNS anyway so that still wouldn't help.)

danwinship · 2017-02-20T16:00:18Z

pkg/sdn/plugin/egress_dns.go

+				glog.Error(err)
+				continue
+			}
+			if equal(olddnsMap, newdnsMap) {


equal() seems like it should be some operation on dnsInfo. Actually, since you don't even care about the contents of the maps here, you could just make dnsInfo.Update() return a bool (indicating whether anything changed) rather than a map (and not even call dnsInfo.Get())

danwinship · 2017-02-20T16:09:15Z

pkg/sdn/plugin/egress_dns.go

+						glog.Warningf("Could not find netid for namespace %q: %v", policy.Namespace, err)
+						break
+					}
+					plugin.updateEgressNetworkPolicyRules(vnid)


Hm... plugin.updateEgressNetworkPolicyRules() is broken; it needs locking (eg, to protect against concurrent reads from/writes to plugin.egressPolicies[]). And then this will need to use the right locking as well.

danwinship · 2017-02-20T16:18:22Z

pkg/sdn/plugin/egress_dns.go

+	return cidrs
+}
+
+func (plugin *OsdnNode) SyncEgressDNSPolicyRules() {


I know we're already terrible about this, but I think it's confusing to keep spreading OsdnNode methods through more and more files. In this case, I think this method should go into egress_network_policy.go.

danwinship · 2017-02-20T16:21:31Z

pkg/sdn/plugin/egress_dns.go

+	}
+}
+
+func (proxy *OsdnProxy) SyncEgressDNSProxyFirewall() {


and this should move to proxy.go...

danwinship · 2017-02-20T16:22:38Z

pkg/sdn/plugin/egress_dns.go

+			// found dns updates
+			for _, policy := range policies.Items {
+				if policy.UID == policyUID {
+					proxy.updateNetworkPolicy(policy)


As with the OVS case, this needs locking, though fortunately the remaining proxy code is already correct. So, you need to grab proxy.lock here.

And you have to call proxy.updateEndpoints() afterward too at the end of the loop if you changed anything.

pravisankar · 2017-02-22T02:49:28Z

@danwinship updated, ptal

pravisankar · 2017-03-01T00:11:29Z

Reiterating the concern that I raised during the standup:
In this PR, we are checking for dns updates every 30 mins. So in case of dns update, there is a period where egress policy may not behave as expected until it syncs these changes. Is this solution acceptable? If not:
(a) Do we want to call sync loop aggressively? (may not be ideal if we don't expect dns updates frequently)
(b) Make this as OpenShift config option (similar to iptablesSyncPeriod)
(c) Make the code a bit more smart by querying TTL for the domain and poll every time when the TTL expires.

@knobunc @eparis

danwinship · 2017-03-03T13:59:39Z

In this PR, we are checking for dns updates every 30 mins. So in case of dns update, there is a period where egress policy may not behave as expected until it syncs these changes. Is this solution acceptable?

I was going to say "yeah, I think most sites use a TTL of at least an hour normally anyway", but poking around, it seems that that's not true, especially for CDN-ed sites.

I don't think syncing more aggressively is a good solution though (either by default or as a config option). But also there's no easy way to get TTL... So I'm not sure what to do.

knobunc · 2017-03-03T14:10:48Z

I think we need to get the TTL... why is that hard? Is it just that the library we are using can't provide it?

danwinship · 2017-03-03T14:09:58Z

pkg/sdn/plugin/egress_dns.go

+			cidrs = append(cidrs, net.IPNet{IP: ip, Mask: net.CIDRMask(32, 32)})
+		} else if ip.To16() != nil { // IPv6 addr
+			// Currently we only handle IPv4 addrs
+			glog.Warningf("ignoring IPv6 addr: %s (domain: %s) for policy: %s", ip.String(), dnsName, policy.Name)


This (and the "else" below it) is dead code now; dnsInfo.Get() can only return valid IPv4 addresses

danwinship · 2017-03-03T14:14:33Z

pkg/sdn/plugin/egress_network_policy.go

@@ -25,12 +26,15 @@ func (plugin *OsdnNode) SetupEgressNetworkPolicy() error {
 			continue
 		}
 		plugin.egressPolicies[vnid] = append(plugin.egressPolicies[vnid], policy)


You need to grab the lock here (SetupEgressNetworkPolicy()) too. multiTenantPlugin.Start() calls vnids.Start() before node.SetupEgressNetworkPolicy(), and vnids.Start() spawns a goroutine which could theoretically end up calling UpdateEgressNetworkPolicyVNID().

danwinship · 2017-03-03T14:15:54Z

pkg/sdn/plugin/egress_network_policy.go

+		}
+
+		for _, policy := range policies.Items {
+			dnsInfo, ok := plugin.egressDNS.pdMap[policy.UID]


this isn't thread-safe is it?

danwinship · 2017-03-03T14:16:21Z

pkg/sdn/plugin/node.go

@@ -60,7 +61,11 @@ type OsdnNode struct {
 	kubeletInitReady   chan struct{}
 	iptablesSyncPeriod time.Duration
 	mtu                uint32
+
+	// Synchronizes operations on egressPolicies
+	egressPoliciesLock sync.Mutex


(it would be nice to see this as a separate commit since it's fixing a separate pre-existing bug)

danwinship · 2017-03-03T14:17:54Z

pkg/sdn/plugin/proxy.go

+
+				proxy.updateNetworkPolicy(policy)
+				if proxy.allEndpoints != nil {
+					proxy.updateEndpoints()


you want to do the updateEndpoints outside the range policies.Items loop

danwinship · 2017-03-03T14:19:26Z

I think we need to get the TTL... why is that hard? Is it just that the library we are using can't provide it?

libc doesn't provide it, so in order to get it you need to write your own resolver basically

knobunc · 2017-03-03T14:39:36Z

I know calling the normal library functions doesn't return it... but is there no dns resolver library for golang already?

danwinship · 2017-03-03T16:13:53Z

There isn't even a good way to do it in C. Among other things, if you want to do it right, you need to parse /etc/nsswitch.conf and make sure you let /etc/hosts override DNS where it's supposed to, etc.

pravisankar · 2017-03-14T00:54:31Z

DNS updates are now based on domain TTL
@openshift/networking PTAL

danwinship · 2017-03-23T20:31:34Z

There isn't even a good way to do it in C. Among other things, if you want to do it right, you need to parse /etc/nsswitch.conf and make sure you let /etc/hosts override DNS where it's supposed to, etc.

Just so that all the discussion is here in the PR: we decided that this doesn't really matter for our use case; this is primarily designed for dealing with site-external hostnames, so stuff like /etc/hosts and zeroconf names don't matter anyway.

knobunc

LGTM. Nice work Ravi.

danwinship · 2017-03-23T20:56:26Z

pkg/sdn/plugin/dns.go

+
+func CheckDNSResolver() error {
+	if _, err := exec.LookPath(DIG); err != nil {
+		return fmt.Errorf("%s is not installed", DIG)


Did you investigate if there are go resolver libraries we could use instead? Given that dig is the sort of thing that isn't just installed-by-default, I wonder if that would be better.

There are some go resolver libraries for DNS but I didn't find a go library that actually returns TTL value for a domain.

If we just log an error instead of returning the error here then this feature will not work and I worry that the cluster admin may not notice this issue until someone tries this feature. I thought, throwing user error here will alert the admin to install the missing library.

danwinship · 2017-03-23T21:00:59Z

pkg/sdn/plugin/dns.go

+	// Cached/Non-authoritative DNS servers only holds the remaining TTL for it to refresh but not the actual absolute TTL for the domain.
+	// Authoritative DNS servers holds actual TTL for the domain. So we need to:
+	// (a) Get all authorirative name servers for the given domain
+	// (b) Query TTL for the domain against one of the name servers from (a)


This seems like a not-really-necessary complication; yes, the first time you query a non-authoritative DNS server it might give you a lower-than-maximum TTL if it already has the name cached, but once that expires, you'll get the max TTL next time and every time after that. So I think you can safely get rid of the get-authoritative-ns step.

We may not know exact TTL from non-auth server and may query few more times than required with this approach but yes, it definitely reduces the complexity.

danwinship · 2017-03-24T12:56:42Z

pkg/sdn/plugin/dns.go

+	}
+	nameServer := strings.Trim(string(out[:]), "\n")
+
+	out, err = exec.Command("sh", []string{"-c", fmt.Sprintf("echo $(%s | awk '{print $2}')", digShellCmdGetTTL(dns, nameServer))}...).Output()


You should get both the TTL and the IPs from dig; right now there's no guarantee that the TTL you get here from the authoritative server actually goes with the IP you get in updateIPs() (which comes from the local server).

Also, the echo $() there is doing nothing isn't it? You should be able to say just fmt.Sprintf("%s | awk '{print $2}'"). (Though if we're going to get both IP and TTL it might end up making more sense to do the parsing in go after the fact rather than using awk here.)

Good point. Right now, current approach is fetch TTL once from auth server, Update IPs for every TTL from non-auth server and another routine that updates TTL periodically from auth servers.

We could simply this to fetch and update TTL and IP from non-auth server and perform the above step again based on last obtained TTL. In this case, we don't need to contact auth server and no need of another routine to update TTL.

danwinship · 2017-03-24T12:57:39Z

pkg/sdn/plugin/dns.go

+		if err != nil {
+			glog.Errorf("Failed to get TTL value from authoritative name server for domain: %q, err: %v, defaulting ttl=%s", dns, err, defaultTTL.String())
+		} else {
+			glog.Warningf("TTL value not set by authoritative name server for domain: %q, defaulting ttl=%s", dns, defaultTTL.String())


That's not possible is it? There's always a TTL field in the response...

Yes, I think this shouldn't happen. I will change this to:

if err != nil || len(out) == 0 { glog.Errorf("Failed to get TTL value from authoritative name server for domain: %q, err: %v, defaulting ttl=%s", dns, err, defaultTTL.String()) }

danwinship · 2017-03-24T14:35:20Z

pkg/sdn/plugin/egress_network_policy.go

@@ -13,6 +14,10 @@ import (
 )

 func (plugin *OsdnNode) SetupEgressNetworkPolicy() error {
+	if err := CheckDNSResolver(); err != nil {
+		return err


Do we really want openshift to fail entirely if dig isn't present? (Rather than just making EgressNetworkPolicy not work?)

(Likewise in proxy.go)

You should just remove this from this commit rather than adding it here and then removing it in a later commit. (Likewise in proxy.go)

danwinship · 2017-03-24T14:52:41Z

pkg/sdn/plugin/egress_network_policy.go

@@ -79,3 +91,57 @@ func (plugin *OsdnNode) UpdateEgressNetworkPolicyVNID(namespace string, oldVnid,
 		plugin.updateEgressNetworkPolicyRules(newVnid)
 	}
 }
+
+func (plugin *OsdnNode) syncEgressDNSPolicyRules() {


This is really complicated and confusing. You should make the EgressDNS code smarter and this code dumber. egress_network_policy.go shouldn't have to deal with the timers itself... EgressDNS should do that internally, and then only wake up egress_network_policy.go when something has actually changed. So this code would just be something like:

func (plugin *OsdnNode) syncEgressDNSPolicyRules() { for { policyNamespace <- plugin.egressDNS.changed if vnid, err := plugin.policy.GetVNID(policyNamespace); err == nil { plugin.updateEgressNetworkPolicyRules(vnid) } else { glog.Warningf("Could not find netid for namespace %q: %v", policyNamespace, err) } } }

(Likewise in proxy.go)

Okay, I will make proxy and egress network policy register for dns changes and the go routine in egress dns that updates TTL and IP will inform all the registered callers when there is an IP update.

danwinship · 2017-03-24T14:53:59Z

pkg/sdn/plugin/egress_dns.go

+)
+
+const (
+	defaultEgressDNSTTLSyncDuration = 8 * time.Hour


This gets defined here, but used from a different file in a different commit. I think you could just have the "syncEgressDNSTTL" goroutine here rather than in egress_network_policy.go though.

This will be removed with the new approach.

danwinship · 2017-03-24T14:57:39Z

images/node/Dockerfile

@@ -22,7 +22,7 @@ RUN curl -L -o /etc/yum.repos.d/origin-next-epel-7.repo https://copr.fedoraproje
    INSTALL_PKGS="libmnl libnetfilter_conntrack conntrack-tools openvswitch \
      libnfnetlink iptables iproute bridge-utils procps-ng ethtool socat openssl \
      binutils xz kmod-libs kmod sysvinit-tools device-mapper-libs dbus \
-      iscsi-initiator-utils" && \
+      iscsi-initiator-utils bind-utils" && \


Do we need similar changes anywhere else? dind images? RPM spec files?

dind already has bind-utils package and may be I should also add to origin.spec? I will check

pravisankar · 2017-03-28T01:36:35Z

@danwinship Updated, PTAL

pecameron · 2017-04-11T14:02:11Z

pkg/sdn/plugin/egress_dns.go

+		tm, ok := dnsInfo.GetMinQueryTime()
+		if !ok {
+			continue
+		}


How long can e.lock be held here? Can this loop indefinite?

No, this loop is not indefinite. e.lock is held till we scan the egress policy dns map once.

pravisankar · 2017-04-11T19:08:59Z

flakes #12072 #10773 #13271
re[test]

danwinship · 2017-04-12T17:19:12Z

LGTM

pravisankar · 2017-04-12T18:53:19Z

flaked again reopened #13318
[test]

dcbw

LGTM

knobunc · 2017-04-12T19:49:32Z

[merge]

pravisankar · 2017-04-12T21:28:01Z

Resolved merge conflicts
[test]

pravisankar · 2017-04-12T23:48:37Z

[ERROR] PID 12674: hack/lib/build/environment.sh:137: `return "${exitcode}"` exited with status 2.
[INFO] 		Stack Trace: 
[INFO] 		  1: hack/lib/build/environment.sh:137: `return "${exitcode}"`
[INFO] 		  2: hack/lib/build/environment.sh:217: os::build::environment::withsource
[INFO] 		  3: hack/env:36: os::build::environment::run
[INFO]   Exiting with code 2.
++ export status=FAILURE
++ status=FAILURE
+ set +o xtrace
########## FINISHED STAGE: FAILURE: RUN CHECK AND VERIFY TASKS ##########

https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_check/1050/consoleFull#181393306558b6e51eb7608a5981914356

failure not related, looks like a flake
re[test]

danwinship · 2017-04-13T14:18:25Z

failure not related, looks like a flake
re[ test ]

FYI (as the bot has already informed you), you can't just ask it to retest without a reason. If it actually is an infrastructure problem and there's no flake bug to link to, the easiest way around that is to rebase and repush.

But anyway, this wasn't a flake, it's just that the bot doesn't do a good job of highlighting the right things when the tests fail to compile:

pkg/sdn/plugin/ovscontroller_test.go:542: not enough arguments in call to oc.UpdateEgressNetworkPolicyRules
...

pravisankar · 2017-04-13T20:31:28Z

[test]

sghosh151 · 2017-04-14T12:36:41Z

Is this config supported on Atomic Host?

…

On Apr 14, 2017 03:45, "OpenShift Bot" ***@***.***> wrote: continuous-integration/openshift-jenkins/merge FAILURE ( https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/346/) (Base Commit: 7044e57 <7044e57> ) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#13002 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpw3CYrjMfMiOnDKJkCVGTWOc2Ql8svks5rvyQSgaJpZM4MEldx> .

dnsName or cidrSelector could be specified to EgressNetworkPolicy.

…tion. - Stores all IPv4 addrs, TTL and next dns update time for the domain - Fetches dns TTL and IP addrs using dig and provides accessor method to periodically check for TTL/IP updates.

- Maintains map of egress policy and its associated dns info. - Provides Add/Delete/Update/Sync operations on egress DNS.

- DNS entries in the policies are verfied for any IP updates based on TTL for the domain and egress policy rules are updated if needed.

- DNS entries in the policies are verfied for any IP updates based on TTL for the domain and proxy firewall rules are updated if needed.

…resolving a domain in egress network policy

We use 'dig' to fetch TTL and IP updates for the egress domain and 'dig' is part of bind-utils package.

openshift-bot · 2017-04-17T19:29:43Z

Evaluated for origin merge up to 7370c28

pravisankar · 2017-04-17T22:42:49Z

flake #13068
re[test]

openshift-bot · 2017-04-17T22:45:20Z

Evaluated for origin test up to 7370c28

openshift-bot · 2017-04-18T00:05:25Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/779/) (Base Commit: 16132e2)

openshift-bot · 2017-04-18T00:37:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/359/) (Base Commit: 16132e2) (Image: devenv-rhel7_6151)

danwinship suggested changes Feb 20, 2017

View reviewed changes

pravisankar force-pushed the dns-names-for-egress-filtering branch 2 times, most recently from 76e9235 to 999de59 Compare February 22, 2017 02:48

pravisankar force-pushed the dns-names-for-egress-filtering branch 2 times, most recently from 0db83df to c9de330 Compare February 22, 2017 11:21

pravisankar mentioned this pull request Feb 23, 2017

Allow domain names in egress network policy openshift/openshift-docs#3824

Merged

pravisankar force-pushed the dns-names-for-egress-filtering branch 2 times, most recently from 825a9cb to 841733f Compare February 24, 2017 02:40

pravisankar changed the title ~~WIP: Support DNS names for egress network policy~~ Support DNS names for egress network policy Feb 24, 2017

pravisankar force-pushed the dns-names-for-egress-filtering branch from 841733f to 9bbfe1e Compare February 27, 2017 22:31

danwinship suggested changes Mar 3, 2017

View reviewed changes

pravisankar force-pushed the dns-names-for-egress-filtering branch 4 times, most recently from c734029 to c291dc7 Compare March 14, 2017 00:51

pravisankar force-pushed the dns-names-for-egress-filtering branch from c291dc7 to a1e9bd0 Compare March 22, 2017 20:14

knobunc approved these changes Mar 24, 2017

View reviewed changes

danwinship suggested changes Mar 24, 2017

View reviewed changes

pravisankar force-pushed the dns-names-for-egress-filtering branch from a1e9bd0 to bd3b21e Compare March 28, 2017 01:32

pecameron reviewed Apr 11, 2017

View reviewed changes

dcbw approved these changes Apr 12, 2017

View reviewed changes

pravisankar force-pushed the dns-names-for-egress-filtering branch from caa8dc9 to 20b119a Compare April 12, 2017 21:26

pravisankar force-pushed the dns-names-for-egress-filtering branch from 20b119a to 1c12bcc Compare April 13, 2017 20:30

Ravi Sankar Penta added 10 commits April 17, 2017 12:27

Added 'DNSname' field to EgressNetworkPolicyPeer object

aa205e9

dnsName or cidrSelector could be specified to EgressNetworkPolicy.

Generic DNS helper that holds map of dns names and associated informa…

f6dee4f

…tion. - Stores all IPv4 addrs, TTL and next dns update time for the domain - Fetches dns TTL and IP addrs using dig and provides accessor method to periodically check for TTL/IP updates.

Test cases for generic DNS helper

3135fc2

DNS handler for egress network policy

e947fae

- Maintains map of egress policy and its associated dns info. - Provides Add/Delete/Update/Sync operations on egress DNS.

Update ovs rules for egress dns entries

2af8570

- DNS entries in the policies are verfied for any IP updates based on TTL for the domain and egress policy rules are updated if needed.

Fix existing egress bug: Synchronize operations on egress policies

1a00492

Update firewall rules for egress dns entries

85bb517

- DNS entries in the policies are verfied for any IP updates based on TTL for the domain and proxy firewall rules are updated if needed.

Drop all traffic and firewall rules if DNS resolver is not found for …

0b099ce

…resolving a domain in egress network policy

Add bind-utils package to node image and origin spec.

7370c28

We use 'dig' to fetch TTL and IP updates for the egress domain and 'dig' is part of bind-utils package.

Auto generated swagger spec/docs/protobuf/conversions

fca780d

pravisankar force-pushed the dns-names-for-egress-filtering branch from 1c12bcc to fca780d Compare April 17, 2017 19:28

openshift-bot merged commit cdc9f74 into openshift:master Apr 18, 2017

Support DNS names for egress network policy #13002

Support DNS names for egress network policy #13002

Conversation

pravisankar commented Feb 17, 2017

pravisankar commented Feb 17, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Feb 22, 2017

pravisankar commented Mar 1, 2017 • edited Loading

danwinship commented Mar 3, 2017

knobunc commented Mar 3, 2017

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

danwinship commented Mar 3, 2017

knobunc commented Mar 3, 2017

danwinship commented Mar 3, 2017

pravisankar commented Mar 14, 2017

danwinship commented Mar 23, 2017

knobunc left a 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

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

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

pravisankar commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Apr 11, 2017

danwinship commented Apr 12, 2017

pravisankar commented Apr 12, 2017 • edited Loading

dcbw left a comment

Choose a reason for hiding this comment

knobunc commented Apr 12, 2017

pravisankar commented Apr 12, 2017

pravisankar commented Apr 12, 2017 • edited Loading

danwinship commented Apr 13, 2017

pravisankar commented Apr 13, 2017

sghosh151 commented Apr 14, 2017 via email

openshift-bot commented Apr 17, 2017

pravisankar commented Apr 17, 2017

openshift-bot commented Apr 17, 2017

openshift-bot commented Apr 18, 2017

openshift-bot commented Apr 18, 2017 • edited Loading

pravisankar commented Mar 1, 2017 •

edited

Loading

pravisankar commented Apr 12, 2017 •

edited

Loading

pravisankar commented Apr 12, 2017 •

edited

Loading

openshift-bot commented Apr 18, 2017 •

edited

Loading