Skip to content

WIP: OKD-259: Support an "OKD" featureset to be enabled by default on OKD clusters #2451

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions config/v1/types_feature.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,12 @@ var (
// your cluster may fail in an unrecoverable way.
CustomNoUpgrade FeatureSet = "CustomNoUpgrade"

// OKD turns on features for OKD. Turning this feature set ON is supported for OKD clusters, but NOT for OpenShift clusters
// this feature set on CANNOT BE UNDONE for OKD clusters and when enabled on OpenShift clusters it PREVENTS UPGRADES.
OKD FeatureSet = "OKD"
Comment on lines +56 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we prevent this from being an option on OCP clusters?

What options do we have for including things in OCP but not OKD? Does OKD ship as a different cluster profile perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes we need to do this. similar to the model that the installer and other repos follow, we can restrict it to be enabled only on OKD clusters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, you're going to generate an API that will be applied to OCP clusters, which means that in theory we have to either a) block this value from being set at admission time, b) prevent the option from even being included in the API schema

I would probably lean towards b, but that means creating separate payload manifests for OKD I believe 🤔

We also currently have no way to create differences like that in the tooling apart from feature gates, but, that isn't a long term thing, so we would need to provide other options

Copy link
Contributor

Choose a reason for hiding this comment

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

block this value from being set at admission time

can we do this in cluster config operator maybe? if the featureset being enabled is OKD and the image is not built for OKD (through the method above), we reject it?

Copy link
Contributor

Choose a reason for hiding this comment

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

But also - the other angle here: Why do we want to prevent it from being turned on for OCP? It will always be a subset of techpreviewNoUpgrade, the only difference being that it will be upgradeable only on OKD clusters (this we can control through CCO as done here). This will mean from an OCP perspective, it will be just another featureset. There are no OKD specific features (nor do i see any in the future).

Copy link
Contributor

Choose a reason for hiding this comment

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

can we do this in cluster config operator maybe?

We used to validate the feature gate transitions and values in the API server directly, but this was removed in favour of CEL, see https://github.com/openshift/kubernetes/pull/1944/files

Config operator doesn't have any admission time validation ability

Why do we want to prevent it from being turned on for OCP?

I don't think we want an extra feature set in OCP. We have TPNU, we have DPNU, we have CNU.

Enabling an OKD feature sat that isn't OKDNU in OCP sends the wrong signal IMO. All of our features sets that have existed in OCP have either been upgradable, or have had NoUpgrade in the name.

The OKD set can be achieved as a version of CNU if a customer so desired.


// AllFixedFeatureSets are the featuresets that have known featuregates. Custom doesn't for instance. LatencySensitive is dead
AllFixedFeatureSets = []FeatureSet{Default, TechPreviewNoUpgrade, DevPreviewNoUpgrade}
AllFixedFeatureSets = []FeatureSet{Default, TechPreviewNoUpgrade, DevPreviewNoUpgrade, OKD}
)

type FeatureGateSpec struct {
Expand All @@ -67,10 +71,11 @@ type FeatureGateSelection struct {
// Turning on or off features may cause irreversible changes in your cluster which cannot be undone.
// +unionDiscriminator
// +optional
// +kubebuilder:validation:Enum=CustomNoUpgrade;DevPreviewNoUpgrade;TechPreviewNoUpgrade;""
// +kubebuilder:validation:Enum=CustomNoUpgrade;DevPreviewNoUpgrade;TechPreviewNoUpgrade;OKD;""
// +kubebuilder:validation:XValidation:rule="oldSelf == 'CustomNoUpgrade' ? self == 'CustomNoUpgrade' : true",message="CustomNoUpgrade may not be changed"
// +kubebuilder:validation:XValidation:rule="oldSelf == 'TechPreviewNoUpgrade' ? self == 'TechPreviewNoUpgrade' : true",message="TechPreviewNoUpgrade may not be changed"
// +kubebuilder:validation:XValidation:rule="oldSelf == 'DevPreviewNoUpgrade' ? self == 'DevPreviewNoUpgrade' : true",message="DevPreviewNoUpgrade may not be changed"
// +kubebuilder:validation:XValidation:rule="oldSelf == 'OKD' ? self == 'OKD' : true",message="OKD may not be changed"
FeatureSet FeatureSet `json:"featureSet,omitempty"`

// customNoUpgrade allows the enabling or disabling of any feature. Turning this feature set on IS NOT SUPPORTED, CANNOT BE UNDONE, and PREVENTS UPGRADES.
Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/470
api.openshift.io/merged-by-featuregates: "true"
include.release.openshift.io/self-managed-high-availability: "true"
release.openshift.io/bootstrap-required: "true"
release.openshift.io/feature-set: OKD
name: authentications.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Authentication
listKind: AuthenticationList
plural: authentications
singular: authentication
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: |-
Authentication specifies cluster-wide settings for authentication (like OAuth and
webhook token authenticators). The canonical name of an instance is `cluster`.

Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
properties:
oauthMetadata:
description: |-
oauthMetadata contains the discovery endpoint data for OAuth 2.0
Authorization Server Metadata for an external OAuth server.
This discovery document can be viewed from its served location:
oc get --raw '/.well-known/oauth-authorization-server'
For further details, see the IETF Draft:
https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
If oauthMetadata.name is non-empty, this value has precedence
over any metadata reference stored in status.
The key "oauthMetadata" is used to locate the data.
If specified and the config map or expected key is not found, no metadata is served.
If the specified metadata is not valid, no metadata is served.
The namespace for this config map is openshift-config.
properties:
name:
description: name is the metadata.name of the referenced config
map
type: string
required:
- name
type: object
serviceAccountIssuer:
description: |-
serviceAccountIssuer is the identifier of the bound service account token
issuer.
The default is https://kubernetes.default.svc
WARNING: Updating this field will not result in immediate invalidation of all bound tokens with the
previous issuer value. Instead, the tokens issued by previous service account issuer will continue to
be trusted for a time period chosen by the platform (currently set to 24h).
This time period is subject to change over time.
This allows internal components to transition to use new service account issuer without service distruption.
type: string
type:
description: |-
type identifies the cluster managed, user facing authentication mode in use.
Specifically, it manages the component that responds to login attempts.
The default is IntegratedOAuth.
enum:
- ""
- None
- IntegratedOAuth
type: string
webhookTokenAuthenticator:
description: |-
webhookTokenAuthenticator configures a remote token reviewer.
These remote authentication webhooks can be used to verify bearer tokens
via the tokenreviews.authentication.k8s.io REST API. This is required to
honor bearer tokens that are provisioned by an external authentication service.

Can only be set if "Type" is set to "None".
properties:
kubeConfig:
description: |-
kubeConfig references a secret that contains kube config file data which
describes how to access the remote webhook service.
The namespace for the referenced secret is openshift-config.

For further details, see:

https://kubernetes.io/docs/reference/access-authn-authz/authentication/#webhook-token-authentication

The key "kubeConfig" is used to locate the data.
If the secret or expected key is not found, the webhook is not honored.
If the specified kube config data is not valid, the webhook is not honored.
properties:
name:
description: name is the metadata.name of the referenced secret
type: string
required:
- name
type: object
required:
- kubeConfig
type: object
webhookTokenAuthenticators:
description: webhookTokenAuthenticators is DEPRECATED, setting it
has no effect.
items:
description: |-
deprecatedWebhookTokenAuthenticator holds the necessary configuration options for a remote token authenticator.
It's the same as WebhookTokenAuthenticator but it's missing the 'required' validation on KubeConfig field.
properties:
kubeConfig:
description: |-
kubeConfig contains kube config file data which describes how to access the remote webhook service.
For further details, see:
https://kubernetes.io/docs/reference/access-authn-authz/authentication/#webhook-token-authentication
The key "kubeConfig" is used to locate the data.
If the secret or expected key is not found, the webhook is not honored.
If the specified kube config data is not valid, the webhook is not honored.
The namespace for this secret is determined by the point of use.
properties:
name:
description: name is the metadata.name of the referenced
secret
type: string
required:
- name
type: object
type: object
type: array
x-kubernetes-list-type: atomic
type: object
status:
description: status holds observed values from the cluster. They may not
be overridden.
properties:
integratedOAuthMetadata:
description: |-
integratedOAuthMetadata contains the discovery endpoint data for OAuth 2.0
Authorization Server Metadata for the in-cluster integrated OAuth server.
This discovery document can be viewed from its served location:
oc get --raw '/.well-known/oauth-authorization-server'
For further details, see the IETF Draft:
https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
This contains the observed value based on cluster state.
An explicitly set value in spec.oauthMetadata has precedence over this field.
This field has no meaning if authentication spec.type is not set to IntegratedOAuth.
The key "oauthMetadata" is used to locate the data.
If the config map or expected key is not found, no metadata is served.
If the specified metadata is not valid, no metadata is served.
The namespace for this config map is openshift-config-managed.
properties:
name:
description: name is the metadata.name of the referenced config
map
type: string
required:
- name
type: object
type: object
required:
- spec
type: object
served: true
storage: true
subresources:
status: {}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ spec:
- CustomNoUpgrade
- DevPreviewNoUpgrade
- TechPreviewNoUpgrade
- OKD
- ""
type: string
x-kubernetes-validations:
Expand All @@ -91,6 +92,8 @@ spec:
- message: DevPreviewNoUpgrade may not be changed
rule: 'oldSelf == ''DevPreviewNoUpgrade'' ? self == ''DevPreviewNoUpgrade''
: true'
- message: OKD may not be changed
rule: 'oldSelf == ''OKD'' ? self == ''OKD'' : true'
type: object
x-kubernetes-validations:
- message: .spec.featureSet cannot be removed
Expand Down
Loading