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

Fix small CRD issue with toGroups #12440

Merged
merged 1 commit into from Jul 9, 2020
Merged

Conversation

lbernail
Copy link
Contributor

@lbernail lbernail commented Jul 7, 2020

Fixes: f0049da ("pkg/k8s: fix all structural issues with CNP validation")

In 1.8.1, the CNP CRD uses the following for egress toGroups rules:

			"toGroups": {
				Type: "object",
				Description: `ToGroups is a list of constraints that will
				gather data from third-party providers and create a new
				derived policy.`,
				Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
					"aws": AWSGroup,
				},
			}

However the rule expects toGroups to be an array. From the doc example:

  egress:
  - toGroups:
    - aws:
        securityGroupsIds:
        - 'sg-0f2146100a88d03c3'

This commit modifies the CRD to be consistent with the rule spec to avoid validation errors from Kubernetes.

Fix toGroups CRD to address validation errors 

@lbernail lbernail requested a review from a team as a code owner July 7, 2020 07:35
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 7, 2020
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

LGTM, can you add a Fixes: f0049da61f4f ("pkg/k8s: fix all structural issues with CNP validation") to your commit?

@aanm aanm added needs-backport/1.7 release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jul 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.2 Jul 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.7 Jul 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.6.11 Jul 7, 2020
@aanm aanm added the kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. label Jul 7, 2020
@coveralls
Copy link

coveralls commented Jul 7, 2020

Coverage Status

Coverage decreased (-0.02%) to 36.918% when pulling d23446c on DataDog:lbernail/togroups-crd into 7afc2a9 on cilium:master.

Fixes: f0049da ("pkg/k8s: fix all structural issues with CNP validation")

Signed-off-by: Laurent Bernaille <laurent.bernaille@datadoghq.com>
@aanm
Copy link
Member

aanm commented Jul 8, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Jul 9, 2020

test flake was fixed in upstream merging

@aanm aanm merged commit 678e7f0 into cilium:master Jul 9, 2020
@joestringer
Copy link
Member

@lbernail @aanm are there any upgrade implications to this change?

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.11 Jul 9, 2020
@joestringer
Copy link
Member

Possible upgrade implication with schema version, please take a look at the v1.6 backports. We will need to come to a shared understanding of that version and consistently backport to other branches based on it.

@lbernail
Copy link
Contributor Author

lbernail commented Jul 10, 2020

are there any upgrade implications to this change?

@joestringer it worked fine from 1.8.1 to master but I haven't tested with earlier versions. Is validation enabled before 1.8? Because if it is it means toGroups has been broken for a while

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.11 Jul 10, 2020
@joestringer
Copy link
Member

joestringer commented Jul 10, 2020

The related code for the schema has been around since v1.6, Not sure which versions that the schema validation is enabled for, but I seem to recall some upgrade notes early in v1.7 cycle around this.

Thinking this through, my take is that this PR tightens the CRD validation and should only affect users who currently run policies that would not pass the new validation. If they follow the upgrade instructions (specifically the preflight check) to validate their policies prior to upgrade, then they will not hit any issues.

@brb brb mentioned this pull request Jul 15, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.2 Jul 15, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.2 Jul 15, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.2 Jul 20, 2020
This was referenced Jul 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.7 Jul 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.7 Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.6.11
Backport done to v1.6
1.7.7
Backport done to v1.7
1.8.2
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

7 participants