Skip to content

Commit

Permalink
Conditionally enable active context type check for plugin override
Browse files Browse the repository at this point in the history 
Unless features.global.plugin-override-on-active-context-type is set to
true, and plugin-level mapping is performed unconditionally, possibly
overriding some other existing CLI command group.

However, when said flag is set, mapping that would override an existing
command is only be allowed if the active context's type matches one of
the types that this mapping supports.

Signed-off-by: Vui Lam <vui.lam@broadcom.com>
  • Loading branch information
@vuil
vuil committed May 1, 2024
1 parent d909ef7 commit 60700c9
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 76 deletions.
50 changes: 38 additions & 12 deletions pkg/command/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,7 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
return nil, err
}

allPlugins, err := pluginsupplier.GetInstalledPlugins()
if err != nil {
return nil, err
}

plugins, err := pluginsupplier.FilterPluginsByActiveContextType(allPlugins)
plugins, err := pluginsupplier.GetInstalledPlugins()
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -168,6 +163,19 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
}
}

// When feature flag is set, plugin-level mapping that might overwrite existing
// commands will be allowed to do so only if the active context type
// matches the supportedContextType of the mapping. So identify plugins that
// need to be subjected to the active context check.
if config.IsFeatureActivated(constants.FeaturePluginOverrideOnActiveContextType) {
pluginsToCheck, remainingPlugins := findPluginsRequiringActiveContextCheck(rootCmd, plugins)
pluginsAllowed, err := pluginsupplier.FilterPluginsByActiveContextType(pluginsToCheck)
if err == nil {
plugins = remainingPlugins
plugins = append(plugins, pluginsAllowed...)
}
}

remapCommandTree(rootCmd, plugins)
updateTargetCommandGroupVisibility()

Expand Down Expand Up @@ -248,6 +256,29 @@ func updateTargetCommandGroupVisibility() {
}
}

func findPluginsRequiringActiveContextCheck(rootCmd *cobra.Command, plugins []cli.PluginInfo) ([]cli.PluginInfo, []cli.PluginInfo) {
var pluginsToCheck []cli.PluginInfo
var remaining []cli.PluginInfo

for i := range plugins {
toCheck := false
for _, mapEntry := range plugins[i].CommandMap {
if mapEntry.SourceCommandPath == "" {
cmd, _ := findSubCommandByPath(rootCmd, mapEntry.DestinationCommandPath)
if cmd != nil {
pluginsToCheck = append(pluginsToCheck, plugins[i])
toCheck = true
break
}
}
}
if !toCheck {
remaining = append(remaining, plugins[i])
}
}
return pluginsToCheck, remaining
}

// setupTargetPlugins sets up the commands for the plugins under the k8s and tmc targets
func setupTargetPlugins() error {
mapTargetToCmd := map[configtypes.Target]*cobra.Command{
Expand All @@ -256,16 +287,11 @@ func setupTargetPlugins() error {
configtypes.TargetOperations: opsCmd,
}

installedPlugins, err := pluginsupplier.GetInstalledPlugins()
plugins, err := pluginsupplier.GetInstalledPlugins()
if err != nil {
return fmt.Errorf("unable to find installed plugins: %w", err)
}

plugins, err := pluginsupplier.FilterPluginsByActiveContextType(installedPlugins)
if err != nil {
return err
}

convertInvokedAs(plugins)

// Insert the plugin commands under the appropriate target command
Expand Down
127 changes: 63 additions & 64 deletions pkg/command/root_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1106,13 +1106,14 @@ func setupFakePluginInfo(p fakePluginRemapAttributes, pluginDir string) *cli.Plu
// Tests behavior of command tree with commands remapped at plugin and command level
func TestCommandRemapping(t *testing.T) {
tests := []struct {
test string
pluginVariants []fakePluginRemapAttributes
args []string
expected []string
unexpected []string
expectedFailure bool
activeContextType configtypes.ContextType
test string
pluginVariants []fakePluginRemapAttributes
args []string
expected []string
unexpected []string
expectedFailure bool
activeContextType configtypes.ContextType
allowActiveContextCheck bool
}{
{
test: "one unmapped k8s plugin",
Expand Down Expand Up @@ -1284,7 +1285,8 @@ func TestCommandRemapping(t *testing.T) {
expected: []string{"hello", "Warning, multiple command groups are being remapped to the same command names : \"dummy, dummy\""},
},
{
test: "mapped plugin with supportedContextType, command is hidden at target when no active context",
// because there is nothing that the mapping would override...
test: "mapped plugin with supportedContextType, command is not hidden at target when no active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
Expand All @@ -1294,8 +1296,9 @@ func TestCommandRemapping(t *testing.T) {
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTanzu},
},
},
args: []string{"kubernetes"},
expected: []string{"No plugins are currently installed for the \"kubernetes\" target"},
args: []string{"kubernetes"},
expected: []string{"dummy2 commands"},
allowActiveContextCheck: true,
},
{
test: "mapped plugin is only one for target, top level should show target",
Expand All @@ -1310,19 +1313,6 @@ func TestCommandRemapping(t *testing.T) {
args: []string{""},
expected: []string{"kubernetes"},
},
{
test: "mapped plugin is only one for target with no active context for type hides target",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
invokedAs: []string{"dummy"},
aliases: []string{"dum"},
},
},
args: []string{"mission-control"},
expected: []string{"No plugins are currently installed for the \"mission-control\" target"},
},
{
test: "mapping plugins when conditional on active contexts",
pluginVariants: []fakePluginRemapAttributes{
Expand Down Expand Up @@ -1360,10 +1350,11 @@ func TestCommandRemapping(t *testing.T) {
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTanzu},
},
},
activeContextType: configtypes.ContextTypeK8s,
args: []string{},
expected: []string{"dummy commands"},
unexpected: []string{"dummy2 commands"},
activeContextType: configtypes.ContextTypeK8s,
allowActiveContextCheck: true,
args: []string{},
expected: []string{"dummy commands"},
unexpected: []string{"dummy2 commands"},
},
{
test: "nesting plugin within another plugin is not supported",
Expand Down Expand Up @@ -1620,7 +1611,7 @@ func TestCommandRemapping(t *testing.T) {
expected: []string{"hello", "Warning, multiple command groups are being remapped to the same command names : \"dummy, dummy\""},
},
{
test: "mapped plugin with supportedContextType, command is hidden at target when no active context",
test: "mapped plugin with supportedContextType, command is not hidden even with no active context type",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
Expand All @@ -1634,8 +1625,9 @@ func TestCommandRemapping(t *testing.T) {
},
},
},
args: []string{"kubernetes"},
expected: []string{"No plugins are currently installed for the \"kubernetes\" target"},
args: []string{"kubernetes"},
expected: []string{"kubernetes", "dummy2 commands"},
unexpected: []string{"No plugins are currently installed for the \"kubernetes\" target"},
},
{
test: "mapped plugin is only one for target, top level should show target",
Expand All @@ -1655,51 +1647,52 @@ func TestCommandRemapping(t *testing.T) {
expected: []string{"kubernetes"},
},
{
test: "mapped plugin is only one for target with no active context for type hides target",
test: "mapping ok with no active context as long as it does not override any existing",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
aliases: []string{"dum"},
name: "dummy2",
target: configtypes.TargetTMC,
aliases: []string{"dum"},
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTMC},
commandMap: []plugin.CommandMapEntry{
plugin.CommandMapEntry{
DestinationCommandPath: "dummy",
DestinationCommandPath: "mission-control dummy",
},
},
},
},
args: []string{"mission-control"},
expected: []string{"No plugins are currently installed for the \"mission-control\" target"},
args: []string{"mission-control"},
expected: []string{"dummy2 commands"},
unexpected: []string{"No plugins are currently installed for the \"mission-control\" target"},
},
{
test: "with supportedContextType and no invokedAs, command is hidden at target when no active context",
test: "mapping plugin ok when found active context type",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTMC},
aliases: []string{"dum"},
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
},
args: []string{"mission-control"},
expected: []string{"No plugins are currently installed for the \"mission-control\" target"},
},
{
test: "with supportedContextType set, target not shown at top level when no active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTMC},
target: configtypes.TargetK8s,
aliases: []string{"dum"},
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTanzu},
commandMap: []plugin.CommandMapEntry{
plugin.CommandMapEntry{
DestinationCommandPath: "dummy",
},
},
},
},
activeContextType: configtypes.ContextTypeTanzu,
args: []string{},
unexpected: []string{"mission-control"},
activeContextType: configtypes.ContextTypeTanzu,
allowActiveContextCheck: true,
args: []string{},
expected: []string{"dummy2 commands"},
unexpected: []string{"dummy commands"},
},
{
test: "mapping plugins when conditional on active contexts",
test: "mapping ok when feature flag not true, regardless of active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy",
Expand All @@ -1718,13 +1711,14 @@ func TestCommandRemapping(t *testing.T) {
},
},
},
activeContextType: configtypes.ContextTypeTanzu,
args: []string{},
expected: []string{"dummy2 commands"},
unexpected: []string{"dummy commands"},
activeContextType: configtypes.ContextTypeK8s,
allowActiveContextCheck: false,
args: []string{},
expected: []string{"dummy2 commands"},
unexpected: []string{"dummy commands"},
},
{
test: "no mapping if active context not one of supportContextType list",
test: "when feature flag set, no mapping if active context not one of supportContextType list",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy",
Expand All @@ -1743,10 +1737,11 @@ func TestCommandRemapping(t *testing.T) {
},
},
},
activeContextType: configtypes.ContextTypeK8s,
args: []string{},
expected: []string{"dummy commands"},
unexpected: []string{"dummy2 commands"},
activeContextType: configtypes.ContextTypeK8s,
allowActiveContextCheck: true,
args: []string{},
expected: []string{"dummy commands"},
unexpected: []string{"dummy2 commands"},
},
{
test: "nesting plugin within another plugin is not supported",
Expand Down Expand Up @@ -1957,6 +1952,10 @@ func TestCommandRemapping(t *testing.T) {
t.Run(spec.test, func(t *testing.T) {
assert := assert.New(t)

configErr := config.ConfigureFeatureFlags(map[string]bool{
"features.global.plugin-override-on-active-context-type": spec.allowActiveContextCheck}, config.SkipIfExists())
assert.Nil(configErr)

r, w, err := os.Pipe()
if err != nil {
t.Error(err)
Expand Down
5 changes: 5 additions & 0 deletions pkg/constants/featureflags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ const (
// FeaturePluginDiscoveryForTanzuContext determines whether to enable context-scoped plugin discovery for Tanzu context.
// This is disabled by default
FeaturePluginDiscoveryForTanzuContext = "features.global.plugin-discovery-for-tanzu-context"

// FeaturePluginOverrideOnActiveContextType determines whether a plugin-level mapping that potentially
// overrides an existing CLI command group should be conditional on the active context type or not.
// When false, the mapping will be unconditionally applied.
FeaturePluginOverrideOnActiveContextType = "features.global.plugin-override-on-active-context-type"
)

// DefaultCliFeatureFlags is used to populate an initially empty config file with default values for feature flags.
Expand Down

0 comments on commit 60700c9

Please sign in to comment.