Skip to content

[WIP] Attempt at feature-gating the default image choice #3313

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

Closed
jkyros wants to merge 1 commit into from
Closed

[WIP] Attempt at feature-gating the default image choice #3313

Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions cmd/machine-config-controller/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle
ctx.InformerFactory.Machineconfiguration().V1().MachineConfigPools(),
ctx.InformerFactory.Machineconfiguration().V1().MachineConfigs(),
ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(),
ctx.ConfigInformerFactory.Config().V1().FeatureGates(),
ctx.ClientBuilder.KubeClientOrDie("render-controller"),
ctx.ClientBuilder.MachineConfigClientOrDie("render-controller"),
),
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func (b *Bootstrap) Run(destDir string) error {
configs = append(configs, kconfigs...)
}

fpools, gconfigs, err := render.RunBootstrap(pools, configs, cconfig)
fpools, gconfigs, err := render.RunBootstrap(pools, configs, cconfig, featureGate)
if err != nil {
return err
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/controller/common/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ import (

mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
mcfgclientset "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"

osev1 "github.com/openshift/api/config/v1"
)

// strToPtr converts the input string to a pointer to itself
Expand Down Expand Up @@ -971,3 +973,35 @@ func GetLongestValidCertificate(certificateList []*x509.Certificate, subjectPref
}
return nil
}

// IsFeatureGateEnabled checks to see if a particular feature gate is enabled given a feature gate config. This function like something
// that should already exist somewhere in a proper library, maybe I missed it?
func IsFeatureGateEnabled(featureGates *osev1.FeatureGate, featureGate string) bool {
// If we have feature gates at all
if featureGates != nil {
// See if there's a feature set
if featureGates.Spec.FeatureSet != "" {
// See if we can look up that set's features
if features, ok := osev1.FeatureSets[featureGates.Spec.FeatureSet]; ok {
// If we can, go through the features and see if it's in the list
for _, feature := range features.Enabled {
if feature == featureGate {
glog.V(2).Infof("Feature %s is present as part of featureSet %s", feature, featureGates.Spec.FeatureSet)
return true
}
}
}
}

// But also, these could be supplied singly, outside of the set in customnoupgrade
// TODO(jkyros): it is probably bad form to specify one
for _, feature := range featureGates.Spec.CustomNoUpgrade.Enabled {
if feature == featureGate {
glog.V(2).Infof("Feature %s is present in CustomNoUpgrade section", feature)
return true
}
}
}
return false

}
37 changes: 31 additions & 6 deletions pkg/controller/render/render_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import (
"time"

"github.com/golang/glog"
osev1 "github.com/openshift/api/config/v1"
oseinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
oselistersv1 "github.com/openshift/client-go/config/listers/config/v1"
mcoResourceApply "github.com/openshift/machine-config-operator/lib/resourceapply"
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
Expand Down Expand Up @@ -68,13 +71,17 @@ type Controller struct {
ccListerSynced cache.InformerSynced

queue workqueue.RateLimitingInterface

featLister oselistersv1.FeatureGateLister
featListerSynced cache.InformerSynced
}

// New returns a new render controller.
func New(
mcpInformer mcfginformersv1.MachineConfigPoolInformer,
mcInformer mcfginformersv1.MachineConfigInformer,
ccInformer mcfginformersv1.ControllerConfigInformer,
featureInformer oseinformersv1.FeatureGateInformer,
kubeClient clientset.Interface,
mcfgClient mcfgclientset.Interface,
) *Controller {
Expand Down Expand Up @@ -109,6 +116,9 @@ func New(
ctrl.ccLister = ccInformer.Lister()
ctrl.ccListerSynced = ccInformer.Informer().HasSynced

ctrl.featLister = featureInformer.Lister()
ctrl.featListerSynced = featureInformer.Informer().HasSynced

return ctrl
}

Expand Down Expand Up @@ -482,7 +492,14 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo
return err
}

generated, err := generateRenderedMachineConfig(pool, configs, cc)
// TODO(jkyros): right now we don't sync every time features change, should we? Or maybe since we care when controllerconfig
// changes we could cheat and just put the featuregates in there?
fg, err := ctrl.featLister.Get(ctrlcommon.ClusterFeatureInstanceName)
if err != nil && !errors.IsNotFound(err) {
return err
}

generated, err := generateRenderedMachineConfig(pool, configs, cc, fg)
if err != nil {
return err
}
Expand Down Expand Up @@ -539,7 +556,7 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo
}

// generateRenderedMachineConfig takes all MCs for a given pool and returns a single rendered MC. For ex master-XXXX or worker-XXXX
func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) (*mcfgv1.MachineConfig, error) {
func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, featureGate *osev1.FeatureGate) (*mcfgv1.MachineConfig, error) {
// Suppress rendered config generation until a corresponding new controller can roll out too.
// https://bugzilla.redhat.com/show_bug.cgi?id=1879099
if genver, ok := cconfig.Annotations[daemonconsts.GeneratedByVersionAnnotationKey]; ok {
Expand All @@ -563,7 +580,15 @@ func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mc
}
}

merged, err := ctrlcommon.MergeMachineConfigs(configs, cconfig.Spec.OSImageURL)
defaultImageURL := cconfig.Spec.OSImageURL
// TODO(jkyros): For now, default to the new format image only if the feature gate is enabled, we will
// come back for this to remove it when the machine-os-content is retired from the payload
if ctrlcommon.IsFeatureGateEnabled(featureGate, "MCONewContainerImageFormat") {
glog.Warningf("Defaulting to OSImageURL %s (instead of: %s) because of feature gate %s", cconfig.Spec.BaseOperatingSystemContainer, defaultImageURL, "MCONewContainerImageFormat")
defaultImageURL = cconfig.Spec.BaseOperatingSystemContainer
}

merged, err := ctrlcommon.MergeMachineConfigs(configs, defaultImageURL)
if err != nil {
return nil, err
}
Expand All @@ -583,7 +608,7 @@ func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mc

// Make it obvious that the OSImageURL has been overridden. If we log this in MergeMachineConfigs, we don't know the name yet, so we're
// logging out here instead so it's actually helpful.
if merged.Spec.OSImageURL != cconfig.Spec.OSImageURL {
if merged.Spec.OSImageURL != defaultImageURL {
glog.Infof("OSImageURL has been overridden via machineconfig in %s (was: %s is: %s)", merged.Name, cconfig.Spec.OSImageURL, merged.Spec.OSImageURL)
}

Expand All @@ -593,7 +618,7 @@ func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mc
// RunBootstrap runs the render controller in bootstrap mode.
// For each pool, it matches the machineconfigs based on label selector and
// returns the generated machineconfigs and pool with CurrentMachineConfig status field set.
func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) ([]*mcfgv1.MachineConfigPool, []*mcfgv1.MachineConfig, error) {
func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig, featureGate *osev1.FeatureGate) ([]*mcfgv1.MachineConfigPool, []*mcfgv1.MachineConfig, error) {
var (
opools []*mcfgv1.MachineConfigPool
oconfigs []*mcfgv1.MachineConfig
Expand All @@ -604,7 +629,7 @@ func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineCo
return nil, nil, err
}

generated, err := generateRenderedMachineConfig(pool, pcs, cconfig)
generated, err := generateRenderedMachineConfig(pool, pcs, cconfig, featureGate)
if err != nil {
return nil, nil, err
}
Expand Down
30 changes: 18 additions & 12 deletions pkg/controller/render/render_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"

oseconfigfake "github.com/openshift/client-go/config/clientset/versioned/fake"
oseinformersv1 "github.com/openshift/client-go/config/informers/externalversions"
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants"
Expand All @@ -41,15 +43,17 @@ var (
type fixture struct {
t *testing.T

client *fake.Clientset
client *fake.Clientset
oseclient *oseconfigfake.Clientset

mcpLister []*mcfgv1.MachineConfigPool
mcLister []*mcfgv1.MachineConfig
ccLister []*mcfgv1.ControllerConfig

actions []core.Action

objects []runtime.Object
objects []runtime.Object
oseobjects []runtime.Object
}

func newFixture(t *testing.T) *fixture {
Expand All @@ -61,11 +65,13 @@ func newFixture(t *testing.T) *fixture {

func (f *fixture) newController() *Controller {
f.client = fake.NewSimpleClientset(f.objects...)
f.oseclient = oseconfigfake.NewSimpleClientset(f.oseobjects...)

i := informers.NewSharedInformerFactory(f.client, noResyncPeriodFunc())
fi := oseinformersv1.NewSharedInformerFactory(f.oseclient, noResyncPeriodFunc())

c := New(i.Machineconfiguration().V1().MachineConfigPools(), i.Machineconfiguration().V1().MachineConfigs(),
i.Machineconfiguration().V1().ControllerConfigs(), k8sfake.NewSimpleClientset(), f.client)
i.Machineconfiguration().V1().ControllerConfigs(), fi.Config().V1().FeatureGates(), k8sfake.NewSimpleClientset(), f.client)

c.mcpListerSynced = alwaysReady
c.mcListerSynced = alwaysReady
Expand Down Expand Up @@ -290,7 +296,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) {
}
cc := newControllerConfig(ctrlcommon.ControllerConfigName)

_, err := generateRenderedMachineConfig(mcp, mcs, cc)
_, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
require.Nil(t, err)

// verify that an invalid ignition config (here a config with content and an empty version,
Expand All @@ -302,7 +308,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) {
require.Nil(t, err)
mcs[1].Spec.Config.Raw = rawIgnCfg

_, err = generateRenderedMachineConfig(mcp, mcs, cc)
_, err = generateRenderedMachineConfig(mcp, mcs, cc, nil)
require.NotNil(t, err)

// verify that a machine config with no ignition content will not fail validation
Expand All @@ -311,7 +317,7 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) {
require.Nil(t, err)
mcs[1].Spec.Config.Raw = rawEmptyIgnCfg
mcs[1].Spec.KernelArguments = append(mcs[1].Spec.KernelArguments, "test1")
_, err = generateRenderedMachineConfig(mcp, mcs, cc)
_, err = generateRenderedMachineConfig(mcp, mcs, cc, nil)
require.Nil(t, err)

}
Expand All @@ -334,7 +340,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) {
}
cc := newControllerConfig(ctrlcommon.ControllerConfigName)

gmc, err := generateRenderedMachineConfig(mcp, mcs, cc)
gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -353,7 +359,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) {
f.mcLister = append(f.mcLister, gmc)
f.objects = append(f.objects, gmc)

expmc, err := generateRenderedMachineConfig(mcp, mcs, cc)
expmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -379,7 +385,7 @@ func TestGenerateMachineConfigOverrideOSImageURL(t *testing.T) {

cc := newControllerConfig(ctrlcommon.ControllerConfigName)

gmc, err := generateRenderedMachineConfig(mcp, mcs, cc)
gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -395,12 +401,12 @@ func TestVersionSkew(t *testing.T) {

cc := newControllerConfig(ctrlcommon.ControllerConfigName)
cc.Annotations[daemonconsts.GeneratedByVersionAnnotationKey] = "different-version"
_, err := generateRenderedMachineConfig(mcp, mcs, cc)
_, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
require.NotNil(t, err)

// Now the same thing without overriding the version
cc = newControllerConfig(ctrlcommon.ControllerConfigName)
gmc, err := generateRenderedMachineConfig(mcp, mcs, cc)
gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
require.Nil(t, err)
require.NotNil(t, gmc)
}
Expand All @@ -423,7 +429,7 @@ func TestDoNothing(t *testing.T) {
}
cc := newControllerConfig(ctrlcommon.ControllerConfigName)

gmc, err := generateRenderedMachineConfig(mcp, mcs, cc)
gmc, err := generateRenderedMachineConfig(mcp, mcs, cc, nil)
if err != nil {
t.Fatal(err)
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/controller/template/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,10 @@ func generateMachineConfigForName(config *RenderConfig, role, name, templateDir,
return nil, fmt.Errorf("error creating MachineConfig from Ignition config: %w", err)
}
// And inject the osimageurl here
mcfg.Spec.OSImageURL = config.OSImageURL
// TODO(jkyros): if we include this here, we can't tell if a user did it or not, it bites us when we're
// trying to switch between oldformat/newformat. We can safely exclude it because it gets superceded by what's in
// controllerconfig when we merge configs anyway
//mcfg.Spec.OSImageURL = config.OSImageURL

return mcfg, nil
}
Expand Down