Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --publish-status-address to 2.x #1509

Merged
merged 3 commits into from
Jul 7, 2021
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 7, 2021

What this PR does / why we need it:
This was originally added to #1451, but later removed. I thought that removal had been reverted, but it wasn't, and I got confused in the PR history and missed that before it went in.

Per #1305 we do want to include this in 2.x for 1.x flag parity. The flag is useful when IP information is missing from the publish service (e.g. bare metal clusters) or does not reflect the true external address of the proxy (e.g. when you chain proxies and have something else in front of Kong).

Lastly, this updates --publish-status-address to use a StringSlice, following the same logic as #1496. Note that 1.x --publish-status-address originally supported only a single IP, but we may as well allow multiple, as it's compatible.

Which issue this PR fixes:
fixes #1305 in conjunction with #1451

Special notes for your reviewer:

The reverted commit also included an additional new feature for deriving the Kong admin API URL from its Service, instead of providing it directly in --kong-admin-url. Per my comments in #1486 (which also includes the same feature) this isn't ready for merge yet, as it breaks some existing functionality (namely HTTPS). The admin API doesn't factor into status information at all, and it should be raised in a separate PR if we wish to include it.

Edit: this actually was already reverted... somewhere in history. It's in the reverted commit, but not in the diff here, so the above can be ignored for the context here.

This updates the changelog to reflect the 2.0.0-alpha.2 release. We forgot to do that for the actual release.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest requested a review from a team as a code owner July 7, 2021 19:25
@rainest rainest temporarily deployed to Configure ci July 7, 2021 19:25 Inactive
@rainest rainest changed the base branch from main to next July 7, 2021 19:25
@github-actions
Copy link

github-actions bot commented Jul 7, 2021

Licenses differ between commit 3ef1829afcc6b6ba47a72d52f50559cfb976f9b7 and base:

+++ pr_licenses.csv	2021-07-07 19:26:42.204339053 +0000
@@ -1,3 +1,8 @@
+github.com/Masterminds/goutils,https://github.com/Masterminds/goutils/blob/master/LICENSE.txt,Apache-2.0
+github.com/Masterminds/semver,https://github.com/Masterminds/semver/blob/master/LICENSE.txt,MIT
+github.com/Masterminds/sprig,https://github.com/Masterminds/sprig/blob/master/LICENSE.txt,MIT
+github.com/alecthomas/template,https://github.com/alecthomas/template/blob/master/LICENSE,BSD-3-Clause
+github.com/alecthomas/units,https://github.com/alecthomas/units/blob/master/COPYING,MIT
 github.com/beorn7/perks/quantile,https://github.com/beorn7/perks/blob/master/quantile/LICENSE,MIT
 github.com/blang/semver/v4,https://github.com/blang/semver/blob/master/v4/LICENSE,MIT
 github.com/bombsimon/logrusr,https://github.com/bombsimon/logrusr/blob/master/LICENCE,MIT
@@ -7,6 +12,7 @@
 github.com/eapache/channels,https://github.com/eapache/channels/blob/master/LICENSE,MIT
 github.com/eapache/queue,https://github.com/eapache/queue/blob/master/LICENSE,MIT
 github.com/evanphx/json-patch,https://github.com/evanphx/json-patch/blob/master/LICENSE,BSD-3-Clause
+github.com/evanphx/json-patch/v5,https://github.com/evanphx/json-patch/blob/master/v5/LICENSE,BSD-3-Clause
 github.com/fatih/color,https://github.com/fatih/color/blob/master/LICENSE.md,MIT
 github.com/fsnotify/fsnotify,https://github.com/fsnotify/fsnotify/blob/master/LICENSE,BSD-3-Clause
 github.com/ghodss/yaml,https://github.com/ghodss/yaml/blob/master/LICENSE,MIT
@@ -25,16 +31,19 @@
 github.com/hashicorp/golang-lru,https://github.com/hashicorp/golang-lru/blob/master/LICENSE,MPL-2.0
 github.com/hashicorp/hcl,https://github.com/hashicorp/hcl/blob/master/LICENSE,MPL-2.0
 github.com/hexops/gotextdiff,https://github.com/hexops/gotextdiff/blob/master/LICENSE,BSD-3-Clause
+github.com/huandu/xstrings,https://github.com/huandu/xstrings/blob/master/LICENSE,MIT
 github.com/imdario/mergo,https://github.com/imdario/mergo/blob/master/LICENSE,BSD-3-Clause
 github.com/json-iterator/go,https://github.com/json-iterator/go/blob/master/LICENSE,MIT
 github.com/magiconair/properties,https://github.com/magiconair/properties/blob/master/LICENSE.md,BSD-2-Clause
 github.com/mattn/go-colorable,https://github.com/mattn/go-colorable/blob/master/LICENSE,MIT
 github.com/mattn/go-isatty,https://github.com/mattn/go-isatty/blob/master/LICENSE,MIT
 github.com/matttproud/golang_protobuf_extensions/pbutil,https://github.com/matttproud/golang_protobuf_extensions/blob/master/pbutil/LICENSE,Apache-2.0
+github.com/mitchellh/copystructure,https://github.com/mitchellh/copystructure/blob/master/LICENSE,MIT
 github.com/mitchellh/mapstructure,https://github.com/mitchellh/mapstructure/blob/master/LICENSE,MIT
+github.com/mitchellh/reflectwalk,https://github.com/mitchellh/reflectwalk/blob/master/LICENSE,MIT
 github.com/modern-go/concurrent,https://github.com/modern-go/concurrent/blob/master/LICENSE,Apache-2.0
 github.com/modern-go/reflect2,https://github.com/modern-go/reflect2/blob/master/LICENSE,Apache-2.0
-github.com/pelletier/go-toml,https://github.com/pelletier/go-toml/blob/master/LICENSE,MIT
+github.com/pelletier/go-toml,https://github.com/pelletier/go-toml/blob/master/LICENSE,Apache-2.0
 github.com/pkg/errors,https://github.com/pkg/errors/blob/master/LICENSE,BSD-2-Clause
 github.com/prometheus/client_golang/prometheus,https://github.com/prometheus/client_golang/blob/master/prometheus/LICENSE,Apache-2.0
 github.com/prometheus/client_model/go,https://github.com/prometheus/client_model/blob/master/go/LICENSE,Apache-2.0
@@ -64,7 +73,7 @@
 go.uber.org/zap,Unknown,MIT
 gomodules.xyz/jsonpatch/v2,Unknown,Apache-2.0
 google.golang.org/protobuf,Unknown,BSD-3-Clause
-gopkg.in/evanphx/json-patch.v4,Unknown,BSD-3-Clause
+gopkg.in/alecthomas/kingpin.v2,Unknown,MIT
 gopkg.in/go-playground/pool.v3,Unknown,MIT
 gopkg.in/inf.v0,Unknown,BSD-3-Clause
 gopkg.in/ini.v1,Unknown,Apache-2.0
@@ -82,5 +91,5 @@
 knative.dev/networking/pkg,Unknown,Apache-2.0
 knative.dev/pkg,Unknown,Apache-2.0
 sigs.k8s.io/controller-runtime,Unknown,Apache-2.0
-sigs.k8s.io/structured-merge-diff/v4/value,Unknown,Apache-2.0
+sigs.k8s.io/structured-merge-diff/v4,Unknown,Apache-2.0
 sigs.k8s.io/yaml,Unknown,MIT```

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #1509 (cb4f256) into next (ce45ce0) will decrease coverage by 0.16%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1509      +/-   ##
==========================================
- Coverage   51.52%   51.36%   -0.17%     
==========================================
  Files          91       91              
  Lines        6292     6300       +8     
==========================================
- Hits         3242     3236       -6     
- Misses       2757     2770      +13     
- Partials      293      294       +1     
Flag Coverage Δ
integration-test 48.27% <70.00%> (-0.89%) ⬇️
unit-test 38.87% <30.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
railgun/internal/ctrlutils/ingress-status.go 61.63% <50.00%> (-3.57%) ⬇️
railgun/manager/run.go 70.52% <100.00%> (ø)
railgun/pkg/config/config.go 92.94% <100.00%> (+0.25%) ⬆️
railgun/internal/diagnostics/server.go 57.57% <0.00%> (-21.22%) ⬇️
...n/internal/proxy/clientgo_cached_proxy_resolver.go 74.03% <0.00%> (-1.93%) ⬇️
...trollers/configuration/zz_generated_controllers.go 47.83% <0.00%> (ø)
pkg/parser/parser.go 84.51% <0.00%> (+1.25%) ⬆️
railgun/internal/ctrlutils/utils.go 86.27% <0.00%> (+3.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce45ce0...cb4f256. Read the comment docs.

@rainest rainest temporarily deployed to Configure ci July 7, 2021 19:29 Inactive
Travis Raines added 3 commits July 7, 2021 14:29
This reverts commit 2fae8d2.

Restore the --publish-status-address flag and associated logic in the
status updater. This flag takes a user-provided IP address and uses it
instead of attempting to determine an address from the proxy Service,
for cases where that information is not available or does not reflect
the effective external address of the proxy.
- Finalize the 2.0.0-alpha.2 date.
- Create the 2.0.0-alpha.3 section.
- Add status features to 2.0.0-alpha.3.
@rainest rainest force-pushed the feat/rg-publish-status-address branch from b2e67f5 to cb4f256 Compare July 7, 2021 19:30
@rainest rainest temporarily deployed to Configure ci July 7, 2021 19:30 Inactive
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
railgun/internal/ctrlutils/ingress-status.go Show resolved Hide resolved
@ccfishk
Copy link
Contributor

ccfishk commented Jul 7, 2021

Travis, thanks for taking care of this and clarifying that PublishStatusAddress can be read when a valid proxy public IP is not available . 💯

Questions:

  1. can controller use PublishStatusAddress (the ip) communicate to kong proxy (whose ip is missed) for configuration update ? Or, is a separate route/ingress IP ?

  2. which module (I don't think is Kong Admin API, is loaderbalancer/or other network resource) is associated to the PublishStatusAddress ? can we retrieve the address instead of hardcode parameter ?

thanks

@rainest rainest merged commit 54cdf57 into next Jul 7, 2021
@rainest rainest deleted the feat/rg-publish-status-address branch July 7, 2021 22:27
@rainest
Copy link
Contributor Author

rainest commented Jul 7, 2021

can controller use PublishStatusAddress (the ip) communicate to kong proxy (whose ip is missed) for configuration update ? Or, is a separate route/ingress IP ?

No, it's the proxy IP, not the admin API IP. In either case, sending something through an external address when an internal path exists is usually not desired: it likely traverses a path with less bandwidth/more latency and may result in ingress/egress charges from a cloud provider.

which module (I don't think is Kong Admin API, is loaderbalancer/or other network resource) is associated to the PublishStatusAddress ? can we retrieve the address instead of hardcode parameter ?

It's an unknown network endpoint that the user promises will ultimately route to the proxy. This flag is intended specifically for cases where you cannot retrieve the address(es) from Kubernetes, e.g. an independent load balancer that points to the proxy's NodePort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KIC 2.0: handle publish-service and publish-status-address flags
3 participants