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

NET-5104 envoy port mapping #2707

Merged
merged 24 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bda5f57
Adds port mapping to avoid running envoy on admin ports
missylbytes Jul 31, 2023
88611e2
add tests, only add to value for privileged ports
missylbytes Aug 1, 2023
34def4c
added changelog
missylbytes Aug 1, 2023
b7adcf9
missed some tests
missylbytes Aug 1, 2023
73b9029
Update control-plane/api-gateway/common/translation.go
missylbytes Aug 2, 2023
3c5d8a5
Update .changelog/2707.txt
missylbytes Aug 2, 2023
531170f
Update control-plane/api-gateway/common/translation.go
missylbytes Aug 2, 2023
db21d04
adds basic check for port conflicts, needs details ironed out
missylbytes Aug 2, 2023
2ae6d92
cleanup
missylbytes Aug 2, 2023
64e446d
fix imports
missylbytes Aug 2, 2023
01a563d
add one more test for user-conflicted ports and mapping-conflicted ports
missylbytes Aug 2, 2023
854289f
moved port-mapping to gatewayclassconfig
missylbytes Aug 7, 2023
eeea753
fix typo, update changelog
missylbytes Aug 7, 2023
c75334c
more naming cleanup
missylbytes Aug 7, 2023
19ada94
Add validation tests for mapping privileged container ports
nathancoleman Aug 7, 2023
9575c29
Fix translation_test.go
nathancoleman Aug 7, 2023
1fceb65
Use correct flag name for privileged port mapping
nathancoleman Aug 7, 2023
fe8267f
Correct casing of ManagedGatewayClass property
nathancoleman Aug 7, 2023
0db299c
Update GatewayClassConfig in test to map privileged container ports
nathancoleman Aug 7, 2023
1b9b752
Update control-plane/api-gateway/binding/binder_test.go
nathancoleman Aug 7, 2023
2dd5c5a
fix linting issue
missylbytes Aug 7, 2023
3d1df20
update comment for binding result
missylbytes Aug 7, 2023
72eb032
lint
missylbytes Aug 8, 2023
cbb7409
Apply suggestions from code review
missylbytes Aug 8, 2023
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
3 changes: 3 additions & 0 deletions .changelog/2707.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:feature
api-gateway: adds ability to map privileged ports on Gateway listeners to unprivileged ports so that containers do not require additional privileges
```
9 changes: 9 additions & 0 deletions charts/consul/templates/crd-gatewayclassconfigs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,15 @@ spec:
description: The name of an existing SecurityContextConstraints
resource to bind to the managed role when running on OpenShift.
type: string
mapPrivilegedContainerPorts:
type: integer
format: int32
minimum: 0
maximum: 64512
description: mapPrivilegedContainerPorts is the value which Consul will add to privileged container port
values (ports < 1024) defined on a Gateway when the number is greater than 0. This cannot be more than
64512 as the highest privileged port is 1023, which would then map to 65535, which is the highest
valid port number.
type: object
type: object
served: true
Expand Down
1 change: 1 addition & 0 deletions charts/consul/templates/gateway-resources-job.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ spec:
{{- if .Values.global.openshift.enabled }}
- -openshift-scc-name={{ .Values.connectInject.apiGateway.managedGatewayClass.openshiftSCCName }}
{{- end }}
- -map-privileged-container-ports={{ .Values.connectInject.apiGateway.managedGatewayClass.mapPrivilegedContainerPorts }}
{{- end}}
resources:
requests:
Expand Down
6 changes: 6 additions & 0 deletions charts/consul/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2206,6 +2206,12 @@ connectInject:
# @type: string
openshiftSCCName: "restricted-v2"

# This value defines the amount we will add to privileged container ports on gateways that use this class.
# This is useful if you don't want to give your containers extra permissions to run privileged ports.
# Example: The gateway listener is defined on port 80, but the underlying value of the port on the container
# will be the 80 + the number defined below.
mapPrivilegedContainerPorts: 0

# Configuration for the ServiceAccount created for the api-gateway component
serviceAccount:
# This value defines additional annotations for the client service account. This should be formatted as a multi-line
Expand Down
13 changes: 7 additions & 6 deletions cli/helm/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -576,12 +576,13 @@ type CopyAnnotations struct {
}

type ManagedGatewayClass struct {
Enabled bool `yaml:"enabled"`
NodeSelector interface{} `yaml:"nodeSelector"`
ServiceType string `yaml:"serviceType"`
UseHostPorts bool `yaml:"useHostPorts"`
CopyAnnotations CopyAnnotations `yaml:"copyAnnotations"`
OpenshiftSCCName string `yaml:"openshiftSCCName"`
Enabled bool `yaml:"enabled"`
NodeSelector interface{} `yaml:"nodeSelector"`
ServiceType string `yaml:"serviceType"`
UseHostPorts bool `yaml:"useHostPorts"`
CopyAnnotations CopyAnnotations `yaml:"copyAnnotations"`
OpenshiftSCCName string `yaml:"openshiftSCCName"`
MapPrivilegedContainerPorts int `yaml:"mapPrivilegedContainerPorts"`
}

type Service struct {
Expand Down
4 changes: 2 additions & 2 deletions control-plane/api-gateway/binding/binder.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func (b *Binder) Snapshot() *Snapshot {

// calculate the status for the gateway
gatewayValidation = validateGateway(b.config.Gateway, registrationPods, b.config.ConsulGateway)
listenerValidation = validateListeners(b.config.Gateway, b.config.Gateway.Spec.Listeners, b.config.Resources)
listenerValidation = validateListeners(b.config.Gateway, b.config.Gateway.Spec.Listeners, b.config.Resources, b.config.GatewayClassConfig)
}

// used for tracking how many routes have successfully bound to which listeners
Expand Down Expand Up @@ -182,7 +182,7 @@ func (b *Binder) Snapshot() *Snapshot {
if b.config.ConsulGateway != nil {
consulStatus = b.config.ConsulGateway.Status
}
entry := b.config.Translator.ToAPIGateway(b.config.Gateway, b.config.Resources)
entry := b.config.Translator.ToAPIGateway(b.config.Gateway, b.config.Resources, gatewayClassConfig)
snapshot.Consul.Updates = append(snapshot.Consul.Updates, &common.ConsulUpdateOperation{
Entry: entry,
OnUpdate: b.handleGatewaySyncStatus(snapshot, &b.config.Gateway, consulStatus),
Expand Down
8 changes: 7 additions & 1 deletion control-plane/api-gateway/binding/result.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,12 @@ var (
// We map anything under here to a custom ListenerConditionReason of Invalid on
// an Accepted status type.
errListenerNoTLSPassthrough = errors.New("TLS passthrough is not supported")

// This custom listener validation error is used to differentiate between an errListenerPortUnavailable because of
// direct port conflicts defined by the user (two listeners on the same port) vs a port conflict because we map
// privileged ports by adding the value passed into the gatewayClassConfig.
// (i.e. one listener on 80 with a privileged port mapping of 2000, and one listener on 2080 would conflict).
errListenerMappedToPrivilegedPortMapping = errors.New("listener conflicts with privileged port mapped by GatewayClassConfig privileged port mapping setting")
)

// listenerValidationResult contains the result of internally validating a single listener
Expand Down Expand Up @@ -291,7 +297,7 @@ func (l listenerValidationResult) programmedCondition(generation int64) metav1.C
func (l listenerValidationResult) acceptedCondition(generation int64) metav1.Condition {
now := timeFunc()
switch l.acceptedErr {
case errListenerPortUnavailable:
case errListenerPortUnavailable, errListenerMappedToPrivilegedPortMapping:
return metav1.Condition{
Type: "Accepted",
Status: metav1.ConditionFalse,
Expand Down
19 changes: 17 additions & 2 deletions control-plane/api-gateway/binding/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func validateCertificateData(secret corev1.Secret) error {

// validateListeners validates the given listeners both internally and with respect to each
// other for purposes of setting "Conflicted" status conditions.
func validateListeners(gateway gwv1beta1.Gateway, listeners []gwv1beta1.Listener, resources *common.ResourceMap) listenerValidationResults {
func validateListeners(gateway gwv1beta1.Gateway, listeners []gwv1beta1.Listener, resources *common.ResourceMap, gwcc *v1alpha1.GatewayClassConfig) listenerValidationResults {
var results listenerValidationResults
merged := make(map[gwv1beta1.PortNumber]mergedListeners)
for i, listener := range listeners {
Expand All @@ -235,7 +235,15 @@ func validateListeners(gateway gwv1beta1.Gateway, listeners []gwv1beta1.Listener
listener: listener,
})
}

// This list keeps track of port conflicts directly on gateways. i.e., two listeners on the same port as
// defined by the user.
seenListenerPorts := map[int]struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really clean implementation! O(1) and easy to read. Excellent work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kudos to @nathancoleman for this one.

// This list keeps track of port conflicts caused by privileged port mappings.
seenContainerPorts := map[int]struct{}{}
portMapping := int32(0)
if gwcc != nil {
portMapping = gwcc.Spec.MapPrivilegedContainerPorts
}
for i, listener := range listeners {
var result listenerValidationResult

Expand All @@ -249,6 +257,10 @@ func validateListeners(gateway gwv1beta1.Gateway, listeners []gwv1beta1.Listener
result.acceptedErr = errListenerUnsupportedProtocol
} else if listener.Port == 20000 { // admin port
result.acceptedErr = errListenerPortUnavailable
} else if _, ok := seenListenerPorts[int(listener.Port)]; ok {
result.acceptedErr = errListenerPortUnavailable
} else if _, ok := seenContainerPorts[common.ToContainerPort(listener.Port, portMapping)]; ok {
result.acceptedErr = errListenerMappedToPrivilegedPortMapping
}

result.routeKindErr = validateListenerAllowedRouteKinds(listener.AllowedRoutes)
Expand All @@ -261,6 +273,9 @@ func validateListeners(gateway gwv1beta1.Gateway, listeners []gwv1beta1.Listener
}

results = append(results, result)

seenListenerPorts[int(listener.Port)] = struct{}{}
seenContainerPorts[common.ToContainerPort(listener.Port, portMapping)] = struct{}{}
}
return results
}
Expand Down
31 changes: 28 additions & 3 deletions control-plane/api-gateway/binding/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,8 +530,10 @@ func TestValidateListeners(t *testing.T) {
t.Parallel()

for name, tt := range map[string]struct {
listeners []gwv1beta1.Listener
expectedAcceptedErr error
listeners []gwv1beta1.Listener
expectedAcceptedErr error
listenerIndexToTest int
mapPrivilegedContainerPorts int32
}{
"valid protocol HTTP": {
listeners: []gwv1beta1.Listener{
Expand Down Expand Up @@ -563,9 +565,32 @@ func TestValidateListeners(t *testing.T) {
},
expectedAcceptedErr: errListenerPortUnavailable,
},
"conflicted port": {
listeners: []gwv1beta1.Listener{
{Protocol: gwv1beta1.TCPProtocolType, Port: 80},
{Protocol: gwv1beta1.TCPProtocolType, Port: 80},
},
expectedAcceptedErr: errListenerPortUnavailable,
listenerIndexToTest: 1,
},
"conflicted mapped port": {
listeners: []gwv1beta1.Listener{
{Protocol: gwv1beta1.TCPProtocolType, Port: 80},
{Protocol: gwv1beta1.TCPProtocolType, Port: 2080},
},
expectedAcceptedErr: errListenerMappedToPrivilegedPortMapping,
listenerIndexToTest: 1,
mapPrivilegedContainerPorts: 2000,
},
} {
t.Run(name, func(t *testing.T) {
require.Equal(t, tt.expectedAcceptedErr, validateListeners(gatewayWithFinalizer(gwv1beta1.GatewaySpec{}), tt.listeners, nil)[0].acceptedErr)
gwcc := &v1alpha1.GatewayClassConfig{
Spec: v1alpha1.GatewayClassConfigSpec{
MapPrivilegedContainerPorts: tt.mapPrivilegedContainerPorts,
},
}

require.Equal(t, tt.expectedAcceptedErr, validateListeners(gatewayWithFinalizer(gwv1beta1.GatewaySpec{}), tt.listeners, nil, gwcc)[tt.listenerIndexToTest].acceptedErr)
})
}
}
Expand Down
4 changes: 4 additions & 0 deletions control-plane/api-gateway/common/helm_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ type HelmConfig struct {
// EnableOpenShift indicates whether we're deploying into an OpenShift environment
// and should create SecurityContextConstraints.
EnableOpenShift bool

// MapPrivilegedServicePorts is the value which Consul will add to privileged container port values (ports < 1024)
// defined on a Gateway.
MapPrivilegedServicePorts int
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocker, but it would be nice to have a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add comment, this is a non-obvious field.

missylbytes marked this conversation as resolved.
Show resolved Hide resolved
}

type ConsulConfig struct {
Expand Down
22 changes: 18 additions & 4 deletions control-plane/api-gateway/common/translation.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ func (t ResourceTranslator) Namespace(namespace string) string {
}

// ToAPIGateway translates a kuberenetes API gateway into a Consul APIGateway Config Entry.
func (t ResourceTranslator) ToAPIGateway(gateway gwv1beta1.Gateway, resources *ResourceMap) *api.APIGatewayConfigEntry {
func (t ResourceTranslator) ToAPIGateway(gateway gwv1beta1.Gateway, resources *ResourceMap, gwcc *v1alpha1.GatewayClassConfig) *api.APIGatewayConfigEntry {
namespace := t.Namespace(gateway.Namespace)

listeners := ConvertSliceFuncIf(gateway.Spec.Listeners, func(listener gwv1beta1.Listener) (api.APIGatewayListener, bool) {
return t.toAPIGatewayListener(gateway, listener, resources)
return t.toAPIGatewayListener(gateway, listener, resources, gwcc)
})

return &api.APIGatewayConfigEntry{
Expand All @@ -81,7 +81,7 @@ var listenerProtocolMap = map[string]string{
"tcp": "tcp",
}

func (t ResourceTranslator) toAPIGatewayListener(gateway gwv1beta1.Gateway, listener gwv1beta1.Listener, resources *ResourceMap) (api.APIGatewayListener, bool) {
func (t ResourceTranslator) toAPIGatewayListener(gateway gwv1beta1.Gateway, listener gwv1beta1.Listener, resources *ResourceMap, gwcc *v1alpha1.GatewayClassConfig) (api.APIGatewayListener, bool) {
namespace := gateway.Namespace

var certificates []api.ResourceReference
Expand All @@ -104,17 +104,31 @@ func (t ResourceTranslator) toAPIGatewayListener(gateway gwv1beta1.Gateway, list
}
}

portMapping := int32(0)
if gwcc != nil {
portMapping = gwcc.Spec.MapPrivilegedContainerPorts
}

return api.APIGatewayListener{
Name: string(listener.Name),
Hostname: DerefStringOr(listener.Hostname, ""),
Port: int(listener.Port),
Port: ToContainerPort(listener.Port, portMapping),
Protocol: listenerProtocolMap[strings.ToLower(string(listener.Protocol))],
TLS: api.APIGatewayTLSConfiguration{
Certificates: certificates,
},
}, true
}

func ToContainerPort(portNumber gwv1beta1.PortNumber, mapPrivilegedContainerPorts int32) int {
if portNumber >= 1024 {
// We don't care about privileged port-mapping, this is a non-privileged port
return int(portNumber)
}

return int(portNumber) + int(mapPrivilegedContainerPorts)
}

func (t ResourceTranslator) ToHTTPRoute(route gwv1beta1.HTTPRoute, resources *ResourceMap) *api.HTTPRouteConfigEntry {
namespace := t.Namespace(route.Namespace)

Expand Down
2 changes: 1 addition & 1 deletion control-plane/api-gateway/common/translation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ func TestTranslator_ToAPIGateway(t *testing.T) {
resources.ReferenceCountCertificate(listenerOneCert)
resources.ReferenceCountCertificate(listenerTwoCert)

actualConfigEntry := translator.ToAPIGateway(input, resources)
actualConfigEntry := translator.ToAPIGateway(input, resources, &v1alpha1.GatewayClassConfig{})

if diff := cmp.Diff(expectedConfigEntry, actualConfigEntry); diff != "" {
t.Errorf("Translator.GatewayToAPIGateway() mismatch (-want +got):\n%s", diff)
Expand Down
Loading