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

Jump to bottom

Add support for RouteGroups #251

Merged
merged 2 commits into from
Sep 17, 2020
Merged

Add support for RouteGroups #251

merged 2 commits into from
Sep 17, 2020

Conversation

mikkeloscar
Copy link
Contributor

@mikkeloscar mikkeloscar commented Sep 14, 2020 •
edited
Loading

Adds support for RouteGroups as an alternative to ingress or externalIngress definitions.

Migration path

The intended migration path from Ingress -> RouteGroup or the other way is the following:

  1. User adds RouteGroup definition and keeps the existing Ingress in place. Proposed it to first add the new RouteGroup with a different hostname such that it won't overlap with the existing Ingress for the initial tests.
  2. Once Ingress and RouteGroup is the same the Ingress can be dropped from the StackSet spec.
  3. stackset-controller will delay the deletion of the Ingress if it detects that a RouteGroup is defined. This allows the underlying proxy (skipper) to configure the routes for RouteGroup before deleting the corresponding routes which were configured for the Ingress. - Default setting is to delete the Ingress no sooner than 5 minutes after the RouteGroup resource has been created. 5 minutes seems like a reasonable time where skipper has plenty of time to be configured AND not too long that it will mess with the routes if the user starts to add additional features via the new RouteGroup configuration.

Traffic weight rounding

Since RouteGroups expect the weight to be in int as opposed to float64 I introduced a rounding which affects the weights when they're calculated to be assigned on the StackSet spec/status thus also changing the behavior slightly for how weights are calculated for ingress or externalIngress. Basically the weights are still stored in float64, but they are rounded to whole numbers still summing up to 100. See the roundWeights function in pkg/core/traffic.go for details.

This can be the first step for eventually getting rid of floats and just use integers for weights. Related #214

WORK IN PROGRESS

  • Make RouteGroup support optional by default
  • Gracefully delete ingress if replaced with RouteGroup and vice versa.
  • Add Unit tests for RouteGroup
  • Add e2e tests for RouteGroup
  • Fix traffic distribution (float64 -> int)

@mikkeloscar mikkeloscar force-pushed the route-group-support branch 17 times, most recently from c47ca35 to 538237d Compare September 16, 2020 13:35
@coveralls
Copy link

coveralls commented Sep 16, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 1869

  • 230 of 314 (73.25%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 70.688%
Changes Missing Coverage Covered Lines Changed/Added Lines %
controller/test_helpers.go 10 12 83.33%
pkg/core/stackset.go 50 56 89.29%
pkg/core/types.go 13 20 65.0%
controller/stack_resources.go 40 48 83.33%
controller/stackset.go 81 106 76.42%
pkg/core/stack_resources.go 0 36 0.0%
Files with Coverage Reduction New Missed Lines %
controller/stackset.go 1 47.11%
Totals Coverage Status
Change from base Build 1864: 0.2%
Covered Lines: 1828
Relevant Lines: 2586
💛 - Coveralls

@mikkeloscar mikkeloscar changed the title [WIP] Add support for RouteGroups Add support for RouteGroups Sep 16, 2020
aryszka
aryszka approved these changes Sep 17, 2020
View reviewed changes
pkg/core/traffic.go Outdated Show resolved Hide resolved
pkg/core/traffic.go Outdated Show resolved Hide resolved
controller/stackset.go Outdated Show resolved Hide resolved
controller/stack_resources.go Outdated Show resolved Hide resolved
@aryszka
Copy link
Contributor

aryszka commented Sep 17, 2020

👍

@aryszka
Copy link
Contributor

aryszka commented Sep 17, 2020

👍

@mikkeloscar
Add support for RouteGroups
5956316 
Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>
@aryszka
Copy link
Contributor

aryszka commented Sep 17, 2020

👍

@mikkeloscar
Require minimum 1 route for RouteGroup
197b7ed 
Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>
@mikkeloscar
Copy link
Contributor Author

👍

@aryszka
Copy link
Contributor

aryszka commented Sep 17, 2020

👍

@mikkeloscar mikkeloscar merged commit e72232e into master Sep 17, 2020
@mikkeloscar mikkeloscar deleted the route-group-support branch September 17, 2020 14:15
@aryszka
Copy link
Contributor

aryszka commented Sep 17, 2020

Some low hanging fruits to improve the coverage:

  • the error cases in StackSetController.collectResources()
  • the normalizWeight edge cases in traffic.go

@muaazsaleem muaazsaleem mentioned this pull request Nov 3, 2020
Open
jonathanbeber pushed a commit that referenced this pull request Nov 17, 2020
@jonathanbeber
Add and check UpdateTimestamp
b487791 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the `IngressSourceSwitchTTL` duration as
expected. During the 3rd step, the `CreationTimestamp` attribute of the
new resource tends to be greater than the `IngressSourceSwitchTTL`
configuration, due to the time spent in the 2nd step. That leads to the
almost instantaneous deletion of the old resource, without considering
the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
jonathanbeber pushed a commit that referenced this pull request Nov 18, 2020
@jonathanbeber
Add and check UpdateTimestamp
5292825 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the [`IngressSourceSwitchTTL`][1]
duration as expected. During the 3rd step, the `CreationTimestamp`
attribute of the new resource tends to be greater than the
`IngressSourceSwitchTTL` configuration, due to the time spent in the 2nd
step. That leads to the almost instantaneous deletion of the old
resource, without considering the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

Also, it adds a new e2e test file. This new test create the above
mentioned migration process and verifies that the controller delete the
old resources after the `IngressSourceSwitchTTL` duration.

As a minor change, this commit also updates the `run_e2e.sh` script,
allowing one to define the environment variable `TEST_NAME`. It will set
the maximum parallelism to 1 and run the given test name.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
jonathanbeber pushed a commit that referenced this pull request Nov 19, 2020
@jonathanbeber
Add and check UpdateTimestamp
97bc9d5 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the [`IngressSourceSwitchTTL`][1]
duration as expected. During the 3rd step, the `CreationTimestamp`
attribute of the new resource tends to be greater than the
`IngressSourceSwitchTTL` configuration, due to the time spent in the 2nd
step. That leads to the almost instantaneous deletion of the old
resource, without considering the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

Also, it adds a new e2e test file. This new test create the above
mentioned migration process and verifies that the controller delete the
old resources after the `IngressSourceSwitchTTL` duration.

As a minor change, this commit also updates the `run_e2e.sh` script,
allowing one to define the environment variable `TEST_NAME`. It will set
the maximum parallelism to 1 and run the given test name.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
jonathanbeber pushed a commit that referenced this pull request Nov 20, 2020
@jonathanbeber
Add and check UpdateTimestamp
5184e3d 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the [`IngressSourceSwitchTTL`][1]
duration as expected. During the 3rd step, the `CreationTimestamp`
attribute of the new resource tends to be greater than the
`IngressSourceSwitchTTL` configuration, due to the time spent in the 2nd
step. That leads to the almost instantaneous deletion of the old
resource, without considering the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

Also, it adds a new e2e test file. This new test create the above
mentioned migration process and verifies that the controller delete the
old resources after the `IngressSourceSwitchTTL` duration.

As a minor change, this commit also updates the `run_e2e.sh` script,
allowing one to define the environment variable `TEST_NAME`. It will set
the maximum parallelism to 1 and run the given test name.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
jonathanbeber pushed a commit that referenced this pull request Nov 20, 2020
@jonathanbeber
Add and check UpdateTimestamp
6682a6a 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the [`IngressSourceSwitchTTL`][1]
duration as expected. During the 3rd step, the `CreationTimestamp`
attribute of the new resource tends to be greater than the
`IngressSourceSwitchTTL` configuration, due to the time spent in the 2nd
step. That leads to the almost instantaneous deletion of the old
resource, without considering the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

Also, it adds a new e2e test file. This new test create the above
mentioned migration process and verifies that the controller delete the
old resources after the `IngressSourceSwitchTTL` duration.

As a minor change, this commit also updates the `run_e2e.sh` script,
allowing one to define the environment variable `TEST_NAME`. It will set
the maximum parallelism to 1 and run the given test name.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
mikkeloscar pushed a commit that referenced this pull request Nov 20, 2020
@muaazsaleem
Add and check UpdateTimestamp (#268)
e26f75c 
The HTTP resources (Ingresses and RouteGroups) migration, introduced on
the [#251][0], checks for the `CreationTimestamp` attribute of the
resources. When the migration follows the path of:

1. adding a new resource (does not matter if it's an Ingress or
   RouteGroup) with a new hostname for migration and testing purposes;
2. validating the configuration of the new resource.
3. dropping the old resource in favor of the new one. For that the new
   resource receives the actual hostname.

the controller does not respect the [`IngressSourceSwitchTTL`][1]
duration as expected. During the 3rd step, the `CreationTimestamp`
attribute of the new resource tends to be greater than the
`IngressSourceSwitchTTL` configuration, due to the time spent in the 2nd
step. That leads to the almost instantaneous deletion of the old
resource, without considering the configured TTL for routes propagation.

This commit introduces a new annotation to the above-mentioned
resources: `stackset-controller.zalando.org/updated-timestamp`. This
annotation is responsible for tracking the last time the controller
updated a resource. Also, it changes the migration process introduced
on the [#251][0] to compare `IngressSourceSwitchTTL` with the newly
added annotation instead of the `CreationTimestamp` attribute. This way,
the change of hostname on the new resource, during the 3rd step, resets
the timer to delete the old resource and guarantees route propagation.

Also, it adds a new e2e test file. This new test create the above
mentioned migration process and verifies that the controller delete the
old resources after the `IngressSourceSwitchTTL` duration.

As a minor change, this commit also updates the `run_e2e.sh` script,
allowing one to define the environment variable `TEST_NAME`. It will set
the maximum parallelism to 1 and run the given test name.

[0]: #251
[1]: https://github.com/zalando-incubator/stackset-controller/blob/e72232e799e5ff8ba5b51d29dc5ddff153907162/cmd/stackset-controller/main.go#L42

Co-authored-by: Muaaz Saleem <muhammad.muaaz.saleem@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aryszka aryszka aryszka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mikkeloscar @coveralls @aryszka