Metrics: Add server for Egress IP/firewall #358

martinkennelly · 2021-10-09T14:08:54Z

Add an insecure HTTP metrics server that serves
metrics on loopback port 29100.

Add metrics to count egress firewalls,
egress firewall rules and egress IPs count.

In order to validate the metrics:

Metric num_egress_ips:

Configure EgressIP: https://docs.openshift.com/container-platform/4.9/networking/openshift_sdn/assigning-egress-ips.html
Exec into SDN pod that is the current LE leader. You can see whos the leader in a config map in the SDN namespace.
curl localhost:29100/metrics | grep num_egress_ips
You should see an integer from step 3 which represents the total number of egress IPs. This should be the same as the amount you created in step 1.

Metric num_egress_firewall_rules:

Configure EgressFirewall: https://docs.openshift.com/container-platform/4.9/networking/openshift_sdn/configuring-egress-firewall.html
Exec into SDN pod that is the current LE leader. You can see whos the leader in a config map in the SDN namespace.
curl localhost:29100/metrics | grep num_egress_firewall_rules
You should see an integer from step 3 which represents the total number of EgressFirewall egress rules. This should be the same as the amount you created in step 1.

Metric num_egress_firewalls:

1. Configure X number EgressFirewall: https://docs.openshift.com/container-platform/4.9/networking/openshift_sdn/configuring-egress-firewall.html
Exec into SDN pod that is the current LE leader. You can see whos the leader in a config map in the SDN namespace.
curl localhost:29100/metrics | grep num_egress_firewalls
You should see an integer from step 3 which represents the total number of EgressFirewalls. This should be the same as the amount you created in step 1.

Signed-off-by: Martin Kennelly mkennell@redhat.com

martinkennelly · 2021-10-11T11:31:20Z

Forced pushed to group cache + mutex together in the same struct.

martinkennelly · 2021-10-11T17:38:24Z

pkg/network/master/metrics/server.go

+const (
+	shutdownTimeout    = time.Millisecond * 50
+	endpoint           = "/metrics"
+	defaultBindAddress = "127.0.0.1:29102"


I was weighing up whether to bind to loopback or not. I went with loopback because it's an insecure HTTP server and I want to protect this server against exposure by mistake. This server will be exposed via RBAC proxy.

Yes, loopback+rbac proxy is correct.
When this merges you also need to update dev-guide/host-port-registry.md in openshift/enhancements to reflect the fact that this port is now used by openshift-sdn-controller.

martinkennelly · 2021-10-12T16:02:59Z

/assign @danwinship
Feel free to reassign/unassign. I notice you are the most active on here..
No rush on review.

pkg/cmd/openshift-sdn-controller/network_controller.go

pkg/network/master/metrics/record.go

danwinship · 2021-10-12T20:38:36Z

pkg/network/master/metrics/server.go

+const (
+	shutdownTimeout    = time.Millisecond * 50
+	endpoint           = "/metrics"
+	defaultBindAddress = "127.0.0.1:29102"


Yes, loopback+rbac proxy is correct.
When this merges you also need to update dev-guide/host-port-registry.md in openshift/enhancements to reflect the fact that this port is now used by openshift-sdn-controller.

pkg/network/master/metrics/server.go

pkg/network/master/egressip.go

martinkennelly · 2021-10-13T10:39:07Z

@danwinship The first iteration was poor. It wasn't a good idea to have a metric for egress firewall per namespace. I got rid of that and just keep a total count for egress firewall rules.

If this merges, I will update the port number reservation in dev-guide/host-port-registry.md in repo openshift/enhancements.
Thank you for the informer education.

martinkennelly · 2021-10-14T11:00:36Z

Forced pushed to change default port from 29102 -> 29100 so as not to possibly conflict with ovn-k port.

martinkennelly · 2021-10-14T12:38:10Z

/retest

martinkennelly · 2021-11-01T10:47:13Z

/retest

martinkennelly · 2021-11-01T13:12:45Z

/retest
CI flaking. Submitted BZ for flake: https://bugzilla.redhat.com/show_bug.cgi?id=2019001

danwinship

/approve
general idea is good, needs a few tweaks, but then anyone can /lgtm it after that

pkg/network/master/metrics/register.go

pkg/network/master/egressnp.go

Add an insecure HTTP metrics server that serves metrics on loopback port 29100. Add metrics to count egress firewalls, egress firewall rules and egress IPs count. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

martinkennelly · 2021-11-03T11:46:10Z

@danwinship I made the changes you requested - ENP metric, renamed file, and unit tests.
During my testing, I decided to change the definition of num_egress_ips to count egress IPs assigned to nodes [1]. This is better than counting all because it lets us know how many egress IPs are currently active on a cluster. I updated the HELP field to inform consumers of this distinction.

I also noticed an error I made in the original code you reviewed. I made the mistake of checking if interface{} was nil for func parameter old in handleAddUpdate. I now check the event type to find out if its an add or update operation [2].

[1] https://github.com/openshift/sdn/pull/358/files#diff-cb8e1be9489f2925e33943b9051dded85e22f36cb5b9905ebeb807f820cb9627R700

[2] https://github.com/openshift/sdn/pull/358/files#diff-2153a20dc3cd4885ce75eac5c9e4a51f6343d6be8b0d154be9973c1e66bad0a0R30

martinkennelly · 2021-11-04T11:01:54Z

/retest
CI flakes. Added BZ for one of them.

alexanderConstantinescu

This is really an lgtm, I just had a small nit that I wanted to give you time to decide what to do about (and if I put a real lgtm, the bot will merge the PR). Let me know

alexanderConstantinescu · 2021-11-10T08:59:24Z

pkg/network/master/metrics/register.go

+var metricEgressIPCount = prometheus.NewGauge(prometheus.GaugeOpts{
+	Namespace: metricSDNNamespace,
+	Subsystem: metricSDNSubsystemController,
+	Name:      "num_egress_ips",


Very small nit (you can discard it and slightest hesitation): call it num_assigned_egress_ips as to not confuse it with the number of egress IP objects

Id love to change it to what you suggested but I want to keep this name consistent with OVN-K and that PR is merged. Its not worth it for me to rename it now.

Understood then :)

alexanderConstantinescu · 2021-11-10T10:05:35Z

/lgtm

openshift-ci · 2021-11-10T10:07:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderConstantinescu, danwinship, martinkennelly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2021

openshift-ci bot requested review from rcarrillocruz and tssurya October 9, 2021 14:09

martinkennelly force-pushed the add_metrics_server branch from 33177fe to 8be36b1 Compare October 11, 2021 08:40

martinkennelly changed the title ~~[WIP] Metrics: Add server for Egress IP/firewall~~ Metrics: Add server for Egress IP/firewall Oct 11, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2021

martinkennelly force-pushed the add_metrics_server branch from 8be36b1 to 8de163e Compare October 11, 2021 11:30

martinkennelly commented Oct 11, 2021

View reviewed changes

openshift-ci bot assigned danwinship Oct 12, 2021

danwinship reviewed Oct 12, 2021

View reviewed changes

martinkennelly force-pushed the add_metrics_server branch 2 times, most recently from 6f1cd9d to cfa0235 Compare October 13, 2021 10:29

martinkennelly force-pushed the add_metrics_server branch from cfa0235 to aae0a7b Compare October 14, 2021 10:58

martinkennelly mentioned this pull request Oct 14, 2021

Host port registry: Add loopback metrics port for SDN openshift/enhancements#932

Merged

danwinship approved these changes Nov 1, 2021

View reviewed changes

pkg/network/master/metrics/register.go Show resolved Hide resolved

pkg/network/master/egressnp.go Outdated Show resolved Hide resolved

pkg/network/master/egressnp.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2021

martinkennelly changed the title ~~Metrics: Add server for Egress IP/firewall~~ WIP: Metrics: Add server for Egress IP/firewall Nov 2, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2021

martinkennelly force-pushed the add_metrics_server branch 2 times, most recently from 6ae2f3b to 1bc06d7 Compare November 3, 2021 11:25

Metrics: Add server to expose egress metrics

e909174

Add an insecure HTTP metrics server that serves metrics on loopback port 29100. Add metrics to count egress firewalls, egress firewall rules and egress IPs count. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

martinkennelly force-pushed the add_metrics_server branch from 1bc06d7 to e909174 Compare November 3, 2021 11:37

martinkennelly changed the title ~~WIP: Metrics: Add server for Egress IP/firewall~~ Metrics: Add server for Egress IP/firewall Nov 3, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2021

alexanderConstantinescu reviewed Nov 10, 2021

View reviewed changes

openshift-ci bot assigned alexanderConstantinescu Nov 10, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2021

martinkennelly changed the title ~~Metrics: Add server for Egress IP/firewall~~ WIP: Metrics: Add server for Egress IP/firewall Nov 10, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021

openshift-merge-robot merged commit a22d08c into openshift:master Nov 10, 2021

martinkennelly changed the title ~~WIP: Metrics: Add server for Egress IP/firewall~~ Metrics: Add server for Egress IP/firewall Nov 10, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021

martinkennelly mentioned this pull request Dec 14, 2021

Bug 2039321: SDN: Expose controller metrics for collection openshift/cluster-network-operator#1250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics: Add server for Egress IP/firewall #358

Metrics: Add server for Egress IP/firewall #358

martinkennelly commented Oct 9, 2021 •

edited

Loading

martinkennelly commented Oct 11, 2021

martinkennelly Oct 11, 2021

danwinship Oct 12, 2021

martinkennelly commented Oct 12, 2021

danwinship Oct 12, 2021

martinkennelly commented Oct 13, 2021

martinkennelly commented Oct 14, 2021

martinkennelly commented Oct 14, 2021

martinkennelly commented Nov 1, 2021

martinkennelly commented Nov 1, 2021

danwinship left a comment

martinkennelly commented Nov 3, 2021

martinkennelly commented Nov 4, 2021

alexanderConstantinescu left a comment

alexanderConstantinescu Nov 10, 2021

martinkennelly Nov 10, 2021 •

edited

Loading

alexanderConstantinescu Nov 10, 2021

alexanderConstantinescu commented Nov 10, 2021

openshift-ci bot commented Nov 10, 2021

Metrics: Add server for Egress IP/firewall #358

Metrics: Add server for Egress IP/firewall #358

Conversation

martinkennelly commented Oct 9, 2021 • edited Loading

martinkennelly commented Oct 11, 2021

martinkennelly Oct 11, 2021

Choose a reason for hiding this comment

danwinship Oct 12, 2021

Choose a reason for hiding this comment

martinkennelly commented Oct 12, 2021

danwinship Oct 12, 2021

Choose a reason for hiding this comment

martinkennelly commented Oct 13, 2021

martinkennelly commented Oct 14, 2021

martinkennelly commented Oct 14, 2021

martinkennelly commented Nov 1, 2021

martinkennelly commented Nov 1, 2021

danwinship left a comment

Choose a reason for hiding this comment

martinkennelly commented Nov 3, 2021

martinkennelly commented Nov 4, 2021

alexanderConstantinescu left a comment

Choose a reason for hiding this comment

alexanderConstantinescu Nov 10, 2021

Choose a reason for hiding this comment

martinkennelly Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

alexanderConstantinescu Nov 10, 2021

Choose a reason for hiding this comment

alexanderConstantinescu commented Nov 10, 2021

openshift-ci bot commented Nov 10, 2021

martinkennelly commented Oct 9, 2021 •

edited

Loading

martinkennelly Nov 10, 2021 •

edited

Loading