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

Add kubebuilder validations in proto for extauth AuthConfig #9481

Merged
merged 62 commits into from
Jun 20, 2024

Conversation

arianaw66
Copy link
Contributor

@arianaw66 arianaw66 commented May 14, 2024

Description

This PR adds kubebuilder markers and CEL validation rules to the AuthConfig CRD.
These are unit tested thoroughly in https://github.com/solo-io/gloo-mesh-enterprise/pull/16951.
The constraints are notated here, primarily in the first two commits, which come from the solo-projects validation code and are therefore guaranteed to be constraints that apply when using Edge.

API changes

No API fields have been changed, but this adds pre-admission validation which can affect customers' pipelines. The changes proposed here should only affect AuthConfig CRs which are not currently ACCEPTED by our translation.

Docs changes

The docs will unfortunately be updated with the kubebuilder markers, as there's no way to hide the additional proto comments from the docs.
The docs team will also add user-facing constraint descriptions according to the notes here.

Context

This is motivation by the validation milestone in progress for Gloo Platform.

Interesting decisions

Further discussion of the relevant factors and decisions can be found on the ExtAuthPolicy validation issue that is a part of that milestone.

Testing steps

See https://github.com/solo-io/gloo-mesh-enterprise/pull/16951 for testing steps

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label May 14, 2024
@arianaw66 arianaw66 changed the title Add maxLength in proto for AuthConfigs.configs Add maxItems in proto for AuthConfigs.configs May 16, 2024
@arianaw66 arianaw66 changed the title Add maxItems in proto for AuthConfigs.configs Add kubebuilder validations in proto for extauth AuthConfig May 23, 2024
@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/gloo-mesh-enterprise/issues/16010

@htpvu
Copy link

htpvu commented Jun 10, 2024

Since this API change effects Gloo Edge users, has it been approved by the relevant stakeholders? I know Field has some concerns with enabling such validations by default, for which we've had discussions around building a toggle in CRDs to conditionally not render these validations.

I'd like to see a signoff from @sam-heilbron @DuncanDoyle and @kcbabo before this merges.

I've brought this change to the attention of @DuncanDoyle @kcbabo and @sam-heilbron, we have their thumbs up on the change and we can merge this PR right after the 1.17 branch is cut.

cc: @arianaw66

@arianaw66
Copy link
Contributor Author

It appears the bump to solo-kit v0.35.1 breaks the build-bot tests
See #9626

@arianaw66 arianaw66 removed the work in progress signals bulldozer to keep pr open (don't auto-merge) label Jun 17, 2024
Copy link
Contributor

@Rachael-Graham Rachael-Graham left a comment

Choose a reason for hiding this comment

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

Approving for now. There's some followup doc work I'd like to do, which will include 1) translating rules into user-friendly comments, and 2) figuring out some way to update solo-kit so that the cel rules themselves dont build into the published docs.

@arianaw66 arianaw66 added the keep pr updated signals bulldozer to keep pr up to date with base branch label Jun 20, 2024
@soloio-bulldozer soloio-bulldozer bot merged commit e308d8f into main Jun 20, 2024
20 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the AuthConfig.configs-maxLength branch June 20, 2024 19:23
Copy link
Contributor

@sheidkamp sheidkamp left a comment

Choose a reason for hiding this comment

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

"Approved"

  • Checked the rules against the CheckIfInvalidAuthConfig function
  • Pulled changes into solo-projects and manually ran the in memory and kubernetes e2e ext-auth tests
  • Ran ./projects/gloo/pkg/syncer/extauth validation unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants