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

introduce flag --feature-gates to karmadactl register #5970

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

introduce flag --feature-gates to karmadactl register #5970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions pkg/karmadactl/register/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
rbacv1 "k8s.io/api/rbac/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
k8srand "k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/wait"
kubeclient "k8s.io/client-go/kubernetes"
Expand All @@ -49,6 +50,7 @@ import (
"k8s.io/kubectl/pkg/util/templates"

"github.com/karmada-io/karmada/pkg/apis/cluster/validation"
"github.com/karmada-io/karmada/pkg/features"
karmadaclientset "github.com/karmada-io/karmada/pkg/generated/clientset/versioned"
"github.com/karmada-io/karmada/pkg/karmadactl/options"
cmdutil "github.com/karmada-io/karmada/pkg/karmadactl/util"
Expand Down Expand Up @@ -196,6 +198,9 @@ func NewCmdRegister(parentCommand string) *cobra.Command {
flags.Int32Var(&opts.CertExpirationSeconds, "cert-expiration-seconds", DefaultCertExpirationSeconds, "The expiration time of certificate.")
flags.BoolVar(&opts.DryRun, "dry-run", false, "Run the command in dry-run mode, without making any server requests.")
flags.StringVar(&opts.ProxyServerAddress, "proxy-server-address", "", "Address of the proxy server that is used to proxy to the cluster.")
flags.StringVar(&opts.FeatureGates, "feature-gates", "", ""+
"A set of key=value pairs that describe feature gates for alpha/experimental features. "+
"Options are:\n"+strings.Join(features.FeatureGate.KnownFeatures(), "\n"))
Comment on lines +201 to +203
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
flags.StringVar(&opts.FeatureGates, "feature-gates", "", ""+
"A set of key=value pairs that describe feature gates for alpha/experimental features. "+
"Options are:\n"+strings.Join(features.FeatureGate.KnownFeatures(), "\n"))
features.FeatureGate.AddFlag(flags)

Can we do that this way, which is the way we did at all the other components, like

karmada/cmd/controller-manager/app/options/options.go

Line 235 in 6144ea5

features.FeatureGate.AddFlag(flags)

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've tried this way, but ran into an issue where I couldn't get the string value of the feature gates when injecting them into the agent's manifest.

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

It is worth noting that, you don't need to introduce an additional field into CommandRegisterOption anymore.

	FeatureGates string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

karmada/vendor/k8s.io/component-base/featuregate/feature_gate.go

Lines 122 to 153 in 7112723

// MutableFeatureGate parses and stores flag gates for known features from
// a string like feature1=true,feature2=false,...
type MutableFeatureGate interface {
FeatureGate
// AddFlag adds a flag for setting global feature gates to the specified FlagSet.
AddFlag(fs *pflag.FlagSet)
// Close sets closed to true, and prevents subsequent calls to Add
Close()
// Set parses and stores flag gates for known features
// from a string like feature1=true,feature2=false,...
Set(value string) error
// SetFromMap stores flag gates for known features from a map[string]bool or returns an error
SetFromMap(m map[string]bool) error
// Add adds features to the featureGate.
Add(features map[Feature]FeatureSpec) error
// GetAll returns a copy of the map of known feature names to feature specs.
GetAll() map[Feature]FeatureSpec
// AddMetrics adds feature enablement metrics
AddMetrics()
// OverrideDefault sets a local override for the registered default value of a named
// feature. If the feature has not been previously registered (e.g. by a call to Add), has a
// locked default, or if the gate has already registered itself with a FlagSet, a non-nil
// error is returned.
//
// When two or more components consume a common feature, one component can override its
// default at runtime in order to adopt new defaults before or after the other
// components. For example, a new feature can be evaluated with a limited blast radius by
// overriding its default to true for a limited number of components without simultaneously
// changing its default for all consuming components.
OverrideDefault(name Feature, override bool) error
}

features.FeatureGate.AddFlag(flags) has no method to get the string value of the feature gate. Hard to complete fmt.Sprintf(‘--feature-gates=%s’, o.FeatureGates).


return cmd
}
Expand Down Expand Up @@ -259,6 +264,9 @@ type CommandRegisterOption struct {
memberClusterEndpoint string
memberClusterClient *kubeclient.Clientset

// FeatureGates for alpha/experimental features of karmada-agent.
FeatureGates string

// rbacResources contains RBAC resources that grant the necessary permissions for pull mode cluster to access to Karmada control plane.
rbacResources *RBACResources
}
Expand Down Expand Up @@ -315,6 +323,15 @@ func (o *CommandRegisterOption) Validate() error {
return fmt.Errorf("need to verify CACertHashes, or set --discovery-token-unsafe-skip-ca-verification=true")
}

if len(o.FeatureGates) != 0 {
if err := features.FeatureGate.Set(o.FeatureGates); err != nil {
return fmt.Errorf("parse flag --feature-gates failed, %s", err.Error())
}
if errs := features.FeatureGate.Validate(); len(errs) != 0 {
return fmt.Errorf("invalid feature gates(%s): %s", o.FeatureGates, utilerrors.NewAggregate(errs).Error())
}
}

return nil
}

Expand Down Expand Up @@ -1030,6 +1047,7 @@ func (o *CommandRegisterOption) makeKarmadaAgentDeployment() *appsv1.Deployment
fmt.Sprintf("--proxy-server-address=%s", o.ProxyServerAddress),
fmt.Sprintf("--leader-elect-resource-namespace=%s", o.Namespace),
fmt.Sprintf("--cluster-namespace=%s", o.ClusterNamespace),
fmt.Sprintf("--feature-gates=%s", o.FeatureGates),
"--cluster-status-update-frequency=10s",
"--metrics-bind-address=:8080",
"--health-probe-bind-address=0.0.0.0:10357",
Expand Down