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

chore: upgrading to 0.30 api, creating v1 or v1beta1 VAP/VAPB #3472

Merged
merged 22 commits into from
Aug 8, 2024
Merged

chore: upgrading to 0.30 api, creating v1 or v1beta1 VAP/VAPB #3472

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4d3f669
upgrading to 0.30 api, creating v1 or v1beta1 VAP/VAPB
JaydipGabani Aug 2, 2024
9298580
adding config to enable beta api in kind cluster for 1.28, 1.29. fixi…
JaydipGabani Aug 2, 2024
f43b6bf
fixing lint
JaydipGabani Aug 3, 2024
9f58850
adding tests
JaydipGabani Aug 5, 2024
ef94027
upgrading to 0.30 api, creating v1 or v1beta1 VAP/VAPB
JaydipGabani Aug 2, 2024
f6b9ebe
adding config to enable beta api in kind cluster for 1.28, 1.29. fixi…
JaydipGabani Aug 2, 2024
c5793ea
fixing lint
JaydipGabani Aug 3, 2024
3a0eede
adding tests
JaydipGabani Aug 5, 2024
bb19964
using runtime.Object for VAP/VAPB versions
JaydipGabani Aug 5, 2024
f8791e6
merging with master
JaydipGabani Aug 5, 2024
1dff1e5
converting v1beta1 to v1 without unstructured
JaydipGabani Aug 5, 2024
de48c99
removing toolchain from go.mod
JaydipGabani Aug 5, 2024
1e26786
removing lib file
JaydipGabani Aug 5, 2024
ed5c510
checking for VAP API availability only when generation is on
JaydipGabani Aug 6, 2024
fe4998e
addressing nits
JaydipGabani Aug 6, 2024
3a77ea3
adding read lock while checking for vap api availability
JaydipGabani Aug 6, 2024
a63f998
Merge branch 'master' into k130
JaydipGabani Aug 6, 2024
6cb62fa
adding label-selector in mutation for backward comp check
JaydipGabani Aug 6, 2024
7e39997
Merge branch 'k130' of github.com:JaydipGabani/gatekeeper into k130
JaydipGabani Aug 6, 2024
f3a4977
using interim variables for read lock
JaydipGabani Aug 6, 2024
2f0b758
adding todo to use r.scheme.convert
JaydipGabani Aug 7, 2024
5b3ffa0
addressing nits
JaydipGabani Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
8 changes: 5 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ GENERATE_VAPBINDING ?= false
VERSION := v3.17.0-beta.0

KIND_VERSION ?= 0.17.0
KIND_CLUSTER_FILE ?= test/bats/tests/kindcluster.yml
# note: k8s version pinned since KIND image availability lags k8s releases
KUBERNETES_VERSION ?= 1.30.0
KUSTOMIZE_VERSION ?= 3.8.9
BATS_VERSION ?= 1.8.2
ORAS_VERSION ?= 0.16.0
BATS_TESTS_FILE ?= test/bats/test.bats
KIND_CLUSTER_FILE ?= test/bats/tests/kindcluster.yml
HELM_VERSION ?= 3.7.2
NODE_VERSION ?= 16-bullseye-slim
YQ_VERSION ?= 4.30.6
Expand Down Expand Up @@ -76,6 +76,7 @@ MANAGER_IMAGE_PATCH := "apiVersion: apps/v1\
\n - --default-create-vap-for-templates=${GENERATE_VAP}\
\n - --default-create-vap-binding-for-constraints=${GENERATE_VAPBINDING}\
\n - --experimental-enable-k8s-native-validation\
\n - --log-level=${LOG_LEVEL}\
\n---\
\napiVersion: apps/v1\
\nkind: Deployment\
Expand All @@ -99,6 +100,7 @@ MANAGER_IMAGE_PATCH := "apiVersion: apps/v1\
\n - --default-create-vap-for-templates=${GENERATE_VAP}\
\n - --default-create-vap-binding-for-constraints=${GENERATE_VAPBINDING}\
\n - --experimental-enable-k8s-native-validation\
\n - --log-level=${LOG_LEVEL}\
\n"

# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
Expand Down Expand Up @@ -172,8 +174,8 @@ e2e-bootstrap: e2e-dependencies
if [ $$(${GITHUB_WORKSPACE}/bin/kind get clusters) ]; then ${GITHUB_WORKSPACE}/bin/kind delete cluster; fi

# Create a new kind cluster
# TODO(ritazh): remove KIND_CLUSTER_FILE when vap feature is GA
if [ $$(echo $(KUBERNETES_VERSION) | cut -d'.' -f2) -lt 28 ]; then ${GITHUB_WORKSPACE}/bin/kind create cluster --image $(KIND_NODE_VERSION) --wait 5m; else ${GITHUB_WORKSPACE}/bin/kind create cluster --config $(KIND_CLUSTER_FILE) --image $(KIND_NODE_VERSION) --wait 5m; fi
# Only enabling VAP beta apis for 1.28, 1.29
if [ $$(echo $(KUBERNETES_VERSION) | cut -d'.' -f2) -lt 28 ] || [ $$(echo $(KUBERNETES_VERSION) | cut -d'.' -f2) -gt 29 ]; then ${GITHUB_WORKSPACE}/bin/kind create cluster --image $(KIND_NODE_VERSION) --wait 5m; else ${GITHUB_WORKSPACE}/bin/kind create cluster --config $(KIND_CLUSTER_FILE) --image $(KIND_NODE_VERSION) --wait 5m; fi

e2e-build-load-image: docker-buildx e2e-build-load-externaldata-image
kind load docker-image --name kind ${IMG} ${CRD_IMG}
Expand Down
4 changes: 4 additions & 0 deletions config/crd/bases/match.gatekeeper.sh_matchcrd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -158,11 +160,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
12 changes: 12 additions & 0 deletions config/crd/bases/mutations.gatekeeper.sh_assign.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -199,11 +201,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -509,11 +513,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

Is adding this backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxsmythe I think so, this page says atomic can apply to any list. It is coming through bumping k8s api - https://github.com/kubernetes/apimachinery/blob/v0.30.3/pkg/apis/meta/v1/types.go#L1250

Copy link
Contributor

@maxsmythe maxsmythe Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Backwards compatibility means that applying this tag will not change merge behavior.

In other words, we should be sure that kubectl apply logic prior to this tag being added and post this tag being added are unchanged.

Because this potentially affects backwards compatibility, we should be certain that this is the case. We should not rely on speculation. In other words, we should do more research than just inferring from a vague statement in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found By default, Server-Side Apply treats custom resources as unstructured data. All keys are treated the same as struct fields, and all lists are considered atomic., here. So before this change, the list was being treated as atomic for patchStratagy=merge. wdyt?

How do I test for backward compatibility for this change?

I tested with deploying CRDs before this change and creating mutators, then I upgraded the CRDs with this change and applied mutators But I think this may be wrong way to test for backward compat, I am not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also see this If listType is missing, the API server interprets a patchStrategy=merge marker as a listType=map and the corresponding patchMergeKey marker as a listMapKey. on the same page, so now I am cofused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked in slack - https://kubernetes.slack.com/archives/C0123CNN8F3/p1722991068800879, waiting for response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the response - atomic is the default - https://kubernetes.slack.com/archives/C0123CNN8F3/p1722991724774759?thread_ts=1722991068.800879&cid=C0123CNN8F3.

@maxsmythe let me know if this resolves your concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pasting the response for reference here about this change -

atomic is the default
that was making it explicit to avoid linting errors about implicit list type

the containing labelselector struct is already annotated as +structType=atomic, which generators should turn into an atomic annotation on the parent selector field, so no list type annotations on the child fields should be relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding more context:

This x-kubernetes-list-type: atomic is getting added for list fields under metav1.labelSelector and metav1.namespaceSelector for mutators. This change is coming from here, where marker listType=atomic is getting added to matchExpression and values field. controller-runtime is looking at this markers and adding the field to manifets.

Copy link
Contributor

Choose a reason for hiding this comment

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

"By default, Server-Side Apply treats custom resources as unstructured data. All keys are treated the same as struct fields, and all lists are considered atomic." Sounds conclusive to me (from your first reply)

matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -562,11 +568,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -872,11 +880,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -925,11 +935,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
4 changes: 4 additions & 0 deletions config/crd/bases/mutations.gatekeeper.sh_assignimage.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -199,11 +201,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
12 changes: 12 additions & 0 deletions config/crd/bases/mutations.gatekeeper.sh_assignmetadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -170,11 +172,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -427,11 +431,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -480,11 +486,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -737,11 +745,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -790,11 +800,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
12 changes: 12 additions & 0 deletions config/crd/bases/mutations.gatekeeper.sh_modifyset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -201,11 +203,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -478,11 +482,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -531,11 +537,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -808,11 +816,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -861,11 +871,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
Loading
Loading