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

Mutation: Cannot reference ports in a pod #1355

Closed
maxsmythe opened this issue Jun 10, 2021 · 4 comments · Fixed by #1394
Closed

Mutation: Cannot reference ports in a pod #1355

maxsmythe opened this issue Jun 10, 2021 · 4 comments · Fixed by #1394
Assignees
Labels
bug Something isn't working mutation
Milestone

Comments

@maxsmythe
Copy link
Contributor

What steps did you take and what happened:

Ports in pods have an integer-type keyfield. Because our location spec assume stings, we cannot match against this, and wind up creating a duplicate port.

Repro steps:

take the following mutator:

apiVersion: mutations.gatekeeper.sh/v1alpha1
kind: Assign
metadata:
  name: set-port-name
spec:
  applyTo:
  - groups: [""]
    kinds: ["Pod"]
    versions: ["v1"]
  location: 'spec.containers[name: opa].ports[containerPort: "8888"].name'
  parameters:
    assign:
      value: "modified"

and create the following object:

apiVersion: v1
kind: Pod
metadata:
  name: opa
  namespace: production
  labels:
    owner: me.agilebank.demo
spec:
  containers:
    - name: opa
      image: openpolicyagent/opa:0.9.2
      args:
        - "run"
        - "--server"
        - "--addr=localhost:8080"
      resources:
        limits:
          cpu: "100m"
          memory: "30Mi"
      ports:
        - containerPort: 8888
          name: unchanged

We get the following error:

Error from server (InternalError): error when creating "opa.yaml": Internal error occurred: v1.Pod.Spec: v1.PodSpec.Containers: []v1.Container: v1.Container.Ports: []v1.ContainerPort: v1.ContainerPort.ContainerPort: readUint32: unexpected character: �, error found in #10 byte of ...|nerPort":"8888","nam|..., bigger context ...|nerPort":8888,"protocol":"TCP"},{"containerPort":"8888","name":"modified"}],"resources":{"limits":{"|...

Which has two issues:

  • We are attempting to create a field of the wrong type
  • We are writing out two ports instead of modifying the existing port

What did you expect to happen:

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

  • Gatekeeper version:
  • Kubernetes version: (use kubectl version):
@maxsmythe maxsmythe added bug Something isn't working mutation labels Jun 10, 2021
@maxsmythe maxsmythe added this to the Mutation Beta milestone Jun 10, 2021
@shomron
Copy link
Contributor

shomron commented Jun 15, 2021

@maxsmythe for the first issue, we can consider adding numeric token support to our parser and exposing this to the matching logic. Specifically this is about numeric support in List nodes' KeyValue - I think reworking that to be interface{} will fit in with how we are utilizing it in mutatorState - are there other places this would be relevant?

Regarding the second issue seems closely related, I see assumptions around string types in setListElementToValue and createMissingElement.

@maxsmythe
Copy link
Contributor Author

The only other thing we use location for is to build the implicit schema. We haven't been using the type of keyValue there for anything. I think we can continue to ignore it.

One thing that's interesting is whether we can use integers to identify the field of a resource (e.g. spec.1234.hello). Maybe we just disallow numeric types there to avoid opening that kettle-o-fish unless we have to?

@shomron shomron self-assigned this Jun 16, 2021
shomron added a commit to shomron/gatekeeper that referenced this issue Jun 24, 2021
Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes open-policy-agent#1355

Signed-off-by: Oren Shomron <shomron@gmail.com>
@shomron
Copy link
Contributor

shomron commented Jun 24, 2021

One thing that's interesting is whether we can use integers to identify the field of a resource (e.g. spec.1234.hello). Maybe we just disallow numeric types there to avoid opening that kettle-o-fish unless we have to?

I've made it so that non-quoted identifiers can no longer start with a number - let me know what you think!

shomron added a commit to shomron/gatekeeper that referenced this issue Jun 24, 2021
Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes open-policy-agent#1355

Signed-off-by: Oren Shomron <shomron@gmail.com>
@maxsmythe
Copy link
Contributor Author

I'm liking it!

shomron added a commit to shomron/gatekeeper that referenced this issue Jul 1, 2021
Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes open-policy-agent#1355

Signed-off-by: Oren Shomron <shomron@gmail.com>
shomron added a commit to shomron/gatekeeper that referenced this issue Jul 1, 2021
Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes open-policy-agent#1355

Signed-off-by: Oren Shomron <shomron@gmail.com>
shomron added a commit that referenced this issue Jul 2, 2021
* Add integer keyValue support to mutation path parser / mutators.

Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes #1355

* Add additional tests, resolve lint warnings.

* Additional test cases for digits in unquoted identifiers

* Fixed additional parsing  roundtripping cases

Signed-off-by: Oren Shomron <shomron@gmail.com>
julianKatz pushed a commit to julianKatz/gatekeeper that referenced this issue Jul 8, 2021
…-policy-agent#1394)

* Add integer keyValue support to mutation path parser / mutators.

Previously, the mutation path parser only supported string types for
keyValues selecting items in a list. This PR adds support for integers,
such that we can now express paths like this:

```
spec.containers[name: opa].ports[containerPort: 8888].name
```

Integers are assumed to be represented in decimal and are interpreted as
positive int64 values.

The mutation code was updated accordingly to match based on type.
Additionally, string serialization of paths was updated to be more
selective on which strings are quoted.

Fixes open-policy-agent#1355

* Add additional tests, resolve lint warnings.

* Additional test cases for digits in unquoted identifiers

* Fixed additional parsing  roundtripping cases

Signed-off-by: Oren Shomron <shomron@gmail.com>
Signed-off-by: juliankatz <juliankatz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mutation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants