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 12039 terminating gateway acl policy fix #4468

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

natemollica-nm
Copy link
Contributor

@natemollica-nm natemollica-nm commented Jan 24, 2025

Changes proposed in this PR

  • Introduce handling of Consul AdminPartition ACL policy rules when global.adminPartitions.enabled: true

Current workflow requires end users to manually update the Terminating Gateway policies applied by the TerminatingGateway resource controller when AdminPartitions are enabled from:

namespace "default" {
  service_prefix "" {
    policy = "write"
  }
}

to

partition "default" {
  namespace "default" {
    service_prefix "" {
      policy    = "write"
      intention = "read"
    }
  }
}

How I've tested this PR

Test Matrix for ACL Policies and Admin Partitions with Terminating Gateway

Test Case Admin Partitions Enabled Partition Name Service Type Expected ACL Policy
Default Partition, Wildcard Yes default Wildcard (*) partition "default" { namespace "default" { service_prefix "" { policy = "write"; intention = "read"; } } }
Default Partition, Specific Yes default Specific (static-server) partition "default" { namespace "default" { service "static-server" { policy = "write"; intention = "read"; } } }
Non-Default Partition, Wildcard Yes dev Wildcard (*) partition "dev" { namespace "default" { service_prefix "" { policy = "write"; intention = "read"; } } }
Non-Default Partition, Specific Yes dev Specific (static-server) partition "dev" { namespace "default" { service "static-server" { policy = "write"; intention = "read"; } } }
No Partition, Wildcard No N/A Wildcard (*) namespace "default" { service_prefix "" { policy = "write"; intention = "read"; } }
No Partition, Specific No N/A Specific (static-server) namespace "default" { service "static-server" { policy = "write"; intention = "read"; } }

How I expect reviewers to test this PR

👀

Checklist

@natemollica-nm natemollica-nm added area/acls Related to ACLs theme/terminating-gateway Related to Consul Terminating Gateway Development labels Jan 24, 2025
@natemollica-nm natemollica-nm requested a review from a team as a code owner January 24, 2025 23:33
sarahalsmiller and others added 4 commits February 3, 2025 10:19
* openshift test

* add temporary pr trigger

* lint fixes

* delete pr trigger

* Update acceptance/tests/api-gateway/api_gateway_tenancy_test.go

* Update acceptance/framework/consul/helm_cluster.go
… (#4470)

* feat: add k8s topology zone info for nodePort service

* original PR: #4301

---------

Co-authored-by: kolorful <[email protected]>
…Volumes and extraEnvionmentVars (#4471)

* update helm chart to allow for configuring google application credentials

* changelog
* Set API gateway security context to comply with best practices

* update deployment security context

* Set SeccompProfile on injected dataplane sidecar

* Drop all capabilities in the injected sidecar

* Set required securityContext properties on connect-inject-init container

* Add changelog entry

* May it please the linter

* May it please the linter

* Add helpful logs to failure message assertion

* Set default value for API gateway's mapPrivilegedContainerPorts

* Update invalidated unit tests

* May it please the linter

* Allow privilege escalation when expected for backwards compatibility

* add init sec comp to mesh gateway deployment

* Update invalidated unit tests

---------

Co-authored-by: Sarah Alsmiller <[email protected]>
@natemollica-nm natemollica-nm requested a review from a team as a code owner March 4, 2025 16:59
Copy link
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

Some suggestions to align parameter descriptions with the style guide. Otherwise LGTM!

Comment on lines +93 to +94
// ConsulPartition indicates the Consul Admin Partition name the controller is
// operating in. Adds this value as metadata on managed resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ConsulPartition indicates the Consul Admin Partition name the controller is
// operating in. Adds this value as metadata on managed resources.
// ConsulPartition specifies the name of the Consul admin partition the controller
// operates in. Adds this value as metadata on managed resources.

Comment on lines +1447 to +1449
# A list of extra volumes to mount onto the snapshot agent. This
# is useful for bringing in extra data that only the snapshot agent needs access
# to. Like storage credentials. The value of this should be a list of objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# A list of extra volumes to mount onto the snapshot agent. This
# is useful for bringing in extra data that only the snapshot agent needs access
# to. Like storage credentials. The value of this should be a list of objects.
# A list of extra volumes to mount onto the snapshot agent. Use this block
# to include extra data that only the snapshot agent needs access
# to, such as storage credentials. This value should be a list of objects.

Comment on lines +1461 to +1464
# - `type` - Type of the volume, must be one of "configMap" or "secret". Case sensitive.
#
# - `name` - Name of the configMap or secret to be mounted. This also controls
# the path that it is mounted to. The volume will be mounted to `/consul/userconfig/<name>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# - `type` - Type of the volume, must be one of "configMap" or "secret". Case sensitive.
#
# - `name` - Name of the configMap or secret to be mounted. This also controls
# the path that it is mounted to. The volume will be mounted to `/consul/userconfig/<name>`.
# - `type` - Type of the volume. Must be one of `configMap` or `secret`. Case sensitive.
#
# - `name` - Name of the configMap or secret to mount. This specification also controls
# the path it mounts to. The volume will be mounted to `/consul/userconfig/<name>`.

Comment on lines +1441 to +1443
# A list of extra environment variables to set on the snapshot agent specifically
# This could be used to configure credentials that the rest of the
# stateful set would not need access to, like GOOGLE_APPLICATION_CREDENTIALS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# A list of extra environment variables to set on the snapshot agent specifically
# This could be used to configure credentials that the rest of the
# stateful set would not need access to, like GOOGLE_APPLICATION_CREDENTIALS
# A list of extra environment variables to set on the snapshot agent specifically.
# Use this parameter to configure credentials that the rest of the
# stateful set would not need access to, like `GOOGLE_APPLICATION_CREDENTIALS`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acls Related to ACLs backport/1.4.x backport/1.5.x backport/1.6.x Changes are backported to 1.6 theme/terminating-gateway Related to Consul Terminating Gateway Development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants