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.

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

#1945: Adding support for regular expression values for the Interface Exclude parameter #1980

Conversation

Projects
None yet
3 participants
@stevegaossou
Copy link
Contributor

commented Mar 11, 2019

Closes #1945.

Description

This PR contains:

  • modifying logic for interface exclude mechanism to allow for regular expressions instead of just exact string matches (#1945)
  • modified existing and added new unit tests (check the commits)
  • this component affects felix and also documentation for calico and libcalico-go:

Todos

  • Unit tests (full coverage)
    • Modified existing test
  • Integration tests (delete as appropriate) In plan/Not needed/Done
  • Documentation
  • Backport
  • Release note

Release Note

Allow the entries for the exclude interface parameter to be regular expression patterns
#1945: Adding support for regular expression values for the Interface…
…Exclude parameter. Still need to modify the validation on parsing interface values.

@stevegaossou stevegaossou requested a review from projectcalico/core-maintainers as a code owner Mar 11, 2019

@stevegaossou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

One outstanding question I have is what we want to allow for the validation of the field now that it allows regexp patterns (since it could support wildcard as well as other symbols).

Namely here in ./config/config_params.go for the IfaceListRegexp parameter.

For example, should we keep the current validation pattern but only allow the introduction of the wildcard symbol * anywhere in the value?

cc @caseydavenport

@neiljerram
Copy link
Member

left a comment

Couple of thoughts here.

Show resolved Hide resolved config/config_params.go Outdated
Show resolved Hide resolved ifacemonitor/iface_monitor.go
@@ -143,7 +151,7 @@ type Config struct {
OpenstackRegion string `config:"region;;die-on-fail"`

InterfacePrefix string `config:"iface-list;cali;non-zero,die-on-fail"`
InterfaceExclude string `config:"iface-list;kube-ipvs0"`
InterfaceExclude string `config:"iface-list-regexp;kube-ipvs0;die-on-fail"`

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 13, 2019

Author Contributor

Calling this out, since it changes the default behaviour.

Added the die-on-fail option, because it seemed sensible for it to halt on failed validation (it's going to panic anyways during the Config.InterfaceExcludes() if any of the individual values do not compile).

But there might be a very good reason why die-on-fail was not there in the first place! Please let me know if that is the case.

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

I'm not sure I'm fully understanding here, but it feels wrong for this detail of behaviour to change. I think it's straightforward to avoid "it's going to panic anyways ... if any of the individual values do not compile" by using regexp.Compile in that code instead of regexp.MustCompile.

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

(die-on-fail will cause Felix to go into a restart loop, so we generally specify die-on-fail only for essential config parameters)

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 14, 2019

Author Contributor

I think it's straightforward to avoid "it's going to panic anyways ... if any of the individual values do not compile" by using regexp.Compile in that code instead of regexp.MustCompile.

Right.

When I originally considered this I came to a conclusion that using regexp.Compile to avoid a panic would require knowing what we want to do when one of the user provided values fails to compile to regexp. What should we do in that case?

We can log an error message as a bare minimum. But if we gracefully continue onwards, it means we don't setup that particular interface exclude value. I just wasn't sure from a user experience standpoint what our acceptable behaviour was. E.g. if I wanted to exclude all interfaces with a name starting with kube but I somehow mess up the syntax such that it doesn't compile. Ideally I would want felix to catch my mistake and halt during startup to tell me so that I can correct myself.

So that was my original intention.

(die-on-fail will cause Felix to go into a restart loop, so we generally specify die-on-fail only for essential config parameters)

Fair point. I will remove the die-on-fail and switch the compiling of values from regexp.MustCompile to regexp.Compile. 👍

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 15, 2019

Author Contributor

Actually looking at the logic for handling die-on-fail (DieOnParseFailure flag) after parsing a parameter it looks like it wipes the error and then sets the value to default, which for InterfaceExclude is kube-ipvs0. That seems undesirable (especially now that we could have regex values), no?

https://github.com/projectcalico/felix/blob/master/config/config_params.go#L335

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 15, 2019

Member

Yes, the behaviour for a parse error for a parameter without die-on-fail is to log a warning and then use the default value. I think that's what we should do here too.

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 15, 2019

Author Contributor

Okay, sounds good. I thought it over again and you're right, there's no reason to change this behaviour from what it's always done. That's what users would expect since it's established.

Changing it back.

@stevegaossou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Woohoo! We're finally 🍏on all tests!

PR for documentation update is here:
projectcalico/calico#2490

@neiljerram
Copy link
Member

left a comment

Sorry, I've generated some rather fragmented review comments here, but I think what it adds up to is:

  • some simplifications
  • do parsing and regexp compilation in Parse, and return a slice of regexps
  • revert some unnecessary or unwanted changes.

PTAL and let me know what you think.

Show resolved Hide resolved config/config_params.go Outdated
Show resolved Hide resolved config/config_params.go Outdated
Show resolved Hide resolved config/config_params.go Outdated
Show resolved Hide resolved config/config_params_test.go Outdated
Entry("InterfaceExclude no regexp", "InterfaceExclude", "/^kube.*/,/veth/", "/^kube.*/,/veth/"),
Entry("InterfaceExclude list regexp invalid", "InterfaceExclude", "kube,//", "kube-ipvs0", true), // empty regexp value
Entry("InterfaceExclude list regexp invalid comma", "InterfaceExclude", "/kube,/,dummy", "kube-ipvs0", true), // bad comma use
Entry("InterfaceExclude list regexp invalid comma", "InterfaceExclude", `/^kube\K/`, "kube-ipvs0", true), // Invalid regexp

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

I think "comma" is wrong here.

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

Also, now that the InterfaceExclude is more complex, I wonder if we are missing an opportunity here to go further in the parsing step. Cf. how EtcdEndpoints is parsed into a []string.

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 15, 2019

Author Contributor

That would be nice. But I'm hesitant to do it because I'm not sure how the change affects the FelixConfig resource.

I think I'd rather not make that change just to make this smaller.

}
}
}
result = raw

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

I think it could be nice to return a slice of regexps here, instead of just the input again.

Show resolved Hide resolved ifacemonitor/iface_monitor.go
@@ -143,7 +151,7 @@ type Config struct {
OpenstackRegion string `config:"region;;die-on-fail"`

InterfacePrefix string `config:"iface-list;cali;non-zero,die-on-fail"`
InterfaceExclude string `config:"iface-list;kube-ipvs0"`
InterfaceExclude string `config:"iface-list-regexp;kube-ipvs0;die-on-fail"`

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

I'm not sure I'm fully understanding here, but it feels wrong for this detail of behaviour to change. I think it's straightforward to avoid "it's going to panic anyways ... if any of the individual values do not compile" by using regexp.Compile in that code instead of regexp.MustCompile.

@@ -143,7 +151,7 @@ type Config struct {
OpenstackRegion string `config:"region;;die-on-fail"`

InterfacePrefix string `config:"iface-list;cali;non-zero,die-on-fail"`
InterfaceExclude string `config:"iface-list;kube-ipvs0"`
InterfaceExclude string `config:"iface-list-regexp;kube-ipvs0;die-on-fail"`

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 14, 2019

Member

(die-on-fail will cause Felix to go into a restart loop, so we generally specify die-on-fail only for essential config parameters)

Show resolved Hide resolved config/config_params.go Outdated
@neiljerram

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@stevegaossou By the way, another thought: when working with Felix config parameters, there are generally a few related places to think about:

  1. Equivalent validation in libcalico-go as well, because that allows calicoctl to catch and report errors with a much nicer UX than Felix either restarting or warning.

  2. Matching doc update for the libcalico-go field.

  3. Matching doc update in the docs.

In this case, the libcalico-go field has no validate tag:

	// InterfaceExclude is a list of interfaces that Felix should exclude when monitoring for host
	// endpoints.  The default value ensures that Felix ignores Kubernetes' IPVS dummy interface,
	// which is used internally by kube-proxy.  [Default: kube-ipvs0]
	InterfaceExclude string `json:"interfaceExclude,omitempty"`

which means that there is no detailed libcalico-go validation here. And it's probably too big a job to start adding that now.

(Yes, it's kinda poor that in principle we have config validation in both Felix and libcalico-go... It would be good to work towards improving that, but I think it's a big task.)

But the updates (2) and (3) will be needed too.

@stevegaossou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

  1. Matching doc update in the docs.

For the calico repo level documentation, this change will be going in the next release 3.7, right?

So I only need to update the ./master/ version of the docs.

👍 or 👎?

@neiljerram
Copy link
Member

left a comment

I've commented on the point about die-on-fail and using the default value. I think that if you agree, and follow through on that, it will significantly reduce the size of this change - so then I think it would make sense for me to review again (hopefully for the last time) after that change is done.

@@ -143,7 +151,7 @@ type Config struct {
OpenstackRegion string `config:"region;;die-on-fail"`

InterfacePrefix string `config:"iface-list;cali;non-zero,die-on-fail"`
InterfaceExclude string `config:"iface-list;kube-ipvs0"`
InterfaceExclude string `config:"iface-list-regexp;kube-ipvs0;die-on-fail"`

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 15, 2019

Member

Yes, the behaviour for a parse error for a parameter without die-on-fail is to log a warning and then use the default value. I think that's what we should do here too.

Switching from MustCompile to Compile and logging any compile error f…
…or InterfaceExclude; removing die-on-fail flag.
@stevegaossou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Just leaving a status update on this PR.

All code review changes have been addressed.

Doc update PRs can be found here:

compileAndAdd(parsedValue)
}
}
return regexpValues

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 18, 2019

Member

Steve, I'm afraid this still isn't how I was expecting - so I guess I've done a poor job of communicating that. I've posted a commit - which is a delta on top of your changes - at neiljerram@77a59b0, which I hope will show more clearly what I've been trying to express.

Could you take a look at that and let me know what you think?

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 18, 2019

Author Contributor

Hey @neiljerram I just took a look at your changes.

I see two main changes:

  • Making the parse do more than just validation (so it returns the slice of Regexp objects)
  • This makes InterfaceExcludes() redundant, so it's been removed

I don't think you did a poor job of communicating this as you had a comment about having the Parse return an array of the values instead of the raw (granted this version now returns a slice of Regexp, not just slice of strings). I remember making a comment that I wasn't 100% sure on how that change would affect other components that use felix (e.g. FelixConfig), specifically this change. I recall you said it doesn't affect them and I see what you mean now. The scope of changing that field from string to []*Regexp is still only internal to felix.

If my understanding is correct, your changes look fine to me, shall I just apply your delta on my branch?

This comment has been minimized.

Copy link
@neiljerram

neiljerram Mar 18, 2019

Member

Yes, I think so, and then I believe we can call this done!

This comment has been minimized.

Copy link
@stevegaossou

stevegaossou Mar 18, 2019

Author Contributor

Applied and pushed! Was just waiting for CI to pass before I added a comment here confirming.

Thanks very much for all the feedback @neiljerram! Much appreciated.

Applying code review recommenations: do all validation and compiling …
…of regexp in RegexpPatternListParam.Parse function, remove InterfaceExcludes function.
@neiljerram
Copy link
Member

left a comment

Looks good now; thanks Steve!

@caseydavenport caseydavenport added this to the Calico v3.7.0 milestone Mar 18, 2019

@caseydavenport caseydavenport merged commit 09ac4f7 into projectcalico:master Mar 19, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@stevegaossou stevegaossou deleted the stevegaossou:stevegaossou-1945-regex-interface-exclude branch Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.