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.

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

Jump to bottom

Verify udn-allowed-default-services config field #4745

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Verify udn-allowed-default-services config field #4745

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
74 changes: 52 additions & 22 deletions go-controller/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import (
"strings"
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"

"github.com/urfave/cli/v2"
gcfg "gopkg.in/gcfg.v1"
lumberjack "gopkg.in/natefinch/lumberjack.v2"
"k8s.io/apimachinery/pkg/api/validation"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/client-go/tools/cache"
"k8s.io/klog/v2"

kexec "k8s.io/utils/exec"
utilnet "k8s.io/utils/net"

Expand Down Expand Up @@ -60,20 +60,20 @@ var (

// Default holds parsed config file parameters and command-line overrides
Default = DefaultConfig{
MTU: 1400,
ConntrackZone: 64000,
EncapType: "geneve",
EncapIP: "",
EncapPort: DefaultEncapPort,
InactivityProbe: 100000, // in Milliseconds
OpenFlowProbe: 180, // in Seconds
OfctrlWaitBeforeClear: 0, // in Milliseconds
MonitorAll: true,
OVSDBTxnTimeout: DefaultDBTxnTimeout,
LFlowCacheEnable: true,
RawClusterSubnets: "10.128.0.0/14/23",
Zone: types.OvnDefaultZone,
UDNAllowedDefaultServices: *cli.NewStringSlice("default/kubernetes", "kube-system/kube-dns"),
MTU: 1400,
ConntrackZone: 64000,
EncapType: "geneve",
EncapIP: "",
EncapPort: DefaultEncapPort,
InactivityProbe: 100000, // in Milliseconds
OpenFlowProbe: 180, // in Seconds
OfctrlWaitBeforeClear: 0, // in Milliseconds
MonitorAll: true,
OVSDBTxnTimeout: DefaultDBTxnTimeout,
LFlowCacheEnable: true,
RawClusterSubnets: "10.128.0.0/14/23",
Zone: types.OvnDefaultZone,
RawUDNAllowedDefaultServices: "default/kubernetes,kube-system/kube-dns",
}

// Logging holds logging-related parsed config file parameters and command-line overrides
Expand Down Expand Up @@ -282,9 +282,13 @@ type DefaultConfig struct {
// Zone name to which ovnkube-node/ovnkube-controller belongs to
Zone string `gcfg:"zone"`

// RawUDNAllowedDefaultServices holds the unparsed UDNAllowedDefaultServices. Should only be
// used inside config module.
RawUDNAllowedDefaultServices string `gcfg:"udn-allowed-default-services"`

// UDNAllowedDefaultServices holds a list of namespaced names of
// default cluster network services accessible from primary user-defined networks
UDNAllowedDefaultServices cli.StringSlice `gcfg:"udn-allowed-default-services"`
UDNAllowedDefaultServices []string
}

// LoggingConfig holds logging-related parsed config file parameters and command-line overrides
Expand Down Expand Up @@ -926,13 +930,13 @@ var CommonFlags = []cli.Flag{
Value: Default.Zone,
Destination: &cliConfig.Default.Zone,
},
&cli.StringSliceFlag{
&cli.StringFlag{
Name: "udn-allowed-default-services",
Usage: "a list of namespaced names of default cluster network services accessible from primary" +
"user-defined networks. If not specified defaults to [\"default/kubernetes\", \"kube-system/kube-dns\"]." +
"Only used when enable-network-segmentation is set",
Value: &Default.UDNAllowedDefaultServices,
Destination: &cliConfig.Default.UDNAllowedDefaultServices,
Value: Default.RawUDNAllowedDefaultServices,
Destination: &cliConfig.Default.RawUDNAllowedDefaultServices,
},
}

Expand Down Expand Up @@ -2124,13 +2128,39 @@ func completeDefaultConfig(allSubnets *ConfigSubnets) error {
allSubnets.Append(ConfigSubnetCluster, subnet.CIDR)
}

Default.UDNAllowedDefaultServices, err = parseServicesNamespacedNames(Default.RawUDNAllowedDefaultServices)
Copy link
Contributor

@ormergi ormergi Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a scenario of parse fails, config.Default will be nil.
Other component may read config, right?
If so, do you think we should change it to not override Default in case of an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default.UDNAllowedDefaultServices will be nil in case the parse fails but this is fine for a slice.
Additionally this is done at startup so a failure to parse RawUDNAllowedDefaultServices will cause ovn-k to exit early.

if err != nil {
return fmt.Errorf("UDN allowed services field is invalid: %v", err)
}

Default.HostMasqConntrackZone = Default.ConntrackZone + 1
Default.OVNMasqConntrackZone = Default.ConntrackZone + 2
Default.HostNodePortConntrackZone = Default.ConntrackZone + 3
Default.ReassemblyConntrackZone = Default.ConntrackZone + 4
return nil
}

// parseServicesNamespacedNames splits the input string by `,` and returns a slice
// of keys that were verified to be a valid namespaced service name. It ignores spaces between the elements.
func parseServicesNamespacedNames(servicesRaw string) ([]string, error) {
var services []string
for _, udnEnabledSVC := range strings.Split(servicesRaw, ",") {
svcKey := strings.TrimSpace(udnEnabledSVC)
namespace, name, err := cache.SplitMetaNamespaceKey(strings.TrimSpace(svcKey))
if namespace == "" {
return nil, fmt.Errorf("UDN enabled service %q no namespace set: %v", svcKey, err)
}
if errs := validation.ValidateNamespaceName(namespace, false); len(errs) != 0 {
return nil, fmt.Errorf("UDN enabled service %q has an invalid namespace: %v", svcKey, err)
}
if errs := validation.NameIsDNSSubdomain(name, false); len(errs) != 0 {
return nil, fmt.Errorf("UDN enabled service %q has an invalid name: %v", svcKey, err)
}
services = append(services, svcKey)
}
return services, nil
}

// getConfigFilePath returns config file path and 'true' if the config file is
// the fallback path (eg not given by the user), 'false' if given explicitly
// by the user
Expand Down
44 changes: 42 additions & 2 deletions go-controller/pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import (
"testing"
"time"

ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
"github.com/urfave/cli/v2"
kexec "k8s.io/utils/exec"

ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"

. "github.com/onsi/ginkgo"
"github.com/onsi/gomega"
)
Expand Down Expand Up @@ -1600,6 +1601,45 @@ foo=bar
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})

It("rejects a config with invalid udn allowed services", func() {
err := ioutil.WriteFile(cfgFile.Name(), []byte(`[default]
udn-allowed-default-services=namespace/invalid.name,test
`), 0o644)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

app.Action = func(ctx *cli.Context) error {
_, err = InitConfig(ctx, kexec.New(), nil)
gomega.Expect(err).To(gomega.HaveOccurred())

return nil
}
cliArgs := []string{
app.Name,
"-config-file=" + cfgFile.Name(),
}
err = app.Run(cliArgs)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})

It("accepts a config with valid udn allowed services", func() {
err := ioutil.WriteFile(cfgFile.Name(), []byte(`[default]
udn-allowed-default-services= ns/svc, ns1/svc1
`), 0o644)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

app.Action = func(ctx *cli.Context) error {
_, err = InitConfig(ctx, kexec.New(), nil)
gomega.Expect(err).ToNot(gomega.HaveOccurred())
gomega.Expect(Default.UDNAllowedDefaultServices).To(gomega.HaveLen(2))
return nil
}
cliArgs := []string{
app.Name,
"-config-file=" + cfgFile.Name(),
}
err = app.Run(cliArgs)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
})
Describe("OvnDBAuth operations", func() {
var certFile, keyFile, caFile string

Expand Down
5 changes: 3 additions & 2 deletions go-controller/pkg/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ import (
"golang.org/x/exp/constraints"
"k8s.io/client-go/tools/cache"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"crypto/rand"

Expand Down Expand Up @@ -582,7 +583,7 @@ func GetServiceEndpointSlices(namespace, svcName, network string, endpointSliceL

// IsUDNEnabledService checks whether the provided namespaced name key is a UDN enabled service specified in config.Default.UDNAllowedDefaultServices
func IsUDNEnabledService(key string) bool {
for _, enabledService := range config.Default.UDNAllowedDefaultServices.Value() {
for _, enabledService := range config.Default.UDNAllowedDefaultServices {
if enabledService == key {
return true
}
Expand Down
Loading