WIP: HOSTEDCP-582: Add CEL validations to hostedcluster v1beta1 #1911
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: imain The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
api/v1beta1/hostedcluster_types.go
Outdated
| @@ -216,24 +235,28 @@ type HostedClusterSpec struct { | |||
| // ".dockerconfigjson" whose value is the pull secret JSON. | |||
| // | |||
| // +immutable | |||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="pullSecret is immutable" | |||
There was a problem hiding this comment.
this si mutable https://issues.redhat.com/browse/HOSTEDCP-593
| BaseDomain string `json:"baseDomain"` | ||
|
|
||
| // PublicZoneID is the Hosted Zone ID where all the DNS records that are | ||
| // publicly accessible to the internet exist. | ||
| // | ||
| // +optional | ||
| // +immutable | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="publicZoneID is immutable" |
There was a problem hiding this comment.
are publicZoneID and PrivateZoneID actually optional? I don't think so cc @sjenning
There was a problem hiding this comment.
Looking at the source code, I think you are right. Would be nice to hear from Seth to confirm though.
There was a problem hiding this comment.
Commented in slack, but I think this is only required if platform is AWS. It get piped into the DNS global config in the guest cluster.
imain
force-pushed
the
v1beta1-hostedcluster-cel
branch
from
501e783
to
f050340
Compare
✅ Deploy Preview for hypershift-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
imain
force-pushed
the
v1beta1-hostedcluster-cel
branch
3 times, most recently
from
349326c
to
d36e213
Compare
|
/hold Please test locally to make sure the manifests can be applied to an OCP 4.12 cluster before removing hold. |
openshift-ci
bot
added
the
do-not-merge/hold
imain
force-pushed
the
v1beta1-hostedcluster-cel
branch
5 times, most recently
from
5381785
to
2987d37
Compare
This uses CEL validations available in kubernetes 1.25 and above, requiring OpenShift 4.12 or greater. I have tested with OpenShift 4.10 and while the validations do not work, they are also ignored so it should be safe to have them in place for all versions. Note that I am not setting maxLength here for the strings. I looked through the source code and it seems for simple string comparisons we should be OK here. I'd really rather not have to set this if we don't have to. However if we run into issues we'll have to think about setting it. Still a WIP as I need to do more testing.
imain
force-pushed
the
v1beta1-hostedcluster-cel
branch
from
2987d37
to
68c7ce4
Compare
openshift-merge-robot
added
the
needs-rebase
|
@imain: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@imain: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
closing by #2148 which will need to sync with IBM folks |
This uses CEL validations available in kubernetes 1.25 and above, requiring OpenShift 4.12 or greater. I have tested with OpenShift 4.10 and while the validations do not work, they are also ignored so it should be safe to have them in place for all versions.
Note that I am not setting maxLength here for the strings. I looked through the source code and it seems for simple string comparisons we should be OK here. I'd really rather not have to set this if we don't have to. However if we run into issues we'll have to think about setting it.
Still a WIP as I need to do more testing.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist