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

feat: allow and deny list support for schema field overwriting (namely label and image fields atm) #7056

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Jan 25, 2022

fixes #6416
Related DD here: go/skaffold-field-overwriting
This PR adds the abillity to configure what K8s Group Kind's and which fields specifically are overwritten by skaffold when it renders & deploys manifests. Use cases and related issues include:

Example usage of the transform field:

deploy:
  transform:
    allow:
    - type: Function.openfaas.com
      image: [.spec.image]
      labels: [.spec.metadata.labels]

Open Questions:

  • Is transform a good name. IIUC, skaffold v2 introduces a render phase which also has something like render.transform as a field. Any opinions on better alternatives for naming or if that might be a source of confusion worth addressing now once V2 is introduced?

Future Work:

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch from c7c18ef to 7e2f507 Compare January 25, 2022 19:25
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 11 times, most recently from 354a266 to 8ff17e2 Compare January 26, 2022 05:37
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #7056 (39b6c0a) into main (290280e) will decrease coverage by 2.04%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7056      +/-   ##
==========================================
- Coverage   70.48%   68.43%   -2.05%     
==========================================
  Files         515      560      +45     
  Lines       23150    26437    +3287     
==========================================
+ Hits        16317    18093    +1776     
- Misses       5776     7094    +1318     
- Partials     1057     1250     +193     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 220 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2134aa...39b6c0a. Read the comment docs.

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 2 times, most recently from be86e5d to 67bb08f Compare January 26, 2022 22:14
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 2 times, most recently from 9bb7832 to 4170f41 Compare January 26, 2022 22:17
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 6 times, most recently from 22c6a14 to c1c4698 Compare January 31, 2022 19:25
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 3 times, most recently from ce8ef96 to d45fb07 Compare March 4, 2022 21:37
@aaron-prindle
Copy link
Contributor Author

The only failing test - the kokoro test failure is a known flake - TestBuildKanikoInsecureRegistry

@aaron-prindle aaron-prindle requested review from tejal29 and removed request for MarlonGamez March 7, 2022 17:51
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 2 times, most recently from 843e134 to 8dddc83 Compare March 7, 2022 20:42
Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

discussed offline with @aaron-prindle. Having resourceSelector be top level can have issues with applying profiles and config upgrades. It's safer nested under deploy.resourceSelector.
Otherwise, LGTM.

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch from 8dddc83 to edd7352 Compare March 8, 2022 23:18
@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Mar 8, 2022

Current status:
field name is changed: transform -> resourceSelector
resourceSelector is nested under deploy: resourceSelector -> deploy.resourceSelector

Possible issue:
When desiging the feature and schema here, there were some UX reasons as to why the field should not be nested under deploy:

Discussed Alternatives and Tradeoffs For Schema Design
In discussion with the Cloud Deploy team it was noted that the desired CUJ might have better UX if the allow & deny lists could be shared more easily across a team which would mean making these options top level in the schema as it will allow this config to be modularized (use skaffold modules).  The CUJ (summarized) was as follows:
Company X is developing a kubernetes application that deploys custom CRDs
As part of properly configuring skaffold for deploying CRDs, Tech-Lead-1 (TL1) adds the company’s custom CRD (eg: Service.my.custom.crd) is to their allow
TL1 then wants to share their configuration across their team/company for their devs using skaffold so they can deploy their CRD application, in order to do so w/o allow being a top level field, a skaffold profile must be used and passed for each skaffold invocation (vs using a module which is more traditional and well understood by users).  

If the Allow & Deny fields are not top level in the schema (eg: deploy.resourceSelector.allow vs resourceSelector.allow) then the configuration is not sharable by modules but instead requires using profiles which can be more complicated. From the Cloud Deploy Team - “This [the original nested deploy.transform.enable alternative] complicates the Cloud Deploy experience, as modules can be referenced within the yaml (IIUC), but profiles need to be explicitly enabled per pipeline/target.”

@gsquared94 can you clarify if this field is nested if modules can still be used interchangeably for this feature? Eg: a company can have a module which specifies deploy.resourceSelector and that can be consumed easily across their company?

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch 2 times, most recently from 1320455 to e891951 Compare March 9, 2022 02:03
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v3 branch from e891951 to 39b6c0a Compare March 9, 2022 05:40
@aaron-prindle aaron-prindle enabled auto-merge (squash) March 9, 2022 06:35
@aaron-prindle aaron-prindle enabled auto-merge (squash) March 9, 2022 06:36
@aaron-prindle
Copy link
Contributor Author

Final status, Cloud Deploy needs this field to be top level for their module usage requirments.

  • field name is changed: transform -> resourceSelector
  • resourceSelector is a top level field

@afirth
Copy link

afirth commented Mar 31, 2022

@aaron-prindle I'm really excited about this feature! Are there already docs for it? If not, perhaps the DD (https://goto.google.com/skaffold-field-overwriting) could be made public?

@aaron-prindle
Copy link
Contributor Author

Awesome @afirth! There are public docs for it here:
https://skaffold.dev/docs/tutorials/skaffold-resource-selector/

Please file any issues w/ the docs or feature that you encounter. Currently I would recommend using this for:

  • Removing any immutable fields from skaffold's overwriting to avoid deployment errors
  • Adding custom CRDs or possibly currently not-status-checked resources to skaffold's status checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold labels cause incompatibility with subsequent standard kubectl commands
5 participants