Disabling ASM mode if the Istio CRD is not available. #942
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @cadmuxe. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/cc @freehan |
k8s-ci-robot
added
the
size/L
| @@ -204,7 +204,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 | |||
| flag.BoolVar(&F.EnableL7Ilb, "enable-l7-ilb", false, | |||
| `Optional, whether or not to enable L7-ILB.`) | |||
| flag.BoolVar(&F.EnableASMConfigMapBasedConfig, "enable-asm-config-map-config", false, "Enable ASMConfigMapBasedConfig") | |||
| flag.StringVar(&F.ASMConfigMapBasedConfigNamespace, "asm-configmap-based-config-namespace", "kube-system,istio-system", "ASM Configmap based config: configmap namespace") | |||
There was a problem hiding this comment.
This is a bug, it's the configmap namespace, but not the skip namespace.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/cmconfig/config.go
Outdated
| @@ -24,7 +24,7 @@ const ( | |||
|
|
|||
| // NewConfig returns a Conifg instances with default values. | |||
| func NewConfig() Config { | |||
| return Config{ASMServiceNEGSkipNamespaces: []string{"kube-system"}} | |||
| return Config{ASMServiceNEGSkipNamespaces: []string{"kube-system", "istio-system"}} | |||
There was a problem hiding this comment.
I would recommend specifying Enable=false as well.
That is more readable.
There was a problem hiding this comment.
istio-system was in the default skip flag, I just forgot to move it here.
There was a problem hiding this comment.
yes, explicitly config is good.
pkg/context/context.go
Outdated
| @@ -127,6 +130,7 @@ func NewControllerContext( | |||
|
|
|||
| // Init inits the Context, so that we can defers some config until the main thread enter actually get the leader lock. | |||
| func (ctx *ControllerContext) Init() { | |||
| klog.Info("Init ControllerContext...") | |||
There was a problem hiding this comment.
klog.V(2).Info("Controller Context initializing with %v", ctx.ControllerContextConfig)
pkg/context/context.go
Outdated
|
|
||
| apiextensionClient, err := apiextensionsclientset.NewForConfig(ctx.KubeConfig) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("Failed to validate DestinationRule CRD: failed to create apiextensionClient, disabling ASM Mode, error: %s", err) |
There was a problem hiding this comment.
I am not sure if this is related to DestinationRule CRD at all. This just means that the clientSet cannot be initialized, right?
There was a problem hiding this comment.
yes, but it's a required step to validate the CRD
There was a problem hiding this comment.
I mean the message should be failed to create extension clientset right? Not validating destinationRule CRD.
| @@ -58,6 +63,23 @@ func (c *ConfigMapConfigController) GetConfig() Config { | |||
| return *c.currentConfig | |||
| } | |||
|
|
|||
| // DisableASMMode disables the ASM Mode by updating the ConfigMap and setting the internal flag. | |||
| func (c *ConfigMapConfigController) DisableASMMode() { | |||
There was a problem hiding this comment.
I am thinking if it is possible to not patch the original config map but just record events. Patching configmap requires some RBAC changes I assume?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cadmuxe, freehan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
lgtm
No description provided.