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

Internal flag #530

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
d10ed54
Support for multiple types in ingresses
BardOve Aug 19, 2024
4d206e3
changes
BardOve Sep 3, 2024
2f60565
Removed unused functions
BardOve Sep 4, 2024
13efe89
tests
BardOve Sep 5, 2024
dc2d01d
added internal flag to routing
BardOve Sep 5, 2024
317898c
newlines
BardOve Sep 10, 2024
b55bad2
moved helperfunction
BardOve Sep 10, 2024
06294d3
errorhandling
BardOve Sep 10, 2024
ce2639c
rename variables
BardOve Sep 10, 2024
42aa262
moved type def and corrected typo
BardOve Sep 10, 2024
49d30c1
renamed function
BardOve Sep 10, 2024
16290f8
tests
BardOve Sep 16, 2024
871f70c
pointer bool to enable nil check.
BardOve Sep 16, 2024
33ac843
Support for multiple types in ingresses
BardOve Aug 19, 2024
9e2889e
changes
BardOve Sep 3, 2024
ca30099
Removed unused functions
BardOve Sep 4, 2024
387fa00
tests
BardOve Sep 5, 2024
0a031d0
added internal flag to routing
BardOve Sep 5, 2024
a2f5fb3
newlines
BardOve Sep 10, 2024
e289bc3
moved helperfunction
BardOve Sep 10, 2024
ddb9785
errorhandling
BardOve Sep 10, 2024
a24ed0f
rename variables
BardOve Sep 10, 2024
d9bd2ab
moved type def and corrected typo
BardOve Sep 10, 2024
871a092
renamed function
BardOve Sep 10, 2024
44ae494
tests
BardOve Sep 16, 2024
653f740
pointer bool to enable nil check.
BardOve Sep 16, 2024
f225423
Merge branch 'internal-flag' of github.com:kartverket/skiperator into…
BardOve Sep 16, 2024
0b54024
Refactor tests.
BardOve Sep 18, 2024
659b295
Merge branch 'main' into internal-flag
evenh Sep 19, 2024
9802c87
Update api/v1alpha1/hosts.go
BardOve Sep 20, 2024
6cbaad4
rename function
BardOve Sep 20, 2024
49b3f7e
rename app and resources in tests
BardOve Sep 27, 2024
9e0e790
Avoid error shadowing.
BardOve Sep 30, 2024
12eb2a8
Avoid error shadowing.
BardOve Sep 30, 2024
6399cbd
Revert "Avoid error shadowing."
BardOve Oct 2, 2024
01d9c30
cleanup
BardOve Oct 8, 2024
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
53 changes: 48 additions & 5 deletions api/v1alpha1/application_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/kartverket/skiperator/api/v1alpha1/digdirator"
"github.com/kartverket/skiperator/api/v1alpha1/istiotypes"
"github.com/kartverket/skiperator/api/v1alpha1/podtypes"
"github.com/kartverket/skiperator/pkg/util"
evenh marked this conversation as resolved.
Show resolved Hide resolved
corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -68,7 +69,7 @@ type ApplicationSpec struct {
// They can optionally be suffixed with a plus and name of a custom TLS secret located in the istio-gateways namespace.
// E.g. "foo.atkv3-dev.kartverket-intern.cloud+env-wildcard-cert"
//+kubebuilder:validation:Optional
Ingresses []string `json:"ingresses,omitempty"`
Ingresses *apiextensionsv1.JSON `json:"ingresses,omitempty"`
BardOve marked this conversation as resolved.
Show resolved Hide resolved

// An optional priority. Supported values are 'low', 'medium' and 'high'.
// The default value is 'medium'.
Expand Down Expand Up @@ -335,6 +336,11 @@ type PrometheusConfig struct {
AllowAllMetrics bool `json:"allowAllMetrics,omitempty"`
}

type Ingresses struct {
BardOve marked this conversation as resolved.
Show resolved Hide resolved
Hostname string
Internal *bool
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing JSON Tags in Ingresses Struct

The fields in the Ingresses struct lack JSON tags, which may lead to issues during JSON marshaling and unmarshaling. To ensure proper serialization, add JSON tags to the struct fields.

Apply the following diff to add the necessary JSON tags:

 type Ingresses struct {
-	Hostname string
-	Internal *bool
+	Hostname string `json:"hostname"`
+	Internal *bool  `json:"internal,omitempty"`
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type Ingresses struct {
Hostname string
Internal *bool
}
type Ingresses struct {
Hostname string `json:"hostname"`
Internal *bool `json:"internal,omitempty"`
}


func NewDefaultReplicas() Replicas {
return Replicas{
Min: 2,
Expand Down Expand Up @@ -442,15 +448,52 @@ func (a *Application) GetCommonSpec() *CommonSpec {
}
}

func MarshalledIngresses(ingresses interface{}) *apiextensionsv1.JSON {
IngressesJSON := &apiextensionsv1.JSON{}
var err error

IngressesJSON.Raw, err = json.Marshal(ingresses)
if err == nil {
return IngressesJSON
}

return nil
}
BardOve marked this conversation as resolved.
Show resolved Hide resolved
BardOve marked this conversation as resolved.
Show resolved Hide resolved

func (s *ApplicationSpec) Hosts() (HostCollection, error) {
var ingressesAsString []string
var ingressesAsObject []Ingresses
BardOve marked this conversation as resolved.
Show resolved Hide resolved
var errorsFound []error
hosts := NewCollection()

var errorsFound []error
for _, ingress := range s.Ingresses {
err := hosts.Add(ingress)
ingresses := MarshalledIngresses(s.Ingresses)
BardOve marked this conversation as resolved.
Show resolved Hide resolved
err := json.Unmarshal(ingresses.Raw, &ingressesAsString)

BardOve marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
err := json.Unmarshal(ingresses.Raw, &ingressesAsObject)
if err != nil {
errorsFound = append(errorsFound, err)
return NewCollection(), errors.Join(errorsFound...)
}

for _, ingress := range ingressesAsObject {
if ingress.Internal == nil {
ingress.Internal = util.PointTo(isInternal(ingress.Hostname))
BardOve marked this conversation as resolved.
Show resolved Hide resolved
}

err := hosts.Add(ingress.Hostname, *ingress.Internal)
if err != nil {
errorsFound = append(errorsFound, err)
return NewCollection(), errors.Join(errorsFound...)
}
}
return hosts, nil
}

for _, ingress := range ingressesAsString {
err := hosts.Add(ingress, isInternal(ingress))
BardOve marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
errorsFound = append(errorsFound, err)
continue
}
}

Expand Down
15 changes: 12 additions & 3 deletions api/v1alpha1/hosts.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,20 @@ package v1alpha1
import (
"fmt"
"github.com/chmike/domain"
"regexp"
"strings"
)

const hostnameSecretSeparator = "+"

var internalPattern = regexp.MustCompile(`[^.]\.skip\.statkart\.no|[^.]\.kartverket-intern.cloud`)

BardOve marked this conversation as resolved.
Show resolved Hide resolved
// TODO: Add a mechanism for validating that the
// hostname is covered by the CustomCertificateSecret if present
type Host struct {
Hostname string
CustomCertificateSecret *string
Internal bool
}

type HostCollection struct {
Expand All @@ -25,6 +29,7 @@ func NewHost(hostname string) (*Host, error) {
}

var h Host

// If hostname is separated by +, the user wants to use a custom certificate
results := strings.Split(hostname, hostnameSecretSeparator)

Expand All @@ -49,7 +54,7 @@ func NewHost(hostname string) (*Host, error) {
if err := domain.Check(h.Hostname); err != nil {
return nil, fmt.Errorf("%s: failed validation: %w", h.Hostname, err)
}

h.Internal = isInternal(hostname)
return &h, nil
}

Expand All @@ -63,14 +68,14 @@ func NewCollection() HostCollection {
}
}

func (hs *HostCollection) Add(hostname string) error {
func (hs *HostCollection) Add(hostname string, internal bool) error {
h, err := NewHost(hostname)
if err != nil {
return err
}

h.Internal = internal
existingValue, alreadyPresent := hs.hosts[h.Hostname]

switch alreadyPresent {
case true:
if existingValue.UsesCustomCert() {
Expand Down Expand Up @@ -105,3 +110,7 @@ func (hs *HostCollection) Hostnames() []string {
func (hs *HostCollection) Count() int {
return len(hs.hosts)
}

func isInternal(hostname string) bool {
return internalPattern.MatchString(hostname)
}
3 changes: 3 additions & 0 deletions api/v1alpha1/routing_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ type RoutingSpec struct {
//+kubebuilder:validation:Optional
//+kubebuilder:default:=true
RedirectToHTTPS *bool `json:"redirectToHTTPS,omitempty"`

//+kubebuilder:validation:Optional
Internal *bool `json:"internal,omitempty"`
}

// +kubebuilder:object:generate=true
Expand Down
9 changes: 7 additions & 2 deletions api/v1alpha1/zz_generated.deepcopy.go

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

4 changes: 1 addition & 3 deletions config/crd/skiperator.kartverket.no_applications.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -646,9 +646,7 @@ spec:
Ingresses must be lowercase, contain no spaces, be a non-empty string, and have a hostname/domain separated by a period
They can optionally be suffixed with a plus and name of a custom TLS secret located in the istio-gateways namespace.
E.g. "foo.atkv3-dev.kartverket-intern.cloud+env-wildcard-cert"
items:
type: string
type: array
x-kubernetes-preserve-unknown-fields: true
istioSettings:
default:
telemetry:
Expand Down
2 changes: 2 additions & 0 deletions config/crd/skiperator.kartverket.no_routings.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ spec:
properties:
hostname:
type: string
internal:
type: boolean
redirectToHTTPS:
default: true
type: boolean
Expand Down
8 changes: 7 additions & 1 deletion pkg/resourcegenerator/deployment/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,17 @@ func Generate(r reconciliation.Reconciliation) error {
}

// add an external link to argocd
ingresses := application.Spec.Ingresses
hosts, err := application.Spec.Hosts()
if err != nil {
ctxLog.Error(err, "could not get hosts from application")
BardOve marked this conversation as resolved.
Show resolved Hide resolved
return err
}
if deployment.Annotations == nil {
deployment.Annotations = make(map[string]string)
}

var ingresses = hosts.Hostnames()

BardOve marked this conversation as resolved.
Show resolved Hide resolved
if len(ingresses) > 0 {
deployment.Annotations[AnnotationKeyLinkPrefix] = fmt.Sprintf("https://%s", ingresses[0])
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/resourcegenerator/istio/gateway/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package gateway

import (
"fmt"

skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
"github.com/kartverket/skiperator/pkg/reconciliation"
"github.com/kartverket/skiperator/pkg/resourcegenerator/resourceutils"
"github.com/kartverket/skiperator/pkg/util"
networkingv1beta1api "istio.io/api/networking/v1beta1"
networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
Expand Down Expand Up @@ -38,7 +38,7 @@ func generateForApplication(r reconciliation.Reconciliation) error {
name := fmt.Sprintf("%s-ingress-%x", application.Name, util.GenerateHashFromName(h.Hostname))
gateway := networkingv1beta1.Gateway{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: name}}

gateway.Spec.Selector = util.GetIstioGatewayLabelSelector(h.Hostname)
gateway.Spec.Selector = resourceutils.GetIstioGatewayLabelSelector(h)

gatewayServersToAdd := []*networkingv1beta1api.Server{}

Expand Down
8 changes: 5 additions & 3 deletions pkg/resourcegenerator/istio/gateway/routing.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ package gateway

import (
"fmt"

skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
"github.com/kartverket/skiperator/pkg/reconciliation"
"github.com/kartverket/skiperator/pkg/util"
"github.com/kartverket/skiperator/pkg/resourcegenerator/resourceutils"
networkingv1beta1api "istio.io/api/networking/v1beta1"
networkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -31,6 +30,9 @@ func generateForRouting(r reconciliation.Reconciliation) error {
if err != nil {
return err
}
if routing.Spec.Internal != nil {
h.Internal = *routing.Spec.Internal
}

gateway := networkingv1beta1.Gateway{ObjectMeta: metav1.ObjectMeta{Namespace: routing.Namespace, Name: routing.GetGatewayName()}}

Expand All @@ -44,7 +46,7 @@ func generateForRouting(r reconciliation.Reconciliation) error {
}
}

gateway.Spec.Selector = util.GetIstioGatewayLabelSelector(h.Hostname)
gateway.Spec.Selector = resourceutils.GetIstioGatewayLabelSelector(h)
gateway.Spec.Servers = []*networkingv1beta1api.Server{
{
Hosts: []string{h.Hostname},
Expand Down
47 changes: 14 additions & 33 deletions pkg/resourcegenerator/networkpolicy/dynamic/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,20 @@ func generateForCommon(r reconciliation.Reconciliation) error {
}

accessPolicy := object.GetCommonSpec().AccessPolicy
var ingresses []string
var hosts = skiperatorv1alpha1.NewCollection()
var inboundPort int32

if r.GetType() == reconciliation.ApplicationType {
ingresses = object.(*skiperatorv1alpha1.Application).Spec.Ingresses
var err error
hosts, err = object.(*skiperatorv1alpha1.Application).Spec.Hosts()
if err != nil {
ctxLog.Error(err, "Failed to get hosts for networkpolicy", "type", r.GetType(), "namespace", namespace)
return err
}
evenh marked this conversation as resolved.
Show resolved Hide resolved
inboundPort = int32(object.(*skiperatorv1alpha1.Application).Spec.Port)
}

ingressRules := getIngressRules(accessPolicy, ingresses, r.IsIstioEnabled(), namespace, inboundPort)
ingressRules := getIngressRules(accessPolicy, hosts, r.IsIstioEnabled(), namespace, inboundPort)
egressRules := getEgressRules(accessPolicy, namespace)

netpolSpec := networkingv1.NetworkPolicySpec{
Expand Down Expand Up @@ -113,20 +119,14 @@ func getEgressRule(outboundRule podtypes.InternalRule, namespace string) network
}

// TODO Clean up better
func getIngressRules(accessPolicy *podtypes.AccessPolicy, ingresses []string, istioEnabled bool, namespace string, port int32) []networkingv1.NetworkPolicyIngressRule {
func getIngressRules(accessPolicy *podtypes.AccessPolicy, hostCollection skiperatorv1alpha1.HostCollection, istioEnabled bool, namespace string, port int32) []networkingv1.NetworkPolicyIngressRule {
var ingressRules []networkingv1.NetworkPolicyIngressRule

if ingresses != nil && len(ingresses) > 0 {
if hasInternalIngress(ingresses) {
ingressRules = append(ingressRules, getGatewayIngressRule(true, port))
}

if hasExternalIngress(ingresses) {
ingressRules = append(ingressRules, getGatewayIngressRule(false, port))
}
for _, host := range hostCollection.AllHosts() {
ingressRules = append(ingressRules, getGatewayIngressRule(host.Internal, port))
}

// Allow grafana-agent to scrape
// Allow grafana-agent and Alloy to scrape
BardOve marked this conversation as resolved.
Show resolved Hide resolved
if istioEnabled {
promScrapeRuleGrafana := networkingv1.NetworkPolicyIngressRule{
From: []networkingv1.NetworkPolicyPeer{
Expand Down Expand Up @@ -226,26 +226,6 @@ func getNamespaceSelector(rule podtypes.InternalRule, appNamespace string) *meta
}
}

func hasExternalIngress(ingresses []string) bool {
for _, hostname := range ingresses {
if !util.IsInternal(hostname) {
return true
}
}

return false
}

func hasInternalIngress(ingresses []string) bool {
for _, hostname := range ingresses {
if util.IsInternal(hostname) {
return true
}
}

return false
}

func getGatewayIngressRule(isInternal bool, port int32) networkingv1.NetworkPolicyIngressRule {
ingressRule := networkingv1.NetworkPolicyIngressRule{
From: []networkingv1.NetworkPolicyPeer{
Expand All @@ -254,6 +234,7 @@ func getGatewayIngressRule(isInternal bool, port int32) networkingv1.NetworkPoli
MatchLabels: map[string]string{"kubernetes.io/metadata.name": "istio-gateways"},
},
PodSelector: &metav1.LabelSelector{

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider centralizing label definitions.

The function getIngressGatewayLabel determines labels based on whether the ingress is internal or external. This is a critical piece of logic that could benefit from being placed in a centralized configuration or constants file, as suggested by the existing TODO comment.

- // TODO Should be in constants or something
+ // Moved to constants for better maintainability

Moving these label definitions to a constants file not only cleans up the code but also makes it easier to manage changes to these labels in the future.

Committable suggestion was skipped due to low confidence.

MatchLabels: getIngressGatewayLabel(isInternal),
},
},
Expand Down
11 changes: 9 additions & 2 deletions pkg/resourcegenerator/networkpolicy/dynamic/routing.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
"github.com/kartverket/skiperator/pkg/reconciliation"
"github.com/kartverket/skiperator/pkg/resourcegenerator/resourceutils"
"github.com/kartverket/skiperator/pkg/util"
networkingv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -27,7 +28,13 @@ func generateForRouting(r reconciliation.Reconciliation) error {
for _, route := range routing.Spec.Routes {
uniqueTargetApps[getNetworkPolicyName(routing, route.TargetApp)] = route
}

h, err := routing.Spec.GetHost()
if routing.Spec.Internal != nil {
h.Internal = *routing.Spec.Internal
}
if err != nil {
return err
}
BardOve marked this conversation as resolved.
Show resolved Hide resolved
for netpolName, route := range uniqueTargetApps {
networkPolicy := networkingv1.NetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -50,7 +57,7 @@ func generateForRouting(r reconciliation.Reconciliation) error {
MatchLabels: util.GetIstioGatewaySelector(),
},
PodSelector: &metav1.LabelSelector{
MatchLabels: util.GetIstioGatewayLabelSelector(routing.Spec.Hostname),
MatchLabels: resourceutils.GetIstioGatewayLabelSelector(h),
},
},
},
Expand Down
Loading