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.

Sign up for GitHub

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

Pass port into autoscaler url from webhook policy #1765

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Pass port into autoscaler url from webhook policy #1765

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51eca79
Pass port into autoscaler url from webhook policy
andrewgrundy Aug 19, 2020
076a137
Adding port description to FleetAutoscaler documentation
andrewgrundy Aug 20, 2020
c741dfd
Adding a feature flag around port description in the FleetAutoscaler …
andrewgrundy Aug 26, 2020
f28bbe8
Merge branch 'master' into fix/autoscale-webhook-port
markmandel Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions pkg/fleetautoscalers/fleetautoscalers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,19 @@ func buildURLFromWebhookPolicy(w *autoscalingv1.WebhookPolicy) (u *url.URL, err
w.Service.Namespace = "default"
}

return createURL(scheme, w.Service.Name, w.Service.Namespace, *w.Service.Path), nil
return createURL(scheme, w.Service.Name, w.Service.Namespace, *w.Service.Path, w.Service.Port), nil
}

// moved to a separate method to cover it with unit tests and check that URL corresponds to a proper pattern
func createURL(scheme, name, namespace, path string) *url.URL {
func createURL(scheme, name, namespace, path string, port *int32) *url.URL {
var hostPort int32 = 8000
markmandel marked this conversation as resolved.
Show resolved Hide resolved
if port != nil {
hostPort = *port
}

return &url.URL{
Scheme: scheme,
Host: fmt.Sprintf("%s.%s.svc:8000", name, namespace),
Host: fmt.Sprintf("%s.%s.svc:%d", name, namespace, hostPort),
Path: path,
}
}
Expand Down
13 changes: 12 additions & 1 deletion pkg/fleetautoscalers/fleetautoscalers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,13 +578,15 @@ func TestApplyWebhookPolicyNilFleet(t *testing.T) {

func TestCreateURL(t *testing.T) {
t.Parallel()
var nonStandardPort int32 = 8888

var testCases = []struct {
description string
scheme string
name string
namespace string
path string
port *int32
expected string
}{
{
Expand All @@ -611,11 +613,20 @@ func TestCreateURL(t *testing.T) {
path: "",
expected: "http://service1.default.svc:8000",
},
{
description: "OK, port specified",
scheme: "http",
name: "service1",
namespace: "default",
path: "scale",
port: &nonStandardPort,
expected: "http://service1.default.svc:8888/scale",
},
}

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
res := createURL(tc.scheme, tc.name, tc.namespace, tc.path)
res := createURL(tc.scheme, tc.name, tc.namespace, tc.path, tc.port)

if assert.NotNil(t, res) {
assert.Equal(t, tc.expected, res.String())
Expand Down
1 change: 1 addition & 0 deletions site/content/en/docs/Reference/fleetautoscaler.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ The `spec` field is the actual `FleetAutoscaler` specification and it is compose
- `namespace` is the kubernetes namespace where webhook is deployed. Optional
If not specified, the "default" would be used
- `path` is an optional URL path which will be sent in any request to this service. (i. e. /scale)
- `port` is optional, it is the port for the service which is hosting the webhook. The default is 8000 for backward compatibility. If given, it should be a valid port number (1-65535, inclusive).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the website should be guarded so that they get exposed on the next release.

See https://agones.dev/site/docs/contribute/#within-a-page

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically this way:

Suggested change
- `port` is optional, it is the port for the service which is hosting the webhook. The default is 8000 for backward compatibility. If given, it should be a valid port number (1-65535, inclusive).
{{% feature publishVersion="1.9.0" %}}
- `port` is optional, it is the port for the service which is hosting the webhook. The default is 8000 for backward compatibility. If given, it should be a valid port number (1-65535, inclusive).
{{% /feature %}}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this might be the last thing before we can merge.

(Confirmed with @thisisnotapril that we can manually approve CLA) @andrewgrundy, you good to make the change?

- `url` gives the location of the webhook, in standard URL form (`[scheme://]host:port/path`). Exactly one of `url` or `service` must be specified. The `host` should not refer to a service running in the cluster; use the `service` field instead. (optional, instead of service)
- `caBundle` is a PEM encoded certificate authority bundle which is used to issue and then validate the webhook's server certificate. Base64 encoded PEM string. Required only for HTTPS. If not present HTTP client would be used.

Expand Down