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

Deserialize custom resources with EnableStrict #253

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

stoyanr
Copy link
Contributor

@stoyanr stoyanr commented Mar 31, 2021

How to categorize this PR?

/area control-plane
/kind enhancement
/priority 3
/platform openstack

What this PR does / why we need it:

  • Adds the EnableStrict option to all places where a codec factory is created, to ensure that extension resource configs are always deserialized in "strict" mode. This means that resources with fields that are not allowed by the API schema will be rejected by validation and will cause errors (categorized as ERR_CONFIGURATION_PROBLEM) during reconciliation as well.
  • Vendors g/g 1.20.0 (includes Deserialize custom resources with EnableStrict gardener#3804) to ensure that a strict decoder is used also for ControlPlaneConfig and WorkerConfig, and errors are correctly categorized.

Which issue(s) this PR fixes:
Fixes gardener/gardener-extension-provider-azure#270, see also gardener/gardener-extension-provider-azure#271 and gardener/gardener-extension-provider-gcp#249.

Special notes for your reviewer:

Release note:

Extension resource configs (`InfrastructureConfig`, `ControlPlaneConfigs`, `WorkerConfig`) are now deserialized in "strict" mode, including during validation by the admission webhook. This means that resources with fields that are not allowed by the API schema will be rejected by validation. Creating new shoots containing such resources will not be possible, and reconciling existing shoots will fail with an appropriate error until you manually update the shoot to make sure any extension resource configs contained in it are valid.

@stoyanr stoyanr requested review from a team as code owners March 31, 2021 12:13
@gardener-robot gardener-robot added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure priority/3 Priority (lower number equals higher priority) needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Mar 31, 2021
@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) 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 Mar 31, 2021
@stoyanr stoyanr marked this pull request as draft March 31, 2021 12:15
@stoyanr stoyanr force-pushed the enable-strict-deserializer branch from 835c315 to a8a7830 Compare April 1, 2021 10:53
@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Apr 1, 2021
@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 Apr 1, 2021
@gardener-robot gardener-robot removed the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Apr 1, 2021
@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 Apr 1, 2021
@stoyanr stoyanr force-pushed the enable-strict-deserializer branch from a8a7830 to aabb46e Compare April 1, 2021 12:48
@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 Apr 1, 2021
@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 Apr 1, 2021
@stoyanr stoyanr marked this pull request as ready for review April 1, 2021 12:53
@rfranzke
Copy link
Member

rfranzke commented Apr 1, 2021

@dkistner @kon-angelo should we hold this PR? Are you validating a release?

@kon-angelo
Copy link
Contributor

/hold

@rfranzke Yes we are validating a release. We can merge this in the coming days

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Apr 1, 2021
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

The TestDefinitions need an update as well.

diff --git a/.test-defs/infrastructure-test.yaml b/.test-defs/infrastructure-test.yaml
index 341b20dd..dd7f648c 100644
--- a/.test-defs/infrastructure-test.yaml
+++ b/.test-defs/infrastructure-test.yaml
@@ -19,4 +19,4 @@ spec:
     --region="$REGION"
     --tenant-name="$TENANT_NAME"
     --user-name="$USER_NAME"
-  image: eu.gcr.io/gardener-project/3rd/golang:1.15.7
+  image: eu.gcr.io/gardener-project/3rd/golang:1.16.3
diff --git a/.test-defs/provider-openstack.yaml b/.test-defs/provider-openstack.yaml
index d742f7bc..31820e6e 100644
--- a/.test-defs/provider-openstack.yaml
+++ b/.test-defs/provider-openstack.yaml
@@ -16,4 +16,4 @@ spec:
     --loadbalancer-provider=$LOADBALANCER_PROVIDER
     --network-worker-cidr=$NETWORK_WORKER_CIDR

-  image: eu.gcr.io/gardener-project/3rd/golang:1.15.7
+  image: eu.gcr.io/gardener-project/3rd/golang:1.16.3

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Apr 5, 2021
@stoyanr stoyanr force-pushed the enable-strict-deserializer branch from aabb46e to d024d02 Compare April 6, 2021 06: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 Apr 6, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Apr 6, 2021

The TestDefinitions need an update as well.

Done

@gardener-robot-ci-2 gardener-robot-ci-2 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 Apr 6, 2021
@rfranzke
Copy link
Member

rfranzke commented Apr 7, 2021

Can you please revendor the v1.20.0 tag now that it's released?

@stoyanr stoyanr force-pushed the enable-strict-deserializer branch from d024d02 to d3a57f8 Compare April 7, 2021 12:35
@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 Apr 7, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Apr 7, 2021

Can you please revendor the v1.20.0 tag now that it's released?

Done

@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 Apr 7, 2021
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Apr 7, 2021
@rfranzke
Copy link
Member

rfranzke commented Apr 7, 2021

Thanks @stoyanr, can you check why the verify step is failing?

@stoyanr stoyanr 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 Apr 8, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 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 Apr 8, 2021
@stoyanr stoyanr merged commit 1935265 into gardener:master Apr 8, 2021
@stoyanr stoyanr deleted the enable-strict-deserializer branch April 8, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/openstack OpenStack platform/infrastructure priority/3 Priority (lower number equals higher priority) reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
7 participants