make lease label and lease namesapce configurable #670
Conversation
Signed-off-by: Imran Pochi <imranpochi@microsoft.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ipochi 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
approved
| @@ -159,6 +165,9 @@ func (o *GrpcProxyAgentOptions) Print() { | |||
| klog.V(1).Infof("AgentIdentifiers set to %s.\n", util.PrettyPrintURL(o.AgentIdentifiers)) | |||
| klog.V(1).Infof("WarnOnChannelLimit set to %t.\n", o.WarnOnChannelLimit) | |||
| klog.V(1).Infof("SyncForever set to %v.\n", o.SyncForever) | |||
| klog.V(1).Infof("CountServerLeases set to %v.\n", o.CountServerLeases) | |||
| @@ -132,6 +136,8 @@ func (o *GrpcProxyAgentOptions) Flags() *pflag.FlagSet { | |||
| flags.BoolVar(&o.SyncForever, "sync-forever", o.SyncForever, "If true, the agent continues syncing, in order to support server count changes.") | |||
| flags.IntVar(&o.XfrChannelSize, "xfr-channel-size", 150, "Set the size of the channel for transferring data between the agent and the proxy server.") | |||
| flags.BoolVar(&o.CountServerLeases, "count-server-leases", o.CountServerLeases, "Enables lease counting system to determine the number of proxy servers to connect to.") | |||
| flags.StringVar(&o.LeaseNamespace, "lease-namespace", o.LeaseNamespace, "Namespace where lease objects are managed.") | |||
There was a problem hiding this comment.
[Nice To Have]: The agent needs to be using the querying the same lease namespace and label as the server is writing to. Would be nice if we either got them from the server or at least validated that they are the same. Might be good to eventually add this as metadata on the handshake.
| @@ -321,6 +330,13 @@ func (o *ProxyRunOptions) Validate() error { | |||
| } | |||
| } | |||
| } | |||
| // Validate labels provided. | |||
| if o.EnableLeaseController { | |||
| _, err := util.ParseLabels(o.LeaseLabel) | |||
There was a problem hiding this comment.
rather than parsing the labels more than once, would it make sense to store the result of the parsed label at this point?
|
|
||
| for _, pair := range pairs { | ||
| keyValue := strings.SplitN(pair, "=", 2) | ||
| if len(keyValue) != 2 { |
There was a problem hiding this comment.
If you use SplitN then keyValue will only be != 2 if there are no '=' in the input. In this case I think it would be safer to use Split. This will allow "foo=bar=err" to return {"foo", "bar", "err"} and cause an error for length 3. As it stands SplitN will return {"foo", "bar=err"} which has length 2.
|
Nice change. I would like to see the Split vs SplitN change. The rest we can treat as feature requests. |
Currently the lease controller system has hard coded values for lease namespace and lease labels.
This PR adds cli flags to make those values configurable.