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

Data dir rfc #410

Merged
merged 9 commits into from
Jul 1, 2024
Merged

Data dir rfc #410

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2b0fe9e
Add validator for data directories
jakefhyde May 6, 2024
d42b9aa
Update go mod
jakefhyde Jun 28, 2024
007ef5a
Add unit tests to data dir validator
jakefhyde Jun 28, 2024
0875ab7
Add docs for data directories
jakefhyde Jun 28, 2024
6a630a2
go generate
jakefhyde Jun 28, 2024
b1f26f2
Address review comments
jakefhyde Jul 1, 2024
ac1086b
go generate
jakefhyde Jul 1, 2024
bb7c01e
Address review comments
jakefhyde Jul 1, 2024
71bde81
Address review comments
jakefhyde Jul 1, 2024
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
19 changes: 16 additions & 3 deletions docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,15 +353,28 @@ When a UserAttribute is updated, the following checks take place:

## Cluster

### Validation Checks

#### On Update

##### Data Directories

Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`.
On update, also prevent new env vars with this name from being added but allow them to be removed. Rancher will perform
a one-time migration to move the system-agent data dir definition to the top level field from the `AgentEnvVars`
section. A secondary validator will ensure that the effective data directory for the `system-agent` is not different
from the one chosen during cluster creation. Additionally, the changing of a data directory for the `system-agent`,
kubernetes distro (RKE2/K3s), and CAPR components is also prohibited.

### Mutation Checks

#### On Update

##### Dynamic Schema Drop

Check for the presence of the `provisioning.cattle.io/allow-dynamic-schema-drop` annotation. If the value is `"true"`,
perform no mutations. If the value is not present or not `"true"`, compare the value of the `dynamicSchemaSpec` field
for each `machinePool`, to its' previous value. If the values are not identical, revert the value for the
Check for the presence of the `provisioning.cattle.io/allow-dynamic-schema-drop` annotation. If the value is `"true"`,
perform no mutations. If the value is not present or not `"true"`, compare the value of the `dynamicSchemaSpec` field
for each `machinePool`, to its' previous value. If the values are not identical, revert the value for the
`dynamicSchemaSpec` for the specific `machinePool`, but do not reject the request.

# rbac.authorization.k8s.io/v1
Expand Down
19 changes: 9 additions & 10 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ require (
github.com/gorilla/mux v1.8.1
github.com/rancher/dynamiclistener v0.6.0-rc1
github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7
github.com/rancher/rancher/pkg/apis v0.0.0-20240627213236-82e36ddee661
github.com/rancher/rancher/pkg/apis v0.0.0-20240628084651-0dd62c26260a
github.com/rancher/rke v1.6.0-rc7
github.com/rancher/wrangler/v3 v3.0.0-rc2
github.com/robfig/cron v1.2.0
Expand Down Expand Up @@ -97,17 +97,16 @@ require (
github.com/josharian/intern v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/mailru/easyjson v0.7.7 // indirect
github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/prometheus/client_golang v1.18.0 // indirect
github.com/prometheus/client_model v0.5.0 // indirect
github.com/prometheus/common v0.45.0 // indirect
github.com/prometheus/procfs v0.12.0 // indirect
github.com/prometheus/client_model v0.6.1 // indirect
github.com/prometheus/common v0.47.0 // indirect
github.com/prometheus/procfs v0.14.0 // indirect
github.com/rancher/aks-operator v1.9.0-rc.9 // indirect
github.com/rancher/eks-operator v1.9.0-rc.9 // indirect
github.com/rancher/fleet/pkg/apis v0.10.0-rc.16 // indirect
Expand All @@ -130,7 +129,7 @@ require (
go.opentelemetry.io/otel/trace v1.20.0 // indirect
go.opentelemetry.io/proto/otlp v1.0.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.26.0 // indirect
go.uber.org/zap v1.27.0 // indirect
golang.org/x/crypto v0.24.0 // indirect
golang.org/x/mod v0.17.0 // indirect
golang.org/x/net v0.26.0 // indirect
Expand All @@ -150,7 +149,7 @@ require (
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/apiextensions-apiserver v0.30.1 // indirect
k8s.io/cloud-provider v0.0.0 // indirect
k8s.io/cloud-provider v0.29.3 // indirect
k8s.io/code-generator v0.30.1 // indirect
k8s.io/component-base v0.30.1 // indirect
k8s.io/component-helpers v0.30.1 // indirect
Expand All @@ -161,9 +160,9 @@ require (
k8s.io/klog/v2 v2.120.1 // indirect
k8s.io/kms v0.30.1 // indirect
k8s.io/kube-aggregator v0.30.0 // indirect
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
k8s.io/kubelet v0.0.0 // indirect
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.29.0 // indirect
k8s.io/kube-openapi v0.0.0-20240430033511-f0e62f92d13f // indirect
k8s.io/kubelet v0.29.3 // indirect
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 // indirect
sigs.k8s.io/cli-utils v0.35.0 // indirect
sigs.k8s.io/cluster-api v1.7.3 // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
Expand Down
30 changes: 14 additions & 16 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0=
github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 h1:jWpvCLoY8Z/e3VKvlsiIGKtc+UG6U5vzxaoagmhXfyg=
github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0/go.mod h1:QUyp042oQthUoa9bqDv0ER0wrtXnBruoNd7aNjkbP+k=
github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg=
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
Expand All @@ -150,12 +148,12 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prometheus/client_golang v1.18.0 h1:HzFfmkOzH5Q8L8G+kSJKUx5dtG87sewO+FoDDqP5Tbk=
github.com/prometheus/client_golang v1.18.0/go.mod h1:T+GXkCk5wSJyOqMIzVgvvjFDlkOQntgjkJWKrN5txjA=
github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw=
github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI=
github.com/prometheus/common v0.45.0 h1:2BGz0eBc2hdMDLnO/8n0jeB3oPrt2D08CekT0lneoxM=
github.com/prometheus/common v0.45.0/go.mod h1:YJmSTw9BoKxJplESWWxlbyttQR4uaEcGyv9MZjVOJsY=
github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E=
github.com/prometheus/client_model v0.6.1/go.mod h1:OrxVMOVHjw3lKMa8+x6HeMGkHMQyHDk9E3jmP2AmGiY=
github.com/prometheus/common v0.47.0 h1:p5Cz0FNHo7SnWOmWmoRozVcjEp0bIVU8cV7OShpjL1k=
github.com/prometheus/common v0.47.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc=
github.com/prometheus/procfs v0.14.0 h1:Lw4VdGGoKEZilJsayHf0B+9YgLGREba2C6xr+Fdfq6s=
github.com/prometheus/procfs v0.14.0/go.mod h1:XL+Iwz8k8ZabyZfMFHPiilCniixqQarAy5Mu67pHlNQ=
github.com/rancher/aks-operator v1.9.0-rc.9 h1:6eIjtaNz40axGgSQLijMrSImnnPLuWdCGUONpChhmYA=
github.com/rancher/aks-operator v1.9.0-rc.9/go.mod h1:JPkilTHa1Exq8VILHOmcxapgHcr9RfPVm7BqjzveMXg=
github.com/rancher/dynamiclistener v0.6.0-rc1 h1:Emwf9o7PMLdQNv4lvFx7xJKxDuDa4Y69GvVEGU9U9Js=
Expand All @@ -170,8 +168,8 @@ github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7 h1:E5AeOkylBXf4APhnH
github.com/rancher/lasso v0.0.0-20240603075835-701e919d08b7/go.mod h1:v0FJLrmL4m6zdWfIB0/qo7qN5QIjVMFyvFGaw8uyWsA=
github.com/rancher/norman v0.0.0-20240604183301-20cd23aadce1 h1:7g0yOiUGfT4zK4N9H0PSijnS/e2YrObi4Gj19JgE1L8=
github.com/rancher/norman v0.0.0-20240604183301-20cd23aadce1/go.mod h1:sGnN5ayvAHLfInOFZ4N1fzZw1IMy3i+9PZA7IxlPsRg=
github.com/rancher/rancher/pkg/apis v0.0.0-20240627213236-82e36ddee661 h1:WKaj28PA/TbCYniGnY8+OYJf5WUN0aZ0lnu8+z7mPVY=
github.com/rancher/rancher/pkg/apis v0.0.0-20240627213236-82e36ddee661/go.mod h1:JUkPk3sIbGBisGpykW0lHrYj/M+iUjcivzYP9kPLjl0=
github.com/rancher/rancher/pkg/apis v0.0.0-20240628084651-0dd62c26260a h1:/y8gub0iNhXOIKiX7fpOgCt/81eh2G7rLKqcm5oCgNc=
github.com/rancher/rancher/pkg/apis v0.0.0-20240628084651-0dd62c26260a/go.mod h1:JUkPk3sIbGBisGpykW0lHrYj/M+iUjcivzYP9kPLjl0=
github.com/rancher/rke v1.6.0-rc7 h1:AfKugPEJtZrjgbcr5zGTwTuv18GJAUUMOMKaXUbT8TU=
github.com/rancher/rke v1.6.0-rc7/go.mod h1:5xRbf3L8PxqJRhABjYRfaBqbpVqAnqyH3maUNQEuwvk=
github.com/rancher/wrangler/v2 v2.2.0-rc6 h1:jMsuOVl7nBuQ5QJqdNkR2yHEf1+rYiyd1gN+mQzIcag=
Expand Down Expand Up @@ -249,8 +247,8 @@ go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo=
go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so=
go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8=
go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
Expand Down Expand Up @@ -368,8 +366,8 @@ k8s.io/kms v0.30.1 h1:gEIbEeCbFiaN2tNfp/EUhFdGr5/CSj8Eyq6Mkr7cCiY=
k8s.io/kms v0.30.1/go.mod h1:GrMurD0qk3G4yNgGcsCEmepqf9KyyIrTXYR2lyUOJC4=
k8s.io/kube-aggregator v0.30.1 h1:ymR2BsxDacTKwzKTuNhGZttuk009c+oZbSeD+IPX5q4=
k8s.io/kube-aggregator v0.30.1/go.mod h1:SFbqWsM6ea8dHd3mPLsZFzJHbjBOS5ykIgJh4znZ5iQ=
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340/go.mod h1:yD4MZYeKMBwQKVht279WycxKyM84kkAx2DPrTXaeb98=
k8s.io/kube-openapi v0.0.0-20240430033511-f0e62f92d13f h1:0LQagt0gDpKqvIkAMPaRGcXawNMouPECM1+F9BVxEaM=
k8s.io/kube-openapi v0.0.0-20240430033511-f0e62f92d13f/go.mod h1:S9tOR0FxgyusSNR+MboCuiDpVWkAifZvaYI1Q2ubgro=
k8s.io/kubelet v0.30.1 h1:6gS1gWjrefUGfC/9n0ITOzxnKyt89FfkIhom70Bola4=
k8s.io/kubelet v0.30.1/go.mod h1:5IUeAt3YlIfLNdT/YfRuCCONfEefm7qfcqz81b002Z8=
k8s.io/kubernetes v1.30.1 h1:XlqS6KslLEA5mQzLK2AJrhr4Z1m8oJfkhHiWJ5lue+I=
Expand All @@ -378,8 +376,8 @@ k8s.io/pod-security-admission v0.30.1 h1:r2NQSNXfnZDnm6KvLv1sYgai1ZXuO+m0qn11/Xy
k8s.io/pod-security-admission v0.30.1/go.mod h1:O5iry5U8N0CvtfI5kfe0CZ0Ct/KYj057j6Pa+QIwp24=
k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 h1:jgGTlFYnhF1PM1Ax/lAlxUPE+KfCIXHaathvJg1C3ak=
k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.29.0 h1:/U5vjBbQn3RChhv7P11uhYvCSm5G2GaIi5AIGBS6r4c=
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.29.0/go.mod h1:z7+wmGM2dfIiLRfrC6jb5kV2Mq/sK1ZP303cxzkV5Y4=
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 h1:2770sDpzrjjsAtVhSeUFseziht227YAWYHLGNM8QPwY=
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3/go.mod h1:Ve9uj1L+deCXFrPOk1LpFXqTg7LCFzFso6PA48q/XZw=
sigs.k8s.io/cli-utils v0.35.0 h1:dfSJaF1W0frW74PtjwiyoB4cwdRygbHnC7qe7HF0g/Y=
sigs.k8s.io/cli-utils v0.35.0/go.mod h1:ITitykCJxP1vaj1Cew/FZEaVJ2YsTN9Q71m02jebkoE=
sigs.k8s.io/cluster-api v1.7.3 h1:DsSRxsA+18jxLqPAo29abZ9kOPK1/xwhSuQb/MROzSs=
Expand Down
19 changes: 16 additions & 3 deletions pkg/resources/provisioning.cattle.io/v1/cluster/Cluster.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
## Validation Checks

### On Update

#### Data Directories

Prevent the creation of new objects with an env var (under `spec.agentEnvVars`) with a name of `CATTLE_AGENT_VAR_DIR`.
On update, also prevent new env vars with this name from being added but allow them to be removed. Rancher will perform
a one-time migration to move the system-agent data dir definition to the top level field from the `AgentEnvVars`
section. A secondary validator will ensure that the effective data directory for the `system-agent` is not different
from the one chosen during cluster creation. Additionally, the changing of a data directory for the `system-agent`,
kubernetes distro (RKE2/K3s), and CAPR components is also prohibited.

## Mutation Checks

### On Update

#### Dynamic Schema Drop

Check for the presence of the `provisioning.cattle.io/allow-dynamic-schema-drop` annotation. If the value is `"true"`,
perform no mutations. If the value is not present or not `"true"`, compare the value of the `dynamicSchemaSpec` field
for each `machinePool`, to its' previous value. If the values are not identical, revert the value for the
Check for the presence of the `provisioning.cattle.io/allow-dynamic-schema-drop` annotation. If the value is `"true"`,
perform no mutations. If the value is not present or not `"true"`, compare the value of the `dynamicSchemaSpec` field
for each `machinePool`, to its' previous value. If the values are not identical, revert the value for the
`dynamicSchemaSpec` for the specific `machinePool`, but do not reject the request.
101 changes: 98 additions & 3 deletions pkg/resources/provisioning.cattle.io/v1/cluster/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ import (
"encoding/base64"
"fmt"
"net/http"
"reflect"
"regexp"
"slices"

v1 "github.com/rancher/rancher/pkg/apis/provisioning.cattle.io/v1"
rkev1 "github.com/rancher/rancher/pkg/apis/rke.cattle.io/v1"
"github.com/rancher/webhook/pkg/admission"
"github.com/rancher/webhook/pkg/clients"
v3 "github.com/rancher/webhook/pkg/generated/controllers/management.cattle.io/v3"
Expand All @@ -26,10 +29,15 @@ import (
"k8s.io/utils/trace"
)

const globalNamespace = "cattle-global-data"
const (
globalNamespace = "cattle-global-data"
systemAgentVarDirEnvVar = "CATTLE_AGENT_VAR_DIR"
)

var mgmtNameRegex = regexp.MustCompile("^c-[a-z0-9]{5}$")
var fleetNameRegex = regexp.MustCompile("^[^-][-a-z0-9]+$")
var (
mgmtNameRegex = regexp.MustCompile("^c-[a-z0-9]{5}$")
fleetNameRegex = regexp.MustCompile("^[^-][-a-z0-9]+$")
)

// NewProvisioningClusterValidator returns a new validator for provisioning clusters
func NewProvisioningClusterValidator(client *clients.Clients) *ProvisioningClusterValidator {
Expand Down Expand Up @@ -78,10 +86,12 @@ type provisioningAdmitter struct {
func (p *provisioningAdmitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
listTrace := trace.New("provisioningClusterValidator Admit", trace.Field{Key: "user", Value: request.UserInfo.Username})
defer listTrace.LogIfLong(admission.SlowTraceDuration)

oldCluster, cluster, err := objectsv1.ClusterOldAndNewFromRequest(&request.AdmissionRequest)
if err != nil {
return nil, err
}

response := &admissionv1.AdmissionResponse{}
if request.Operation == admissionv1.Create || request.Operation == admissionv1.Update {
if err := p.validateClusterName(request, response, cluster); err != nil || response.Result != nil {
Expand All @@ -103,6 +113,14 @@ func (p *provisioningAdmitter) Admit(request *admission.Request) (*admissionv1.A
if err := p.validateCloudCredentialAccess(request, response, oldCluster, cluster); err != nil || response.Result != nil {
return response, err
}

if response = p.validateAgentEnvVars(request, oldCluster, cluster); response.Result != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would handle the "ok" cases differently than this. Right now this depends on response.Result being nil for ResponseAllowed() which I think is very dangerous long term. I would recommend one of the following options:

  • Only return a response if it fails validation and return nil otherwise. In this function, check response != nil to see if you should break with a response
  • In this function, check if !response.Allowed - this way, if it fails we short-circuit and return early.
  • Return a bool and an *admissionv1.AdmissionResponse. Use the bool to check if the response was allowed or not.

Big thing here is not to closely couple this code to what the allowed response does like it's currently doing.

return response, err
}

if response = p.validateDataDirectories(request, oldCluster, cluster); response.Result != nil {
return response, err
}
}

if err := p.validatePSACT(request, response, cluster); err != nil || response.Result != nil {
Expand All @@ -113,6 +131,83 @@ func (p *provisioningAdmitter) Admit(request *admission.Request) (*admissionv1.A
return response, nil
}

// validateAgentEnvVars will validate the provisioning cluster object's AgentEnvVars, ensuring that the
// "CATTLE_AGENT_VAR_DIR" env var is not allowed to be added to an existing cluster, or set upon create for a new
// cluster.
func (p *provisioningAdmitter) validateAgentEnvVars(request *admission.Request, oldCluster, newCluster *v1.Cluster) *admissionv1.AdmissionResponse {
if newCluster.Spec.RKEConfig == nil {
return admission.ResponseAllowed()
}
if request.Operation == admissionv1.Create {
if slices.ContainsFunc(newCluster.Spec.AgentEnvVars, func(envVar rkev1.EnvVar) bool {
return envVar.Name == systemAgentVarDirEnvVar
}) {
return admission.ResponseBadRequest(
fmt.Sprintf(`"%s" cannot be set within "cluster.Spec.RKEConfig.AgentEnvVars": use "cluster.Spec.RKEConfig.DataDirectories.SystemAgent"`, systemAgentVarDirEnvVar))
}
return admission.ResponseAllowed()
}
if request.Operation == admissionv1.Update {
var oldCount, newCount int
for _, envVar := range oldCluster.Spec.AgentEnvVars {
if envVar.Name == systemAgentVarDirEnvVar {
oldCount++
}
}
for _, envVar := range newCluster.Spec.AgentEnvVars {
if envVar.Name == systemAgentVarDirEnvVar {
newCount++
}
}
if newCount > oldCount {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this works as intended. From what I can see here, I could change this value as long as I added/removed in a single step. For example, if I change CATTLE_AGENT_VAR_DIR from /var/lib/rke2 to /var/lib/new, that would work.

That being said, I'm not 100% sure that this is intentional - open to hearing otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly it's not super clear but it is functionally correct: this function is making sure that new CATTLE_AGENT_VAR_DIR vars are only allowed to be added, not removed. The other function is supposed to do validation, which is covered by this case:

	oldEnvVars := slices.DeleteFunc(slices.Clone(oldCluster.Spec.AgentEnvVars), func(envVar rkev1.EnvVar) bool {
		return envVar.Name != systemAgentVarDirEnvVar
	})
	// will be true up until rancher performs one time migration to set SystemAgent data directory
	if len(oldEnvVars) > 0 {
		newEnvVars := slices.DeleteFunc(slices.Clone(newCluster.Spec.AgentEnvVars), func(envVar rkev1.EnvVar) bool {
			return envVar.Name != systemAgentVarDirEnvVar
		})
		if reflect.DeepEqual(newEnvVars, oldEnvVars) && oldCluster.Spec.RKEConfig.DataDirectories.SystemAgent == newCluster.Spec.RKEConfig.DataDirectories.SystemAgent {
			// rancher migration has not been performed yet
			return admission.ResponseAllowed()
		}
		if newCluster.Spec.RKEConfig.DataDirectories.SystemAgent != oldEnvVars[len(oldEnvVars)-1].Value {
			return admission.ResponseBadRequest(
				fmt.Sprintf(`"%s" cannot be set within "cluster.Spec.RKEConfig.AgentEnvVars": use "cluster.Spec.RKEConfig.DataDirectories.SystemAgent"`, systemAgentVarDirEnvVar))
		}

so if we were defining the env var previously, and we haven't changed either the env var or data directory field in this update, assume it's fine (actually as I look at it we shouldn't allow this without validating the other data directories but you get the idea).

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but the current method will only stop me from adding/removing the env var right? So if I just change it, it's allowed. From your reply we don't want that, it needs to be the same value as before correct?

return admission.ResponseBadRequest(
fmt.Sprintf(`"%s" cannot be set within "cluster.Spec.RKEConfig.AgentEnvVars": use "cluster.Spec.RKEConfig.DataDirectories.SystemAgent"`, systemAgentVarDirEnvVar))
}
}

return admission.ResponseAllowed()
}

// validateDataDirectories will validate updates to the cluster object to ensure the data directories are not changed.
// The only exception when a data directory is allowed to be changed is if cluster.Spec.AgentEnvVars has an env var with
// a name of "CATTLE_AGENT_VAR_DIR", which Rancher will perform a one-time migration to set the
// cluster.Spec.RKEConfig.DataDirectories.SystemAgent field for the cluster. validateAgentEnvVars will ensure
// "CATTLE_AGENT_VAR_DIR" is not added, so this exception only applies to the one-time Rancher migration.
func (p *provisioningAdmitter) validateDataDirectories(request *admission.Request, oldCluster, newCluster *v1.Cluster) *admissionv1.AdmissionResponse {
if request.Operation != admissionv1.Update {
return admission.ResponseAllowed()
}
if newCluster.Spec.RKEConfig == nil {
return admission.ResponseAllowed()
}
oldSystemAgentVarDirEnvVars := slices.DeleteFunc(slices.Clone(oldCluster.Spec.AgentEnvVars), func(envVar rkev1.EnvVar) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It occurs to me that you could do this better with a simple extraction func to find the variable. Something like:

def getEnvVar(name string, envVars []rkev1.EnvVar) *rkev1.EnvVar{
    for _, envVar := range envVars{
        if envVar.Name == name{
            return &envVar
        }
    }
    return nil
}

Main benefit that we get here is that we are no longer cloning/modifying a slice, so I'd imagine the performance here is going to be a bit better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with this suggestion is that it returns the first occurrence of the env var, and we want the last, as it's possible to specify an env var multiple times.

return envVar.Name != systemAgentVarDirEnvVar
})
// will be true up until rancher performs one time migration to set SystemAgent data directory if env var was previously set
if len(oldSystemAgentVarDirEnvVars) > 0 {
newSystemAgentVarDirEnvVars := slices.DeleteFunc(slices.Clone(newCluster.Spec.AgentEnvVars), func(envVar rkev1.EnvVar) bool {
return envVar.Name != systemAgentVarDirEnvVar
})
if !reflect.DeepEqual(newSystemAgentVarDirEnvVars, oldSystemAgentVarDirEnvVars) || oldCluster.Spec.RKEConfig.DataDirectories.SystemAgent != newCluster.Spec.RKEConfig.DataDirectories.SystemAgent {
if newCluster.Spec.RKEConfig.DataDirectories.SystemAgent != oldSystemAgentVarDirEnvVars[len(oldSystemAgentVarDirEnvVars)-1].Value {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think that this is a bit tough to follow. I think this would also be easier to write once the actual env var is extracted and you can move out of slices.

In addition, since the second if statement doesn't have an else (or any other additional logic). it can be combined into one if statement.

// only valid during migration
return admission.ResponseBadRequest(
fmt.Sprintf(`"%s" cannot be set within "cluster.Spec.RKEConfig.AgentEnvVars": use "cluster.Spec.RKEConfig.DataDirectories.SystemAgent"`, systemAgentVarDirEnvVar))
}
}
} else if oldCluster.Spec.RKEConfig.DataDirectories.SystemAgent != newCluster.Spec.RKEConfig.DataDirectories.SystemAgent {
return admission.ResponseBadRequest("SystemAgent data directory cannot be changed after cluster creation")
}
if oldCluster.Spec.RKEConfig.DataDirectories.Provisioning != newCluster.Spec.RKEConfig.DataDirectories.Provisioning {
return admission.ResponseBadRequest("Provisioning data directory cannot be changed after cluster creation")
}
if oldCluster.Spec.RKEConfig.DataDirectories.K8sDistro != newCluster.Spec.RKEConfig.DataDirectories.K8sDistro {
return admission.ResponseBadRequest("Distro data directory cannot be changed after cluster creation")
}

return admission.ResponseAllowed()
}

func (p *provisioningAdmitter) validateCloudCredentialAccess(request *admission.Request, response *admissionv1.AdmissionResponse, oldCluster, newCluster *v1.Cluster) error {
if newCluster.Spec.CloudCredentialSecretName == "" ||
oldCluster.Spec.CloudCredentialSecretName == newCluster.Spec.CloudCredentialSecretName {
Expand Down
Loading