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

Gate #57

Merged
merged 32 commits into from
Oct 3, 2022
Merged

Gate #57

merged 32 commits into from
Oct 3, 2022

Conversation

davidhadas
Copy link
Contributor

This PR finalizes the guard-gate

@knative-prow
Copy link

knative-prow bot commented Sep 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2022
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2022
@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #57 (68080b9) into main (11ee095) will increase coverage by 2.10%.
The diff coverage is 84.46%.

❗ Current head 68080b9 differs from pull request most recent head 098db68. Consider uploading reports for the commit 098db68 to get more accurate results

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   79.03%   81.13%   +2.10%     
==========================================
  Files          32       35       +3     
  Lines        2599     2931     +332     
==========================================
+ Hits         2054     2378     +324     
  Misses        433      433              
- Partials      112      120       +8     
Impacted Files Coverage Δ
cmd/guard-service/services.go 87.50% <0.00%> (+4.16%) ⬆️
pkg/guard-gate/client.go 66.97% <33.33%> (-0.31%) ⬇️
cmd/guard-service/main.go 72.03% <70.45%> (+72.03%) ⬆️
pkg/guard-gate/gate.go 88.54% <88.54%> (ø)
pkg/guard-gate/session.go 93.26% <93.26%> (ø)
pkg/guard-gate/state.go 94.00% <100.00%> (ø)
pkg/guard-utils/santize.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 26, 2022
@davidhadas
Copy link
Contributor Author

/retest all

@knative-prow
Copy link

knative-prow bot commented Sep 26, 2022

@davidhadas: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test build-tests
  • /test integration-tests
  • /test unit-tests

Use /test all to run all jobs.

In response to this:

/retest all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@davidhadas
Copy link
Contributor Author

/test all

@davidhadas
Copy link
Contributor Author

/hold
update #58 first to make this PR simpler

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2022
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 26, 2022
@davidhadas
Copy link
Contributor Author

/unhold

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 29, 2022
utils "knative.dev/security-guard/pkg/guard-utils"
)

const plugVersion string = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally we could pick this dynamically rather than having to hardcore (and have to remember to update on each release)... pretty sure we do this elsewhere in knative, let me find an example...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems for executables there is a way todo that using ldflags - from outside during the build of a new release
go build -ldflags="-X 'main.Version=v1.0.0'"

I don't know for packages - or if it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think I was thinking of a combination of knative/pkg#2548 and knative-extensions/kn-plugin-quickstart#327

Still wonder if there's a better way to get this info (maybe similar to what we do in serving to add the version as a label), but it isn't blocking

cmd/guard-service/main.go Fixed Show fixed Hide fixed
cmd/guard-service/main.go Fixed Show fixed Hide fixed
pkg/guard-gate/client.go Fixed Show fixed Hide fixed
Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

Are the various "Log entries created from user input" warnings something we should address?

@@ -0,0 +1,27 @@
# Copyright 2019 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to double-check that this doesn't overwrite the existing config-deployment... might be better to just have the setup-guard-service script patch the existing configmap with the current QP image...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested by adding a test: boom parameter to config.deployment - this parameter remains when doing security-guard ko apply -Rf ./config while the image changes - note that the _example is removed after such a change.

after ko apply -Rf config/core at serving

data:
  _example: |-
    ################################
    #                              #
    #    EXAMPLE CONFIGURATION     #
    #                              #
    ################################

    # This block is not actually functional configuration,
    ...
    # NOTE THAT THIS IS AN EXPERIMENTAL / ALPHA FEATURE
    concurrency-state-endpoint: ""
  queueSidecarImage: us.icr.io/knat/queue-39be6f1d08a095bd076a71d288d295b6@sha256:086b8a7d8bc2104672609a1918274f2e4b075076f52c0fbbc4c3f044b64eb5fe
  test: boom

after ko apply -Rf ./config at security-guard

data:
  queue-sidecar-image: us.icr.io/knat/queue-958f2fa0da7e00e1e412a537ecf9d1cc@sha256:021e6438111346ea5d5aff064d6c3ad2cea6c3a92cc7e7708f644f8a1fc34a1e
  test: boom

hack/update-guard-service.sh Outdated Show resolved Hide resolved
hack/setup-guard-service.sh Outdated Show resolved Hide resolved
hack/update-guard-service.sh Outdated Show resolved Hide resolved
utils "knative.dev/security-guard/pkg/guard-utils"
)

const plugVersion string = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Think I was thinking of a combination of knative/pkg#2548 and knative-extensions/kn-plugin-quickstart#327

Still wonder if there's a better way to get this info (maybe similar to what we do in serving to add the version as a label), but it isn't blocking

pkg/guard-gate/session.go Outdated Show resolved Hide resolved
@davidhadas
Copy link
Contributor Author

Are the various "Log entries created from user input" warnings something we should address?

This seems like a false positive - the data is sanitized in place, maybe this is why the alert is issued.

@psschwei
Copy link
Contributor

psschwei commented Oct 3, 2022

Are the various "Log entries created from user input" warnings something we should address?

This seems like a false positive - the data is sanitized in place, maybe this is why the alert is issued.

They aren't being triggered anymore after the most recent commits

Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2022
@knative-prow knative-prow bot merged commit 97a0246 into knative-extensions:main Oct 3, 2022
@davidhadas davidhadas deleted the gate branch December 11, 2022 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants