Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

Add AWS validator for core resources #438

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

timuthy
Copy link
Contributor

@timuthy timuthy commented Nov 14, 2019

What this PR does / why we need it:
This PR adds a validating webhook that is responsible for checking resources of the group core.gardener.cloud (at the moment only shoot resources).

With GEP-4 cloud specific information was externalized and as a consequence cannot be validated by the Gardener anymore. On the other hand, it should be prevented that invalid shoot objects are created or updated.

Example:

apiVersion: core.gardener.cloud/v1alpha1
kind: Shoot
...
spec:
...
  provider:
    type: aws
    infrastructureConfig:
      apiVersion: aws.provider.extensions.gardener.cloud/v1alpha1
      kind: InfrastructureConfig
      networks:
        vpc:
          cidr: 10.251.0.0/16
        zones:
        - internal: 10.251.112.0/22
          name: eu-west-1a
          public: 10.251.96.0/22
          workers: 10.251.0.0/19
    workers:
      ...
      zones:
      - eu-west-1c

The above example is supposed to be denied because a worker group refers to an availability zone (eu-west-1c) which is not part of the InfrastructureConfig.

Since the gardener-extensions repository is already a home for cloud specific logic, this PR adds cloud specific validations for core resources to this repo.

Part of #407

Special notes for your reviewer:
Validations for other cloud providers will follow.

Release note:

The `gardener-extensions` now offers a validating webhook which checks shoot resources of type `aws` in the recently introduced `core.gardener.cloud` group. Operators should register this webhook in the Garden cluster to further prevent invalid modifications on `aws` shoots.

@timuthy timuthy requested a review from a team as a code owner November 14, 2019 12:39
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 14, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 14, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 14, 2019
@timuthy timuthy added the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Nov 14, 2019
@timuthy
Copy link
Contributor Author

timuthy commented Nov 14, 2019

Please do not merge since I'm also changing the ValidateInfrastructureConfigUpdate to allow zones to be added.
See gardener/gardener#1587
/cc @zanetworker

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 15, 2019
@timuthy
Copy link
Contributor Author

timuthy commented Nov 15, 2019

The validation logic which allows to add availability zones has been added.

I dropped the restriction that the zone order must be retained because in the new API group you cannot mix the networks and the names anymore. Furthermore I noticed that in the old API it's possible to configure the exact same zone multiple times with different networks which doesn't make any sense, right @zanetworker @rfranzke ?

Makefile Outdated Show resolved Hide resolved
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 20, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 20, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 20, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 20, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 20, 2019
@timuthy timuthy removed the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Nov 20, 2019
@timuthy
Copy link
Contributor Author

timuthy commented Nov 20, 2019

@rfranzke, @zanetworker, @tim-ebert thanks for your reviews.

I addressed your comments, PTAL.

Copy link
Contributor

@zanetworker zanetworker left a comment

Choose a reason for hiding this comment

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

Looks good :) just a couple of small comments.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 21, 2019
zanetworker
zanetworker previously approved these changes Nov 21, 2019
@ialidzhikov
Copy link
Contributor

Is it delegated to g/garden-setup to install the validator charts?

@rfranzke
Copy link
Contributor

The responsible party that creates the ControllerRegistration for the extension controller will also do deploy the extension validator charts.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 27, 2019
@timuthy
Copy link
Contributor Author

timuthy commented Nov 27, 2019

@rfranzke could you please have another look?

rfranzke
rfranzke previously approved these changes Nov 27, 2019
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm (just a few minor suggestions :))

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 27, 2019
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm, nice work!

@rfranzke rfranzke merged commit e64c2e7 into gardener-attic:master Nov 27, 2019
@rfranzke rfranzke mentioned this pull request Jan 20, 2020
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants