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.

Sign up for GitHub

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

Jump to bottom

Refactor cosigned to take advantage of duck typing. #637

Merged
merged 3 commits into from
Sep 11, 2021
Merged

Refactor cosigned to take advantage of duck typing. #637

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f37305c
Refactor cosigned to take advantage of duck typing.
mattmoor Sep 8, 2021
b494c78
Make port configurable, pull tests out into a script.
mattmoor Sep 9, 2021
c78cfe1
Drop GO111MODULE, drop v1beta1 admission review, improve flag desc, h…
mattmoor Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions .github/workflows/kind-e2e-cosigned.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
name: Cosigned KinD E2E

on:
pull_request:
branches: [ 'main', 'release-*' ]

defaults:
run:
shell: bash
working-directory: ./src/github.com/sigstore/cosign

jobs:

e2e-tests:
name: e2e tests
runs-on: ubuntu-latest
strategy:
fail-fast: false # Keep running if one leg fails.
matrix:
k8s-version:
- v1.19.11
- v1.20.7
- v1.21.1

include:
# Map between K8s and KinD versions.
# This is attempting to make it a bit clearer what's being tested.
# See: https://github.com/kubernetes-sigs/kind/releases
- k8s-version: v1.19.11
kind-version: v0.11.1
kind-image-sha: sha256:07db187ae84b4b7de440a73886f008cf903fcf5764ba8106a9fd5243d6f32729
cluster-suffix: c${{ github.run_id }}.local
- k8s-version: v1.20.7
kind-version: v0.11.1
kind-image-sha: sha256:cbeaf907fc78ac97ce7b625e4bf0de16e3ea725daf6b04f930bd14c67c671ff9
cluster-suffix: c${{ github.run_id }}.local
- k8s-version: v1.21.1
kind-version: v0.11.1
kind-image-sha: sha256:69860bda5563ac81e3c0057d654b5253219618a22ec3a346306239bba8cfa1a6
cluster-suffix: c${{ github.run_id }}.local

env:
GOPATH: ${{ github.workspace }}
GO111MODULE: on
mattmoor marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/google/go-containerregistry/pull/125 allows insecure registry for
# '*.local' hostnames.
REGISTRY_NAME: registry.local
REGISTRY_PORT: 5000
KO_DOCKER_REPO: registry.local:5000/cosigned

steps:
- name: Set up Go 1.16.x
uses: actions/setup-go@v2
with:
go-version: 1.16.x

- name: Install Dependencies
working-directory: ./
run: |
echo '::group:: install ko'
curl -L https://github.com/google/ko/releases/download/v0.8.3/ko_0.8.3_Linux_x86_64.tar.gz | tar xzf - ko
chmod +x ./ko
sudo mv ko /usr/local/bin
echo '::endgroup::'

- name: Check out code onto GOPATH
uses: actions/checkout@v2
with:
path: ./src/github.com/sigstore/cosign

- name: Install Cosign
run: |
go install ./cmd/cosign

# This KinD setup is based on what we use for knative/serving on GHA, and it includes several "fun"
# monkey wrenches (e.g. randomizing cluster suffix: `.svc.cluster.local`) to make sure we don't bake
# in any invalid assumptions about a particular Kubernetes configuration.
- name: Install KinD
run: |
set -x
# Disable swap otherwise memory enforcement doesn't work
# See: https://kubernetes.slack.com/archives/CEKK1KTN2/p1600009955324200
sudo swapoff -a
sudo rm -f /swapfile
# Use in-memory storage to avoid etcd server timeouts.
# https://kubernetes.slack.com/archives/CEKK1KTN2/p1615134111016300
# https://github.com/kubernetes-sigs/kind/issues/845
sudo mkdir -p /tmp/etcd
sudo mount -t tmpfs tmpfs /tmp/etcd
curl -Lo ./kind https://github.com/kubernetes-sigs/kind/releases/download/${{ matrix.kind-version }}/kind-$(uname)-amd64
chmod +x ./kind
sudo mv kind /usr/local/bin

- name: Configure KinD Cluster
run: |
set -x
# KinD configuration.
cat > kind.yaml <<EOF
apiVersion: kind.x-k8s.io/v1alpha4
kind: Cluster
# Configure registry for KinD.
containerdConfigPatches:
- |-
[plugins."io.containerd.grpc.v1.cri".registry.mirrors."$REGISTRY_NAME:$REGISTRY_PORT"]
endpoint = ["http://$REGISTRY_NAME:$REGISTRY_PORT"]
# This is needed in order to support projected volumes with service account tokens.
# See: https://kubernetes.slack.com/archives/CEKK1KTN2/p1600268272383600
kubeadmConfigPatches:
- |
apiVersion: kubeadm.k8s.io/v1beta2
kind: ClusterConfiguration
metadata:
name: config
apiServer:
extraArgs:
"service-account-issuer": "kubernetes.default.svc"
"service-account-signing-key-file": "/etc/kubernetes/pki/sa.key"
networking:
dnsDomain: "${{ matrix.cluster-suffix }}"
nodes:
- role: control-plane
image: kindest/node:${{ matrix.k8s-version }}@${{ matrix.kind-image-sha }}
extraMounts:
- containerPath: /var/lib/etcd
hostPath: /tmp/etcd
- role: worker
image: kindest/node:${{ matrix.k8s-version }}@${{ matrix.kind-image-sha }}
EOF

- name: Create KinD Cluster
run: |
set -x
kind create cluster --config kind.yaml

- name: Setup local registry
run: |
# Run a registry.
docker run -d --restart=always \
-p $REGISTRY_PORT:$REGISTRY_PORT --name $REGISTRY_NAME registry:2

# Connect the registry to the KinD network.
docker network connect "kind" $REGISTRY_NAME

# Make the $REGISTRY_NAME -> 127.0.0.1, to tell `ko` to publish to
# local reigstry, even when pushing $REGISTRY_NAME:$REGISTRY_PORT/some/image
sudo echo "127.0.0.1 $REGISTRY_NAME" | sudo tee -a /etc/hosts

- name: Install cosigned
run: |
ko apply -Bf config/

# Update the cosign verification-key secret with a proper key pair.
cosign generate-key-pair k8s://cosign-system/verification-key

# Wait for the webhook to come up and become Ready
kubectl rollout status --timeout 5m --namespace cosign-system deployments/webhook

- name: Run Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather prefer using ginkgo e2e tests instead of this raw script. It is easier to maintain and less error prone. We were planing to add e2e tests next week too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again instead of having the whole piece of code here, it would be easier to maintain if we call scripts instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created ./test/e2e_test_cosigned.sh with this for now, which mostly just outlines what was here.

My goal here wasn't to set a precedent for how we build all future tests, but to at least get a workflow set up which validates something presubmit, so we don't end up with another -log_dir situation.

Suffice it to say that I'd be happy to see all of the "tests" rewritten in something better, but wanted some measure of validation before checking this in. Rather than bikeshed on which test framework to rewrite the tests in here, my inclination would be to get some test coverage in, and then we can follow up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

run: |
./test/e2e_test_cosigned.sh

- name: Collect diagnostics
if: ${{ failure() }}
run: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use scripts (potentially stored in a hack directory) instead of this long yaml file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of this logic is to dump useful diagnostic information from the ephemeral cluster before it is torn down. If we find ourselves creating more workflows that copy/paste this (within the same repo) then it might be useful to extract, but I don't think it's useful for local environments because they stick around for post-mortem inspection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I simple solution would have been to use kind export logs to a directory so it can be available as a bundle and download it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can certainly change this to do that. This snippet is just one I've been using across a variety of projects for this, and it's nice to be able to jump to directly the pod logs for the component you care about.

In Knative we uploaded the full logs to GCS, and they got very very large. Not sure this will end up with as much logging or as many tests, but we can tweak this however we want 👍

# Add more namespaces to dump here.
for ns in cosign-system; do
kubectl get pods -n${ns}

echo '::group:: describe'
kubectl describe pods -n${ns}
echo '::endgroup::'

for x in $(kubectl get pods -n${ns} -oname); do

echo "::group:: describe $x"
kubectl describe -n${ns} $x
echo '::endgroup::'

echo "::group:: $x logs"
kubectl logs -n${ns} $x --all-containers
echo '::endgroup::'

done
done
32 changes: 32 additions & 0 deletions cmd/cosign/webhook/depcheck_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//
// Copyright 2021 The Sigstore Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package main_test

import (
"testing"

"knative.dev/pkg/depcheck"
)

func TestNoDeps(t *testing.T) {
depcheck.AssertNoDependency(t, map[string][]string{
"github.com/sigstore/cosign/cmd/cosign/webhook": {
// This conflicts with klog, we error on startup about
// `-log_dir` being defined multiple times.
"github.com/golang/glog",
},
})
}
1 change: 1 addition & 0 deletions cmd/cosign/webhook/kodata/HEAD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../../../.git/HEAD
1 change: 1 addition & 0 deletions cmd/cosign/webhook/kodata/LICENSE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../../../LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why do we need this here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

HEAD and refs are symlinked in so that knative.dev/pkg/changeset can infuse various things (e.g. structured logging) with the changeset at which the image was built.

This is here as part of license compliance. Really this repo should also start running github.com/google/go-licenses as well to produce a third_party/VENDOR-LICENSE/... that we symlink here as well for license compliance, but that is sufficiently beyond the scope of my change that I left it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add these kodata files to .gitignore ?

Copy link
Member Author

Choose a reason for hiding this comment

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

They aren't generated, and they are generally checked in because you want the published images to contain the metadata. If there were a way of generating these, we could conceivably use .gitignore, but ko isn't going to create these (kodata is its convention, not the rest)

1 change: 1 addition & 0 deletions cmd/cosign/webhook/kodata/refs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../../../.git/refs
131 changes: 55 additions & 76 deletions cmd/cosign/webhook/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,99 +17,78 @@ package main

import (
"context"
goflag "flag"
"net"
"net/http"
"os"
"strconv"
"flag"

flag "github.com/spf13/pflag"
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log/zap"

"github.com/sigstore/cosign/pkg/cosign/kubernetes/webhook"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/pkg/signals"
"knative.dev/pkg/webhook"
"knative.dev/pkg/webhook/certificates"
"knative.dev/pkg/webhook/resourcesemantics"
"knative.dev/pkg/webhook/resourcesemantics/validation"

cwebhook "github.com/sigstore/cosign/pkg/cosign/kubernetes/webhook"
)

func main() {
ctrl.SetLogger(zap.New(func(o *zap.Options) {
o.Development = true
}))
var secretName = flag.String("secret-name", "", "The name of the secret in the webhook's namespace.")

var (
metricsAddr = net.ParseIP("127.0.0.1")
metricsPort uint16 = 8080
func main() {
opts := webhook.Options{
ServiceName: "webhook",
Port: 8443,
Copy link
Contributor

Choose a reason for hiding this comment

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

Port should remain configurable.
Metrics port and other settings would need to be configurable as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The metrics port (if you are using prometheus) is configurable through config-observability: https://github.com/knative/pkg/blob/9a4b6128207c17418c4524f9f9a07cd9cb3babda/metrics/config.go#L101-L104

IIRC 9090 is the typical prometheus port, so IDK where 8080 came from. Hosting metrics on a port is also kind of an anti-pattern because it doesn't work well in ephemeral environments (a la serverless), so other metrics options (like stackdriver) push metrics and don't expose a port.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take that back. The configmap controls a nested configuration. The prometheus port is configurable through environment variables: https://github.com/knative/pkg/blob/50410e0b833abc1a464d334b9e52e2c873dafcd4/metrics/config.go#L55-L56

SecretName: "webhook-certs",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be the value of secretName ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, secretName holds the name of the signing key. This is the secret that holds the TLS cert, which we reconcile with certificates.NewController, and which the sharedmain logic uses to host a TLS-terminated endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider updating the description on L39 and expanding which secret it is, sumtin like:

The name of the secret in the webhook's namespace that holds the TLS certificate used for signing

Or maybe change the flag from secret-name to signing-secret-name or just signing-secret?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the description, this is for verification (vs. signing). I was thinking earlier that we might be able to just use a configmap for this since it's mainly intended for the public key (vs. proper "secret" data), but that's beyond the scope of the change.

}
ctx := webhook.WithOptions(signals.NewContext(), opts)

bindAddr = net.ParseIP("0.0.0.0")
bindPort uint16 = 8443
// Allow folks to configure the port the webhook serves on.
flag.IntVar(&opts.Port, "secure-port", opts.Port, "The port on which to serve HTTPS.")

tlsCertDirectory string
secretKeyRef string
// This calls flag.Parse()
sharedmain.MainWithContext(ctx, "cosigned",
certificates.NewController,
NewValidatingAdmissionController,
)
}

klog.InitFlags(goflag.CommandLine)
flags := flag.NewFlagSet("main", flag.ExitOnError)
flags.AddGoFlagSet(goflag.CommandLine)
flags.StringVar(&secretKeyRef, "secret-key-ref", "", "The secret that includes pub/private key pair")
flags.IPVar(&metricsAddr, "metrics-address", metricsAddr, "The address the metric endpoint binds to.")
flags.Uint16Var(&metricsPort, "metrics-port", metricsPort, "The port the metric endpoint binds to.")
flags.IPVar(&bindAddr, "bind-address", bindAddr, ""+
"The IP address on which to listen for the --secure-port port.")
flags.Uint16Var(&bindPort, "secure-port", bindPort, "The port on which to serve HTTPS.")
flags.StringVar(&tlsCertDirectory, "tls-cert-dir", tlsCertDirectory, "The directory where the TLS certs are located.")

err := flags.Parse(os.Args[1:])
if err != nil {
klog.Error(err)
os.Exit(1)
}

cosignedValidationFuncs := map[schema.GroupVersionKind]webhook.ValidationFunc{
corev1.SchemeGroupVersion.WithKind("Pod"): webhook.ValidateSignedResources,
batchv1.SchemeGroupVersion.WithKind("Job"): webhook.ValidateSignedResources,
appsv1.SchemeGroupVersion.WithKind("Deployment"): webhook.ValidateSignedResources,
appsv1.SchemeGroupVersion.WithKind("StatefulSet"): webhook.ValidateSignedResources,
appsv1.SchemeGroupVersion.WithKind("ReplicateSet"): webhook.ValidateSignedResources,
appsv1.SchemeGroupVersion.WithKind("DaemonSet"): webhook.ValidateSignedResources,
}

cosignedValidationHook := webhook.NewFuncAdmissionValidator(webhook.Scheme, cosignedValidationFuncs, secretKeyRef)
func NewValidatingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
validator := cwebhook.NewValidator(ctx, *secretName)

opts := ctrl.Options{
Scheme: webhook.Scheme,
MetricsBindAddress: net.JoinHostPort(metricsAddr.String(), strconv.Itoa(int(metricsPort))),
Host: bindAddr.String(),
Port: int(bindPort),
CertDir: tlsCertDirectory,
}
return validation.NewAdmissionController(ctx,
// Name of the resource webhook.
"cosigned.sigstore.dev",
mattmoor marked this conversation as resolved.
Show resolved Hide resolved

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), opts)
if err != nil {
klog.Error(err, "Failed to create manager")
os.Exit(1)
}
// The path on which to serve the webhook.
"/validations",

// Get the controller manager webhook server.
webhookServer := mgr.GetWebhookServer()
// The resources to validate.
map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
corev1.SchemeGroupVersion.WithKind("Pod"): &duckv1.Pod{},

// Register the webhooks in the server.
webhookServer.Register("/validations", cosignedValidationHook)
appsv1.SchemeGroupVersion.WithKind("ReplicaSet"): &duckv1.WithPod{},
appsv1.SchemeGroupVersion.WithKind("Deployment"): &duckv1.WithPod{},
appsv1.SchemeGroupVersion.WithKind("StatefulSet"): &duckv1.WithPod{},
appsv1.SchemeGroupVersion.WithKind("DaemonSet"): &duckv1.WithPod{},
batchv1.SchemeGroupVersion.WithKind("Job"): &duckv1.WithPod{},
},

// Add healthz and readyz handlers to webhook server. The controller-runtime AddHealthzCheck/AddReadyzCheck methods
// are served via separate http server - better to serve these from the same webhook http server.
webhookServer.WebhookMux.Handle("/readyz/", http.StripPrefix("/readyz/", &healthz.Handler{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Are we losing the healthz and readyz checks with these changes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the Knative webhook logic has relatively sophisticated logic to respond to probes. You can see these configured in config/webhook.yaml.

webhookServer.WebhookMux.Handle("/healthz/", http.StripPrefix("/healthz/", &healthz.Handler{}))
// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
func(ctx context.Context) context.Context {
ctx = duckv1.WithPodValidator(ctx, validator.ValidatePod)
ctx = duckv1.WithPodSpecValidator(ctx, validator.ValidatePodSpecable)
return ctx
},

klog.Info("Starting the webhook...")
// Whether to disallow unknown fields.
// We pass false because we're using partial schemas.
false,

// Start the server by starting a previously-set-up manager
if err := mgr.Start(context.Background()); err != nil {
klog.Error(err)
os.Exit(1)
}
// Extra validating callbacks to be applied to resources.
nil,
)
}
Loading