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: A new parent interface #3234

Merged
merged 2 commits into from
Nov 27, 2024
Merged
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
37 changes: 37 additions & 0 deletions apis/common/parent/interface.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// 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 parent

import (
"context"

"sigs.k8s.io/controller-runtime/pkg/client"
)

type Parent interface {
// The external format of the Parent.
String() string
// Verify that the desired parent (from .spec) matches the actual parent in .status.externalRef.
// This ensures the parent remains unchanged.
// We currently don't enforce parent immutability using a webhook or CRD CEL due to legacy reasons.
MatchActual(Parent) error
}

// ParentBuilder builds a Parent object from a ParentRef.
// - ParentRef is the Config Connector API reference for identifying a resource's logical parent.
// - The Parent object provides helper functions for parent-related logic in direct reconciliation.
type ParentBuilder interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the krm object should implement ResolveIdentity(ctx context.Context, reader client.Reader) (Identity, error)

If we want Identity to be typed, it would have to be something like this:

type ObjectWithIdentity[T] interface {
  ResolveIdentity(ctx contetx.Context, reader client.Reader) (*T, error)
}

// Parent API reference builds its corresponding Parent object.
Build(ctx context.Context, reader client.Reader, othernamespace string, parent Parent) error
}
132 changes: 132 additions & 0 deletions apis/common/parent/project.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// 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 parent

import (
"context"
"fmt"
"strings"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var _ Parent = &ProjectParent{}

type ProjectParent struct {
ProjectID string
}

var _ ParentBuilder = &ProjectRef{}

// Project specifies the resource's GCP hierarchy (Project/Folder/Organization).
// +kubebuilder:object:generate:=true
type ProjectRef struct {
/* The `projectID` field of a project, when not managed by Config Connector. */
External string `json:"external,omitempty"`
/* The `name` field of a `Project` resource. */
Name string `json:"name,omitempty"`
/* The `namespace` field of a `Project` resource. */
Namespace string `json:"namespace,omitempty"`
}

// Builds a the ProjectParent from ProjectRef.
// If `projectRef.external` is given, parse projectID from External, otherwise find the ConfigConnector project
// according to `projectRef.name` and `projectRef.namespace`.
func (p *ProjectRef) Build(ctx context.Context, reader client.Reader, othernamespace string, parent Parent) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect we don't need this function. We need a function that resolves a ProjectRef to a ProjectIdentity, and ProjectIdentity should have a function that can return the ProjectID. It might be rename this function to map from ProjectRef -> ProjectIdentity.

We would have 3 concepts: Foo, FooRef and FooIdentity

FooIdentity is essentially the fully-qualified name, I don't know if we should name it FooLink or something like that instead, but I do think that just using these 3 concepts will simplify our code a lot.

projectParent, ok := parent.(*ProjectParent)
if !ok {
return fmt.Errorf("build invalid parent, except %T", &ProjectParent{})
}
if p.External != "" {
if p.Name != "" {
return fmt.Errorf("cannot specify both name and external on project reference")
}

tokens := strings.Split(p.External, "/")
if len(tokens) == 1 {
projectParent.ProjectID = tokens[0]
return nil
}
if len(tokens) == 2 && tokens[0] == "projects" {
projectParent.ProjectID = tokens[1]
return nil
}
return fmt.Errorf("format of project external=%q was not known (use projects/<projectId> or <projectId>)", p.External)
}

if p.Name == "" {
return fmt.Errorf("must specify either name or external on project reference")
}

key := types.NamespacedName{
Namespace: p.Namespace,
Name: p.Name,
}
if key.Namespace == "" {
key.Namespace = othernamespace
}

project := &unstructured.Unstructured{}
project.SetGroupVersionKind(schema.GroupVersionKind{
Group: "resourcemanager.cnrm.cloud.google.com",
Version: "v1beta1",
Kind: "Project",
})
if err := reader.Get(ctx, key, project); err != nil {
if apierrors.IsNotFound(err) {
return fmt.Errorf("referenced Project %v not found", key)
}
return fmt.Errorf("error reading referenced Project %v: %w", key, err)
}

projectID, _, err := unstructured.NestedString(project.Object, "spec", "resourceID")
if err != nil {
return fmt.Errorf("reading spec.resourceID from %v %v/%v: %w", project.GroupVersionKind().Kind, p.Namespace, p.Name, err)
}
if projectID == "" {
projectID = project.GetName()
}
projectParent.ProjectID = projectID
return nil
}

func ParseProjectParent(external string) (*ProjectParent, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could make this a method on FooIdentity, something like ParseExternal() error

tokens := strings.Split(external, "/")
if len(tokens) != 2 || tokens[0] != "projects" {
return nil, fmt.Errorf("format of Project external=%q was not known (use projects/<projectId>)", external)
}

return &ProjectParent{
ProjectID: tokens[1],
}, nil
}

func (p *ProjectParent) String() string {
return "projects/" + p.ProjectID
}

func (p *ProjectParent) MatchActual(actualI Parent) error {
actual, ok := actualI.(*ProjectParent)
if !ok {
return fmt.Errorf("parent format changed, desired %T", p)
}
if p.ProjectID != actual.ProjectID {
return fmt.Errorf("spec.projectRef changed, desired %s, actual %s", p.ProjectID, actual.ProjectID)
}
return nil
}
88 changes: 88 additions & 0 deletions apis/common/parent/projectlocation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// 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 parent

import (
"context"
"fmt"
"strings"

"sigs.k8s.io/controller-runtime/pkg/client"
)

var _ Parent = &ProjectAndLocationParent{}

type ProjectAndLocationParent struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can merge ProjectAndLocationParent and ProjectAndLocationRef, it makes this file shorter. But it adds additional complexity to the controller on error handling, and the order of the function calls to set the external.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want Foo, FooRef and FooIdentity. I think this is FooIdentity, and ProjectAndLocationRef is FooRef

ProjectID string
Location string
}

func (p *ProjectAndLocationParent) String() string {
return "projects/" + p.ProjectID + "/locations/" + p.Location
}

func (p *ProjectAndLocationParent) MatchActual(actualI Parent) error {
actual, ok := actualI.(*ProjectAndLocationParent)
if !ok {
return fmt.Errorf("parent format changed, desired %T", p)
}
if p.ProjectID != actual.ProjectID {
return fmt.Errorf("spec.projectRef changed, desired %s, actual %s", p.ProjectID, actual.ProjectID)
}
if p.Location != actual.Location {
return fmt.Errorf("spec.location changed, desired %s, actual %s", p.Location, actual.Location)
}
return nil
}

var _ ParentBuilder = &ProjectAndLocationRef{}

// ProjectAndLocationParent specifies the resource's GCP hierarchy (Project/Folder/Organization) and its geographical location.
// +kubebuilder:object:generate:=true
type ProjectAndLocationRef struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is actually ProjectAndLocationIdentity, because we want to embed this into FooIdentity, which gets embedded into Foo

// +required
ProjectRef *ProjectRef `json:"projectRef"`

// +required
Location string `json:"location"`
}

func (p *ProjectAndLocationRef) Build(ctx context.Context, reader client.Reader, othernamespace string, parent Parent) error {
projectAndLocation, ok := parent.(*ProjectAndLocationParent)
if !ok {
return fmt.Errorf("build invalid parent, except %T", &ProjectAndLocationParent{})
}
project := new(ProjectParent)
if err := p.ProjectRef.Build(ctx, reader, othernamespace, project); err != nil {
return err
}
if project.ProjectID == "" {
return fmt.Errorf("cannot resolve project")
}
projectAndLocation.ProjectID = project.ProjectID
projectAndLocation.Location = p.Location
return nil
}

func ParseProjectAndLocationParent(external string) (*ProjectAndLocationParent, error) {
tokens := strings.Split(external, "/")
if len(tokens) != 4 || tokens[0] != "projects" || tokens[2] != "locations" {
return nil, fmt.Errorf("format of ProjectAndLocation external=%q was not known (use projects/<projectId>/locations/<location>)", external)
}

return &ProjectAndLocationParent{
ProjectID: tokens[1],
Location: tokens[3],
}, nil
}
54 changes: 54 additions & 0 deletions apis/common/parent/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion dev/tasks/generate-crds
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,7 @@ done

# clean up apis/config/crd for now
cd ${REPO_ROOT}
rm -rf apis/config/crd
rm -rf apis/config/crd

# controller-gen sometimes leaves empty imports in the api files.
go run -mod=readonly golang.org/x/tools/cmd/goimports@latest -w apis
Loading