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

Support pre-sync annotations #1433

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
197 changes: 197 additions & 0 deletions e2e/testcases/presync_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// Copyright 2024 Google LLC
//
// 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 e2e

import (
"fmt"
"testing"

"kpt.dev/configsync/e2e/nomostest"
"kpt.dev/configsync/e2e/nomostest/ntopts"
"kpt.dev/configsync/e2e/nomostest/policy"
"kpt.dev/configsync/e2e/nomostest/registryproviders"
nomostesting "kpt.dev/configsync/e2e/nomostest/testing"
"kpt.dev/configsync/e2e/nomostest/testpredicates"
"kpt.dev/configsync/e2e/nomostest/testwatcher"
"kpt.dev/configsync/pkg/api/configsync"
"kpt.dev/configsync/pkg/api/configsync/v1beta1"
"kpt.dev/configsync/pkg/core"
"kpt.dev/configsync/pkg/core/k8sobjects"
"kpt.dev/configsync/pkg/kinds"
"kpt.dev/configsync/pkg/metadata"
"kpt.dev/configsync/pkg/reposync"
"kpt.dev/configsync/pkg/rootsync"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func TestAddPreSyncAnnotationRepoSync(t *testing.T) {
repoSyncID := core.RepoSyncID(configsync.RepoSyncName, namespaceRepo)
nt := nomostest.New(t, nomostesting.SyncSource,
ntopts.RequireOCIProvider,
ntopts.SyncWithGitSource(repoSyncID),
ntopts.RepoSyncPermissions(policy.RBACAdmin(), policy.CoreAdmin()))
rootSyncGitRepo := nt.SyncSourceGitReadWriteRepository(nomostest.DefaultRootSyncID)
repoSyncKey := repoSyncID.ObjectKey
gitSource := nt.SyncSources[repoSyncID]
bookinfoRole := k8sobjects.RoleObject(core.Name("bookinfo-admin"))
image, err := nt.BuildAndPushOCIImage(repoSyncKey, registryproviders.ImageInputObjects(nt.Scheme, bookinfoRole))
if err != nil {
nt.T.Fatal(err)
}
nt.T.Log("Create RepoSync with OCI image")
repoSyncOCI := nt.RepoSyncObjectOCI(repoSyncKey, image.OCIImageID().WithoutDigest(), "", image.Digest)
nt.Must(rootSyncGitRepo.Add(nomostest.StructuredNSPath(repoSyncID.Namespace, repoSyncID.Name), repoSyncOCI))
nt.Must(rootSyncGitRepo.CommitAndPush("Set the RepoSync to sync from OCI"))
nt.Must(nt.WatchForAllSyncs())

err = nt.Watcher.WatchObject(kinds.RepoSyncV1Beta1(), repoSyncID.Name, repoSyncID.Namespace,
testwatcher.WatchPredicates(
checkRepoSyncPreSyncAnnotations(),
))
if err != nil {
nt.T.Fatalf("Source annotation not updated for RepoSync %v", err)
}
nt.T.Log("Set the RepoSync to sync from Git")
nt.SyncSources[repoSyncID] = gitSource
repoSyncGit := nomostest.RepoSyncObjectV1Beta1FromNonRootRepo(nt, repoSyncID.ObjectKey)
nt.Must(rootSyncGitRepo.Add(nomostest.StructuredNSPath(repoSyncID.Namespace, repoSyncID.Name), repoSyncGit))
nt.Must(rootSyncGitRepo.CommitAndPush("Set the RepoSync to sync from Git"))
nt.Must(nt.WatchForAllSyncs())

err = nt.Watcher.WatchObject(kinds.RepoSyncV1Beta1(), repoSyncID.Name, repoSyncID.Namespace,
testwatcher.WatchPredicates(
testpredicates.MissingAnnotation(metadata.SourceURLAnnotationKey),
))
if err != nil {
nt.T.Fatalf("Source annotations still exist when RepoSync is syncing from Git %v", err)
}
}

func TestAddPreSyncAnnotationRootSync(t *testing.T) {
rootSyncID := nomostest.DefaultRootSyncID
rootSyncKey := rootSyncID.ObjectKey
nt := nomostest.New(t, nomostesting.SyncSource,
ntopts.SyncWithGitSource(rootSyncID, ntopts.Unstructured),
ntopts.RequireOCIProvider,
)
gitSource := nt.SyncSources[rootSyncID]
bookinfoRole := k8sobjects.RoleObject(core.Name("bookinfo-admin"))
image, err := nt.BuildAndPushOCIImage(rootSyncKey, registryproviders.ImageInputObjects(nt.Scheme, bookinfoRole))
if err != nil {
nt.T.Fatal(err)
}
nt.T.Log("Create RootSync with OCI image")
rootSyncOCI := nt.RootSyncObjectOCI(rootSyncKey.Name, image.OCIImageID().WithoutDigest(), "", image.Digest)
nt.Must(nt.KubeClient.Apply(rootSyncOCI))
nt.Must(nt.WatchForAllSyncs())

err = nt.Watcher.WatchObject(kinds.RootSyncV1Beta1(), rootSyncID.Name, rootSyncID.Namespace,
testwatcher.WatchPredicates(
checkRootSyncPreSyncAnnotations(),
))
if err != nil {
nt.T.Fatalf("Source annotation not updated for RootSync %v", err)
}

nt.T.Log("Set the RootSync to sync from Git")
nt.SyncSources[rootSyncID] = gitSource
rootSyncGit := nomostest.RootSyncObjectV1Beta1FromRootRepo(nt, rootSyncID.Name)
nt.Must(nt.KubeClient.Apply(rootSyncGit))
nt.Must(nt.WatchForAllSyncs())

err = nt.Watcher.WatchObject(kinds.RootSyncV1Beta1(), rootSyncID.Name, rootSyncID.Namespace,
testwatcher.WatchPredicates(
testpredicates.MissingAnnotation(metadata.SourceURLAnnotationKey),
))
if err != nil {
nt.T.Fatalf("Source annotations still exist when RootSync is syncing from Git %v", err)
}
}

func checkRootSyncPreSyncAnnotations() testpredicates.Predicate {
return func(o client.Object) error {
if o == nil {
return testpredicates.ErrObjectNotFound
}
rs, ok := o.(*v1beta1.RootSync)
if !ok {
return testpredicates.WrongTypeErr(o, &v1beta1.RootSync{})
}
syncingCondition := rootsync.GetCondition(rs.Status.Conditions, v1beta1.RootSyncSyncing)
syncingCommit := syncingCondition.Commit
syncingImage := rs.Spec.Oci.Image
commitAnnotation, ok := o.GetAnnotations()[metadata.SourceCommitAnnotationKey]
if !ok {
return fmt.Errorf("object %q does not have annotation %q", o.GetName(), metadata.SourceCommitAnnotationKey)
}
repoAnnotation, ok := o.GetAnnotations()[metadata.SourceURLAnnotationKey]
if !ok {
return fmt.Errorf("object %q does not have annotation %q", o.GetName(), metadata.SourceURLAnnotationKey)
}
if syncingCommit != commitAnnotation {
return fmt.Errorf("object %s has commit annotation %s, but is being synced with commit %s",
o.GetName(),
commitAnnotation,
syncingCommit,
)
}
if syncingImage != repoAnnotation {
return fmt.Errorf("object %s has commit annotation %s, but is being synced with commit %s",
o.GetName(),
commitAnnotation,
syncingCommit,
)
}
return nil
}
}

func checkRepoSyncPreSyncAnnotations() testpredicates.Predicate {
return func(o client.Object) error {
if o == nil {
return testpredicates.ErrObjectNotFound
}
rs, ok := o.(*v1beta1.RepoSync)
if !ok {
return testpredicates.WrongTypeErr(o, &v1beta1.RepoSync{})
}
syncingCondition := reposync.GetCondition(rs.Status.Conditions, v1beta1.RepoSyncSyncing)
syncingCommit := syncingCondition.Commit
syncingImage := rs.Spec.Oci.Image
commitAnnotation, ok := o.GetAnnotations()[metadata.SourceCommitAnnotationKey]
if !ok {
return fmt.Errorf("object %q does not have annotation %q", o.GetName(), metadata.SourceCommitAnnotationKey)
}
repoAnnotation, ok := o.GetAnnotations()[metadata.SourceURLAnnotationKey]
if !ok {
return fmt.Errorf("object %q does not have annotation %q", o.GetName(), metadata.SourceURLAnnotationKey)
}
if syncingCommit != commitAnnotation {
return fmt.Errorf("object %s has commit annotation %s, but is being synced with commit %s",
o.GetName(),
commitAnnotation,
syncingCommit,
)
}
if syncingImage != repoAnnotation {
return fmt.Errorf("object %s has commit annotation %s, but is being synced with commit %s",
o.GetName(),
commitAnnotation,
syncingCommit,
)
}
return nil
}
}
10 changes: 10 additions & 0 deletions pkg/metadata/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,16 @@ const (
// reconciler-manager reads it. If set to true, the reconciler-manager will
// create the reconciler with the Namespace controller in the reconciler container.
DynamicNSSelectorEnabledAnnotationKey = configsync.ConfigSyncPrefix + "dynamic-ns-selector-enabled"

// SourceURLAnnotationKey stores the source URL where the source commit is fetched from.
// It is only set for OCI and Helm source, and unset for the Git source.
// It can be used for OCI signature verification.
SourceURLAnnotationKey = configsync.ConfigSyncPrefix + "source-url"

// SourceCommitAnnotationKey stores the Git source commit fetched by git-sync,
// the OCI image digest fetched by oci-sync, or the Helm chart version fetched by helm-sync.
// It can be used for OCI signature verification.
SourceCommitAnnotationKey = configsync.ConfigSyncPrefix + "source-commit"
)

// Lifecycle annotations
Expand Down
36 changes: 36 additions & 0 deletions pkg/parse/namespace.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package parse
import (
"context"
"fmt"
"reflect"
"strconv"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -174,6 +175,41 @@ func (p *namespace) setSourceStatusWithRetries(ctx context.Context, newStatus *S
return nil
}

func (p *namespace) setSourceAnnotations(ctx context.Context, commit string) error {
rs := &v1beta1.RepoSync{}
if err := p.Client.Get(ctx, reposync.ObjectKey(p.Scope, p.SyncName), rs); err != nil {
return status.APIServerError(err, "failed to get RepoSync for parser")
}
existing := rs.DeepCopy()

// Always update the source-commit annotation
currentSourceCommit := rs.GetAnnotations()[metadata.SourceCommitAnnotationKey]
if commit != currentSourceCommit {
core.SetAnnotation(rs, metadata.SourceCommitAnnotationKey, commit)
}

// Update the source-url annotation based on the source type
var newSourceURL string
if p.Options.SourceType == configsync.OciSource || p.Options.SourceType == configsync.HelmSource {
newSourceURL = p.Options.SourceRepo
}

currentImageURL := rs.GetAnnotations()[metadata.SourceURLAnnotationKey]
if newSourceURL != currentImageURL {
if newSourceURL == "" {
core.RemoveAnnotations(rs, metadata.SourceURLAnnotationKey)
} else {
core.SetAnnotation(rs, metadata.SourceURLAnnotationKey, newSourceURL)
}
}

// Patch the RepoSync if any annotations were updated
if !reflect.DeepEqual(existing.GetAnnotations(), rs.GetAnnotations()) {
return p.Client.Patch(ctx, rs, client.MergeFrom(existing), client.FieldOwner(configsync.FieldManager))
}
return nil
}

func (p *namespace) setRequiresRendering(ctx context.Context, renderingRequired bool) error {
rs := &v1beta1.RepoSync{}
if err := p.Client.Get(ctx, reposync.ObjectKey(p.Scope, p.SyncName), rs); err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/parse/opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ type Parser interface {
K8sClient() client.Client
// setRequiresRendering sets the requires-rendering annotation on the RSync
setRequiresRendering(ctx context.Context, renderingRequired bool) error
setSourceAnnotations(ctx context.Context, commit string) error
}

func (o *Options) k8sClient() client.Client {
Expand Down
36 changes: 36 additions & 0 deletions pkg/parse/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package parse
import (
"context"
"fmt"
"reflect"
"strconv"

"github.com/elliotchance/orderedmap/v2"
Expand Down Expand Up @@ -261,6 +262,41 @@ func setSourceStatusFields(source *v1beta1.SourceStatus, newStatus *SourceStatus
source.LastUpdate = newStatus.LastUpdate
}

func (p *root) setSourceAnnotations(ctx context.Context, commit string) error {
rs := &v1beta1.RootSync{}
if err := p.Client.Get(ctx, rootsync.ObjectKey(p.SyncName), rs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the get necessary? Seems like this patch does not require a get

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 compares the annotations with the new values to avoid unnecessary patching.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, instead of merging from existing object, we can patch directly with the raw patch data, client.RawPatch. With that, we don't need to GET and check for changes.

return status.APIServerError(err, "failed to get RootSync for parser")
}
existing := rs.DeepCopy()

// Always update the source-commit annotation
currentSourceCommit := rs.GetAnnotations()[metadata.SourceCommitAnnotationKey]
if commit != currentSourceCommit {
core.SetAnnotation(rs, metadata.SourceCommitAnnotationKey, commit)
}

// Update the source-url annotation based on the source type
var newSourceURL string
if p.Options.SourceType == configsync.OciSource || p.Options.SourceType == configsync.HelmSource {
newSourceURL = p.Options.SourceRepo
}

currentImageURL := rs.GetAnnotations()[metadata.SourceURLAnnotationKey]
if newSourceURL != currentImageURL {
if newSourceURL == "" {
core.RemoveAnnotations(rs, metadata.SourceURLAnnotationKey)
} else {
core.SetAnnotation(rs, metadata.SourceURLAnnotationKey, newSourceURL)
}
}

// Patch the RepoSync if any annotations were updated
if !reflect.DeepEqual(existing.GetAnnotations(), rs.GetAnnotations()) {
return p.Client.Patch(ctx, rs, client.MergeFrom(existing), client.FieldOwner(configsync.FieldManager))
}
return nil
}

func (p *root) setRequiresRendering(ctx context.Context, renderingRequired bool) error {
rs := &v1beta1.RootSync{}
if err := p.Client.Get(ctx, rootsync.ObjectKey(p.SyncName), rs); err != nil {
Expand Down
10 changes: 10 additions & 0 deletions pkg/parse/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ func DefaultRunFunc(ctx context.Context, p Parser, trigger string, state *reconc
// pull the source commit and directory with retries within 5 minutes.
gs.Commit, syncDir, gs.Errs = hydrate.SourceCommitAndDirWithRetry(util.SourceRetryBackoff, opts.SourceType, opts.SourceDir, opts.SyncDir, opts.ReconcilerName)

// Add pre-sync annotations to the object.
// If updating the object fails, it's likely due to a signature verification error
// from the webhook. In this case, add the error as a source error.
if gs.Errs == nil {
err := p.setSourceAnnotations(ctx, gs.Commit)
if err != nil {
gs.Errs = status.Append(gs.Errs, status.SourceError.Wrap(err).Build())
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no test cases which cover this scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, setting or removing annotations should always succeed since the end-to-end test involving the webhook check has not been implemented yet. In the future, when the webhook service that verifies image signatures is in place, we will be able to simulate error scenarios where patching (setting annotations) fails, and then validate the source errors accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO that functionality is fundamental to this feature and the feature should not be submitted without test coverage for it. What's the motivation for submitting this feature before validating the webhook user journey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I could keep this open until the commit for the full user journey is ready. The original intention was to keep the steps small and make this change only adds the annotations, maybe I can leave the part of returning the SourceError out from this PR for now?

}
}

// Generate source spec from Reconciler config
gs.Spec = SourceSpecFromFileSource(opts.FileSource, opts.SourceType, gs.Commit)

Expand Down
Loading